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

third_party/WebKit/Source/web/FullscreenController.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 70 matching lines @@
   // Notify all local frames that we have entered fullscreen.
   for (Frame* frame = m_webViewImpl->page()->mainFrame(); frame;
        frame = frame->tree().traverseNext()) {
     if (!frame->isLocalFrame())
       continue;
     if (Document* document = toLocalFrame(frame)->document()) {
       if (Fullscreen* fullscreen = Fullscreen::fromIfExists(*document))
         fullscreen->didEnterFullscreen();
     }
   }
+
+  // TODO(foolip): If the top level browsing context (main frame) ends up with
+  // no fullscreen element, exit fullscreen again to recover.
 }
 
 void FullscreenController::didExitFullscreen() {
   // The browser process can exit fullscreen at any time, e.g. if the user
   // presses Esc. After |Browser::EnterFullscreenModeForTab()|,
   // |Browser::ExitFullscreenModeForTab()| will make it seem like we exit when
   // not even in fullscreen. Do nothing.
   if (m_state == State::Initial)
     return;
 
   updatePageScaleConstraints(true);
 
-  // Set |m_state| so that any |exitFullscreen()| calls from within
-  // |Fullscreen::didExitFullscreen()| do not call
-  // |WebFrameClient::exitFullscreen()| again.
-  // TODO(foolip): Remove this when state changes and events are synchronized
-  // with animation frames. https://crbug.com/402376
-  m_state = State::ExitingFullscreen;
-
-  // Notify all local frames that we have exited fullscreen.
-  // TODO(foolip): This should only need to notify the topmost local roots. That
-  // doesn't currently work because |Fullscreen::m_currentFullScreenElement|
-  // isn't set for the topmost document when an iframe goes fullscreen, but can
-  // be done once |m_currentFullScreenElement| is gone and all state is in the
-  // fullscreen element stack. https://crbug.com/402421
-  for (Frame* frame = m_webViewImpl->page()->mainFrame(); frame;
-       frame = frame->tree().traverseNext()) {
-    if (!frame->isLocalFrame())
-      continue;
-    if (Document* document = toLocalFrame(frame)->document()) {
-      if (Fullscreen* fullscreen = Fullscreen::fromIfExists(*document))
-        fullscreen->didExitFullscreen();
-    }
-  }
-
   // We need to wait until style and layout are updated in order to properly
   // restore scroll offsets since content may not be overflowing in the same way
   // until they are.
   m_state = State::NeedsScrollAndScaleRestore;
+
+  // Notify the topmost local frames that we have exited fullscreen.
+  // |Fullscreen::didExitFullscreen()| will take care of descendant frames.
+  for (Frame* frame = m_webViewImpl->page()->mainFrame(); frame;) {
+    Frame* nextFrame = frame->tree().traverseNext();
+
+    if (frame->isRemoteFrame()) {
+      frame = nextFrame;
+      continue;
+    }
+
+    DCHECK(frame->isLocalRoot());
+    if (Document* document = toLocalFrame(frame)->document()) {
+      if (Fullscreen* fullscreen = Fullscreen::fromIfExists(*document))
+        fullscreen->didExitFullscreen();
+    }
+
+    // Skip over all descendant frames.
+    while (nextFrame && nextFrame->tree().isDescendantOf(frame))
+      nextFrame = nextFrame->tree().traverseNext();
+    frame = nextFrame;
+  }
 }
 
 void FullscreenController::enterFullscreen(LocalFrame& frame) {
   // If already fullscreen or exiting fullscreen, synchronously call
   // |didEnterFullscreen()|. When exiting, the coming |didExitFullscren()| call
   // will again notify all frames.
   if (m_state == State::Fullscreen || m_state == State::ExitingFullscreen) {
     State oldState = m_state;
     m_state = State::EnteringFullscreen;
     didEnterFullscreen();
@@ skipping 13 matching lines @@
                                 : WebSize();
     m_initialVisualViewportOffset = m_webViewImpl->visualViewportOffset();
   }
 
   // If already entering fullscreen, just wait.
   if (m_state == State::EnteringFullscreen)
     return;
 
   DCHECK(m_state == State::Initial ||
          m_state == State::NeedsScrollAndScaleRestore);
   webFrameClient(frame).enterFullscreen();

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

I was surprised this happens unconditionally now.  This used to be gated on
forCrossProcessDescendant(), but looks like that changed in your
https://codereview.chromium.org/2550703002.  It seems this will cause redundant
ToggleFullscreen IPCs to be sent to the browser process from all the
cross-process ancestors, which might trigger more redundant WillEnterFullscreen
IPCs to ancestor renderers. It appears things are still working, probably
because we eventually realize that we're already in fullscreen mode both on
browser and renderer sides, but it'd still be nice to avoid sending those
redundant IPCs here somehow.  Is there a way to still check whether we're making
a request for a cross-process descendant here?

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

I dropped that check in https://codereview.chromium.org/2550703002/ to get rid
of FullscreenController::m_provisionalFullscreenElement, although I didn't stop
to wonder if sending the IPC twice would break things.

It would be easy to pass through forCrossProcessDescendant and in some quick
testing it looks like none of FullscreenController::enterFullscreen except
`m_state = State::EnteringFullscreen` is actually needed to pass
SitePerProcessInteractiveBrowserTest.Fullscreen*, so I wonder if the m_initial*
state matters here.

Since the redundant IPC is already happening I won't avoid it in this CL, but
I've filed https://crbug.com/675475 for what I think is the root problem here.
I've had significant difficulty understanding how things currently actually
work. Perhaps it would be simple enough if documented exhaustively, but it'd
still be pretty weird that WebViewImpl owns the controller when there's more
than one WebViewImpl.

[alexmos, 2016/12/19 22:53:26]

Agreed about no need to fix in this CL since this is already happening.

Yes, it'd be nice to move at least the state tracking from FullscreenController
to the browser process, though I'm not sure what would happen to the code
dealing with video overlays, page scale, etc.  It's interesting that the only
thing needed for the tests right now is the m_state change; it probably helped
that you refactored it to get rid of m_provisionalFullscreenElement, etc. :)  I
don't remember if saving/restoring the m_initial* sizing vars in OOPIF cases was
important for something.  scrollOffset clearly doesn't matter, as it only deals
with local main frames, but not sure about page scale and visual viewport
offset.

I'll mention that another tricky case I remember now was opening devtools when
in fullscreen from an OOPIF.  Unfortunately, I don't think we have a test for
that - I should probably add it.

[foolip, 2016/12/19 23:07:54]

Yeah, the saving/restoring of m_initial* state can't be entirely unaware of the
global fullscreen state, but perhaps it can be triggered by a state transition
in the browser process in a way that the renderer doesn't need to always know
the current global state. (Or if it does, that it's at least merely a mirroring
of the global state, not separately maintained.)

The video overlay stuff should be possible to move into HTMLVideoElement with
enough willpower, it was just convenient (and low risk) to leave on
FullscreenController since it deals entirely with WebViewImpl.
Ah, that's certainly not something I've noticed, a test for whatever it was
would be nice or I'll probably break it :)

 
   m_state = State::EnteringFullscreen;
 }
 
 void FullscreenController::exitFullscreen(LocalFrame& frame) {
   // If not in fullscreen, ignore any attempt to exit. In particular, when
   // entering fullscreen, allow the transition into fullscreen to complete. Note
   // that the browser process is ultimately in control and can still exit
   // fullscreen at any time.
   if (m_state != State::Fullscreen)
     return;
 
   webFrameClient(frame).exitFullscreen();
 
   m_state = State::ExitingFullscreen;
 }
 
 void FullscreenController::fullscreenElementChanged(Element* fromElement,
                                                     Element* toElement) {
   DCHECK_NE(fromElement, toElement);
 
   if (toElement) {
-    DCHECK(Fullscreen::isCurrentFullScreenElement(*toElement));
+    DCHECK(Fullscreen::isFullscreenElement(*toElement));
 
     if (isHTMLVideoElement(*toElement)) {
       HTMLVideoElement& videoElement = toHTMLVideoElement(*toElement);
       videoElement.didEnterFullscreen();
 
       // If the video uses overlay fullscreen mode, make the background
       // transparent.
       if (videoElement.usesOverlayFullscreenVideo() &&
           m_webViewImpl->layerTreeView()) {
         m_webViewImpl->layerTreeView()->setHasTransparentBackground(true);
       }
     }
   }
 
   if (fromElement) {
-    DCHECK(!Fullscreen::isCurrentFullScreenElement(*fromElement));
+    DCHECK(!Fullscreen::isFullscreenElement(*fromElement));
 
     if (isHTMLVideoElement(*fromElement)) {
       // If the video used overlay fullscreen mode, restore the transparency.
       if (m_webViewImpl->layerTreeView()) {
         m_webViewImpl->layerTreeView()->setHasTransparentBackground(
             m_webViewImpl->isTransparent());
       }
 
       HTMLVideoElement& videoElement = toHTMLVideoElement(*fromElement);
       videoElement.didExitFullscreen();
@@ skipping 54 matching lines @@
   // again to ensure the final constraints pick up the latest contents size.
   m_webViewImpl->didChangeContentsSize();
   if (m_webViewImpl->mainFrameImpl() &&
       m_webViewImpl->mainFrameImpl()->frameView())
     m_webViewImpl->mainFrameImpl()->frameView()->setNeedsLayout();
 
   m_webViewImpl->updateMainFrameLayoutSize();
 }
 
 }  // namespace blink