Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(120)

Issue 2008873004: Reland: Add support for entering/exiting HTML fullscreen from OOPIFs. (Closed)

Created:
4 years, 7 months ago by alexmos
Modified:
4 years, 6 months ago
Reviewers:
Charlie Reis, dcheng
CC:
dcheng, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-dom_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, eae+blinkwatch, jam, kinuko+watch, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-blink_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, rwlbuis, sof, site-isolation-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Reland: 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 This is a reland of https://codereview.chromium.org/1914643005/, with the tests fixed for Mac by introducing an observer that waits for the browser to finish the fullscreen transition (see issue 614304). BUG=550497 TBR=dcheng@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/1f7eac4a2affa46f6c7eb53aba6a5baa72375e4c Cr-Commit-Position: refs/heads/master@{#396027}

Patch Set 1 : Original CL #

Patch Set 2 : Disable tests for Mac #

Patch Set 3 : Fix tests on Mac #

Unified diffs Side-by-side diffs Delta from patch set Stats (+696 lines, -18 lines) Patch
M chrome/browser/site_per_process_interactive_browsertest.cc View 1 2 3 chunks +487 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 2 chunks +39 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 chunk +1 line, -3 lines 0 comments Download
M content/common/frame_messages.h View 1 chunk +6 lines, -0 lines 0 comments Download
M content/renderer/render_frame_proxy.h View 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/render_frame_proxy.cc View 2 chunks +5 lines, -0 lines 0 comments Download
A content/test/data/fullscreen_frame.html View 1 chunk +48 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Element.cpp View 1 chunk +19 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Fullscreen.h View 1 chunk +8 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Fullscreen.cpp View 4 chunks +15 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/web/FullscreenController.h View 2 chunks +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/FullscreenController.cpp View 4 chunks +11 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp View 2 chunks +4 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/web/WebViewImpl.h View 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebViewImpl.cpp View 3 chunks +35 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebView.h View 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 18 (9 generated)
alexmos
Charlie, can you please take a look? I think I've fixed the tests for Mac: ...
4 years, 6 months ago (2016-05-25 17:23:32 UTC) #3
Charlie Reis
Nice find. Diff from patch 1 to 3 LGTM!
4 years, 6 months ago (2016-05-25 18:20:26 UTC) #5
alexmos
Thanks. TBR-ing dcheng@ since other than the test update, there are no changes compared to ...
4 years, 6 months ago (2016-05-25 18:24:22 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008873004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008873004/40001
4 years, 6 months ago (2016-05-25 18:25:12 UTC) #9
dcheng
lgtm
4 years, 6 months ago (2016-05-25 18:25:19 UTC) #10
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/227993)
4 years, 6 months ago (2016-05-25 20:37:52 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008873004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008873004/40001
4 years, 6 months ago (2016-05-25 20:39:44 UTC) #14
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 6 months ago (2016-05-25 23:05:10 UTC) #16
commit-bot: I haz the power
4 years, 6 months ago (2016-05-25 23:06:29 UTC) #18
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/1f7eac4a2affa46f6c7eb53aba6a5baa72375e4c
Cr-Commit-Position: refs/heads/master@{#396027}

Powered by Google App Engine
This is Rietveld 408576698