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)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 56
Iteration:
56.3 - Jul 24
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.
Blocks: 1171344
I wonder if the pagehide event would work here instead.
(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 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+
Whiteboard: [photon-performance]
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
https://hg.mozilla.org/mozilla-central/rev/e3be5a6ffbcf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Iteration: --- → 56.3 - Jul 24
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: