Closed
Bug 1381659
Opened 7 years ago
Closed 7 years ago
contentSearchUI.js should not add a beforeunload listener on all about:home pages
Categories
(Firefox :: Search, enhancement, P1)
Firefox
Search
Tracking
()
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: florian, Assigned: nhnt11)
References
Details
(Whiteboard: [photon-performance])
Attachments
(1 file)
http://searchfox.org/mozilla-central/rev/88180977d79654af025158d8ebeb8c2aa11940eb/browser/base/content/contentSearchUI.js#754 // If a search is loaded in the same tab, ensure the suggestions dropdown // is hidden immediately when the page starts loading and not when it first // appears, in order to provide timely feedback to the user. window.addEventListener("beforeunload", () => { this._hideSuggestions(); }); beforeunload event listeners are very expensive, because they require exchanging a message between the parent and content processes before we can close a tab. They also disable the bfcache. If possible, this code should be changed to avoid relying on this event. If that's not possible, this listener should be added only when the suggestions panel is open, and removed immediately after it's closed. Also, the whole _makeTable function looks like it could be called lazily the first time we show the panel, instead of immediately when loading the page.
Comment 1•7 years ago
|
||
I wonder if the pagehide event would work here instead.
Reporter | ||
Comment 2•7 years ago
|
||
(In reply to Mike Conley (:mconley) - Digging out of needinfo / review backlog. from comment #1) > I wonder if the pagehide event would work here instead. That seems to happen too late. The purpose of this code is to hide the suggestion panel immediately if the click in it triggers a load in the same tab, but to keep the panel open if the search is happening in a new background tab (eg. when using cmd+click). Unfortunately the code in the content process doesn't know about this, it's decided by the code at http://searchfox.org/mozilla-central/rev/88180977d79654af025158d8ebeb8c2aa11940eb/browser/modules/ContentSearch.jsm#231 in the parent process.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8887652 [details] Bug 1381659 - Send a message from ContentSearch.jsm to blur the suggestions UI after a search instead of a beforeunload listener. https://reviewboard.mozilla.org/r/158544/#review163852 Thanks! :-) ::: browser/modules/ContentSearch.jsm:241 (Diff revision 1) > // where === "current"), openUILinkIn will not work because that tab is no > // longer the current one. For this case we manually load the URI. > if (where === "current") { > + // Since we're going to load the search in the same browser, blur the search > + // UI to prevent further interaction before we start loading. > + this._reply(msg, "Blur"); I wonder if we should make this message name describe what's happening, eg. 'SearchResultsWillReplacePage' (can't really come up with a good short but descriptive name)... but I don't really care, so "Blur" is ok I guess.
Attachment #8887652 -
Flags: review?(florian) → review+
Reporter | ||
Updated•7 years ago
|
Whiteboard: [photon-performance]
Updated•7 years ago
|
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Flags: qe-verify-
Priority: -- → P1
Pushed by nhnt11@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e3be5a6ffbcf Send a message from ContentSearch.jsm to blur the suggestions UI after a search instead of a beforeunload listener. r=florian
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e3be5a6ffbcf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Updated•7 years ago
|
Iteration: --- → 56.3 - Jul 24
You need to log in
before you can comment on or make changes to this bug.
Description
•