New issue
Advanced search Search tips

Issue 614304 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: ----



Sign in to add a comment

interactive_ui_tests failures on Mac10.10 and Mac10.11

Project Member Reported by kolos@chromium.org, May 24 2016

Issue description

interactive_ui_tests interactive_ui_tests 
failures:
SitePerProcessInteractiveBrowserTest.FullscreenElementInABA
SitePerProcessInteractiveBrowserTest.FullscreenElementInMultipleSubframes
SitePerProcessInteractiveBrowserTest.FullscreenElementInSubframe

Suspect CLs:
* OOPIF: Replicate allowFullscreen flag.
alexmos@chromium.org (https://codereview.chromium.org/1938753002)

* Add support for entering/exiting HTML fullscreen from OOPIFs.
alexmos@chromium.org (https://codereview.chromium.org/1914643005)


 

Comment 1 by kolos@chromium.org, May 24 2016

I will revert the latter CL (Add support...) where the failed tests were created.

Comment 2 by kolos@chromium.org, May 24 2016

Labels: Sheriff-Chromium
Project Member

Comment 3 by bugdroid1@chromium.org, May 24 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ac446a2973988dd917c7934b925c4878dc34bd43

commit ac446a2973988dd917c7934b925c4878dc34bd43
Author: kolos <kolos@chromium.org>
Date: Tue May 24 09:18:31 2016

Revert of Add support for entering/exiting HTML fullscreen from OOPIFs. (patchset #13 id:240001 of https://codereview.chromium.org/1914643005/ )

Reason for revert:
interactive_ui_tests failures on Mac10.10 and Mac10.11 (https://build.chromium.org/p/chromium.mac/builders/Mac10.11%20Tests/builds/1684)

BUG= 614304 

Original issue's description:
> Add support for entering/exiting HTML fullscreen from OOPIFs.
>
> Currently, entering HTML fullscreen utilizes a number of ancestor
> frame walks.  This is because entering fullscreen for an element (1)
> alters layout for all its ancestor <iframes> (by applying
> :-webkit-full-screen styles to them, which applies some UA CSS rules),
> (2) fires fullscreenchange in all ancestor documents on <iframe>
> elements along the ancestor chain, and (3) makes
> document.webkitFullscreenElement return the <iframe> element
> containing the fullscreened element in ancestor frames.
>
> Currently, the logic behind this assumes that all ancestor frames are
> local and does nothing when it encounters a remote frame.  This CL
> takes the first step to fix this.
>
> The existing flow for entering HTML fullscreen goes like this:
> 1. JS on the page calls element.webkitRequestFullscreen()
> 2. Fullscreen::requestFullscreen():
>    a. Checks if fullscreen is allowed by ancestors (allowFullscreen)
>    b. Creates fullscreenchange events for |element| and its ancestor
>       <iframes> and saves them in its m_eventQueue.
>    c. Puts |element| on its fullscreen element stack.
> 3. FullscreenController::enterFullScreenForElement(element)
>    a. Saves |element| as provisional fullscreen element.
>    b. Saves some page sizing info (to be restored after fullscreen)
> 4. RenderFrameImpl::enterFullscreen sends a FrameHostMsg_ToggleFullscreen
>    to browser process.
> 5. RenderFrameHostImpl::OnToggleFullscreen() asks the current tab to
>    enter fullscreen.
> 6. After the tab is resized for fullscreen, browser process generates a
>    ViewMsg_Resize to the main frame renderer, with
>    is_fullscreen_granted=1 in ResizeParams.
> 7. RenderWidget::Resize realizes there's a fullscreen change and calls
>    into FullscreenController::didEnterFullScreen().
> 8. FullscreenController::didEnterFullScreen picks up the stored
>    provisional fullscreen element and calls
>    Fullscreen::didEnterFullScreenForElement on it.
> 9. Fullscreen::didEnterFullScreenForElement(element):
>    a. Adjusts layout on |element| for fullscreen
>    b. Sets fullscreen CSS styles on the fullscreen element and all
>       ancestor elements.
>    c. Fires fullscreenchange events from step 2b.
>
> The main changes in this CL are:
>
> - in step 5, before entering fullscreen for tab, we will send an IPC
>   to ancestor OOPIFs so that they do steps 2 and 3 for their
>   respective ancestor <iframe> elements (i.e., set up provisional
>   fullscreen elements and prepare their share of the fullscreenchange
>   events).
> - ViewMsg_Resize will trigger fullscreen in subframe widgets in
>   addition to main frame ones.  When fullscreen is entered,
>   ViewMsg_Resize is sent to all widgets on the page.  WebFrameWidgets
>   now also support picking up the fullscreen change and calling into
>   the page's FullscreenController.
> - blink::Fullscreen and FullscreenController now support entering
>   fullscreen for <iframe> elements which contain out-of-process
>   fullscreen elements.
>
> There are still various issues to be dealt with in followup CLs:
> - further refactoring ancestor walks in Fullscreen::requestFullscreen
>   and exitFullscreen to be compatible with hierarchies with a
>   RemoteFrame between LocalFrames (such as A-B-A).  This is needed to
>   fire all ancestor fullscreenchange events properly in such cases.
> - dealing with nested fullscreen elements.
> - optimizing fullscreen layout in ancestor processes.
> - correcting background color when fullscreening an OOP <iframe>
>   element.
>
> More info in design doc: https://goo.gl/Y5wdqi
>
> BUG= 550497 
> CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
>
> Committed: https://crrev.com/3cf9343f4602d4ec11717cb6ff56a793c1d5f84b
> Cr-Commit-Position: refs/heads/master@{#395500}

TBR=dcheng@chromium.org,creis@chromium.org,falken@chromium.org,alexmos@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 550497 

Review-Url: https://codereview.chromium.org/1997413003
Cr-Commit-Position: refs/heads/master@{#395553}

[modify] https://crrev.com/ac446a2973988dd917c7934b925c4878dc34bd43/chrome/browser/site_per_process_interactive_browsertest.cc
[modify] https://crrev.com/ac446a2973988dd917c7934b925c4878dc34bd43/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/ac446a2973988dd917c7934b925c4878dc34bd43/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/ac446a2973988dd917c7934b925c4878dc34bd43/content/common/frame_messages.h
[modify] https://crrev.com/ac446a2973988dd917c7934b925c4878dc34bd43/content/renderer/render_frame_proxy.cc
[modify] https://crrev.com/ac446a2973988dd917c7934b925c4878dc34bd43/content/renderer/render_frame_proxy.h
[delete] https://crrev.com/5a2ee1339095755400f7249f6add4e97d29717c6/content/test/data/fullscreen_frame.html
[modify] https://crrev.com/ac446a2973988dd917c7934b925c4878dc34bd43/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/ac446a2973988dd917c7934b925c4878dc34bd43/third_party/WebKit/Source/core/dom/Fullscreen.cpp
[modify] https://crrev.com/ac446a2973988dd917c7934b925c4878dc34bd43/third_party/WebKit/Source/core/dom/Fullscreen.h
[modify] https://crrev.com/ac446a2973988dd917c7934b925c4878dc34bd43/third_party/WebKit/Source/web/FullscreenController.cpp
[modify] https://crrev.com/ac446a2973988dd917c7934b925c4878dc34bd43/third_party/WebKit/Source/web/FullscreenController.h
[modify] https://crrev.com/ac446a2973988dd917c7934b925c4878dc34bd43/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp
[modify] https://crrev.com/ac446a2973988dd917c7934b925c4878dc34bd43/third_party/WebKit/Source/web/WebViewImpl.cpp
[modify] https://crrev.com/ac446a2973988dd917c7934b925c4878dc34bd43/third_party/WebKit/Source/web/WebViewImpl.h
[modify] https://crrev.com/ac446a2973988dd917c7934b925c4878dc34bd43/third_party/WebKit/public/web/WebView.h

Comment 4 by kolos@chromium.org, May 24 2016

Status: Fixed (was: Available)
Cc: creis@chromium.org
I think I know why the tests were failing on Mac.  Mac has a one-second animation when entering fullscreen, and during that animation fullscreen transitions are ignored, according to a comment in BridgedNativeWidget::ToggleDesiredFullscreenState in bridged_native_widget.mm:

  // If there is currently an animation into or out of fullscreen, then AppKit
  // emits the string "not in fullscreen state" to stdio and does nothing. For
  // this case, schedule a transition back into the desired state when the
  // animation completes.

Indeed, the tests attempt to exit fullscreen right after entering it, without waiting for this animation, so the exit fullscreen request is dropped; "not in fullscreen state" also shows up in the failed test output.  I've also confirmed that waiting for an extra second before exiting fullscreen in the tests fixes this on my Mac.  

I'm going to explore how to best fix this, and in the meantime I'll reland the enter/exit fullscreen CL with the test disabled for Mac, as leaving just the first fullscreen CL in exposes isolate-extensions trial users to broken fullscreen behavior.  I'll fix the test in a followup.

Sign in to add a comment