Sync fullscreen element removal with the spec

Source/core/dom/FullscreenElementStack.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  *           (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
@@ skipping 542 matching lines @@
         // If the element was removed from our tree, also message the documentElement.
         if (!target->inDocument() && document()->documentElement()) {
             ASSERT(isPrefixed(event->type()));
             eventQueue.append(createEvent(event->type(), *document()->documentElement()));
         }
 
         target->dispatchEvent(event);
     }
 }
 
-void FullscreenElementStack::fullScreenElementRemoved()
+void FullscreenElementStack::elementRemoved(Element& element)
 {
-    m_fullScreenElement->setContainsFullScreenElementOnAncestorsCrossingFrameBoundaries(false);
-    fullyExitFullscreen();
-}
+    // If an element element in a fullscreen element stack is removed from a document document, run

[falken, 2014/08/14 04:12:47]

I find these hard to read. How about putting italicized words in pipes like
|element| and |document|.

[philipj_slow, 2014/08/14 08:10:39]

I had done this, but then I started worrying about consistency. Let's do it
everywhere:
https://codereview.chromium.org/473803002/

+    // these steps:
 
-void FullscreenElementStack::removeFullScreenElementOfSubtree(Node* node, bool amongChildrenOnly)
-{
-    if (!m_fullScreenElement)
+    // 1. If element was at the top of document's fullscreen element stack, act as if the
+    // exitFullscreen() method was invoked on that document.
+    if (fullscreenElement() == &element) {
+        exitFullscreen();
         return;
+    }
 
-    // If the node isn't in a document it can't have a fullscreen'd child.
-    if (!node->inDocument())
-        return;
-
-    bool elementInSubtree = false;
-    if (amongChildrenOnly)
-        elementInSubtree = m_fullScreenElement->isDescendantOf(node);
-    else
-        elementInSubtree = (m_fullScreenElement == node) || m_fullScreenElement->isDescendantOf(node);
-
-    if (elementInSubtree)
-        fullScreenElementRemoved();
+    // 2. Otherwise, remove element from document's fullscreen element stack.
+    for (size_t i = 0; i < m_fullScreenElementStack.size(); ++i) {
+        if (m_fullScreenElementStack[i].first.get() == &element) {
+            m_fullScreenElementStack.remove(i);
+            return;
+        }
+    }

[falken, 2014/08/14 04:12:47]

Is it worth doing NOTREACHED() here?

[philipj_slow, 2014/08/14 08:10:39]

It's actually reachable, because elementRemoved will be called without knowing
if the element is in fact in the fullscreen element stack. I could try to use
containsFullScreenElement() to optimize, but I want to remove that flag
eventually because it's only really needed for the :-webkit-full-screen-ancestor
pseudo-class which can hopefully be removed after the top-layer rewrite.

I'll add a comment here, but if you prefer I could determine "in fullscreen
element stack" first and then do the steps per spec.

 }
 
 void FullscreenElementStack::clearFullscreenElementStack()
 {
     m_fullScreenElementStack.clear();
 }
 
 void FullscreenElementStack::popFullscreenElementStack()
 {
     if (m_fullScreenElementStack.isEmpty())
@@ skipping 10 matching lines @@
 void FullscreenElementStack::trace(Visitor* visitor)
 {
     visitor->trace(m_fullScreenElement);
     visitor->trace(m_fullScreenElementStack);
     visitor->trace(m_fullScreenRenderer);
     visitor->trace(m_eventQueue);
     DocumentSupplement::trace(visitor);
 }
 
 } // namespace blink