Closed
Bug 1310737
Opened 7 years ago
Closed 7 years ago
Keyword bookmarks don't expand keyword query "%S" or POST data
Categories
(Firefox :: Bookmarks & History, defect, P1)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 52
Tracking | Status | |
---|---|---|
firefox49 | --- | unaffected |
firefox50 | --- | unaffected |
firefox51 | --- | verified |
firefox52 | --- | verified |
People
(Reporter: flod, Assigned: mak)
References
(Depends on 1 open bug)
Details
(Keywords: regression, Whiteboard: [fxsearch])
Attachments
(2 files)
58 bytes,
text/x-review-board-request
|
adw
:
review+
|
Details |
58.29 KB,
patch
|
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•7 years ago
|
||
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).
Blocks: 1306639
Keywords: regression
Assignee | ||
Updated•7 years ago
|
Priority: -- → P1
Whiteboard: [fxsearch]
Assignee | ||
Updated•7 years ago
|
Summary: Keyword bookmarks don't expand keyword query "%S" → Keyword bookmarks don't expand keyword query "%S" or POST data
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
status-firefox49:
--- → unaffected
status-firefox50:
--- → unaffected
status-firefox51:
--- → affected
Assignee | ||
Comment 3•7 years ago
|
||
we also need to fix the encoding (see duped bug 1310953) but it should come for free once we port getShortcutOrURI
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
Ugh, I forgot bug 1312018. will update the patch and tests for it.
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
Looks like there were failures on try. Does that try push on reviewboard correspond to the latest revision?
Assignee | ||
Comment 8•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
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•7 years ago
|
||
mozreview-review |
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.
Attachment #8805585 -
Flags: review?(adw) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
I actually found a bug in browser_urlbarEnter.js test and thus this is likely to fix an intermittent.
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/74b268465bc7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment 18•7 years ago
|
||
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??
Flags: needinfo?(mak77)
![]() |
||
Comment 20•7 years ago
|
||
spum |
Yep, the bug 1312018 fixed this problem.
Assignee | ||
Comment 21•7 years ago
|
||
(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.
Flags: needinfo?(mak77)
Assignee | ||
Comment 22•7 years ago
|
||
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.
Flags: needinfo?(mak77)
Assignee | ||
Comment 23•7 years ago
|
||
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.
Attachment #8809058 -
Flags: approval-mozilla-aurora?
Comment 24•7 years ago
|
||
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.
Attachment #8809058 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 25•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/476ff24916373b77e5cc0b1f2658188e3939406d
Comment 26•7 years ago
|
||
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•7 years ago
|
||
Thanks Almas!
You need to log in
before you can comment on or make changes to this bug.
Description
•