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

Issue 482543002: Ignore fullscreen requests for the current fullscreen element (Closed)

Created:
6 years, 4 months ago by philipj_slow
Modified:
6 years, 4 months ago
Reviewers:
falken
CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, sof, eae+blinkwatch, rwlbuis
Project:
blink
Visibility:
Public.

Description

Ignore fullscreen requests for the current fullscreen element http://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen This spec change was made in order to simplify the fix for mis-nested fullscreen in iframes: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26568 (comments 9-13) The requestFullscreen() implementation is not in sync with the spec, but an early return is equivalent until the "run the remaining steps asynchronously" bit is implemented. In order to be affected by this change, one would have to request fullscreen for the same element twice and do something meaningful in the fullscreenerror event, which is likely rare. Note that exitFullscreen() already does nothing if the fullscreen element stack is empty, so there is some symmetry to this. TEST=LayoutTests/fullscreen/api/element-request-fullscreen-top.html TEST=LayoutTests/fullscreen/api/element-request-fullscreen-non-top.html BUG=403741 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180444

Patch Set 1 #

Patch Set 2 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -23 lines) Patch
A + LayoutTests/fullscreen/api/element-request-fullscreen-non-top.html View 2 chunks +3 lines, -7 lines 0 comments Download
A LayoutTests/fullscreen/api/element-request-fullscreen-top.html View 1 chunk +27 lines, -0 lines 0 comments Download
M LayoutTests/fullscreen/full-screen-twice-newapi.html View 1 chunk +0 lines, -12 lines 0 comments Download
M LayoutTests/fullscreen/full-screen-twice-newapi-expected.txt View 1 chunk +0 lines, -4 lines 0 comments Download
M Source/core/dom/Fullscreen.cpp View 1 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
philipj_slow
PTAL. Baby steps to make regressions easier to bisect.
6 years, 4 months ago (2014-08-16 22:16:42 UTC) #1
philipj_slow
https://codereview.chromium.org/485443002/ is the next step, but it doesn't have tests yet.
6 years, 4 months ago (2014-08-16 22:37:23 UTC) #2
falken
lgtm
6 years, 4 months ago (2014-08-18 02:58:44 UTC) #3
philipj_slow
rebase
6 years, 4 months ago (2014-08-18 08:56:15 UTC) #4
philipj_slow
The CQ bit was checked by philipj@opera.com
6 years, 4 months ago (2014-08-18 08:59:18 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/philipj@opera.com/482543002/20001
6 years, 4 months ago (2014-08-18 09:00:28 UTC) #6
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_blink_rel on tryserver.blink ...
6 years, 4 months ago (2014-08-18 10:13:29 UTC) #7
commit-bot: I haz the power
6 years, 4 months ago (2014-08-18 10:56:15 UTC) #8
Message was sent while issue was closed.
Committed patchset #2 (20001) as 180444

Powered by Google App Engine
This is Rietveld 408576698