1216049 - Remove Element::IsFullScreenAncestor() method

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1216049 part 1 - Remove the use of Element::IsFullScreenAncestor() in Element::UnbindFromTree().

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/

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1216049 part 2 - Remove Element::IsFullScreenAncestor() and replace its trivial callsites.

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

Comment 3

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

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+

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

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?

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

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?

Comment 6

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

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 7

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

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.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Carsten Book [:Tomcat]]

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