Closed Bug 1594622 Opened 5 years ago Closed 5 years ago

Remove the context.preselected property and rely on result.heuristic instead

Categories

(Firefox :: Address Bar, task, P1)

task
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 72
Iteration:
72.2 - Nov 4 - 17
Tracking Status
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- fixed

People

(Reporter: adw, Assigned: adw)

References

Details

Attachments

(1 file)

For the nudges experiment, we need to open and show the urlbar view with a single tip. The tip's button should be preselected. See https://mozilla.invisionapp.com/share/S5SQCACBVAF#/screens/370460250_Search_Tips_Experiment

The urlbar webextension API doesn't have a way for extensions to set context.preselected. (Quantumbar does pass contexts to extensions via the onBehaviorRequested and onResultsRequested events. Extensions can set context.preselected in their event listeners, but that does not work as expected. After calling a listener, context.preselected remains undefined inside UrlbarProviderExtension. Adding a preselected property to the Query type in the urlbar.json schema doesn't help. I'm guessing that's due to process separation.)

At first I considered adding a preselected property to the Result type in urlbar.json. Then extensions could set it just like they can heuristic. But we always set preselected and heuristic together, it looks like. Actually, this is the only place I could find: https://searchfox.org/mozilla-central/rev/d061ba55ac76f41129618d638f4ef674303ec103/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm#240

So I think we should just get rid of context.preselected and rely on heuristic instead. afaict we added it close to the beginning of quantumbar development, and it seems we don't really need it. Tests pass locally with this patch. I'll also push to try.

This way extensions can just set heuristic and then the result will be preselected.

Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/44afbc73fe15 Quantumbar: Remove the context.preselected property and rely on result.heuristic instead r=mak
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 72
Flags: qe-verify-
Flags: in-testsuite+

Comment on attachment 9107091 [details]
Bug 1594622 - Quantumbar: Remove the context.preselected property and rely on result.heuristic instead

Beta/Release Uplift Approval Request

  • User impact if declined: We'd like the option of running quantumbar experiments on 71 that require this patch (bug 1564506, bug 1568594). Otherwise we'd need to wait until 72.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: Bug 1590461 should be uplifted first to avoid conflicts
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This patch does two things: (1) fix a bug in the urlbar webextensions implementation (so that urlbar results from extensions can be the first, pre-selected result in the urlbar view), (2) make a small internal change to urlbar (by removing a redundancy). In both cases, there's no expected impact to the normal outward appearance/behavior of the urlbar. Has tests.
  • String changes made/needed:
Attachment #9107091 - Flags: approval-mozilla-beta?
Regressions: 1595340

Drew, I'd like to wait on your input on the caused regression in bug 1595340 before evaluating an uplift to beta.

Flags: needinfo?(adw)

I'm not sure why that failure happened. I don't know enough about Fission to guess how it's involved -- but there was only one failure, which isn't a lot of data, so maybe it's not actually Fission specific. I can't reproduce it locally with Fission enabled. It will probably take some time for me to reproduce it on try to figure out what's happening.

My first guess is that there's some timing issue that the test isn't accounting for, and Fission may be making it worse or exposing it in the first place. I doubt that this is a real problem in Firefox, but it would take some investigation to be sure.

I'll investigate.

Flags: needinfo?(adw)

Comment on attachment 9107091 [details]
Bug 1594622 - Quantumbar: Remove the context.preselected property and rely on result.heuristic instead

Canceling the uplift request because I have higher priority things to work on than bug 1595340, and uplift is more of a nice-to-have anyway.

Attachment #9107091 - Flags: approval-mozilla-beta?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: