Closed
Bug 972341
Opened 11 years ago
Closed 10 years ago
document.hidden should be true for xul:browsers in hidden windows
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: ttaubert, Assigned: ttaubert)
References
Details
Attachments
(6 files, 15 obsolete files)
(deleted),
patch
|
smaug
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
markh
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
document.hidden returns false for <xul:browser> elements loaded in hidden windows. It would be great if that returned false as expected and would then emit visibility change events after the docShell has been swapped into a visible tab.
Assignee | ||
Comment 1•11 years ago
|
||
While this works fine I expect that we can't do this as it would break in e10s, no? If we have a <xul:browser remote="true">, would we need to send messages? I would also like to write tests once I have some more feedback on the approach.
Attachment #8375476 -
Flags: feedback?(bzbarsky)
Assignee | ||
Comment 2•11 years ago
|
||
I may be jumping the gun here but with document.hidden working correctly for browsers in the hidden window we can simplify some changes from 910036.
Attachment #8375487 -
Flags: review?(adw)
Comment 3•11 years ago
|
||
Comment on attachment 8375487 [details] [diff] [review] 0002-Bug-972341-Simplify-changes-from-bug-910036-to-use-d.patch Review of attachment 8375487 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: browser/base/content/newtab/page.js @@ +18,5 @@ > > // Listen for 'unload' to unregister this page. > addEventListener("unload", this, false); > > + // Listen for visibility changes to start capturing missing thumbnails. We listen for only one visibility change, so our intent here would be made clearer by making "changes" singular. "Wait for the document to become visible..." would make it clearer still. @@ +143,5 @@ > aEvent.stopPropagation(); > } > break; > + case "visibilitychange": > + if (!document.hidden) { Since the listener is only added when document.hidden and it's removed here, will this conditional ever be false?
Attachment #8375487 -
Flags: review?(adw) → review+
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Drew Willcoxon :adw from comment #3) > We listen for only one visibility change, so our intent here would be made > clearer by making "changes" singular. "Wait for the document to become > visible..." would make it clearer still. Agreed, that's better. > Since the listener is only added when document.hidden and it's removed here, > will this conditional ever be false? No, good catch. Thanks!
![]() |
||
Comment 5•11 years ago
|
||
Comment on attachment 8375476 [details] [diff] [review] 0001-Bug-972341-document.hidden-should-be-true-for-xul-br.patch Do we want to only affect the document visibility state? Or should the docshell itself be marked as inactive? That is, do we want to only affect the visibility API or also freeze the refresh driver, mark the presshell as inactive, etc, etc? Oh, and also correctly propagate to out-of-process descendants. And it would automatically pick up the right state when swapping in/out of kids, since moving docshells around in the tree grab's the parent's active state... Seems to me like we should simply set the root docshell of the hidden window as inactive; I'm a bit surprised we're not doing that already.
![]() |
||
Comment 6•11 years ago
|
||
In fact, somewhere in nsAppShellService::CreateHiddenWindowHelper we could take the bit that grabs the docshell, move it out of the if/else bit and SetIsActive(false) there. The only worry is whether this will cause issues on Mac with the menu, I guess.
![]() |
||
Updated•11 years ago
|
Attachment #8375476 -
Flags: feedback?(bzbarsky)
Do we slow down non-refresh-driver timers? That could screw up the SocialAPI's frame worker.
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #5) > That is, do we want to only affect the visibility API or also freeze the > refresh driver, mark the presshell as inactive, etc, etc? Oh, and also > correctly propagate to out-of-process descendants. And it would > automatically pick up the right state when swapping in/out of kids, since > moving docshells around in the tree grab's the parent's active state... I think we only want to affect the visibility API to let pages know they are not visible. Marking the docShell as inactive would probably screw up the SocialAPI (as Kyle said), the about:newtab preloader that preloads pages in the hidden window and the OOP thumbnail creation that as well maintains a browser in the hidden window. Now that I think about it, could this change possible screw up thumbnails because pages behave differently with hidden=true? Possibly, although I wouldn't really worry too much about that.
![]() |
||
Comment 10•11 years ago
|
||
> I think we only want to affect the visibility API to let pages know they are not visible.
That seems really really hacky to me, honestly...
If you do want to do it that way, you need a reliable way to know when you're entering/leaving the "hidden window", however that's defined.
Preloaded pages _should_ be marked as hidden in the docshell sense, imo, as should thumbnails. You don't want those spamming the refresh driver and event loop, I would think, just like you don't want background tabs doing it.
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8375487 -
Attachment is obsolete: true
Attachment #8376826 -
Flags: review+
Assignee | ||
Comment 12•11 years ago
|
||
This patch makes CreateHiddenWindowHelper() grab the hidden window's docShell and set it inactive. It also introduces DocShell::SetIsActiveInternal() to allow setting the active state of chrome docshells because I thought we probably still wouldn't want to allow access to that from JavaScript.
Attachment #8375476 -
Attachment is obsolete: true
Attachment #8376831 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 13•11 years ago
|
||
This second part takes care of passing the parent docShell's active state to remote browsers.
Attachment #8376832 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 14•11 years ago
|
||
Tests are missing currently but I would get started on those after having some more feedback. From some quick local test runs it looks like the social code and thumbnailing still work. I can't see anything wrong with the Mac menu either so far. How can we check that those won't break? I will push to try either way.
Assignee | ||
Comment 15•11 years ago
|
||
Another note: I did some smoke tests to check whether the preloader and OOP thumbnailing have inactive docShells and that works fine with the attached patches.
Assignee | ||
Comment 16•11 years ago
|
||
Try looks good, except for Jetpack tests. I wonder if we should allow activating chrome docShells but not disabling them. So the social frame worker and Jetpack could explicitly opt into active docShells in the hidden window. Or maybe they could do without that with a little rewrite. https://tbpl.mozilla.org/?tree=Try&rev=4583966ff8cc
![]() |
||
Comment 17•11 years ago
|
||
Comment on attachment 8376831 [details] [diff] [review] 0001-Bug-972341-Mark-a-hidden-window-s-docShell-as-inacti.patch Please add the missing space before "void" and mark the method as notxpcom as well, and r=me.
Attachment #8376831 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 18•11 years ago
|
||
Comment on attachment 8376832 [details] [diff] [review] 0002-Bug-972341-Propagate-docShell.isActive-to-remote-bro.patch Why is this using the root item, not parentAsItem? Also, now that I look again I don't see anything that would propagate the active state to the kid if it changes dynamically... am I just missing it?
Attachment #8376832 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #8376831 -
Attachment is obsolete: true
Attachment #8378220 -
Flags: review+
Assignee | ||
Comment 21•11 years ago
|
||
What would the best way be to keep remote browsers in the loop about changes to their parent docShells' active state? My first thought was to add mRemoteBrowser as a nsIDocShellTreeItem to the tree and handle those in SetIsActiveInternal() but unfortunately SetIsActiveInternal() uses the nsDocLoader child list. In the future we may want to propagate changes to the allow* flags as well like we do for non-remote docShells currently. So either we find something that works for all of those or defer this to a later moment.
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 22•11 years ago
|
||
> What would the best way be to keep remote browsers in the loop about changes to their
> parent docShells' active state?
Probably having the frameloader explicitly tell the parent docshell about remote browsers or something; some sort of add/remove operation to parallel what frameloader does with a docshell child. Then we can propagate whatever state we want in the add?
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 23•11 years ago
|
||
Ok, so you say we should maintaining a separate list of remote browsers in the docShell. That should work, thanks.
Assignee | ||
Comment 24•11 years ago
|
||
Will work on tests in a separate patch.
Attachment #8378280 -
Attachment is obsolete: true
Attachment #8378581 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 25•11 years ago
|
||
It's probably better to exclude browser and app frames from this visibility propagation. It unbreaks mochitest-2 and doesn't touch b2g that way.
Attachment #8378581 -
Attachment is obsolete: true
Attachment #8378581 -
Flags: review?(bzbarsky)
Attachment #8379022 -
Flags: review?(bzbarsky)
Comment 26•11 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #16) > Try looks good, except for Jetpack tests. I wonder if we should allow > activating chrome docShells but not disabling them. So the social frame > worker and Jetpack could explicitly opt into active docShells in the hidden > window. Or maybe they could do without that with a little rewrite. Looks like the latest patch doesn't do this? I'm not clear on what effect the checks from comment 25 have (i.e. whether OwnerIsBrowserOrAppFrame() is true for the jetpack/social frames). Makes me nervous to have jetpack and social docshells be marked "inactive", since those are used for functionality that if often exposed externally in some other way (e.g. it could be bad if a setInterval in the frameworker script is throttled). I don't think we have good test coverage for this, unfortunately...
Assignee | ||
Comment 27•11 years ago
|
||
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #26) > (In reply to Tim Taubert [:ttaubert] from comment #16) > > Try looks good, except for Jetpack tests. I wonder if we should allow > > activating chrome docShells but not disabling them. So the social frame > > worker and Jetpack could explicitly opt into active docShells in the hidden > > window. Or maybe they could do without that with a little rewrite. > > Looks like the latest patch doesn't do this? I'm not clear on what effect > the checks from comment 25 have (i.e. whether OwnerIsBrowserOrAppFrame() is > true for the jetpack/social frames). Yeah, the latest patch still breaks JP tests. I should have mentioned that I don't plan to land it in its current version without addressing test failures, of course. > Makes me nervous to have jetpack and social docshells be marked "inactive", > since those are used for functionality that if often exposed externally in > some other way (e.g. it could be bad if a setInterval in the frameworker > script is throttled). I don't think we have good test coverage for this, > unfortunately... Yeah, I kind of figured that our test suite might not expose problems with the patch. I thought about having an attribute that enables (or disables) visibility propagation that we could use to keep jetpack and social behavior as is. I'd rather have one that disables it so that new features run into and handle it properly when being built.
Assignee | ||
Comment 28•11 years ago
|
||
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #26) > Makes me nervous to have jetpack and social docshells be marked "inactive", > since those are used for functionality that if often exposed externally in > some other way (e.g. it could be bad if a setInterval in the frameworker > script is throttled). I don't think we have good test coverage for this, > unfortunately... This patch introduces docShell.setForceActive(bool) which can be used to force an active docShell no matter what its parent state is. The patch changes the social frame worker to use that, adapting Jetpack is easy as well.
Assignee | ||
Comment 29•11 years ago
|
||
This patch fixes a11y m-oth tests using docShell.setForceActive(true) for the hiddenWindow docShells. We don't create accessibles for inactive docShells so we need to provide what the test assumes. https://tbpl.mozilla.org/?tree=Try&rev=afc40fad48e0
Assignee | ||
Comment 30•11 years ago
|
||
I have a patch that fixes the Jetpack failures by using docShell.setForceActive(true) when creating a new frame in the hidden window. I'll need to figure out how to submit patches to Jetpack and how to run those on try.
Assignee | ||
Comment 31•11 years ago
|
||
Re-based and changed a few things since bug 978540 landed.
Attachment #8379022 -
Attachment is obsolete: true
Attachment #8379022 -
Flags: review?(bzbarsky)
Attachment #8384611 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 32•11 years ago
|
||
Simple rebase.
Attachment #8380362 -
Attachment is obsolete: true
Attachment #8380362 -
Flags: review?(bzbarsky)
Attachment #8384612 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 33•11 years ago
|
||
Comment on attachment 8384611 [details] [diff] [review] 0002-Bug-972341-Propagate-docShell.isActive-to-remote-bro.patch, v5 I just noticed that Boris says he's overloaded with reviews.
Attachment #8384611 -
Flags: review?(bugs)
Assignee | ||
Updated•11 years ago
|
Attachment #8384612 -
Flags: review?(bugs)
![]() |
||
Comment 34•11 years ago
|
||
This review is one of the main reasons I say that. I do plan to get to it today or worst-case tomorrow.
Comment 35•11 years ago
|
||
Comment on attachment 8384611 [details] [diff] [review] 0002-Bug-972341-Propagate-docShell.isActive-to-remote-bro.patch, v5 This doesn't deal with frameloader swapping in any way. Also, I don't understand why !OwnerIsBrowserOrAppFrame() checks?
Attachment #8384611 -
Flags: review?(bugs) → review-
Updated•11 years ago
|
Attachment #8384612 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 36•11 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #35) > This doesn't deal with frameloader swapping in any way. Does it have to? SwapWithOtherLoader() just bails out if GetExistingDocShell() returns null which AFAICT means we don't support docShell swapping for remote browsers. https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#1074 > Also, I don't understand why !OwnerIsBrowserOrAppFrame() checks? The main purpose of this patch was to correct behavior for Firefox and its tabbrowsers. B2G has its own mechanism of propagating visibility to mozbrowser iframes when switching applications and this would probably break with correct propagation. I think this would be best addressed in a follow-up. I don't know enough about standalone app frames to say whether those would break as well, so I excluded them here.
Comment 37•11 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #36) > (In reply to Olli Pettay [:smaug] from comment #35) > > This doesn't deal with frameloader swapping in any way. > > Does it have to? SwapWithOtherLoader() just bails out if > GetExistingDocShell() returns null which AFAICT means we don't support > docShell swapping for remote browsers. > > https://mxr.mozilla.org/mozilla-central/source/content/base/src/ > nsFrameLoader.cpp#1074 Oh, crap. That is broken. Let's fix that in a followup. Filed Bug 979239. > > Also, I don't understand why !OwnerIsBrowserOrAppFrame() checks? > > The main purpose of this patch was to correct behavior for Firefox and its > tabbrowsers. B2G has its own mechanism of propagating visibility to > mozbrowser iframes when switching applications and this would probably break > with correct propagation. Why would it? I don't assume b2g to use isActive stuff. Didn't find any use of it in b2g/ > I think this would be best addressed in a > follow-up. I don't know enough about standalone app frames to say whether > those would break as well, so I excluded them here. I'm worried about adding inconsistencies to the platform, which may never get fixed. I've tried to get rid of use of OwnerIsBrowserOrAppFrame() checks.
Assignee | ||
Comment 38•11 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #37) > Why would it? I don't assume b2g to use isActive stuff. > Didn't find any use of it in b2g/ https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#881 > I'm worried about adding inconsistencies to the platform, which may never > get fixed. > I've tried to get rid of use of OwnerIsBrowserOrAppFrame() checks. Fair enough, I don't like the special casing of that either.
![]() |
||
Comment 39•11 years ago
|
||
Comment on attachment 8384611 [details] [diff] [review] 0002-Bug-972341-Propagate-docShell.isActive-to-remote-bro.patch, v5 I guess Olli is on top of this at this point. I'm sorry for the lag... :(
Attachment #8384611 -
Flags: review?(bzbarsky)
![]() |
||
Updated•11 years ago
|
Attachment #8384612 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 40•10 years ago
|
||
This patch is quite similar to the previous ones but it does two more things: 1) I removed the OwnerIsBrowserOrAppFrame() checks from the nsFrameLoader. TabChild::RecvSetIsDocShellActive() however still checks it and just does nothing for browser or app frames. B2G has its very own way of dealing with docShell active states and it would be great to fix that in a follow-up rather to have it block this bug. 2) nsDocShell::SetIsActive() does now allow changing a chrome docShell's active state to true. That is a great way to allow the social worker and the add-on SDK to force active docShells. I assume the condition was built in to prevent chrome docShells from becoming inactive.
Attachment #8376826 -
Attachment is obsolete: true
Attachment #8378220 -
Attachment is obsolete: true
Attachment #8380609 -
Attachment is obsolete: true
Attachment #8384611 -
Attachment is obsolete: true
Attachment #8384612 -
Attachment is obsolete: true
Attachment #8424512 -
Flags: review?(bugs)
Assignee | ||
Comment 42•10 years ago
|
||
Forcing social frame worker docShells to be active.
Attachment #8424514 -
Flags: review?(mhammond)
Assignee | ||
Comment 43•10 years ago
|
||
Forcing add-on SDK frames to be active.
Attachment #8424516 -
Flags: review?(dtownsend+bugmail)
Assignee | ||
Comment 44•10 years ago
|
||
Updated patch to simplify some about:newtab code.
Attachment #8424517 -
Flags: review?(adw)
Updated•10 years ago
|
Attachment #8424514 -
Flags: review?(mhammond) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8424516 -
Attachment is obsolete: true
Attachment #8424516 -
Flags: review?(dtownsend+bugmail)
Assignee | ||
Updated•10 years ago
|
Attachment #8424512 -
Attachment is obsolete: true
Attachment #8424512 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #8424517 -
Attachment is obsolete: true
Attachment #8424517 -
Flags: review?(adw)
Assignee | ||
Updated•10 years ago
|
Attachment #8424516 -
Attachment is obsolete: false
Attachment #8424516 -
Flags: review?(dtownsend+bugmail)
Assignee | ||
Updated•10 years ago
|
Attachment #8424512 -
Attachment is obsolete: false
Attachment #8424512 -
Flags: review?(bugs)
Assignee | ||
Comment 45•10 years ago
|
||
This should fix a11y m-oth try failures.
Attachment #8424747 -
Flags: review?(trev.saunders)
Assignee | ||
Comment 46•10 years ago
|
||
Forgot to remove an occurrence of |allowBackgroundCaptures| which caused try timeouts.
Attachment #8424764 -
Flags: review?(adw)
Assignee | ||
Comment 47•10 years ago
|
||
Comment on attachment 8424747 [details] [diff] [review] 0006-Bug-972341-Wait-until-hidden-frames-are-ready-before.patch Argh, those fragile tests drive me nuts. Still failing on try :(
Attachment #8424747 -
Flags: review?(trev.saunders)
Comment 48•10 years ago
|
||
Comment on attachment 8424512 [details] [diff] [review] 0001-Bug-972341-Propagate-docShell-active-states-to-remot.patch This really need to be split to two pieces. Or setIsActiveInternal should be removed. It is really odd to see such method in a public interface. Why would we ever want to set chrome docshell inactive? It would be odd to see chrome setTimeout to not support < 1s values.
Attachment #8424512 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 49•10 years ago
|
||
This looks quite promising. We don't need to care about cleaning up as a11y tests have their own test run and the browser will restart before running all other mochitests.
Attachment #8424747 -
Attachment is obsolete: true
Assignee | ||
Comment 50•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #48) > This really need to be split to two pieces. Fair enough, I can totally split this into smaller parts. > Why would we ever want to set chrome docshell inactive? > It would be odd to see chrome setTimeout to not support < 1s values. When I started to work on this issue I tried to make document.hidden return true just based on the fact that it belonged to the hidden window. In comment #5 Boris pointed out that the right solution would be to mark the docShell as actually hidden. What's the suggested way forward now?
Comment 51•10 years ago
|
||
Oh, I'm not actually sure what bz means with root docshell there. Root docshell in the child process (that would be a content docshell - at least usually), or root docshell in the parent process. That would be a chrome docshell. Making a chrome docshell to not have normal timeout handling etc. would be rather regression prone.
Flags: needinfo?(bzbarsky)
Comment 52•10 years ago
|
||
Anyhow, I'd make the root content docshell active/inactive (doesn't matter whether the docshell is in parent or child process)
![]() |
||
Comment 53•10 years ago
|
||
> Making a chrome docshell to not have normal timeout handling etc. would be rather > regression prone. Sure. I was specifically talking about the hidden window. > I'd make the root content docshell active/inactive That becomes the responsibility of everyone injecting junk into the hidden window then, right?
Flags: needinfo?(bzbarsky)
Comment 54•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #53) > That becomes the responsibility of everyone injecting junk into the hidden > window then, right? I don't actually remember whether we use chrome or content docshell as the toplevel shell for hidden window, but anyhow, making something inactive there should be, IMO, responsibility of the user of the hidden window.
![]() |
||
Comment 55•10 years ago
|
||
Then why do we need any core changes at all? Users of the hidden window can already do that, right?
Assignee | ||
Comment 56•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #55) > Then why do we need any core changes at all? Users of the hidden window can > already do that, right? Yeah, that way around it would have been a lot easier :| I'll move part 0005 to bug 973532 and wontfix this one. Thanks for clearing this up.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Assignee | ||
Updated•10 years ago
|
Attachment #8424516 -
Flags: review?(dtownsend+bugmail)
Assignee | ||
Updated•10 years ago
|
Attachment #8424764 -
Flags: review?(adw)
Assignee | ||
Comment 57•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #54) > (In reply to Boris Zbarsky [:bz] from comment #53) > > That becomes the responsibility of everyone injecting junk into the hidden > > window then, right? > > I don't actually remember whether we use chrome or content docshell as the > toplevel shell for hidden window, but anyhow, making something inactive > there should be, IMO, responsibility of the user of the > hidden window. Thinking a little about this I agree even more that this is the right solution. There are so many users of the hidden window and making all of their docShells inactive could have unintended consequences. Having everyone be responsible for their own docShells makes a lot of sense.
Assignee | ||
Comment 58•10 years ago
|
||
Olli, are we still interested in forwarding docShell activity state changes to remote browsers? Should I split that off into a new bug? The attached patches should implement that already.
Flags: needinfo?(bugs)
Comment 59•10 years ago
|
||
Having some easy way for parent to in-activate child-process docshells sounds useful.
Flags: needinfo?(bugs)
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•