Implement media cast buttons

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

[Peter Beverloo, 2014-05-20 11:24:46]

https://codereview.chromium.org/291163004/diff/1/Source/core/css/mediaControl...
File Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/291163004/diff/1/Source/core/css/mediaControl...
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 defining
|media-cast-off-button| as the keyword in CSSValueKeywords.in. This will cause
it to ignore the declaration as it's an invalid keyword.

Since the CSSValueKeywords.in file generates an enumaration which has to be in
the same order as ControlPart, we cast the the MediaCastOffButton value to
MediaCastOffButtonPart (in
StyleBuilderFunctions::applyValueCSSPropertyWebkitAppearance). This mapping now
doesn't exist, which is why the switch() case in RenderTheme doesn't get hit.

tl;dr: s/media-enter-cast-off-button/media-cast-off-button/.

[aberent, 2014-06-23 13:43:55]

Still for information only, but getting closer to final version.

[aberent, 2014-06-23 13:49:02]

Blink changes, work in progress.

[aberent, 2014-08-04 16:14:53]

On 2014/06/23 13:49:02, aberent wrote:
> Blink changes, work in progress.

The corresponding Chromium CL is https://codereview.chromium.org/349923007/ and
the Clank patch is https://chrome-internal-review.googlesource.com/167055
(Google internal only). These work but both still need some tidying up before
they are ready for review.

[aberent, 2014-08-04 16:14:55]

On 2014/06/23 13:49:02, aberent wrote:
> Blink changes, work in progress.

The corresponding Chromium CL is https://codereview.chromium.org/349923007/ and
the Clank patch is https://chrome-internal-review.googlesource.com/167055
(Google internal only). These work but both still need some tidying up before
they are ready for review.

[aberent, 2014-08-04 16:20:46]

Blink side. Has no effect until the Chromium and Clank CLs also land.

[acolwell GONE FROM CHROMIUM, 2014-08-05 19:36:35]

https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/contr...
File LayoutTests/media/controls-cast-button.html (right):

https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/contr...
LayoutTests/media/controls-cast-button.html:14: findMediaElement();
I'm pretty sure you need to put this code inside a test() call so the assert_xxx
methods work properly. See
http://darobin.github.io/test-harness-tutorial/docs/using-testharness.html

https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/contr...
File LayoutTests/media/controls-overlay-cast-button.html (right):

https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/contr...
LayoutTests/media/controls-overlay-cast-button.html:14: findMediaElement();
ditto. This code needs to be inside a test() call.

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:3645: if (!shouldShowControls() ||
!inDocument()) {
Doesn't removing this end up causing the controls to get created even in
situations where they shouldn't be shown? Why are you making this change?

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2287: m_player->requestRemotePlayback();
Use webMediaPlayer() here and below.

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2287: m_player->requestRemotePlayback();
ASSERT(m_remoteRoutesAvailable) here and below so we can make sure the UI
doesn't spam the WebMediaPlayer?

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:3755: if (event && event->type() ==
EventTypeNames::webkitfullscreenchange) {
nit: Revert. Change isn't necessary.

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:112: bool isCasting() const { return
m_casting; }
nit: s/Casting/PlayingRemotely/ since all your other methods appear to be in
terms of "remote playback/routes"?

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:585: bool m_casting : 1;
ditto

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:224: // Doing it in
preDispatchEventHandler prevents any interference from JavaScript.
This behavior won't prevent user controls from receiving clicks if their
controls happen to be in the same position as the cast button right?

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:625: void
MediaControlOverlayCastButtonElement::defaultEventHandler(Event* event)
Why do we need 2 classes here? The code looks identical except for the pseudoID.
Am I missing something? ISTM you should just have 1 class w/ an m_id member to
hold the pseudoID.

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:628: if
(mediaElement().isCasting()) {
Shouldn't there be a mediaElement().hasRemoteRoutes() check be in here somewhere
to prevent the UI from spamming the WebMediaPlayer when remote routes aren't
available?

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
Source/core/html/shadow/MediaControls.cpp:216: if
(mediaElement().shouldShowControls()) {
This doesn't seem right. It feels like you are adding special logic here to
handle the one new mediaElementFocused() call you added. ISTM that you are
actually trying to handle a different situation than what this method was really
intended to do. Why not just put this code at the place you added a
mediaElementFocused() call instead of using shouldShowControls() to
differentiate the calls?

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
Source/core/html/shadow/MediaControls.cpp:370: if
(!mediaElement().shouldShowControls() && !mediaElement().autoplay() &&
mediaElement().paused()) {
Why does autoplay() matter here?

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
File Source/core/html/shadow/MediaControls.h (right):

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
Source/core/html/shadow/MediaControls.h:74: void refreshCastButtonVisibility();
nit: Put this near the other functions related to casting.

https://codereview.chromium.org/291163004/diff/110030/Source/platform/graphic...
File Source/platform/graphics/media/MediaPlayer.h (right):

https://codereview.chromium.org/291163004/diff/110030/Source/platform/graphic...
Source/platform/graphics/media/MediaPlayer.h:70: virtual void
mediaPlayerRemoteRouteAvailabilityChanged(bool) = 0;
nit: New methods should not be added to this interface because it is going away.
Please use mediaElement() directly in WebMediaPlayerClientImpl and make the
method names match the names you use for WebMediaPlayerClient.

https://codereview.chromium.org/291163004/diff/110030/Source/platform/graphic...
Source/platform/graphics/media/MediaPlayer.h:110: virtual void
requestRemotePlayback() = 0;
nit: New methods should not be added to this interface. It is going away.
HTMLMediaElement should call the WebMediaPlayer directly.

[aberent, 2014-08-12 17:25:35]

On 2014/08/05 19:36:35, acolwell wrote:
>
https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/contr...
> File LayoutTests/media/controls-cast-button.html (right):
> 
>
https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/contr...
> LayoutTests/media/controls-cast-button.html:14: findMediaElement();
> I'm pretty sure you need to put this code inside a test() call so the
assert_xxx
> methods work properly. See
> http://darobin.github.io/test-harness-tutorial/docs/using-testharness.html
> 
>
https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/contr...
> File LayoutTests/media/controls-overlay-cast-button.html (right):
> 
>
https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/contr...
> LayoutTests/media/controls-overlay-cast-button.html:14: findMediaElement();
> ditto. This code needs to be inside a test() call.
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
> File Source/core/html/HTMLMediaElement.cpp (left):
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
> Source/core/html/HTMLMediaElement.cpp:3645: if (!shouldShowControls() ||
> !inDocument()) {
> Doesn't removing this end up causing the controls to get created even in
> situations where they shouldn't be shown? Why are you making this change?
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
> File Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
> Source/core/html/HTMLMediaElement.cpp:2287: m_player->requestRemotePlayback();
> Use webMediaPlayer() here and below.
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
> Source/core/html/HTMLMediaElement.cpp:2287: m_player->requestRemotePlayback();
> ASSERT(m_remoteRoutesAvailable) here and below so we can make sure the UI
> doesn't spam the WebMediaPlayer?
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
> Source/core/html/HTMLMediaElement.cpp:3755: if (event && event->type() ==
> EventTypeNames::webkitfullscreenchange) {
> nit: Revert. Change isn't necessary.
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
> File Source/core/html/HTMLMediaElement.h (right):
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
> Source/core/html/HTMLMediaElement.h:112: bool isCasting() const { return
> m_casting; }
> nit: s/Casting/PlayingRemotely/ since all your other methods appear to be in
> terms of "remote playback/routes"?
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
> Source/core/html/HTMLMediaElement.h:585: bool m_casting : 1;
> ditto
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
> File Source/core/html/shadow/MediaControlElements.cpp (right):
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
> Source/core/html/shadow/MediaControlElements.cpp:224: // Doing it in
> preDispatchEventHandler prevents any interference from JavaScript.
> This behavior won't prevent user controls from receiving clicks if their
> controls happen to be in the same position as the cast button right?
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
> Source/core/html/shadow/MediaControlElements.cpp:625: void
> MediaControlOverlayCastButtonElement::defaultEventHandler(Event* event)
> Why do we need 2 classes here? The code looks identical except for the
pseudoID.
> Am I missing something? ISTM you should just have 1 class w/ an m_id member to
> hold the pseudoID.
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
> Source/core/html/shadow/MediaControlElements.cpp:628: if
> (mediaElement().isCasting()) {
> Shouldn't there be a mediaElement().hasRemoteRoutes() check be in here
somewhere
> to prevent the UI from spamming the WebMediaPlayer when remote routes aren't
> available?
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
> File Source/core/html/shadow/MediaControls.cpp (right):
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
> Source/core/html/shadow/MediaControls.cpp:216: if
> (mediaElement().shouldShowControls()) {
> This doesn't seem right. It feels like you are adding special logic here to
> handle the one new mediaElementFocused() call you added. ISTM that you are
> actually trying to handle a different situation than what this method was
really
> intended to do. Why not just put this code at the place you added a
> mediaElementFocused() call instead of using shouldShowControls() to
> differentiate the calls?
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
> Source/core/html/shadow/MediaControls.cpp:370: if
> (!mediaElement().shouldShowControls() && !mediaElement().autoplay() &&
> mediaElement().paused()) {
> Why does autoplay() matter here?
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
> File Source/core/html/shadow/MediaControls.h (right):
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
> Source/core/html/shadow/MediaControls.h:74: void
refreshCastButtonVisibility();
> nit: Put this near the other functions related to casting.
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/platform/graphic...
> File Source/platform/graphics/media/MediaPlayer.h (right):
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/platform/graphic...
> Source/platform/graphics/media/MediaPlayer.h:70: virtual void
> mediaPlayerRemoteRouteAvailabilityChanged(bool) = 0;
> nit: New methods should not be added to this interface because it is going
away.
> Please use mediaElement() directly in WebMediaPlayerClientImpl and make the
> method names match the names you use for WebMediaPlayerClient.
> 
>
https://codereview.chromium.org/291163004/diff/110030/Source/platform/graphic...
> Source/platform/graphics/media/MediaPlayer.h:110: virtual void
> requestRemotePlayback() = 0;
> nit: New methods should not be added to this interface. It is going away.
> HTMLMediaElement should call the WebMediaPlayer directly.

There will be a slight delay before I do any further work on this. I have been
asked to work on something else this week, and will be on vacation for the next
two weeks.

[aberent, 2014-08-22 14:08:34]

PTAL

https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/contr...
File LayoutTests/media/controls-cast-button.html (right):

https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/contr...
LayoutTests/media/controls-cast-button.html:14: findMediaElement();
On 2014/08/05 19:36:34, acolwell wrote:
> I'm pretty sure you need to put this code inside a test() call so the
assert_xxx
> methods work properly. See
> http://darobin.github.io/test-harness-tutorial/docs/using-testharness.html

Done.

https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/contr...
File LayoutTests/media/controls-overlay-cast-button.html (right):

https://codereview.chromium.org/291163004/diff/110030/LayoutTests/media/contr...
LayoutTests/media/controls-overlay-cast-button.html:14: findMediaElement();
On 2014/08/05 19:36:34, acolwell wrote:
> ditto. This code needs to be inside a test() call.

Done.

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:3645: if (!shouldShowControls() ||
!inDocument()) {
On 2014/08/05 19:36:34, acolwell wrote:
> Doesn't removing this end up causing the controls to get created even in
> situations where they shouldn't be shown? Why are you making this change?

The problem I have is that I need the overlay cast button precisely when I don't
have standard controls. The way I have done this is by including it in the
controls, always creating the controls, and controlling carefully their
visibility. The alternative, I think, would have been to have a second shadow
DOM, duplicating much of logic of the controls, but I could see this causing all
sorts of difficulties (I was particularly put off from that option by the
ASSERT_WITH_SECURITY_IMPLICATIONS on line 3659).

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2287: m_player->requestRemotePlayback();
On 2014/08/05 19:36:34, acolwell wrote:
> Use webMediaPlayer() here and below.

Done.

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2287: m_player->requestRemotePlayback();
On 2014/08/05 19:36:34, acolwell wrote:
> Use webMediaPlayer() here and below.

Done.

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:3755: if (event && event->type() ==
EventTypeNames::webkitfullscreenchange) {
On 2014/08/05 19:36:34, acolwell wrote:
> nit: Revert. Change isn't necessary.

Done.

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:112: bool isCasting() const { return
m_casting; }
On 2014/08/05 19:36:34, acolwell wrote:
> nit: s/Casting/PlayingRemotely/ since all your other methods appear to be in
> terms of "remote playback/routes"?

Done.

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:585: bool m_casting : 1;
On 2014/08/05 19:36:34, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:224: // Doing it in
preDispatchEventHandler prevents any interference from JavaScript.
On 2014/08/05 19:36:34, acolwell wrote:
> This behavior won't prevent user controls from receiving clicks if their
> controls happen to be in the same position as the cast button right? 

This particular code simply makes the overlay cast button visible when the video
is touched or clicked. The event is still handled as normal.

If the site controls also only appear when the video is clicked (not uncommon)
and are in the same position as the cast button then there could be a problem
with the next click, however I don't know any sites that put their video
controls at the top left of the site. This potential problem also applies in the
existing version, where the cast button was overlaid by the Clank UI.

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:625: void
MediaControlOverlayCastButtonElement::defaultEventHandler(Event* event)
On 2014/08/05 19:36:34, acolwell wrote:
> Why do we need 2 classes here? The code looks identical except for the
pseudoID.
> Am I missing something? ISTM you should just have 1 class w/ an m_id member to
> hold the pseudoID.

Done.

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:628: if
(mediaElement().isCasting()) {
On 2014/08/05 19:36:34, acolwell wrote:
> Shouldn't there be a mediaElement().hasRemoteRoutes() check be in here
somewhere
> to prevent the UI from spamming the WebMediaPlayer when remote routes aren't
> available?

The logic is such that the cast button can never be visible when the
hasRemoteRoutes() is false, so this seems unnecessary.

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
Source/core/html/shadow/MediaControls.cpp:216: if
(mediaElement().shouldShowControls()) {
On 2014/08/05 19:36:34, acolwell wrote:
> This doesn't seem right. It feels like you are adding special logic here to
> handle the one new mediaElementFocused() call you added. ISTM that you are
> actually trying to handle a different situation than what this method was
really
> intended to do. Why not just put this code at the place you added a
> mediaElementFocused() call instead of using shouldShowControls() to
> differentiate the calls?

Done.

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
Source/core/html/shadow/MediaControls.cpp:370: if
(!mediaElement().shouldShowControls() && !mediaElement().autoplay() &&
mediaElement().paused()) {
On 2014/08/05 19:36:34, acolwell wrote:
> Why does autoplay() matter here?

Some pages (e.g. vimeo.com) have an autoplay background video, which doesn't
autoplay on Clank (we prevent it) so starts paused. In such cases we don't want
to automatically show the cast button, since it looks strange and is unlikely to
correspond with anything the user wants to do.

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
File Source/core/html/shadow/MediaControls.h (right):

https://codereview.chromium.org/291163004/diff/110030/Source/core/html/shadow...
Source/core/html/shadow/MediaControls.h:74: void refreshCastButtonVisibility();
On 2014/08/05 19:36:34, acolwell wrote:
> nit: Put this near the other functions related to casting.

Done.

https://codereview.chromium.org/291163004/diff/110030/Source/platform/graphic...
File Source/platform/graphics/media/MediaPlayer.h (right):

https://codereview.chromium.org/291163004/diff/110030/Source/platform/graphic...
Source/platform/graphics/media/MediaPlayer.h:70: virtual void
mediaPlayerRemoteRouteAvailabilityChanged(bool) = 0;
On 2014/08/05 19:36:34, acolwell wrote:
> nit: New methods should not be added to this interface because it is going
away.
> Please use mediaElement() directly in WebMediaPlayerClientImpl and make the
> method names match the names you use for WebMediaPlayerClient.

Done.

https://codereview.chromium.org/291163004/diff/110030/Source/platform/graphic...
Source/platform/graphics/media/MediaPlayer.h:110: virtual void
requestRemotePlayback() = 0;
On 2014/08/05 19:36:35, acolwell wrote:
> nit: New methods should not be added to this interface. It is going away.
> HTMLMediaElement should call the WebMediaPlayer directly.

Done.

[aberent, 2014-08-22 14:25:10]

abarth@chromium.org: Owner review please of changes in Source/platform,
Source/web, and public/Platform; acolwell@ is doing the main review.

[abarth-chromium, 2014-08-23 05:53:15]

https://codereview.chromium.org/291163004/diff/150001/Source/core/css/CSSValu...
File Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/291163004/diff/150001/Source/core/css/CSSValu...
Source/core/css/CSSValueKeywords.in:657: media-overlay-cast-on-button
Don't these need to be prefixed with -internal- to avoid leaking them to the
web?

https://codereview.chromium.org/291163004/diff/150001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/291163004/diff/150001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:594:
DEFINE_STATIC_LOCAL(AtomicString, id_overlay,
("-webkit-media-controls-overlay-cast-button",
AtomicString::ConstructFromLiteral));
Please do not add any more -webkit- prefixed names.  Instead, you should use
-internal- names.

[acolwell GONE FROM CHROMIUM, 2014-09-06 00:32:02]

https://codereview.chromium.org/291163004/diff/150001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/291163004/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2287: if (!m_player)
Please add code to clearMediaPlayer() and createMediaPlayer() that sets
m_remoteRoutesAvailable to false when m_player assigned. I think if you do that
then this guard and the one in the method below aren't necessary.

https://codereview.chromium.org/291163004/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:3231: m_playingRemotely = false;
You'll probably need to reset this on m_player assignment as well.

[aberent, 2014-09-09 18:35:30]

PTAL

https://codereview.chromium.org/291163004/diff/150001/Source/core/css/CSSValu...
File Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/291163004/diff/150001/Source/core/css/CSSValu...
Source/core/css/CSSValueKeywords.in:657: media-overlay-cast-on-button
On 2014/08/23 05:53:15, abarth wrote:
> Don't these need to be prefixed with -internal- to avoid leaking them to the
> web?

Done.

https://codereview.chromium.org/291163004/diff/150001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/291163004/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:2287: if (!m_player)
On 2014/09/06 00:32:02, acolwell wrote:
> Please add code to clearMediaPlayer() and createMediaPlayer() that sets
> m_remoteRoutesAvailable to false when m_player assigned. I think if you do
that
> then this guard and the one in the method below aren't necessary.

Done.

https://codereview.chromium.org/291163004/diff/150001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:3231: m_playingRemotely = false;
On 2014/09/06 00:32:01, acolwell wrote:
> You'll probably need to reset this on m_player assignment as well.

Done.

https://codereview.chromium.org/291163004/diff/150001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/291163004/diff/150001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:594:
DEFINE_STATIC_LOCAL(AtomicString, id_overlay,
("-webkit-media-controls-overlay-cast-button",
AtomicString::ConstructFromLiteral));
On 2014/08/23 05:53:15, abarth wrote:
> Please do not add any more -webkit- prefixed names.  Instead, you should use
> -internal- names.

Done. Note that I had to add the new names to CSSSelector.cpp, since its code
has special handling of -webkit-* names but not of -internal-* names.

[acolwell GONE FROM CHROMIUM, 2014-09-10 23:35:24]

lgtm % comments

https://codereview.chromium.org/291163004/diff/170001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/291163004/diff/170001/Source/core/html/shadow...
Source/core/html/shadow/MediaControls.cpp:373: if
(!mediaElement().shouldShowControls() && !mediaElement().autoplay() &&
mediaElement().paused()) {
nit: Please add a comment about why the autoplay check is here. It isn't really
clear from the code why this is the right condition for the overlay to be shown.

https://codereview.chromium.org/291163004/diff/170001/Source/core/rendering/R...
File Source/core/rendering/RenderMediaControls.cpp (right):

https://codereview.chromium.org/291163004/diff/170001/Source/core/rendering/R...
Source/core/rendering/RenderMediaControls.cpp:355: static bool
paintMediaOverlayCastButton(RenderObject* object, const PaintInfo& paintInfo,
const IntRect& rect)
This looks identical to the method above. Why do we need both methods?

[aberent, 2014-09-11 16:38:19]

https://codereview.chromium.org/291163004/diff/170001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/291163004/diff/170001/Source/core/html/shadow...
Source/core/html/shadow/MediaControls.cpp:373: if
(!mediaElement().shouldShowControls() && !mediaElement().autoplay() &&
mediaElement().paused()) {
On 2014/09/10 23:35:23, acolwell_leaving_chromium_9-23 wrote:
> nit: Please add a comment about why the autoplay check is here. It isn't
really
> clear from the code why this is the right condition for the overlay to be
shown.

Done.

https://codereview.chromium.org/291163004/diff/170001/Source/core/rendering/R...
File Source/core/rendering/RenderMediaControls.cpp (right):

https://codereview.chromium.org/291163004/diff/170001/Source/core/rendering/R...
Source/core/rendering/RenderMediaControls.cpp:355: static bool
paintMediaOverlayCastButton(RenderObject* object, const PaintInfo& paintInfo,
const IntRect& rect)
On 2014/09/10 23:35:23, acolwell_leaving_chromium_9-23 wrote:
> This looks identical to the method above. Why do we need both methods?

Done.

[abarth-chromium, 2014-09-11 16:42:54]

LGTM

https://codereview.chromium.org/291163004/diff/190001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/291163004/diff/190001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:218: if (event &&
(event->type() == EventTypeNames::click || event->type() ==
EventTypeNames::touchstart) && mediaElement().hasRemoteRoutes() &&
!mediaElement().shouldShowControls())
Why click and touchstart?  Isn't click enough?

https://codereview.chromium.org/291163004/diff/190001/Source/core/rendering/R...
File Source/core/rendering/RenderThemeChromiumSkia.cpp (right):

https://codereview.chromium.org/291163004/diff/190001/Source/core/rendering/R...
Source/core/rendering/RenderThemeChromiumSkia.cpp:321: }
Please add blank lines between functions.

[abarth-chromium, 2014-09-11 16:42:57]

LGTM

[aberent, 2014-09-11 17:08:32]

https://codereview.chromium.org/291163004/diff/190001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/291163004/diff/190001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:218: if (event &&
(event->type() == EventTypeNames::click || event->type() ==
EventTypeNames::touchstart) && mediaElement().hasRemoteRoutes() &&
!mediaElement().shouldShowControls())
On 2014/09/11 16:42:54, abarth wrote:
> Why click and touchstart?  Isn't click enough?

If the JS has a touchstart handler, as on style.com videos, then this can
prevent translation of the touch into a click, hence the need for touchstart. I
have expanded to comment to explain this.

https://codereview.chromium.org/291163004/diff/190001/Source/core/rendering/R...
File Source/core/rendering/RenderThemeChromiumSkia.cpp (right):

https://codereview.chromium.org/291163004/diff/190001/Source/core/rendering/R...
Source/core/rendering/RenderThemeChromiumSkia.cpp:321: }
On 2014/09/11 16:42:54, abarth wrote:
> Please add blank lines between functions.

Done.

[aberent, 2014-09-11 17:08:43]

The CQ bit was checked by aberent@chromium.org

[commit-bot: I haz the power, 2014-09-11 17:09:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/291163004/210001

[commit-bot: I haz the power, 2014-09-11 18:36:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-11 18:36:31]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[aberent, 2014-09-12 18:53:36]

Patchset #13 (id:230001) has been deleted

[aberent, 2014-09-15 10:35:40]

Patchset #14 (id:270001) has been deleted

[aberent, 2014-09-15 11:00:10]

On 2014/09/11 18:36:31, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

PTAL - 
I have had to rebase the expected results of many of the video layout tests
since the rendering tree changes, although the appearance does not. See
TestExpectations.
I also had to update the cast button layout tests for the change in name of the
cast buttons, and fixed minor bugs in MediaControls::createTextTrackDisplay and
mediaControlsAndroid.css.

[Peter Beverloo, 2014-09-15 12:59:36]

peter@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2014-09-15 12:59:38]

The test failure is odd, I haven't been able to spot the error yet. A few other
random nits while I try some other things.

https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElementTypes.h (right):

https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElementTypes.h:70: 
micro nit: excess blank line.

https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.h:85: void*
preDispatchEventHandler(Event*) OVERRIDE;
+virtual

https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow...
Source/core/html/shadow/MediaControls.cpp:517: m_overlayEnclosure ->
insertBefore(textDisplayContainer.release(), m_overlayCastButton);
Are you sure that this is right? Previously it would insert
|textDisplayContainer| before the overlay play button (make it the first element
in the enclosure), now it inserts |textDisplayContainer| before the cast button,
which could be *after* the overlay play button, meaning that the text track
controls will be between the overlay play and cast buttons.

https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow...
Source/core/html/shadow/MediaControls.cpp:557: visitor->trace(m_enclosure);
+visitor->trace(m_castButton);
+visitor->trace(m_overlayCastButton);

[aberent, 2014-09-16 09:44:18]

https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElementTypes.h (right):

https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElementTypes.h:70: 
On 2014/09/15 12:59:38, Peter Beverloo wrote:
> micro nit: excess blank line.

Done.

https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.h:85: void*
preDispatchEventHandler(Event*) OVERRIDE;
On 2014/09/15 12:59:38, Peter Beverloo wrote:
> +virtual

Done.

https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow...
Source/core/html/shadow/MediaControls.cpp:517: m_overlayEnclosure ->
insertBefore(textDisplayContainer.release(), m_overlayCastButton);
On 2014/09/15 12:59:38, Peter Beverloo wrote:
> Are you sure that this is right? Previously it would insert
> |textDisplayContainer| before the overlay play button (make it the first
element
> in the enclosure), now it inserts |textDisplayContainer| before the cast
button,
> which could be *after* the overlay play button, meaning that the text track
> controls will be between the overlay play and cast buttons.

No, I am not. Will fix.

https://codereview.chromium.org/291163004/diff/310001/Source/core/html/shadow...
Source/core/html/shadow/MediaControls.cpp:557: visitor->trace(m_enclosure);
On 2014/09/15 12:59:38, Peter Beverloo wrote:
> +visitor->trace(m_castButton);
> +visitor->trace(m_overlayCastButton);

Done.

[aberent, 2014-09-16 09:47:12]

https://codereview.chromium.org/291163004/diff/330001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/291163004/diff/330001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:692: crbug.com/414373 media/media-reparent.html [
Crash ]
These last two will be removed once https://codereview.chromium.org/575563002/
lands, but avoid having to combine the two CLs (which are logically
independent).

[aberent, 2014-09-16 16:44:08]

The CQ bit was checked by aberent@chromium.org

[commit-bot: I haz the power, 2014-09-16 16:44:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/291163004/350001

[commit-bot: I haz the power, 2014-09-16 18:16:58]

Committed patchset #17 (id:350001) as 182077