Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

Issue 426593010: Set fullscreenEnabled to false when fullscreen is not supported. (Closed)

Created:
4 years, 6 months ago by Ignacio Solla
Modified:
4 years, 6 months ago
Reviewers:
philipj_slow, dglazkov
CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, sof, eae+blinkwatch, rwlbuis, aelias_OOO_until_Jul13, qinmin
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Set fullscreenEnabled to false when fullscreen is not supported. According to the latest version of the living standard, fullscreenEnabled must be false if fullscreen is not supported: http://fullscreen.spec.whatwg.org/ More details about the latest update to the standard: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26479 https://github.com/whatwg/fullscreen/commit/d35a070d443632d0071b49318e40092667ac9801 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179520

Patch Set 1 #

Total comments: 5

Patch Set 2 : Address review comments #

Total comments: 2

Patch Set 3 : Update test #

Patch Set 4 : Rebase #

Patch Set 5 : Fix mergetool auto updates during rebase #

Patch Set 6 : Rebase #

Patch Set 7 : Rebase against tot #

Unified diffs Side-by-side diffs Delta from patch set Stats (+41 lines, -7 lines) Patch
A LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html View 1 2 1 chunk +17 lines, -0 lines 0 comments Download
A LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported-expected.txt View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M Source/core/dom/FullscreenElementStack.cpp View 1 4 5 3 chunks +19 lines, -7 lines 0 comments Download

Messages

Total messages: 39 (0 generated)
Ignacio Solla
philipj@opera.com: Please review changes in FullscreenElementStack dglazkov@chromium.org: Please review changes in LayoutTests
4 years, 6 months ago (2014-08-01 11:05:38 UTC) #1
philipj_slow
I have to run, but I'll take a look on Monday. Add falken if you ...
4 years, 6 months ago (2014-08-01 11:08:31 UTC) #2
philipj_slow
I have to run, but I'll take a look on Monday. Add falken if you ...
4 years, 6 months ago (2014-08-01 11:08:32 UTC) #3
philipj_slow
LGTM % nits. There's also HTMLVideoElement.webkitSupportsFullscreen, but since that doesn't take the allowfullscreen attributes into ...
4 years, 6 months ago (2014-08-04 09:24:13 UTC) #4
Ignacio Solla
And also renamed fullscreenEnabled to fullscreenSupported https://codereview.chromium.org/426593010/diff/1/LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html File LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html (right): https://codereview.chromium.org/426593010/diff/1/LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html#newcode10 LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html:10: var iframe = ...
4 years, 6 months ago (2014-08-04 11:43:33 UTC) #5
philipj_slow
https://codereview.chromium.org/426593010/diff/1/LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html File LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html (right): https://codereview.chromium.org/426593010/diff/1/LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html#newcode10 LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html:10: var iframe = document.documentElement.appendChild(document.createElement('iframe')); On 2014/08/04 11:43:33, Ignacio Solla ...
4 years, 6 months ago (2014-08-04 12:18:51 UTC) #6
philipj_slow
https://codereview.chromium.org/426593010/diff/20001/LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html File LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html (right): https://codereview.chromium.org/426593010/diff/20001/LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html#newcode2 LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html:2: <div>This tests that the <code>fullscreenSupported</code> property is false when ...
4 years, 6 months ago (2014-08-04 12:23:01 UTC) #7
Ignacio Solla
On 2014/08/04 12:18:51, philipj wrote: > https://codereview.chromium.org/426593010/diff/1/LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html > File LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html > (right): > > https://codereview.chromium.org/426593010/diff/1/LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html#newcode10 ...
4 years, 6 months ago (2014-08-04 12:31:53 UTC) #8
Ignacio Solla
https://codereview.chromium.org/426593010/diff/20001/LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html File LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html (right): https://codereview.chromium.org/426593010/diff/20001/LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html#newcode2 LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html:2: <div>This tests that the <code>fullscreenSupported</code> property is false when ...
4 years, 6 months ago (2014-08-04 12:31:59 UTC) #9
philipj_slow
LGTM, I hope this can land ASAP since I have conflicting patches brewing :)
4 years, 6 months ago (2014-08-04 12:35:14 UTC) #10
Ignacio Solla
On 2014/08/04 12:35:14, philipj wrote: > LGTM, I hope this can land ASAP since I ...
4 years, 6 months ago (2014-08-04 12:36:43 UTC) #11
philipj_slow
On 2014/08/04 12:36:43, Ignacio Solla wrote: > On 2014/08/04 12:35:14, philipj wrote: > > LGTM, ...
4 years, 6 months ago (2014-08-04 12:39:54 UTC) #12
Ignacio Solla
The CQ bit was checked by igsolla@chromium.org
4 years, 6 months ago (2014-08-04 12:47:34 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/igsolla@chromium.org/426593010/40001
4 years, 6 months ago (2014-08-04 12:47:55 UTC) #14
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink ...
4 years, 6 months ago (2014-08-04 13:47:01 UTC) #15
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
4 years, 6 months ago (2014-08-04 14:10:08 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/19390)
4 years, 6 months ago (2014-08-04 14:10:10 UTC) #17
Ignacio Solla
The CQ bit was checked by igsolla@chromium.org
4 years, 6 months ago (2014-08-04 14:11:56 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/igsolla@chromium.org/426593010/40001
4 years, 6 months ago (2014-08-04 14:12:59 UTC) #19
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink ...
4 years, 6 months ago (2014-08-04 14:35:19 UTC) #20
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
4 years, 6 months ago (2014-08-04 14:57:21 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/19400)
4 years, 6 months ago (2014-08-04 14:57:22 UTC) #22
Ignacio Solla
The CQ bit was checked by igsolla@chromium.org
4 years, 6 months ago (2014-08-04 15:36:08 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/igsolla@chromium.org/426593010/80001
4 years, 6 months ago (2014-08-04 15:37:17 UTC) #24
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
4 years, 6 months ago (2014-08-04 15:37:25 UTC) #25
commit-bot: I haz the power
Failed to apply patch for Source/core/dom/FullscreenElementStack.cpp: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
4 years, 6 months ago (2014-08-04 15:37:26 UTC) #26
Ignacio Solla
On 2014/08/04 15:37:26, I haz the power (commit-bot) wrote: > Failed to apply patch for ...
4 years, 6 months ago (2014-08-04 15:42:26 UTC) #27
philipj_slow
You never have to wait for rolls inside of Blink, try simply rebasing the patch.
4 years, 6 months ago (2014-08-05 05:30:32 UTC) #28
Ignacio Solla
The CQ bit was checked by igsolla@chromium.org
4 years, 6 months ago (2014-08-05 08:58:02 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/igsolla@chromium.org/426593010/100001
4 years, 6 months ago (2014-08-05 08:58:37 UTC) #30
Ignacio Solla
On 2014/08/05 05:30:32, philipj wrote: > You never have to wait for rolls inside of ...
4 years, 6 months ago (2014-08-05 09:05:08 UTC) #31
philipj_slow
On 2014/08/05 09:05:08, Ignacio Solla wrote: > On 2014/08/05 05:30:32, philipj wrote: > > You ...
4 years, 6 months ago (2014-08-05 09:13:05 UTC) #32
Ignacio Solla
On 2014/08/05 09:13:05, philipj wrote: > On 2014/08/05 09:05:08, Ignacio Solla wrote: > > On ...
4 years, 6 months ago (2014-08-05 09:24:57 UTC) #33
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink ...
4 years, 6 months ago (2014-08-05 13:06:22 UTC) #34
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
4 years, 6 months ago (2014-08-05 13:58:54 UTC) #35
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20924)
4 years, 6 months ago (2014-08-05 13:58:55 UTC) #36
Ignacio Solla
The CQ bit was checked by igsolla@chromium.org
4 years, 6 months ago (2014-08-05 14:25:17 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/igsolla@chromium.org/426593010/120001
4 years, 6 months ago (2014-08-05 14:26:56 UTC) #38
commit-bot: I haz the power
4 years, 6 months ago (2014-08-05 20:23:09 UTC) #39
Message was sent while issue was closed.
Change committed as 179520

Powered by Google App Engine
This is Rietveld 408576698