Created attachment 92432 [details]
Patch

<rdar://problem/9390280>

Comment on attachment 92432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92432&action=review

> Source/WebCore/dom/Document.cpp:3378
> +#if ENABLE(FULLSCREEN_API)
> +    // If the current full screen element or any of its ancestors is removed, set the current
> +    // full screen element to the document root, and fire a fullscreenchange event to inform 
> +    // clients of the DOM.

It’s really inelegant to have to put full-screen-specific code right into the basic DOM manipulation. I also expect that calling the contains function every time a node is removed will have a measurable performance impact.

Is there a better way to do this?

Maybe m_fullScreenElement should be 0 rather than the document element, when there is no special element. Then there would be a quick null check.

Or maybe you should add a check that m_fullScreenElement is != documentElement() before you check anything else for less performance impact.

(In reply to comment #3)
> (From update of attachment 92432 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92432&action=review
> 
> > Source/WebCore/dom/Document.cpp:3378
> > +#if ENABLE(FULLSCREEN_API)
> > +    // If the current full screen element or any of its ancestors is removed, set the current
> > +    // full screen element to the document root, and fire a fullscreenchange event to inform 
> > +    // clients of the DOM.
> 
> It’s really inelegant to have to put full-screen-specific code right into the basic DOM manipulation. I also expect that calling the contains function every time a node is removed will have a measurable performance impact.

In the most frequent case, where m_fullScreenElement is 0; contains bails out immediately.  So for that case, the performance impact is negligable.  For the case where we're in full screen, the performace hit is walking up the tree from the full screen element to the removed one, once.

> Is there a better way to do this?

Signs point to yes!  I believe this code can be moved into Node::willRemove(), which can do a simple "am I the full screen element?" pointer equality check.

> Maybe m_fullScreenElement should be 0 rather than the document element, when there is no special element. Then there would be a quick null check.

That's possible, though it might introduce other problems.  And it probably won't be necessary if I move things into the function listed above.

> Or maybe you should add a check that m_fullScreenElement is != documentElement() before you check anything else for less performance impact.

Lets see if it's necessary after moving things around a bit.

I'll get a new patch up soon.  Thanks!

Created attachment 92479 [details]
Patch

Comment on attachment 92479 [details]
Patch

 void Node::willRemove()
 {
+#if ENABLE(FULLSCREEN_API)
+    if (document()->webkitCurrentFullScreenElement() == this)
+        document()->fullScreenElementWillBeRemoved();
+#endif
 }

Can an arbitrary node be full screen? Even an attribute node?

Other elements need special handling when being removed, e.g. a focused one. The situation seems similar at a first glance - can the same technique be used to detect when a fullscreen node is being removed?

Also, fullscreen looks very much like a renderer behavior, not a DOM one.

(In reply to comment #6)
> (From update of attachment 92479 [details])
>  void Node::willRemove()
>  {
> +#if ENABLE(FULLSCREEN_API)
> +    if (document()->webkitCurrentFullScreenElement() == this)
> +        document()->fullScreenElementWillBeRemoved();
> +#endif
>  }
> 
> Can an arbitrary node be full screen? Even an attribute node?

Any element.  I doubt anyone would try taking an attribute node full screen, but if they tried, we'd dutifully show the full screen UI.

> Other elements need special handling when being removed, e.g. a focused one. The situation seems similar at a first glance - can the same technique be used to detect when a fullscreen node is being removed?

We could, in theory, add an ivar to Element called m_isFullScreen (or something), and check that instead of querying the document.  But would that be advantageous at all?

> Also, fullscreen looks very much like a renderer behavior, not a DOM one.

What do you mean?

(In reply to comment #7)
> (In reply to comment #6)
> > Other elements need special handling when being removed, e.g. a focused one. The situation seems similar at a first glance - can the same technique be used to detect when a fullscreen node is being removed?
> 
> We could, in theory, add an ivar to Element called m_isFullScreen (or something), and check that instead of querying the document.  But would that be advantageous at all?


It looks like focused nodes are removed in ContainerNode::removeChild() and ContainerNode::removeChildren().  ContainerNode uncritically calls document()->removeFocusedNodeOfSubtree(...) with a pointer to the child being removed.  What we are doing here is functionally identical, however, it could be refactored to be technically identical.

Created attachment 92557 [details]
Patch

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 92479 [details] [details])
> >  void Node::willRemove()
> >  {
> > +#if ENABLE(FULLSCREEN_API)
> > +    if (document()->webkitCurrentFullScreenElement() == this)
> > +        document()->fullScreenElementWillBeRemoved();
> > +#endif
> >  }
> > 
> > Can an arbitrary node be full screen? Even an attribute node?
> 
> Any element.  I doubt anyone would try taking an attribute node full screen, but if they tried, we'd dutifully show the full screen UI.

I don't think so - the fullscreen API only exists in Element.idl. You could probably limit this to Element::willRemove()

Comment on attachment 92557 [details]
Patch

Since this exactly matches what we do for focus, I see no reason to object. If there is a better way to do this, we can refactor both these code paths in the future.

Committed r86185: <http://trac.webkit.org/changeset/86185>

Revision r86185 cherry-picked into qtwebkit-2.2 with commit 2cacab2 <http://gitorious.org/webkit/qtwebkit/commit/2cacab2>