OOPIF: Process all local ancestors when requesting and exiting 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)
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
@@ skipping 103 matching lines @@
 
 static Event* createEvent(const AtomicString& type, EventTarget& target)
 {
     EventInit initializer;
     initializer.setBubbles(isPrefixed(type));
     Event* event = Event::create(type, initializer);
     event->setTarget(&target);
     return event;
 }
 
+static Document& topmostDocument(Document& document)

[foolip, 2016/06/09 13:12:36]

Can you name this so that it's clear that it's not the top-level browsing
context's document, but the locally topmost document? Spelling out the
difference between this and Document::topDocument() would be good, because
without this CL leading me to think about A-B-A cases I'm not sure I would
understand how it's different.

[alexmos, 2016/06/09 21:36:55]

Renamed to topmostLocalAncestor and added a comment explaining how it's
different from top frame's document.  Let me know if that name sounds better.

[foolip, 2016/06/22 16:23:58]

That's great, and the comments throughout this CL were just great. I will try to
emulate :)

+{
+    Document* topmost = &document;
+    Frame* frame = document.frame();
+    while (frame) {
+        frame = frame->tree().parent();
+        if (frame && frame->isLocalFrame())
+            topmost = toLocalFrame(frame)->document();
+    }
+    return *topmost;
+}
+
 const char* Fullscreen::supplementName()
 {
     return "Fullscreen";
 }
 
 Fullscreen& Fullscreen::from(Document& document)
 {
     Fullscreen* fullscreen = fromIfExists(document);
     if (!fullscreen) {
         fullscreen = new Fullscreen(document);
@@ skipping 101 matching lines @@
             document()->addConsoleMessage(
                 ConsoleMessage::create(JSMessageSource, WarningMessageLevel, message));
             break;
         }
 
         // Fullscreen is not supported.
         if (!fullscreenIsSupported(element.document()))
             break;
 
         // 2. Let doc be element's node document. (i.e. "this")
-        Document* currentDoc = document();
+        Frame* currentFrame = document()->frame();

[foolip, 2016/06/09 13:12:36]

The spec quotes are a lot let useful when the names don't match, thinking about
ways to minimize the difference.

First option, could the docs list be populated with all the documents for local
frames even if there are remote frames with documents in between them per spec?
Then much of the following logic would remain unchanged, and one would only need
to convince oneself that it's not observable.

Second option, if a Frame is equivalent to a browsing context in spec terms,
change the spec to talk about browsing contexts instead. But I think it'd still
be unnatural to say "Frame* browsingContext" so it still wouldn't match.

[alexmos, 2016/06/09 21:36:55]

The main reason I originally kept ancestors as Frames is for the check in step
4.3 on the following document's container (the old topElement !=
followingDoc->localOwner() check).  With an A-B-A2 hierarchy, we need B's owner
when checking A's topElement (B won't have a document in A's process, but it
will have a local owner()).  With Frames, this is straightforward: the ancestors
list is {A,B,A2}, followingFrame is B, and we can just use its owner. If we keep
the ancestor list as documents (your first option), the list will be {A, A2},
followingDoc is A2, and we'll need to do another traversal from A2 to get B's
owner.

That said, I was worried about departing from spec comments myself.  I switched
to the latter approach (your first option) -- this better follows the spec
quotes and results in fewer overall changes to requestFullscreen, at the cost of
a bit more complexity in the 4.3 check.  Let me know if you think this is
better.

[foolip, 2016/06/22 16:23:58]

Looks great :)

 
         // 3. Let docs be all doc's ancestor browsing context's documents (if any) and doc.
-        HeapDeque<Member<Document>> docs;
+        HeapDeque<Member<Frame>> ancestors;
 
         do {
-            docs.prepend(currentDoc);
-            currentDoc = currentDoc->localOwner() ? &currentDoc->localOwner()->document() : nullptr;
-        } while (currentDoc);
+            ancestors.prepend(currentFrame);
+            currentFrame = currentFrame->tree().parent();
+        } while (currentFrame);
 
         // 4. For each document in docs, run these substeps:
-        HeapDeque<Member<Document>>::iterator current = docs.begin(), following = docs.begin();
+        HeapDeque<Member<Frame>>::iterator current = ancestors.begin(), following = ancestors.begin();
 
         do {
             ++following;
 
             // 1. Let following document be the document after document in docs, or null if there is no
             // such document.
-            Document* currentDoc = *current;
-            Document* followingDoc = following != docs.end() ? *following : nullptr;
+            Frame* currentFrame = *current;
+            Frame* followingFrame = following != ancestors.end() ? *following : nullptr;
 
             // 2. If following document is null, push context object on document's fullscreen element
             // stack, and queue a task to fire an event named fullscreenchange with its bubbles attribute
             // set to true on the document.
-            if (!followingDoc) {
-                from(*currentDoc).pushFullscreenElementStack(element, requestType);
-                enqueueChangeEvent(*currentDoc, requestType);
+            if (!followingFrame) {
+                from(*toLocalFrame(currentFrame)->document()).pushFullscreenElementStack(element, requestType);
+                enqueueChangeEvent(*toLocalFrame(currentFrame)->document(), requestType);
                 continue;
             }
 
             // 3. Otherwise, if document's fullscreen element stack is either empty or its top element
             // is not following document's browsing context container,
+            if (currentFrame->isRemoteFrame())

[foolip, 2016/06/09 13:12:36]

Assuming that this isn't reverted to a list of only local documents, can you
move this to before step 3 and document why the observable behavior will still
match what the spec says?

[alexmos, 2016/06/09 21:36:55]

I did revert to a list of only local documents, so this is gone now.  I also
added a note about how this affects observable behavior and the spec to step 3's
comment.

+                continue;
+            Document* currentDoc = toLocalFrame(currentFrame)->document();
             Element* topElement = fullscreenElementFrom(*currentDoc);
-            if (!topElement || topElement != followingDoc->localOwner()) {
+            if (!topElement || topElement != toHTMLFrameOwnerElement(followingFrame->owner())) {
                 // ...push following document's browsing context container on document's fullscreen element
                 // stack, and queue a task to fire an event named fullscreenchange with its bubbles attribute
                 // set to true on document.
-                from(*currentDoc).pushFullscreenElementStack(*followingDoc->localOwner(), requestType);
+                from(*currentDoc).pushFullscreenElementStack(*toHTMLFrameOwnerElement(followingFrame->owner()), requestType);
                 enqueueChangeEvent(*currentDoc, requestType);
                 continue;
             }
 
             // 4. Otherwise, do nothing for this document. It stays the same.
-        } while (++current != docs.end());
+        } while (++current != ancestors.end());
 
         m_forCrossProcessAncestor = forCrossProcessAncestor;
 
         // 5. Return, and run the remaining steps asynchronously.
         // 6. Optionally, perform some animation.
         document()->frameHost()->chromeClient().enterFullScreenForElement(&element);
 
         // 7. Optionally, display a message indicating how the user can exit displaying the context object fullscreen.
         return;
     } while (false);
 
     enqueueErrorEvent(element, requestType);
 }
 
 void Fullscreen::fullyExitFullscreen(Document& document)
 {
     // To fully exit fullscreen, run these steps:
 
     // 1. Let |doc| be the top-level browsing context's document.
-    Document& doc = document.topDocument();
+    //
+    // For OOPIF, use the Document of the topmost local ancestor frame.  Each

[foolip, 2016/06/09 13:12:36]

"Use the document... because OOPIF something something" so that it doesn't sound
like this code path is only for OOPIF. The reader might notice the lack of "if
(oopif)" but still doubt what's going on, as I am now in a first reading :)

[alexmos, 2016/06/09 21:36:55]

Rephrased the comment - let me know if it's better. :)

+    // renderer process for the current page will separately call
+    // fullyExitFullscreen.
+    Document& doc = topmostDocument(document);
 
     // 2. If |doc|'s fullscreen element stack is empty, terminate these steps.
     if (!fullscreenElementFrom(doc))
         return;
 
     // 3. Remove elements from |doc|'s fullscreen element stack until only the top element is left.
     size_t stackSize = from(doc).m_fullScreenElementStack.size();
     from(doc).m_fullScreenElementStack.remove(0, stackSize - 1);
     DCHECK_EQ(from(doc).m_fullScreenElementStack.size(), 1u);
 
@@ skipping 48 matching lines @@
         newTop = fullscreenElementFrom(*currentDoc);
         if (newTop && (!newTop->inShadowIncludingDocument() || newTop->document() != currentDoc))
             continue;
 
         // 2. Queue a task to fire an event named fullscreenchange with its bubbles attribute set to true
         // on doc.
         enqueueChangeEvent(*currentDoc, requestType);
 
         // 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.
-        if (!newTop && currentDoc->localOwner()) {
-            currentDoc = &currentDoc->localOwner()->document();
-            continue;
+        //
+        // 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.

[foolip, 2016/06/09 13:12:36]

What cases is that?

[alexmos, 2016/06/09 21:36:55]

I described it in a comment on my original message, which probably got lost :) :
https://codereview.chromium.org/2040973003/diff/20001/third_party/WebKit/Sour...

Basically, it's about fixing https://crbug.com/617369, both for OOPIF and
non-OOPIF cases.

[foolip, 2016/06/22 16:23:58]

Can you link to it here, unless it's too specific or something?

[alexmos, 2016/06/22 18:04:22]

Done.

+        if (!newTop) {
+            Frame* frame = currentDoc->frame()->tree().parent();
+            while (frame && frame->isRemoteFrame())
+                frame = frame->tree().parent();
+            if (frame) {

[foolip, 2016/06/09 13:12:36]

OK, so this just skips over the remote frames and otherwise does what the spec
says. Some symmetry with that for the requestFullscreen code path is what I'm
hoping for above, if it can be had.

[alexmos, 2016/06/09 21:36:55]

Let me know if you feel this still needs improvement after I revamped the
requestFullscreen path.

[foolip, 2016/06/22 16:23:58]

I think the symmetry is there now. What still feels a bit strange is that one
enters the web-exposed requestFullscreen code path in multiple processes, but
unless that actually causes any trouble it's probably just fine :)

+                currentDoc = toLocalFrame(frame)->document();
+                continue;
+            }
         }
 
         // 4. Otherwise, set doc to null.
         currentDoc = 0;
     }
 
     // 6. Return, and run the remaining steps asynchronously.
     // 7. Optionally, perform some animation.
 
     FrameHost* host = document()->frameHost();
@@ skipping 104 matching lines @@
     m_fullScreenElement = nullptr;
 
     if (document()->frame())
         document()->frame()->eventHandler().scheduleHoverStateUpdate();
 
     // When fullyExitFullscreen is called, we call exitFullscreen on the topDocument(). That means
     // that the events will be queued there. So if we have no events here, start the timer on the
     // exiting document.
     Document* exitingDocument = document();
     if (m_eventQueue.isEmpty())
-        exitingDocument = &document()->topDocument();
+        exitingDocument = &topmostDocument(*document());
     DCHECK(exitingDocument);
     from(*exitingDocument).m_eventQueueTimer.startOneShot(0, BLINK_FROM_HERE);
 
     m_forCrossProcessAncestor = false;
 }
 
 void Fullscreen::setFullScreenLayoutObject(LayoutFullScreen* layoutObject)
 {
     if (layoutObject == m_fullScreenLayoutObject)
         return;
@@ skipping 110 matching lines @@
 DEFINE_TRACE(Fullscreen)
 {
     visitor->trace(m_fullScreenElement);
     visitor->trace(m_fullScreenElementStack);
     visitor->trace(m_eventQueue);
     Supplement<Document>::trace(visitor);
     ContextLifecycleObserver::trace(visitor);
 }
 
 } // namespace blink