Last Comment Bug 1310737 - Keyword bookmarks don't expand keyword query "%S" or POST data
: Keyword bookmarks don't expand keyword query "%S" or POST data
Status: VERIFIED FIXED
[fxsearch]
: regression
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: Unspecified Unspecified
P1 major with 2 votes (vote)
: Firefox 52
Assigned To: Marco Bonardo [::mak]
:
: Marco Bonardo [::mak]
Mentors:
: 1310953 (view as bug list)
Depends on: 1315514
Blocks: 1126143 1232132 1306639 1313620 1314013
  Show dependency treegraph
 
Reported: 2016-10-17 09:47 PDT by Francesco Lodolo [:flod]
Modified: 2017-01-09 01:54 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
verified
verified

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
Bug 1310737 - Urlbar doesn't properly handle %S and POST keywords. (58 bytes, text/x-review-board-request)
2016-10-28 09:27 PDT, Marco Bonardo [::mak]
adw: review+
Details | Review
Aurora patch (58.29 KB, patch)
2016-11-09 08:58 PST, Marco Bonardo [::mak]
gchang: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Francesco Lodolo [:flod] 2016-10-17 09:47:50 PDT
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 User image Marco Bonardo [::mak] 2016-10-17 10:15:29 PDT
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 2 User image Marco Bonardo [::mak] 2016-10-18 02:42:42 PDT
*** Bug 1310953 has been marked as a duplicate of this bug. ***
Comment 3 User image Marco Bonardo [::mak] 2016-10-18 02:43:48 PDT
we also need to fix the encoding (see duped bug 1310953) but it should come for free once we port getShortcutOrURI
Comment 4 User image Marco Bonardo [::mak] 2016-10-28 09:27:21 PDT Comment hidden (mozreview-request)
Comment 5 User image Marco Bonardo [::mak] 2016-10-28 09:32:31 PDT
Ugh, I forgot bug 1312018. will update the patch and tests for it.
Comment 6 User image Marco Bonardo [::mak] 2016-10-28 10:14:55 PDT Comment hidden (mozreview-request)
Comment 7 User image Drew Willcoxon :adw 2016-10-28 23:34:21 PDT
Looks like there were failures on try.  Does that try push on reviewboard correspond to the latest revision?
Comment 8 User image Marco Bonardo [::mak] 2016-10-29 01:55:21 PDT
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 User image Marco Bonardo [::mak] 2016-10-29 02:57:42 PDT Comment hidden (mozreview-request)
Comment 10 User image Marco Bonardo [::mak] 2016-10-29 09:40:54 PDT Comment hidden (mozreview-request)
Comment 11 User image Marco Bonardo [::mak] 2016-10-29 09:45:09 PDT
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 User image Drew Willcoxon :adw 2016-10-30 21:03:54 PDT
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 User image Marco Bonardo [::mak] 2016-10-31 01:24:33 PDT Comment hidden (mozreview-request)
Comment 14 User image Marco Bonardo [::mak] 2016-10-31 01:26:29 PDT
I actually found a bug in browser_urlbarEnter.js test and thus this is likely to fix an intermittent.
Comment 15 User image Marco Bonardo [::mak] 2016-10-31 01:49:29 PDT Comment hidden (mozreview-request)
Comment 16 User image Pulsebot 2016-10-31 05:35:39 PDT
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/74b268465bc7
Urlbar doesn't properly handle %S and POST keywords. r=adw
Comment 17 User image Phil Ringnalda (:philor) 2016-10-31 18:38:44 PDT
https://hg.mozilla.org/mozilla-central/rev/74b268465bc7
Comment 18 User image Gerry Chang [:gchang] 2016-11-01 23:44:14 PDT
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 19 User image Alice0775 White 2016-11-01 23:59:39 PDT
*** Bug 1312018 has been marked as a duplicate of this bug. ***
Comment 20 User image Alice0775 White 2016-11-02 00:18:03 PDT
Yep, the bug 1312018 fixed this problem.
Comment 21 User image Marco Bonardo [::mak] 2016-11-02 01:49:18 PDT
(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 User image Marco Bonardo [::mak] 2016-11-02 02:08:21 PDT
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 User image Marco Bonardo [::mak] 2016-11-09 08:58:20 PST
Created attachment 8809058 [details] [diff] [review]
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 User image Gerry Chang [:gchang] 2016-11-09 19:33:31 PST
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 26 User image Almas 2017-01-06 02:40:05 PST
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 User image Bogdan Maris, QA [:bogdan_maris] 2017-01-09 01:54:54 PST
Thanks Almas!

Note You need to log in before you can comment on or make changes to this bug.