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

Issue 291163004: Implement media cast buttons (Closed)

Created:
6 years, 7 months ago by aberent
Modified:
6 years, 3 months ago
CC:
darktears, apavlov+blink_chromium.org, whywhat, blink-reviews, blink-reviews-css, blink-reviews-html_chromium.org, blink-reviews-rendering, dglazkov+blink, eae+blinkwatch, ed+blinkwatch_opera.com, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, jchaffraix+rendering, leviw+renderwatch, pdr., philipj_slow, rwlbuis, rune+blink, nessy, vcarbune.chromium, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Implement media cast buttons This implements a Cast button in the default media controls, and a second, overlayed Cast button on videos that don't use the default controls. Pressing the Cast button asks the browser UI to bring up cast device selection menu. If a device is selected the video or audio will be played on that device, rather than locally. The Cast buttons are only displayed when there are cast devices available. Currently Blink will only ever be told that there are cast devices available by Chrome for Android. BUG=390125 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182077

Patch Set 1 #

Total comments: 1

Patch Set 2 : Fix CSS #

Patch Set 3 : Working version, but not yet complete #

Patch Set 4 : Update for infomation and storage while I wait for feedback on crbug/388738 #

Patch Set 5 : Almost done - still needs layout tests #

Patch Set 6 : Add layout tests #

Patch Set 7 : Tidy up button name #

Total comments: 32

Patch Set 8 : Respond to review comments #

Patch Set 9 : Make sure cast button intercepts touches #

Total comments: 8

Patch Set 10 : Respond to comments #

Total comments: 4

Patch Set 11 : Fix acolwell@'s nits. #

Total comments: 4

Patch Set 12 : Fix abarth@'s nits. #

Patch Set 13 : Fix layout tests #

Patch Set 14 : Fix layout tests #

Patch Set 15 : Rebase to fix merge problem with TestExpectations #

Total comments: 8

Patch Set 16 : Fix or mark as crashing remaining layout tests #

Total comments: 1

Patch Set 17 : Reenable previously crashing tests - now fixed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+549 lines, -17 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +43 lines, -1 line 0 comments Download
A LayoutTests/media/controls-cast-button.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +86 lines, -0 lines 0 comments Download
A LayoutTests/media/controls-overlay-cast-button.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +62 lines, -0 lines 0 comments Download
M LayoutTests/media/media-captions-no-controls.html View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +0 lines, -2 lines 0 comments Download
M LayoutTests/media/video-controls-overlay-play-button.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +9 lines, -1 line 0 comments Download
M Source/core/accessibility/AXMediaControls.cpp View 1 2 3 4 5 6 7 2 chunks +8 lines, -0 lines 0 comments Download
M Source/core/css/CSSPrimitiveValueMappings.h View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download
M Source/core/css/CSSSelector.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/css/CSSValueKeywords.in View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/css/mediaControls.css View 1 2 3 4 5 6 7 8 9 2 chunks +32 lines, -0 lines 0 comments Download
M Source/core/css/mediaControlsAndroid.css View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +6 lines, -0 lines 0 comments Download
M Source/core/html/HTMLMediaElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 4 chunks +11 lines, -0 lines 0 comments Download
M Source/core/html/HTMLMediaElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 8 chunks +52 lines, -3 lines 0 comments Download
M Source/core/html/shadow/MediaControlElementTypes.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +4 lines, -0 lines 0 comments Download
M Source/core/html/shadow/MediaControlElements.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +21 lines, -0 lines 0 comments Download
M Source/core/html/shadow/MediaControlElements.cpp View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +56 lines, -0 lines 0 comments Download
M Source/core/html/shadow/MediaControls.h View 1 2 3 4 5 6 7 8 9 3 chunks +8 lines, -0 lines 0 comments Download
M Source/core/html/shadow/MediaControls.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 11 chunks +74 lines, -9 lines 0 comments Download
M Source/core/rendering/RenderMediaControls.cpp View 1 2 3 4 5 6 7 8 9 10 2 chunks +20 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderTheme.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/rendering/RenderTheme.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderThemeChromiumSkia.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/rendering/RenderThemeChromiumSkia.cpp View 1 2 3 4 5 6 7 8 9 10 11 15 16 1 chunk +5 lines, -0 lines 0 comments Download
M Source/core/testing/Internals.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/testing/Internals.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +5 lines, -0 lines 0 comments Download
M Source/core/testing/Internals.idl View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
M Source/platform/ThemeTypes.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M Source/web/WebMediaPlayerClientImpl.h View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download
M Source/web/WebMediaPlayerClientImpl.cpp View 1 2 3 4 5 6 7 1 chunk +15 lines, -0 lines 0 comments Download
M public/platform/WebLocalizedString.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +4 lines, -0 lines 0 comments Download
M public/platform/WebMediaPlayer.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M public/platform/WebMediaPlayerClient.h View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 32 (6 generated)
Peter Beverloo
https://codereview.chromium.org/291163004/diff/1/Source/core/css/mediaControls.css File Source/core/css/mediaControls.css (right): https://codereview.chromium.org/291163004/diff/1/Source/core/css/mediaControls.css#newcode281 Source/core/css/mediaControls.css:281: -webkit-appearance: media-enter-cast-off-button; You're using the |media-enter-cast-off-button| keyword here, while ...
6 years, 7 months ago (2014-05-20 11:24:46 UTC) #1
aberent
Still for information only, but getting closer to final version.
6 years, 6 months ago (2014-06-23 13:43:55 UTC) #2
aberent
Blink changes, work in progress.
6 years, 6 months ago (2014-06-23 13:49:02 UTC) #3
aberent
On 2014/06/23 13:49:02, aberent wrote: > Blink changes, work in progress. The corresponding Chromium CL ...
6 years, 4 months ago (2014-08-04 16:14:53 UTC) #4
aberent
On 2014/06/23 13:49:02, aberent wrote: > Blink changes, work in progress. The corresponding Chromium CL ...
6 years, 4 months ago (2014-08-04 16:14:55 UTC) #5
aberent
Blink side. Has no effect until the Chromium and Clank CLs also land.
6 years, 4 months ago (2014-08-04 16:20:46 UTC) #6
acolwell GONE FROM CHROMIUM
https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/controls-cast-button.html File LayoutTests/media/controls-cast-button.html (right): https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/controls-cast-button.html#newcode14 LayoutTests/media/controls-cast-button.html:14: findMediaElement(); I'm pretty sure you need to put this ...
6 years, 4 months ago (2014-08-05 19:36:35 UTC) #7
aberent
On 2014/08/05 19:36:35, acolwell wrote: > https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/controls-cast-button.html > File LayoutTests/media/controls-cast-button.html (right): > > https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/controls-cast-button.html#newcode14 > ...
6 years, 4 months ago (2014-08-12 17:25:35 UTC) #8
aberent
PTAL https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/controls-cast-button.html File LayoutTests/media/controls-cast-button.html (right): https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/controls-cast-button.html#newcode14 LayoutTests/media/controls-cast-button.html:14: findMediaElement(); On 2014/08/05 19:36:34, acolwell wrote: > I'm ...
6 years, 4 months ago (2014-08-22 14:08:34 UTC) #9
aberent
abarth@chromium.org: Owner review please of changes in Source/platform, Source/web, and public/Platform; acolwell@ is doing the ...
6 years, 4 months ago (2014-08-22 14:25:10 UTC) #10
abarth-chromium
https://codereview.chromium.org/291163004/diff/150001/Source/core/css/CSSValueKeywords.in File Source/core/css/CSSValueKeywords.in (right): https://codereview.chromium.org/291163004/diff/150001/Source/core/css/CSSValueKeywords.in#newcode657 Source/core/css/CSSValueKeywords.in:657: media-overlay-cast-on-button Don't these need to be prefixed with -internal- ...
6 years, 4 months ago (2014-08-23 05:53:15 UTC) #11
acolwell GONE FROM CHROMIUM
https://codereview.chromium.org/291163004/diff/150001/Source/core/html/HTMLMediaElement.cpp File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/291163004/diff/150001/Source/core/html/HTMLMediaElement.cpp#newcode2287 Source/core/html/HTMLMediaElement.cpp:2287: if (!m_player) Please add code to clearMediaPlayer() and createMediaPlayer() ...
6 years, 3 months ago (2014-09-06 00:32:02 UTC) #12
aberent
PTAL https://codereview.chromium.org/291163004/diff/150001/Source/core/css/CSSValueKeywords.in File Source/core/css/CSSValueKeywords.in (right): https://codereview.chromium.org/291163004/diff/150001/Source/core/css/CSSValueKeywords.in#newcode657 Source/core/css/CSSValueKeywords.in:657: media-overlay-cast-on-button On 2014/08/23 05:53:15, abarth wrote: > Don't ...
6 years, 3 months ago (2014-09-09 18:35:30 UTC) #13
acolwell GONE FROM CHROMIUM
lgtm % comments https://codereview.chromium.org/291163004/diff/170001/Source/core/html/shadow/MediaControls.cpp File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/291163004/diff/170001/Source/core/html/shadow/MediaControls.cpp#newcode373 Source/core/html/shadow/MediaControls.cpp:373: if (!mediaElement().shouldShowControls() && !mediaElement().autoplay() && mediaElement().paused()) ...
6 years, 3 months ago (2014-09-10 23:35:24 UTC) #14
aberent
https://codereview.chromium.org/291163004/diff/170001/Source/core/html/shadow/MediaControls.cpp File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/291163004/diff/170001/Source/core/html/shadow/MediaControls.cpp#newcode373 Source/core/html/shadow/MediaControls.cpp:373: if (!mediaElement().shouldShowControls() && !mediaElement().autoplay() && mediaElement().paused()) { On 2014/09/10 ...
6 years, 3 months ago (2014-09-11 16:38:19 UTC) #15
abarth-chromium
LGTM https://codereview.chromium.org/291163004/diff/190001/Source/core/html/shadow/MediaControlElements.cpp File Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/291163004/diff/190001/Source/core/html/shadow/MediaControlElements.cpp#newcode218 Source/core/html/shadow/MediaControlElements.cpp:218: if (event && (event->type() == EventTypeNames::click || event->type() ...
6 years, 3 months ago (2014-09-11 16:42:54 UTC) #16
abarth-chromium
LGTM
6 years, 3 months ago (2014-09-11 16:42:57 UTC) #17
aberent
https://codereview.chromium.org/291163004/diff/190001/Source/core/html/shadow/MediaControlElements.cpp File Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/291163004/diff/190001/Source/core/html/shadow/MediaControlElements.cpp#newcode218 Source/core/html/shadow/MediaControlElements.cpp:218: if (event && (event->type() == EventTypeNames::click || event->type() == ...
6 years, 3 months ago (2014-09-11 17:08:32 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/291163004/210001
6 years, 3 months ago (2014-09-11 17:09:24 UTC) #20
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/24459)
6 years, 3 months ago (2014-09-11 18:36:31 UTC) #22
aberent
On 2014/09/11 18:36:31, I haz the power (commit-bot) wrote: > Try jobs failed on following ...
6 years, 3 months ago (2014-09-15 11:00:10 UTC) #25
Peter Beverloo
The test failure is odd, I haven't been able to spot the error yet. A ...
6 years, 3 months ago (2014-09-15 12:59:38 UTC) #27
aberent
https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow/MediaControlElementTypes.h File Source/core/html/shadow/MediaControlElementTypes.h (right): https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow/MediaControlElementTypes.h#newcode70 Source/core/html/shadow/MediaControlElementTypes.h:70: On 2014/09/15 12:59:38, Peter Beverloo wrote: > micro nit: ...
6 years, 3 months ago (2014-09-16 09:44:18 UTC) #28
aberent
https://codereview.chromium.org/291163004/diff/330001/LayoutTests/TestExpectations File LayoutTests/TestExpectations (right): https://codereview.chromium.org/291163004/diff/330001/LayoutTests/TestExpectations#newcode692 LayoutTests/TestExpectations:692: crbug.com/414373 media/media-reparent.html [ Crash ] These last two will ...
6 years, 3 months ago (2014-09-16 09:47:12 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/291163004/350001
6 years, 3 months ago (2014-09-16 16:44:26 UTC) #31
commit-bot: I haz the power
6 years, 3 months ago (2014-09-16 18:16:58 UTC) #32
Message was sent while issue was closed.
Committed patchset #17 (id:350001) as 182077

Powered by Google App Engine
This is Rietveld 408576698