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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 187193002: Fix an issue that subtitle is not shown for native fullscreen video that is inside an iframe (Closed)

Created:
6 years, 9 months ago by qinmin
Modified:
6 years, 9 months ago
CC:
blink-reviews, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, pdr.
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Fix an issue that subtitle is not shown for native fullscreen video that is inside an iframe Subtitle support for native fullscreen video is enabled in https://codereview.chromium.org/22454003/. However, there are a couple bugs: 1. FullscreenElementStack::currentFullScreenElementFrom() returns NULL if the fullscreen element is in a child frame, we need to use FullscreenElementStack::fullscreenElementFrom(). 2. When the video element is appended to the top level frame, we should exclude it when rebuilding the compositing layer tree. A new layout test is added: fullscreen/full-screen-iframe-allowed-video.html This CL also fixes a bug that some of the fullscreen pixel tests are no longer running due to https://codereview.chromium.org/24438004 BUG=349222, 347843 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168578

Patch Set 1 #

Total comments: 12

Patch Set 2 : addressing comments #

Total comments: 4

Patch Set 3 : nits #

Patch Set 4 : rebase #

Patch Set 5 : rebase virtual/android/fullscreen test expectations #

Messages

Total messages: 19 (0 generated)
qinmin
PTAL
6 years, 9 months ago (2014-03-05 00:15:28 UTC) #1
esprehn
https://codereview.chromium.org/187193002/diff/1/LayoutTests/fullscreen/full-screen-iframe-allowed-video.html File LayoutTests/fullscreen/full-screen-iframe-allowed-video.html (right): https://codereview.chromium.org/187193002/diff/1/LayoutTests/fullscreen/full-screen-iframe-allowed-video.html#newcode3 LayoutTests/fullscreen/full-screen-iframe-allowed-video.html:3: <head> Leave out <html>, <head> and <body> then this ...
6 years, 9 months ago (2014-03-05 00:22:40 UTC) #2
qinmin
https://codereview.chromium.org/187193002/diff/1/LayoutTests/fullscreen/full-screen-iframe-allowed-video.html File LayoutTests/fullscreen/full-screen-iframe-allowed-video.html (right): https://codereview.chromium.org/187193002/diff/1/LayoutTests/fullscreen/full-screen-iframe-allowed-video.html#newcode3 LayoutTests/fullscreen/full-screen-iframe-allowed-video.html:3: <head> On 2014/03/05 00:22:41, esprehn wrote: > Leave out ...
6 years, 9 months ago (2014-03-05 01:34:42 UTC) #3
esprehn
lgtm, please fix nits before landing. https://codereview.chromium.org/187193002/diff/10001/LayoutTests/fullscreen/full-screen-iframe-allowed-video.html File LayoutTests/fullscreen/full-screen-iframe-allowed-video.html (right): https://codereview.chromium.org/187193002/diff/10001/LayoutTests/fullscreen/full-screen-iframe-allowed-video.html#newcode23 LayoutTests/fullscreen/full-screen-iframe-allowed-video.html:23: else { else ...
6 years, 9 months ago (2014-03-05 04:36:03 UTC) #4
qinmin
https://codereview.chromium.org/187193002/diff/10001/LayoutTests/fullscreen/full-screen-iframe-allowed-video.html File LayoutTests/fullscreen/full-screen-iframe-allowed-video.html (right): https://codereview.chromium.org/187193002/diff/10001/LayoutTests/fullscreen/full-screen-iframe-allowed-video.html#newcode23 LayoutTests/fullscreen/full-screen-iframe-allowed-video.html:23: else { On 2014/03/05 04:36:03, esprehn wrote: > else ...
6 years, 9 months ago (2014-03-05 18:45:25 UTC) #5
qinmin
The CQ bit was checked by qinmin@chromium.org
6 years, 9 months ago (2014-03-05 18:45:32 UTC) #6
qinmin
The CQ bit was unchecked by qinmin@chromium.org
6 years, 9 months ago (2014-03-05 18:45:44 UTC) #7
qinmin
The CQ bit was checked by qinmin@chromium.org
6 years, 9 months ago (2014-03-05 18:46:44 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/187193002/30001
6 years, 9 months ago (2014-03-05 18:47:25 UTC) #9
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-05 18:47:31 UTC) #10
commit-bot: I haz the power
Failed to apply patch for LayoutTests/TestExpectations: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
6 years, 9 months ago (2014-03-05 18:47:32 UTC) #11
qinmin
The CQ bit was checked by qinmin@chromium.org
6 years, 9 months ago (2014-03-05 18:52:50 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/187193002/50001
6 years, 9 months ago (2014-03-05 18:53:03 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/187193002/50001
6 years, 9 months ago (2014-03-05 21:00:40 UTC) #14
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-05 21:22:04 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink
6 years, 9 months ago (2014-03-05 21:22:05 UTC) #16
qinmin
The CQ bit was checked by qinmin@chromium.org
6 years, 9 months ago (2014-03-05 21:36:17 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/187193002/60010
6 years, 9 months ago (2014-03-05 21:36:26 UTC) #18
commit-bot: I haz the power
6 years, 9 months ago (2014-03-06 05:54:41 UTC) #19
Message was sent while issue was closed. 
Change committed as 168578

Powered by Google App Engine
This is Rietveld 408576698