Sync requestFullscreen() and exitFullscreen() algorithms with the spec

third_party/WebKit/Source/core/dom/Fullscreen.h

Index: third_party/WebKit/Source/core/dom/Fullscreen.h
diff --git a/third_party/WebKit/Source/core/dom/Fullscreen.h b/third_party/WebKit/Source/core/dom/Fullscreen.h
index a93806df06f2cf48317309277284fd6908d50385..c66ce3530991cb5cfc196dbb351e81e1951c9e4a 100644
--- a/third_party/WebKit/Source/core/dom/Fullscreen.h
+++ b/third_party/WebKit/Source/core/dom/Fullscreen.h
@@ -35,7 +35,6 @@
 #include "core/dom/Document.h"
 #include "core/dom/Element.h"
 #include "platform/Supplementable.h"
-#include "platform/Timer.h"
 #include "platform/geometry/LayoutRect.h"
 #include "wtf/Deque.h"
 #include "wtf/RefPtr.h"
@@ -59,9 +58,8 @@ class CORE_EXPORT Fullscreen final
   static Fullscreen* fromIfExists(Document&);
   static Element* fullscreenElementFrom(Document&);
   static Element* fullscreenElementForBindingFrom(TreeScope&);
-  static Element* currentFullScreenElementFrom(Document&);
-  static Element* currentFullScreenElementForBindingFrom(Document&);
-  static bool isCurrentFullScreenElement(const Element&);
+  static size_t fullscreenElementStackSizeFrom(Document&);
+  static bool isFullscreenElement(const Element&);
 
   enum class RequestType {
     // Element.requestFullscreen()
@@ -69,25 +67,26 @@ class CORE_EXPORT Fullscreen final
     // Element.webkitRequestFullscreen()/webkitRequestFullScreen() and
     // HTMLVideoElement.webkitEnterFullscreen()/webkitEnterFullScreen()
     Prefixed,
+    // For WebRemoteFrameImpl to notify that a cross-process descendant frame
+    // has requested and is about to enter fullscreen.
+    PrefixedForCrossProcessDescendant,

[alexmos, 2016/12/15 07:58:53]

When we add plumbing for the request type in OOPIF fullscreen requests (in
willEnterFullscreen), seems like we will end up adding an
UnprefixedForCrossProcessDescendant here, and having to check for that in
addition to Unprefixed in places like createChangeEvent.  Was that how you
wanted to do that eventually?  I guess if we kept the forCrossProcessDescendant
bit separate from this, that would also need to be in the fullscreen element
stack, which seems ugly.

FWIW, I started working on a CL for plumbing the request type for OOPIFs a while
back, but got distracted with other things before finishing.  We should figure
out how to finish it before unprefixed fullscreen ships - when do you think that
might happen?

[foolip, 2016/12/19 17:36:57]

Having both bits of information in RequestType was nifty and indeed the reason I
did this. Rather than adding UnprefixedForCrossProcessDescendant I think this
will have to be turned into a bitfield of size two to represent all four
combinations.

I'll be OOO until January, but I hope to ship unprefixed fullscreen in M57 or
M58, so we'll have to revisit this soon. (First though, moving to top layer.
Getting rid of :-webkit-full-screen-ancestor would also simplify things a bit,
and it's not clear yet if that'll happen before unprefixing.)

   };
 
   static void requestFullscreen(Element&);
-
-  // |forCrossProcessDescendant| is used in OOPIF scenarios and is set to
-  // true when fullscreen is requested for an out-of-process descendant
-  // element.
-  static void requestFullscreen(Element&,
-                                RequestType,
-                                bool forCrossProcessDescendant = false);
+  static void requestFullscreen(Element&, RequestType);
 
   static void fullyExitFullscreen(Document&);
-  static void exitFullscreen(Document&);
+
+  enum class ExitType {
+    // Exits fullscreen for one element in the document.
+    Default,
+    // Fully exits fullscreen for the document.
+    Fully,
+  };
+
+  static void exitFullscreen(Document&, ExitType = ExitType::Default);
 
   static bool fullscreenEnabled(Document&);
-  // TODO(foolip): The fullscreen element stack is modified synchronously in
-  // requestFullscreen(), which is not per spec and means that
-  // |fullscreenElement()| is not always the same as
-  // |currentFullScreenElement()|, see https://crbug.com/402421.
   Element* fullscreenElement() const {
     return !m_fullscreenElementStack.isEmpty()
                ? m_fullscreenElementStack.back().first.get()
@@ -107,20 +106,6 @@ class CORE_EXPORT Fullscreen final
 
   void elementRemoved(Element&);
 
-  // Returns true if the current fullscreen element stack corresponds to a
-  // container for an actual fullscreen element in a descendant
-  // out-of-process iframe.
-  bool forCrossProcessDescendant() { return m_forCrossProcessDescendant; }

[alexmos, 2016/12/15 07:58:53]

See my note in FullscreenController::enterFullscreen about this.

-
-  // Mozilla API
-  // TODO(foolip): |currentFullScreenElement()| is a remnant from before the
-  // fullscreen element stack. It is still maintained separately from the
-  // stack and is is what the :-webkit-full-screen pseudo-class depends on. It
-  // should be removed, see https://crbug.com/402421.
-  Element* currentFullScreenElement() const {
-    return m_currentFullScreenElement.get();
-  }
-
   // ContextLifecycleObserver:
   void contextDestroyed() override;
 
@@ -133,33 +118,28 @@ class CORE_EXPORT Fullscreen final
 
   Document* document();
 
+  static void enqueueTaskForRequest(Document&,
+                                    Element&,
+                                    RequestType,
+                                    bool error);
+  static void runTaskForRequest(Document*, Element*, RequestType, bool error);
+
+  static void enqueueTaskForExit(Document&, ExitType);
+  static void runTaskForExit(Document*, ExitType);
+
   void clearFullscreenElementStack();
   void popFullscreenElementStack();
   void pushFullscreenElementStack(Element&, RequestType);
+  void fullscreenElementChanged(Element*, Element*, RequestType);

[mlamouri (slow - plz ping), 2016/12/14 16:24:12]

I would name the two different `Element*` parameters here because otherwise one
has to guess it is old/new unless they look at the implementation.

[foolip, 2016/12/19 17:36:57]

Done, all args named. (I imitated ChromeClient::focusedNodeChanged, but guessing
RequestType is even harder than the from/to element.)

 
-  void enqueueChangeEvent(Document&, RequestType);
-  void enqueueErrorEvent(Element&, RequestType);
-  void eventQueueTimerFired(TimerBase*);
+  using ElementStackEntry = std::pair<Member<Element>, RequestType>;
+  using ElementStack = HeapVector<ElementStackEntry>;
+  ElementStack m_pendingRequests;
+  ElementStack m_fullscreenElementStack;
 
-  Member<Element> m_pendingFullscreenElement;
-  HeapVector<std::pair<Member<Element>, RequestType>> m_fullscreenElementStack;
-  Member<Element> m_currentFullScreenElement;
   LayoutFullScreen* m_fullScreenLayoutObject;
-  Timer<Fullscreen> m_eventQueueTimer;
-  HeapDeque<Member<Event>> m_eventQueue;
   LayoutRect m_savedPlaceholderFrameRect;
   RefPtr<ComputedStyle> m_savedPlaceholderComputedStyle;
-
-  // TODO(alexmos, dcheng): Currently, this assumes that if fullscreen was
-  // entered for an element in an out-of-process iframe, then it's not
-  // possible to re-enter fullscreen for a different element in this
-  // document, since that requires a user gesture, which can't be obtained
-  // since nothing in this document is visible, and since user gestures can't
-  // be forwarded across processes. However, the latter assumption could
-  // change if https://crbug.com/161068 is fixed so that cross-process
-  // postMessage can carry user gestures.  If that happens, this should be
-  // moved to be part of |m_fullscreenElementStack|.
-  bool m_forCrossProcessDescendant;
 };
 
 inline Fullscreen* Fullscreen::fromIfExists(Document& document) {
@@ -168,9 +148,9 @@ inline Fullscreen* Fullscreen::fromIfExists(Document& document) {
   return fromIfExistsSlow(document);
 }
 
-inline bool Fullscreen::isCurrentFullScreenElement(const Element& element) {
+inline bool Fullscreen::isFullscreenElement(const Element& element) {
   if (Fullscreen* found = fromIfExists(element.document()))
-    return found->currentFullScreenElement() == &element;
+    return found->fullscreenElement() == &element;
   return false;
 }