Closed
Bug 1096534
Opened 10 years ago
Closed 10 years ago
ContentSearch should load the search URL in the browser/tab sending the search message, not the current/selected browser in the top-level chrome window
Categories
(Firefox :: Search, defect)
Tracking
()
People
(Reporter: adw, Assigned: adw, Mentored)
References
Details
Attachments
(1 file, 1 obsolete file)
6.03 KB,
patch
|
adw
:
review+
Sylvestre
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1093571 +++
On about:newtab:
1. Choose a search engine that provides suggestions, like Google
2. In the search box, quickly type gibberish and press enter
3. Quickly open a new tab (Ctrl/Cmd+T)
If you're quick enough and the search suggestions take long enough to come in, then the search results URL loads in the newly opened tab. It should load in the about:newtab tab.
ContentSearch._onMessageSearch has access to the xul:browser that requested the search (via msg.target), but instead it gets the top-level chrome window and uses it to load the URL (browserWin.loadURI). That's dumb. It should call loadURI on msg.target or something equivalent instead. This should be pretty simple to fix.
http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm?rev=6818a0733662#208
Flags: firefox-backlog+
Updated•10 years ago
|
Flags: qe-verify+
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 2•10 years ago
|
||
This is an embarrassing bug with a quick fix so I hope it's OK that I worked on it.
In case it's not clear, the reason for the `loaded` bool is to avoid silently catching errors thrown by recordSearchInHealthReport. We definitely want to spot those.
This patch also fixes a JS strict warning introduced by bug 1036908:
JavaScript strict warning: resource:///modules/ContentSearch.jsm, line 211: ReferenceError: reference to undefined property data.selection
Updated•10 years ago
|
Iteration: --- → 37.1
Comment 3•10 years ago
|
||
Comment on attachment 8528525 [details] [diff] [review]
patch
Review of attachment 8528525 [details] [diff] [review]:
-----------------------------------------------------------------
I'd usually request a test (or to modify the existing test making it open a new tab when a search starts), but looks like in this case it may not be easily predictable the order in which open tab and search messages are handled in e10s mode?
::: browser/modules/ContentSearch.jsm
@@ +219,5 @@
> + // message and the time that we received it. In that case, trying to call
> + // any method on it will throw.
> + }
> + if (loaded) {
> + let browser = msg.target.ownerDocument.defaultView;
nit: Just for code clarity, you could let browser = msg.target; at the top, and let win = browser.ownerDoc... here
@@ +222,5 @@
> + if (loaded) {
> + let browser = msg.target.ownerDocument.defaultView;
> + browser.BrowserSearch.recordSearchInHealthReport(engine, data.whence,
> + data.selection || null);
> + }
I think it's simpler if you early "return Promise.resolve();" in the above catch... then you can avoid the loaded boolean.
Attachment #8528525 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 5•10 years ago
|
||
Thanks, Marco. Landed with a test and comments addressed.
https://hg.mozilla.org/integration/fx-team/rev/945080e133d2
Attachment #8528525 -
Attachment is obsolete: true
Attachment #8533273 -
Flags: review+
Comment 6•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Updated•10 years ago
|
QA Contact: petruta.rasa
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8533273 [details] [diff] [review]
checked-in patch with test
Approval Request Comment
[Feature/regressing bug #]:
Bug 962490, search box in about:newtab
[User impact if declined]:
This bug has been present since 31, but it became much easier to trigger after search suggestions were added to about:newtab in 34 (bug 1028985). So so far, this bug has been easy to trigger in only one release. If we let the fix ride the trains instead of uplifting it, the bug will be easy to trigger in the 35 and 36 releases, too.
[Describe test coverage new/current, TBPL]:
The fix comes with a new browser-chrome test. Searching on about:newtab was already covered by existing related automated tests.
[Risks and why]:
Moderate risk given that this touches code related to searching on about:newtab, but the risk is mitigated by automated test coverage. I ran the new test against both Aurora and Beta before requesting uplift approval.
[String/UUID change made/needed]:
None
Attachment #8533273 -
Flags: approval-mozilla-beta?
Attachment #8533273 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8533273 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 8•10 years ago
|
||
Flags: in-testsuite+
Updated•10 years ago
|
Attachment #8533273 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 9•10 years ago
|
||
Comment 10•10 years ago
|
||
Reproduced using Firefox 34.0 and DuckDuckGo as default search engine.
Verified as fixed using Firefox 35 beta 3, latest Dev Edition 36.0a2 and latest Nightly 37.0a1 2014-12-12 under Win 7 64-bit, Ubuntu 14.04 32-bit and Mac OSX 10.9.5.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•