Closed
Bug 1470002
Opened 6 years ago
Closed 6 years ago
Remove PageVisibility:Hide/Show from the tree
Categories
(Firefox :: General, enhancement, P1)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 63
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.
Comment 1•6 years ago
|
||
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)
Assignee | ||
Comment 2•6 years ago
|
||
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)
Comment 3•6 years ago
|
||
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.
Assignee | ||
Comment 4•6 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
mozreview-review |
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 8•6 years ago
|
||
mozreview-review |
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 9•6 years ago
|
||
mozreview-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 10•6 years ago
|
||
mozreview-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+
Comment 11•6 years ago
|
||
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.]
Updated•6 years ago
|
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•6 years ago
|
||
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
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6eda16d80314 https://hg.mozilla.org/mozilla-central/rev/0a78d0ffe366
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•