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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1363023005: Implement FullScreen using top layer. (Closed)

Created:
5 years, 3 months ago by dsinclair
Modified:
5 years, 2 months ago
CC:
aelias_OOO_until_Jul13, blink-reviews, bokan, dcheng
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Implement FullScreen using top layer. This CL is based off of https://codereview.chromium.org/788073004 by jchaffraix@. Replace RenderFullscreen with top layer - Take II This patch is strongly inspired by https://codereview.chromium.org/134753003/ but is a complete rewrite of the change to: - understand why every bits was needed (some were not). - better document the differences we have with the specification so that we can track and fix them. The issue with re-attaching plugins went away due to https://codereview.chromium.org/23618022, which moved the plugin's lifecycle to the DOM. The tests' changes are because fullscreen doesn't center by default anymore but stretches the element to the viewport's size. Some tests were also not accounting for the backdrop and where thus modified. That change didn't land due to plugin persistence issues. Those issues should now be resolved and plugins should survive over reattach. BUG=240576, 246077, 398599 ORIGINAL_CL=https://codereview.chromium.org/1139033006/ Committed: https://crrev.com/6fe9d9762eb66a24bbbd3a5c2badf6e5ea98d1e2 Cr-Commit-Position: refs/heads/master@{#353782}

Patch Set 1 #

Total comments: 40

Patch Set 2 : #

Total comments: 2

Patch Set 3 : #

Total comments: 23

Patch Set 4 : #

Total comments: 2

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : Re-add accidentally removed style #

Total comments: 16

Patch Set 8 : Rebase to master #

Patch Set 9 #

Patch Set 10 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+326 lines, -541 lines) Patch
M third_party/WebKit/LayoutTests/fast/css/invalidation/fullscreen.html View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/invalidation/fullscreen-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fullscreen/enter-exit-full-screen-hover.html View 2 chunks +9 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fullscreen/full-screen-iframe-zIndex-expected.html View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html View 1 2 3 1 chunk +37 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer-expected.html View 1 1 chunk +9 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fullscreen/full-screen-render-inline-expected.html View 1 chunk +1 line, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/fullscreen/full-screen-stacking-context.html View 1 2 3 4 5 1 chunk +51 lines, -56 lines 0 comments Download
M third_party/WebKit/LayoutTests/fullscreen/full-screen-stacking-context-expected.html View 1 2 3 4 5 6 1 chunk +16 lines, -8 lines 0 comments Download
A third_party/WebKit/LayoutTests/fullscreen/full-screen-video-has-backdrop.html View 1 2 3 4 5 6 7 8 1 chunk +44 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fullscreen/full-screen-video-has-backdrop-expected.html View 1 2 3 4 5 1 chunk +15 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fullscreen/full-screen-zIndex-expected.html View 1 chunk +5 lines, -6 lines 0 comments Download
M third_party/WebKit/LayoutTests/fullscreen/parent-flow-inline-with-block-child-expected.html View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/fullscreen/resources/empty.html View 1 chunk +7 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fullscreen/video-webkit-transform-expected.html View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/core.gypi View 1 2 3 4 5 6 7 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/css/fullscreen.css View 1 2 3 4 5 6 7 8 9 1 chunk +26 lines, -46 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp View 1 2 3 4 3 chunks +14 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 1 chunk +6 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Fullscreen.h View 1 2 3 4 5 6 7 8 4 chunks +5 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Fullscreen.cpp View 1 2 3 4 5 6 7 8 8 chunks +13 lines, -47 lines 0 comments Download
M third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp View 2 chunks +0 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBlock.cpp View 1 2 3 4 5 6 7 1 chunk +1 line, -5 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBox.h View 1 2 3 4 5 6 7 2 chunks +4 lines, -4 lines 0 comments Download
D third_party/WebKit/Source/core/layout/LayoutFullScreen.h View 1 chunk +0 lines, -63 lines 0 comments Download
D third_party/WebKit/Source/core/layout/LayoutFullScreen.cpp View 1 chunk +0 lines, -203 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutInline.cpp View 1 2 3 4 2 chunks +0 lines, -12 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutMenuList.cpp View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutObject.h View 1 2 3 4 5 6 7 3 chunks +1 line, -7 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutObject.cpp View 1 2 3 4 5 6 7 1 chunk +0 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutVideo.h View 1 2 3 4 1 chunk +0 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutVideo.cpp View 1 2 3 4 2 chunks +0 lines, -42 lines 0 comments Download
M third_party/WebKit/Source/web/FullscreenController.h View 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/web/FullscreenController.cpp View 1 2 3 4 5 6 7 8 3 chunks +17 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/web/tests/WebFrameTest.cpp View 1 2 3 4 5 4 chunks +38 lines, -3 lines 0 comments Download

Messages

Total messages: 25 (6 generated)
esprehn
https://codereview.chromium.org/1363023005/diff/1/third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer-expected.html File third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer-expected.html (right): https://codereview.chromium.org/1363023005/diff/1/third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer-expected.html#newcode8 third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer-expected.html:8: <body> leave out body https://codereview.chromium.org/1363023005/diff/1/third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html File third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html (right): https://codereview.chromium.org/1363023005/diff/1/third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html#newcode3 ...
5 years, 3 months ago (2015-09-24 20:17:02 UTC) #2
philipj_slow
https://codereview.chromium.org/1363023005/diff/1/third_party/WebKit/Source/core/css/fullscreen.css File third_party/WebKit/Source/core/css/fullscreen.css (right): https://codereview.chromium.org/1363023005/diff/1/third_party/WebKit/Source/core/css/fullscreen.css#newcode15 third_party/WebKit/Source/core/css/fullscreen.css:15: object-fit: contain !important; Why not transform:none !important here? So ...
5 years, 2 months ago (2015-09-28 09:06:46 UTC) #4
dsinclair
Review comments from original CL are included in this change as well. https://codereview.chromium.org/1363023005/diff/1/third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer-expected.html File third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer-expected.html ...
5 years, 2 months ago (2015-09-28 17:14:58 UTC) #5
dsinclair
+rune, +leviw, +jchaffraix as they reviewed the original CL as well.
5 years, 2 months ago (2015-09-28 17:15:55 UTC) #7
dsinclair
https://codereview.chromium.org/1363023005/diff/20001/third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html File third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html (right): https://codereview.chromium.org/1363023005/diff/20001/third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html#newcode29 third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html:29: testRunner.notifyDone(); Looks like I broke the test and it's ...
5 years, 2 months ago (2015-09-28 18:19:36 UTC) #8
rune
On 2015/09/28 17:15:55, dsinclair wrote: > +rune, +leviw, +jchaffraix as they reviewed the original CL ...
5 years, 2 months ago (2015-09-28 19:07:39 UTC) #9
dsinclair
https://codereview.chromium.org/1363023005/diff/40001/third_party/WebKit/Source/web/FullscreenController.cpp File third_party/WebKit/Source/web/FullscreenController.cpp (right): https://codereview.chromium.org/1363023005/diff/40001/third_party/WebKit/Source/web/FullscreenController.cpp#newcode140 third_party/WebKit/Source/web/FullscreenController.cpp:140: element->document().addToTopLayer(element); This fixes up the WebView unit test crash ...
5 years, 2 months ago (2015-09-28 19:22:04 UTC) #10
esprehn
https://codereview.chromium.org/1363023005/diff/40001/third_party/WebKit/Source/web/FullscreenController.cpp File third_party/WebKit/Source/web/FullscreenController.cpp (right): https://codereview.chromium.org/1363023005/diff/40001/third_party/WebKit/Source/web/FullscreenController.cpp#newcode140 third_party/WebKit/Source/web/FullscreenController.cpp:140: element->document().addToTopLayer(element); On 2015/09/28 at 19:22:04, dsinclair wrote: > This ...
5 years, 2 months ago (2015-09-28 20:33:48 UTC) #11
philipj_slow
lgtm % nits https://codereview.chromium.org/1363023005/diff/40001/third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html File third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html (right): https://codereview.chromium.org/1363023005/diff/40001/third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html#newcode21 third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html:21: var element = document.getElementById('elem'); This looks ...
5 years, 2 months ago (2015-09-29 08:05:23 UTC) #12
philipj_slow
https://codereview.chromium.org/1363023005/diff/40001/third_party/WebKit/Source/core/css/fullscreen.css File third_party/WebKit/Source/core/css/fullscreen.css (right): https://codereview.chromium.org/1363023005/diff/40001/third_party/WebKit/Source/core/css/fullscreen.css#newcode24 third_party/WebKit/Source/core/css/fullscreen.css:24: :-webkit-full-screen::backdrop { The spec was just changed, so this ...
5 years, 2 months ago (2015-09-29 12:31:52 UTC) #13
Julien - ping for review
lgtm. Missing from the original patch are: - the 'final' changes on LayoutBox's functions. - ...
5 years, 2 months ago (2015-10-01 00:24:41 UTC) #14
dsinclair
https://chromiumcodereview.appspot.com/1363023005/diff/40001/third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html File third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html (right): https://chromiumcodereview.appspot.com/1363023005/diff/40001/third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html#newcode21 third_party/WebKit/LayoutTests/fullscreen/full-screen-is-in-top-layer.html:21: var element = document.getElementById('elem'); On 2015/09/29 08:05:22, philipj wrote: ...
5 years, 2 months ago (2015-10-01 14:50:14 UTC) #15
philipj_slow
Still LGTM, very excited to see this happen :) https://chromiumcodereview.appspot.com/1363023005/diff/40001/third_party/WebKit/Source/core/css/fullscreen.css File third_party/WebKit/Source/core/css/fullscreen.css (right): https://chromiumcodereview.appspot.com/1363023005/diff/40001/third_party/WebKit/Source/core/css/fullscreen.css#newcode24 third_party/WebKit/Source/core/css/fullscreen.css:24: ...
5 years, 2 months ago (2015-10-01 15:00:23 UTC) #16
dsinclair
esprehn@ PTAL. I believe I've got all the tests fixed (just waiting on win layout ...
5 years, 2 months ago (2015-10-07 17:09:51 UTC) #18
esprehn
lgtm, looks legit! Very exciting to see us get rid of the all that FullScreen ...
5 years, 2 months ago (2015-10-09 10:34:00 UTC) #19
dsinclair
https://codereview.chromium.org/1363023005/diff/140001/third_party/WebKit/LayoutTests/fullscreen/full-screen-video-has-backdrop.html File third_party/WebKit/LayoutTests/fullscreen/full-screen-video-has-backdrop.html (right): https://codereview.chromium.org/1363023005/diff/140001/third_party/WebKit/LayoutTests/fullscreen/full-screen-video-has-backdrop.html#newcode35 third_party/WebKit/LayoutTests/fullscreen/full-screen-video-has-backdrop.html:35: document.removeEventListener("keypress", thunk, false); On 2015/10/09 10:33:59, esprehn wrote: > ...
5 years, 2 months ago (2015-10-13 15:50:43 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1363023005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1363023005/200001
5 years, 2 months ago (2015-10-13 16:00:16 UTC) #23
commit-bot: I haz the power
Committed patchset #10 (id:200001)
5 years, 2 months ago (2015-10-13 17:49:49 UTC) #24
commit-bot: I haz the power
5 years, 2 months ago (2015-10-13 17:51:01 UTC) #25
Message was sent while issue was closed. 
Patchset 10 (id:??) landed as
https://crrev.com/6fe9d9762eb66a24bbbd3a5c2badf6e5ea98d1e2
Cr-Commit-Position: refs/heads/master@{#353782}

Powered by Google App Engine
This is Rietveld 408576698