contentSearchUI.js should not add a beforeunload listener on all about:home pages

RESOLVED FIXED in Firefox 56

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: florian, Assigned: nhnt11)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-performance])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Blocks: 1171344
I wonder if the pagehide event would work here instead.
(Reporter)

Comment 2

2 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.
(Reporter)

Comment 4

2 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

2 years ago
Whiteboard: [photon-performance]
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Flags: qe-verify-
Priority: -- → P1

Comment 5

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e3be5a6ffbcf
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
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.