Closed Bug 1304027 Opened 8 years ago Closed 8 years ago

Uplift recent awesomebar improvements to Firefox 50

Categories

(Firefox :: Address Bar, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 50
Tracking Status
firefox50 + verified
firefox51 --- unaffected
firefox52 --- unaffected

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [fxsearch])

Attachments

(3 files)

[Tracking Requested - why for this release]:

We made various improvements to Firefox 51's awesomebar that will fix the following issues:

a. Enter sometimes is not delayed until the first result comes, that means when typing fast sometimes we may open the expected url, sometimes a search page. The behavior is not consistent.
b. Enter sometimes is ignored and handled later, at the wrong time, that means we may visit a "random" page from user's history.
c. Enter may be delayed by a long time, even 2 or 3 seconds, that means from when the user presses Enter to when we start loading the page, we could be waiting that time.

Fixing this requires all the recent patches we landed, that is:
1. partial uplift of Bug 1180944. Only the autocomplete code part, not the one-off feature, since other patches are built on top of that autocomplete code.
2. Bug 1301093, all the three parts to fix the wrong enter handling
3. Bug 1283329, this fixes the long delay
4. Bug 1302472, this fixes a regression in toolkit/autocomplete that may affect some add-ons (currently under review)

So I'm building a merged patch with all these changes.
Every part has an automated test-case.

I know this is big, scary and non-trivial. The fact is that these awesomebar improvements are so important for our end users that delaying them to Firefox 51 (24th of January) may be problematic.
I'm currently testing patches on Try.

This is an heads-up for release-drivers.
Flags: needinfo?(rkothari)
Depends on: 1301093, 1283329, 1302472
Assignee: nobody → mak77
Priority: -- → P1
Whiteboard: [fxsearch]
Attached patch patchSplinter Review
Approval Request Comment
[Feature/regressing bug #]: new awesomebar redesign released in Firefox 48
[User impact if declined]: see the descriptions in comment 0
[Describe test coverage new/current, TreeHerder]: automated tests, currently in 51 and 52
[Risks and why]: This is a merged patch touching various parts of autocomplete, there is the usual risk of currently unknown regressions. In the worst case, we should be able to just backout this patch and the perf problem will come back as before, but we won't take regressions.
[String/UUID change made/needed]: there are a couple API changes in xpidl, but they are backwards compatible for JS consumers.
Attachment #8793346 - Flags: approval-mozilla-beta?
I have a Try run of this here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b0dd67f65de9b16adf51f923bab04f7d7c69579

There are a bunch of failures like "dumpID is present and not an empty string" or "dumpID was not present despite crash reporting being enabled" but I honestly don't see how those could be related to these changes. Could be just an artifact of pushing Beta to Try. Sheriffs could know more about this. Relevant autocomplete tests are all fine.
Hi Marco, I am glad to see the uplift is ready to land this week. I will keep an eye on regressions filed under awesome bar the next couple of weeks. I'd appreciate your support in doing that as well. Let's hope this is a risk worth taking and we won't need back outs.

Can we do a health check on things around Beta5 timeframe i.e. on October 7th to assess regressions/end-user feedback that has come in to make a Go/NoGo decision for this?
Flags: needinfo?(rkothari) → needinfo?(mak77)
(In reply to Ritu Kothari (:ritu) from comment #3)
> Can we do a health check on things around Beta5 timeframe i.e. on October
> 7th to assess regressions/end-user feedback that has come in to make a
> Go/NoGo decision for this?

Sure, I'm all-in. I constantly check regression bugs filed under Location Bar, Places and periodically check the Firefox reddit for in-the-wild feedback.
Flags: needinfo?(mak77)
Comment on attachment 8793346 [details] [diff] [review]
patch

This is a pretty big change but since the end-user impact here is pretty noticeable, it makes sense to uplift in 50 and stabilize it for the next 6 week. Beta50+
Attachment #8793346 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1306639
 I ran a test suite to verify if there are any regressions and covered the dependencies, on Fx 50.0b10 across the following platforms: Windows 10 x64, Ubuntu 14.04 LTS and Mac OS X 10.11. 
 No issues regressed, however one new small issue was discovered : bug 1313080 . The issue is regarding aliased search engines accountancy, a cornered situation. I will mark this issue as verified.

Cheers!
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Attached image gtk2_with_patch.jpg
When I build firefox52 with gtk2 backend. 
If i apply this patch, the urlbar has some error, mouse click is not ok. see attachement file gtk2_with_patch.jpg
If i remove this patch, everything is ok.
When I build firefox52 with gtk2 backend. 
If i apply this patch, the urlbar has some error, mouse click is not ok. see attachement file gtk2_with_patch.jpg
If i enable hardware acceleraction, result is gtk2_with_patch.jpg.
If i disable hardware acceleraction, result is gtk2_with_patch_disable_acceleraction.jpg
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: