Add a nextLocalAncestor helper for Fullscreen

third_party/WebKit/Source/core/dom/Fullscreen.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)
@@ skipping 176 matching lines @@
 }
 
 Event* createEvent(const AtomicString& type, EventTarget& target) {
   EventInit initializer;
   initializer.setBubbles(isPrefixed(type));
   Event* event = Event::create(type, initializer);
   event->setTarget(&target);
   return event;
 }
 
+// Walks the frame tree and returns the first local ancestor frame, if any.
+LocalFrame* nextLocalAncestor(Frame& frame) {
+  Frame* parent = frame.tree().parent();
+  if (!parent)
+    return nullptr;
+  if (parent->isLocalFrame())
+    return toLocalFrame(parent);
+  return nextLocalAncestor(*parent);
+}
+
+// Walks the document's frame tree and returns the document of the first local
+// ancestor frame, if any.
+Document* nextLocalAncestor(Document& document) {
+  LocalFrame* frame = document.frame();
+  if (!frame)
+    return nullptr;
+  LocalFrame* next = nextLocalAncestor(*document.frame());
+  if (!next)
+    return nullptr;
+  DCHECK(next->document());
+  return next->document();
+}
+
 // Helper to walk the ancestor chain and return the Document of the topmost
 // local ancestor frame. Note that this is not the same as the topmost frame's
 // Document, which might be unavailable in OOPIF scenarios. For example, with
 // OOPIFs, when called on the bottom frame's Document in a A-B-C-B hierarchy in
 // process B, this will skip remote frame C and return this frame: A-[B]-C-B.
 Document& topmostLocalAncestor(Document& document) {
-  Document* topmost = &document;
-  Frame* frame = document.frame();
-  while (frame) {
-    frame = frame->tree().parent();
-    if (frame && frame->isLocalFrame())
-      topmost = toLocalFrame(frame)->document();
-  }
-  return *topmost;
+  if (Document* next = nextLocalAncestor(document))
+    return topmostLocalAncestor(*next);
+  return document;
 }
 
 // Helper to find the browsing context container in |doc| that embeds the
 // |descendant| Document, possibly through multiple levels of nesting.  This
 // works even in OOPIF scenarios like A-B-A, where there may be remote frames
 // in between |doc| and |descendant|.
 HTMLFrameOwnerElement* findContainerForDescendant(const Document& doc,
                                                   const Document& descendant) {
   Frame* frame = descendant.frame();
   while (frame->tree().parent() != doc.frame())
@@ skipping 155 matching lines @@
     //
     // For OOPIF scenarios, |docs| will only contain documents for local
     // ancestors, and remote ancestors will be processed in their
     // respective processes.  This preserves the spec's event firing order
     // for local ancestors, but not for remote ancestors.  However, that
     // difference shouldn't be observable in practice: a fullscreenchange
     // event handler would need to postMessage a frame in another renderer
     // process, where the message should be queued up and processed after
     // the IPC that dispatches fullscreenchange.
     HeapDeque<Member<Document>> docs;
-
-    docs.prepend(&document);
-    for (Frame* frame = document.frame()->tree().parent(); frame;
-         frame = frame->tree().parent()) {
-      if (frame->isLocalFrame())
-        docs.prepend(toLocalFrame(frame)->document());
-    }
+    for (Document* doc = &document; doc; doc = nextLocalAncestor(*doc))
+      docs.prepend(doc);
 
     // 4. For each document in docs, run these substeps:
     HeapDeque<Member<Document>>::iterator current = docs.begin(),
                                           following = docs.begin();
 
     do {
       ++following;
 
       // 1. Let following document be the document after document in docs, or
       // null if there is no such document.
@@ skipping 131 matching lines @@
 
     // 3. If doc's fullscreen element stack is empty and doc's browsing context
     // has a browsing context container, set doc to that browsing context
     // container's node document.
     //
     // OOPIF: If browsing context container's document is in another
     // process, keep moving up the ancestor chain and looking for a
     // browsing context container with a local document.
     // TODO(alexmos): Deal with nested fullscreen cases, see
     // https://crbug.com/617369.
-    if (!newTop) {
-      Frame* frame = currentDoc->frame()->tree().parent();
-      while (frame && frame->isRemoteFrame())
-        frame = frame->tree().parent();
-      if (frame) {
-        currentDoc = toLocalFrame(frame)->document();
-        continue;
-      }
-    }
+    if (!newTop)
+      currentDoc = nextLocalAncestor(*currentDoc);

[alexmos, 2016/12/12 17:18:03]

Doesn't this need a "continue" to match the old behavior?  Otherwise seems like
currentDoc will get set to nullptr and drop out of the loop.

[foolip, 2016/12/12 17:27:44]

Oh my, good catch. Will see why no tests fail because of this bug.

[foolip, 2016/12/12 22:34:09]

I wrote a test and updated the description. The test would fail because one of
the events when exiting would go missing.

 
     // 4. Otherwise, set doc to null.
     currentDoc = nullptr;
   }
 
   // 6. Return, and run the remaining steps asynchronously.
   // 7. Optionally, perform some animation.
 
   // Only exit fullscreen mode if the fullscreen element stack is empty.
   if (!newTop) {
@@ skipping 249 matching lines @@
 DEFINE_TRACE(Fullscreen) {
   visitor->trace(m_pendingFullscreenElement);
   visitor->trace(m_fullscreenElementStack);
   visitor->trace(m_currentFullScreenElement);
   visitor->trace(m_eventQueue);
   Supplement<Document>::trace(visitor);
   ContextLifecycleObserver::trace(visitor);
 }
 
 }  // namespace blink