1310737 - Keyword bookmarks don't expand keyword query "%S" or POST data

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Francesco Lodolo [:flod]]

I have several keyword bookmarks using %S, and they stopped working in Nightly. I can reproduce in at least 52.0a1 (2016-10-16), on two different machines.

For example I have a bookmark https://bugzilla.mozilla.org/show_bug.cgi?id=%S with keyword "b", "b 123456" takes me to https://bugzilla.mozilla.org/show_bug.cgi?id=%S instead of https://bugzilla.mozilla.org/show_bug.cgi?id=123456

Comment 1

[Marco Bonardo [:mak]]

I suspect bug 1306639, now it's the backend generating the url, and we're doing
replace("%s", queryString); fixing that would be trivial but...

The browser code is far more complex http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/browser/base/content/browser.js#2148

Indeed we likely also broke POST keywords.
It may be the right time to finally move the code to the backend and remove getShortcutOrURIAndPostData from urlbarbindings (and maybe even simplify it for other consumers).

Comment 3

[Marco Bonardo [:mak]]

we also need to fix the encoding (see duped bug 1310953) but it should come for free once we port getShortcutOrURI

Comment 4

[Marco Bonardo [:mak]]

Attachment: Bug 1310737 - Urlbar doesn't properly handle %S and POST keywords.

Review commit: https://reviewboard.mozilla.org/r/89342/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89342/

Comment 5

[Marco Bonardo [:mak]]

Ugh, I forgot bug 1312018. will update the patch and tests for it.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8805585 [details]
Bug 1310737 - Urlbar doesn't properly handle %S and POST keywords.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89342/diff/1-2/

Comment 7

[Drew Willcoxon :adw]

Looks like there were failures on try.  Does that try push on reviewboard correspond to the latest revision?

Comment 8

[Marco Bonardo [:mak]]

the X is fixed, the b-c are not.
The problem is simple though, by removing getShortcutOrURI from handleCommand, it doesn't work anymore to just set .value and press Enter.
There are 2 ways:
1. update these tests to do the "right" thing (simulate input like a normal user)
2. reintroduce getShortcutOrURI in the handleCommand else branch

While I was initially tempted to go path 1, I then thought the most add-ons compatible choice is path 2. Since most of the encoding/decoding code is now shared between GSOU and UC, it should still be a good compromise.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8805585 [details]
Bug 1310737 - Urlbar doesn't properly handle %S and POST keywords.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89342/diff/2-3/

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8805585 [details]
Bug 1310737 - Urlbar doesn't properly handle %S and POST keywords.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89342/diff/3-4/

Comment 11

[Marco Bonardo [:mak]]

there are still a couple things I'm thinking about.
1. I'm not a big fan of the fallback I introduced because creating a URL is more expensive than just checking for some boolean... but it should be acceptable. So far I can't think of a better way to understand if the result came from the search component or not.
2. It would be great to provide a way for add-ons to intercept Enter and replace the result, for bug 1313620. I'm wondering something about overrideValue. I'll think about that.

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 8805585 [details]
Bug 1310737 - Urlbar doesn't properly handle %S and POST keywords.

https://reviewboard.mozilla.org/r/89342/#review88668

This is a pretty big patch but I've looked it over a couple of times now and don't see anything clearly wrong (naturally!)...

::: browser/base/content/browser.js:2074
(Diff revision 2)
> - * @param callback (optional, deprecated)
> - *        The callback function invoked when done. This parameter is
> - *        deprecated, please use the Promise that is returned.
>   *
> - * @return Promise<{ postData, url, mayInheritPrincipal }>
> + * @return {Promise}
> + * @resolves <{ url, postData, mayInheritPrincipal }>. If it's not possible

Nit: I think the < and > here are unnecessary and a little confusing.

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8805585 [details]
Bug 1310737 - Urlbar doesn't properly handle %S and POST keywords.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89342/diff/4-5/

Comment 14

[Marco Bonardo [:mak]]

I actually found a bug in browser_urlbarEnter.js test and thus this is likely to fix an intermittent.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8805585 [details]
Bug 1310737 - Urlbar doesn't properly handle %S and POST keywords.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89342/diff/5-6/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74b268465bc7
Urlbar doesn't properly handle %S and POST keywords. r=adw

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/74b268465bc7

Comment 18

[Gerry Chang [:gchang]]

Hi ::mak,
Since this bug is a regression and also affects 51, do you think it's worth uplifting to 51 if this patch is not too risky??

Comment 20

[Alice0775 White]

Yep, the bug 1312018 fixed this problem.

Comment 21

[Marco Bonardo [:mak]]

(In reply to Gerry Chang [:gchang] from comment #18)
> Hi ::mak,
> Since this bug is a regression and also affects 51, do you think it's worth
> uplifting to 51 if this patch is not too risky??

Yes, I plan to request approval for an uplift, I was actually trying to understand if bug 1303333 will be uplifted cause it bitrots this patch. I will ask Alessio today.

Comment 22

[Marco Bonardo [:mak]]

OK, we will wait for bug 1303333 data validation and uplift approval, that should happen on Monday, otherwise we should heavily unbitrot both that patch and this one, since they partly touch the same code. We'll queue up after it.

Comment 23

[Marco Bonardo [:mak]]

Attachment: Aurora patch

NOTE: this also includes the typo fix in bug 1315514 (along with 2 additional tests from there)

Approval Request Comment
[Feature/regressing bug #]: Bug 1306639
[User impact if declined]: A large number of bookmark keywords don't work in the location bar
[Describe test coverage new/current, TreeHerder]: automated tests in Nightly
[Risks and why]: The patch is mostly moving existing code to share it across components and we have a ton of tests on autocomplete. The risk looks acceptable.
[String/UUID change made/needed]: none

This is the unbitrotted patch for Aurora, I'm currently running it through Try, I'll take care of the push by myself.

Comment 24

[Gerry Chang [:gchang]]

Comment on attachment 8809058 [details] [diff] [review]
Aurora patch

This patch fixes a regression related to keyword bookmarks and also includes tests. Take it in 51 aurora.

Comment 25

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/476ff24916373b77e5cc0b1f2658188e3939406d

Comment 26

[Almas Hossain[:mAlmas]]

I have reproduced this bug with Nightly 52.0a1 (2016-10-17) (32-bit)! 

This bug's fix is verified with latest Developer Edition (Aurora) and Beta.

Build ID    :	20170105004018
User Agent  : 	Mozilla/5.0 (Windows NT 10.0; rv:52.0) Gecko/20100101 Firefox/52.0


Build ID    :  	20170105155013
User Agent  :	Mozilla/5.0 (Windows NT 10.0; rv:51.0) Gecko/20100101 Firefox/51.0

 [testday-20170106]

Comment 27

[Bogdan Maris, Desktop QA]

Thanks Almas!