666365 - Figure out what TabChild::SetVisibility needs to do

Status: NEW

(Core :: DOM: Core & HTML, defect, P3)

Description

[:Felipe Gomes (needinfo for replies!)]

Currently it's called but implemented as NS_NOTREACHED. This is similar to Bug 617804.

Comment 1

[Chris Peterson [:cpeterson]]

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Comment 2

[Mike Conley (:mconley) (:⚙️)]

So this has been a bit of a trip.

TabChild implements nsIEmbeddingSiteWindow, which means that it implements the visibility attribute. Originally, I believe that attribute was exposed to embedders to allow them to hide and show the embedded web content easily, and to read the hidden / shown state.

So that seems to be why TabChild has it.

From here on out, I was interested in who is supposed to call SetVisibility on things. Specifically, I care about who calls SetVisibility(false), since this is a thing that TabChild does not support (it's current implementation makes it so that we ignore SetVisibility, and always return true for GetVisibility).

nsDocShellTreeOwner is apparently the one who makes calls to SetVisibility for nsIEmbeddingSiteWindow's, and it's really just forwarding calls to its own SetVisibility. Yes, nsDocShellTreeOwner gets a visibility attribute as well, by way of the nsIBaseWindow interface.

So the question is: who normally calls SetVisibility(false) on an nsDocShellTreeOwner? DXR reports this list as quite small:

dom/base/nsFocusManager.cpp
691     baseWindow->SetVisibility(true);

dom/base/nsFrameLoader.cpp
705     baseWindow->SetVisibility(true);
861     baseWin->SetVisibility(false);

dom/ipc/TabChild.cpp
1623     baseWindow->SetVisibility(true);

embedding/components/windowwatcher/nsWindowWatcher.cpp
2181     treeOwnerAsWin->SetVisibility(true);

xpfe/appshell/nsXULWindow.cpp
872     shellAsWin->SetVisibility(aVisibility);

Only one spot - in nsFrameLoader.cpp, on line 861.

That's in the nsFrameLoader::Hide method, here: https://dxr.mozilla.org/mozilla-central/rev/5e0140b6d11821e0c2a2de25bc5431783f03380a/dom/base/nsFrameLoader.cpp#861

And it looks like we intentionally do not support hiding remote frameloaders - we do an early return in that case: https://dxr.mozilla.org/mozilla-central/rev/5e0140b6d11821e0c2a2de25bc5431783f03380a/dom/base/nsFrameLoader.cpp#851

So far, the way I see it - the only reason we'd care about TabChild::SetVisibility is if for some reason, nsFrameLoader::Hide on a remote browser needs to set it to false. And I'm not sure it does.

The list of things that call nsFrameLoader::Hide are small:

dom/base/nsFrameLoader.cpp
748     Hide();

layout/generic/nsSubDocumentFrame.cpp
145     frameloader->Hide();
955     mFrameLoader->Hide();

The first one is part of the initialization of the frame - and it looks like it's supposed to re-hide if ::Hide was called sometime while nsFrameLoader::Show was called.

And the only other times ::Hide is called are from within nsSubDocumentFrame.

I see two places where it’s called from within nsSubDocumentFrame. The first is in ::Init, where it looks like we do this if the subframe was reframed into a new document somehow… and the presentation is different, so we hide it. Presumably to show it once AsyncFrameInit has run, which calls ShowViewer, which will then call frameloader->Show.

I don’t _think_ we associate an nsSubDocumentFrame with a remote nsFrameLoader… if we do, we might want to care about the reframing case, but I think that’s edge-casey.

The other place where nsSubDocumentFrame calls nsFrameLoader::Hide is from within the nsHideViewer runnable, which is fired when an nsSubDocumentFrame is pulled out of the DOM.

So, I think we’re doing the right thing IF there’s no direct relationship between a remote frameloader and an nsSubDocumentFrame.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Hey smaug, is there any kind of relationship possible between a remote frameloader and an nsSubDocumentFrame in the parent process? I suspect not, but wanted to check.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

So nsSubDocumentFrame is the primary nsIFrame for the nsFrameLoader::mOwnerContent.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=06bc3102b900#4316

I think the Hide in the Init() is bogus since if we've moved the xul:browser or html:iframe to another document, we've created a new nsFrameLoader for it.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Ah, I see. So if I understand correctly, if a remote frame is in the midst of being re-framed, we don't do what we'd normally do for a non-remote iframe, which is to SetVisibility to false on it until the reframe is complete. Does that sound correct? And if so, is that problematic?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Well, I think in principle if one has display: none; remote frame, we should mark that visibility hidden. But whether that matters currently, not sure. For tabs we anyway manually do all the
activate/deactivate stuff and so.

But might make sense to send some update to remote frame when frameloader gets Show() or Hide() call.
And looks like in Show() case we already do that, but Hide could also send some message and then
we could just update TabChild's docshell's visibility based on those messages.
Though, that would need some testing. I'm a bit worried what nsDocumentViewer::Hide() would do when called on child in this case.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

So, in conclusion, I think we should give this a shot. There might be some perf win potential here, but we'd need to try it. The implications really aren't clear at this point - we're pretty deep in the machine here.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information