Closed Bug 1470002 Opened 6 years ago Closed 6 years ago

Remove PageVisibility:Hide/Show from the tree

Categories

(Firefox :: General, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

We removed social API, but the pagehide listener in content.js stuck around and is still broadcasting. There's also a mention in devtools somehow. We should nuke this stuff from orbit.
SocialAPI did have a pagehide/show event, but it's not PageVisibility[1].  Are you clear which one you're looking at?

[1] https://developer.mozilla.org/en-US/docs/Web/API/Page_Visibility_API
Flags: needinfo?(gijskruitbosch+bugs)
I mean:

https://dxr.mozilla.org/mozilla-central/search?q=pagevisibility%3Ahide&redirect=false

which was added in:

https://hg.mozilla.org/mozilla-central/rev/5467a8d9d844

which was bug 1257723. So yeah, something to do with social, maybe "Social API" was the wrong social? The receiving code was in browser-social.js in any case... :-)
Flags: needinfo?(gijskruitbosch+bugs)
That was socialapi.  However it also looks like other things use this via a PageVisibility:Show listener in browser.js.  Other things seem to rely on using a pageshow eventlistener directly, so that should be changeable.
(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> That was socialapi.  However it also looks like other things use this via a
> PageVisibility:Show listener in browser.js.  Other things seem to rely on
> using a pageshow eventlistener directly, so that should be changeable.

Yeah, I think we don't need the :Show thing either, but I'm currently struggling to fix up some devtools responsive mode tests that have managed to start relying on it...
Comment on attachment 8986714 [details]
Bug 1470002 - remove pageshow message passing, too,

https://reviewboard.mozilla.org/r/252008/#review258422

::: browser/base/content/browser.js
(Diff revision 1)
> -    // This pageshow listener needs to be registered before we may call
> -    // swapBrowsersAndCloseOther() to receive pageshow events fired by that.
> -    window.messageManager.addMessageListener("PageVisibility:Show", function(message) {

We also call asyncUpdateUI from the tabbrowser's locationchange handling, so I don't think this is needed at all. Certainly I couldn't find a case where either the RSS subscribe button or the search engine listing in the page action menu became confused when switching tabs, loading pages, or detaching/re-attaching tabs (which would call swapBrowsersAndCloseOther). So I just removed it. Drew, maybe you know of something I missed?
Comment on attachment 8986713 [details]
Bug 1470002 - remove pagehide message passing from browser code,

https://reviewboard.mozilla.org/r/252006/#review258484
Attachment #8986713 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8986714 [details]
Bug 1470002 - remove pageshow message passing, too,

https://reviewboard.mozilla.org/r/252008/#review258552

Thanks for doing this cleanup! Sorry you had to peer into the void of RDM... :S

::: devtools/client/responsive.html/test/browser/head.js:305
(Diff revision 1)
> -      if (message.target != browser) {
> -        return;
> -      }
> -      mm.removeMessageListener("PageVisibility:Show", onShow);
> -      resolve();
> -    };
> +    browser = ui.getViewportBrowser();
> +  }
> +  info("Waiting for pageshow from " + (ui ? "responsive" : "regular") + " browser");
> +  // Need to wait an extra tick after pageshow to ensure everyone is up-to-date,
> +  // hence the executeSoon.
> +  return BrowserTestUtils.waitForContentEvent(browser, "pageshow")

If you like, you could make `waitForPageShow` an async function and then do:

```
await BrowserTestUtils.waitForContentEvent(browser, "pageshow");
return waitForTick();
```

(waitForTick[1] is a DevTools test helper for executeSoon.)

[1]: https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/devtools/client/shared/test/shared-head.js#360
Attachment #8986714 - Flags: review?(jryans) → review+
Comment on attachment 8986714 [details]
Bug 1470002 - remove pageshow message passing, too,

https://reviewboard.mozilla.org/r/252008/#review258574
Attachment #8986714 - Flags: review?(adw) → review+
Yeah, as far as I know the page actions and RSS button don't rely on this, so this looks good to me [Argh, I left this comment in the review, not sure why it didn't show up when I published it.]
Priority: -- → P1
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6eda16d80314
remove pagehide message passing from browser code, r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/0a78d0ffe366
remove pageshow message passing, too, r=adw,jryans
https://hg.mozilla.org/mozilla-central/rev/6eda16d80314
https://hg.mozilla.org/mozilla-central/rev/0a78d0ffe366
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: