Apart from the :-moz-full-screen-ancestor pseudo-class, currently there is another place which uses the fullscreen ancestor flag, which is Element::IsFullScreenAncestor() method. Thus, to remove the ancestor flag, we would need to remove this method and all its uses first. There is currently only one non-trivial use of that method, which is in Element::UnbindFromTree(). Since this method is always recursively called for every element, I think we can just check the fullscreen flag for each element when this method is called, and remove them from the fullscreen stack.

Since UnbindFromTree() would eventually be called synchronously for every element unbound, we don't have to check whether the root of unbound elements is a fullscreen ancestor. Review commit: https://reviewboard.mozilla.org/r/61252/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61252/

Review commit: https://reviewboard.mozilla.org/r/61254/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61254/

Comment on attachment 8766274 [details] Bug 1216049 part 1 - Remove the use of Element::IsFullScreenAncestor() in Element::UnbindFromTree(). https://reviewboard.mozilla.org/r/61252/#review58128 un-virtualizing should be done as a separate patch. ::: dom/base/Element.cpp:1736 (Diff revision 1) > HasFlag(NODE_FORCE_XBL_BINDINGS) ? OwnerDoc() : GetComposedDoc(); > > if (HasPointerLock()) { > nsIDocument::UnlockPointer(); > } > - if (aNullParent) { > + if (OwnerDoc()->GetFullscreenElement() == this) { GetFullscreenElement is a virtual method(, though I don't know why). I'm not happy to add a new virtual call to rather hot UnbindFromTree. So, in order to do this, please un-virtualize GetFullscreenElement. With that, r+

Let's try a slightly different way: > if (mState.HasState(NS_EVENT_STATE_FULL_SCREEN)) { > MOZ_ASSERT(OwnerDoc()->GetFullscreenElement() == this); > ... > } Do you think this is fine?

The easiest way to devirtualize this is to mark nsDocument::GetFullscreenElement final, and cast the return value of OwnerDoc() to nsDocument. Actually I wonder why we have nsIDocument and nsDocument... It seems to me nsDocument is the only direct subclass of nsIDocument, and thus any nsIDocument can actually be casted to nsDocument safely... Shouldn't we just merge those two classes so that lots of methods can be devirtualized?

nsIDocument used to be used by some binary addons, IIRC. (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4) > Let's try a slightly different way: > > if (mState.HasState(NS_EVENT_STATE_FULL_SCREEN)) { > > MOZ_ASSERT(OwnerDoc()->GetFullscreenElement() == this); > > ... > > } > > Do you think this is fine? Hmm, so what guarantees OwnerDoc()->GetFullscreenElement() == this? Based on EventStateManager::SetFullScreenState, the state can be on several elements.

Comment on attachment 8766275 [details] Bug 1216049 part 2 - Remove Element::IsFullScreenAncestor() and replace its trivial callsites. https://reviewboard.mozilla.org/r/61254/#review58134 ::: dom/base/nsDocument.cpp:11655 (Diff revision 1) > // been GC'd since they were added to the stack) and elements which are > // no longer in this document. > while (!mFullScreenStack.IsEmpty()) { > Element* element = FullScreenStackTop(); > if (!element || !element->IsInUncomposedDoc() || element->OwnerDoc() != this) { > - NS_ASSERTION(!element->IsFullScreenAncestor(), > + NS_ASSERTION(!element->State().HasState(NS_EVENT_STATE_FULL_SCREEN), Could we assert also that the elements doesn't have NS_EVENT_STATE_FULL_SCREEN_ANCESTOR set.

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #6) > Hmm, so what guarantees OwnerDoc()->GetFullscreenElement() == this? > Based on EventStateManager::SetFullScreenState, the state can be on several > elements. Oops... I forgot that fullscreen state can be applied to multiple elements... Then I guess I can just remove the assertion, or change it to assert that the element is inside the fullscreen stack... The spec's behavior is a bit more complicated than this, which I'll probably look into later.

Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a6b605e4badc part 1 - Remove the use of Element::IsFullScreenAncestor() in Element::UnbindFromTree(). r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7998891c59 part 2 - Remove Element::IsFullScreenAncestor() and replace its trivial callsites. r=smaug

https://hg.mozilla.org/mozilla-central/rev/a6b605e4badc https://hg.mozilla.org/mozilla-central/rev/6e7998891c59