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

Status: RESOLVED FIXED

(Firefox :: Address Bar, task, P1)

Description

[Drew Willcoxon :adw]

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.

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1594622 - Quantumbar: Remove the context.preselected property and rely on result.heuristic instead

Please see bug 1594622 for a description.

Comment 2

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59fba8f33b409dd3134bebb2b05d7076754e038f

Comment 3

[Drew Willcoxon :adw]

Made some changes, so another try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ab86a30ca3e93f568beb6021bdd9064e4f9e7de

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44afbc73fe15
Quantumbar: Remove the context.preselected property and rely on result.heuristic instead r=mak

Comment 5

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/44afbc73fe15

Comment 6

[Drew Willcoxon :adw]

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:

Comment 7

[Pascal Chevrel:pascalc (PTO until April 26)]

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

Comment 8

[Drew Willcoxon :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.

Comment 9

[Drew Willcoxon :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.