Media Remoting: Add interstitial elements to media element shadow dom.

Media Remoting: Add interstitial elements to media element shadow dom.

This CL add the following elements to media element shadow dom when 
media remoting starts.
1. Add a disable button to allow user stop remoting and fall back to
tab mirroring. This is to solve the accessiblity regression caused by 
media remoting with lacking support for closed captions.
2. Add a background image, cast icon and a text message. These three 
are currently rendered as a video frame in media renderer. Move them
to media element to keep consistent with the disabel button and solve 
the font renderering issue on Windows 8/10.

The old interstitial (rendered as a video frame) will be removed in a
later CL. 

BUG=700572, 697672
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2767823002
Cr-Commit-Position: refs/heads/master@{#464604}
Committed: https://chromium.googlesource.com/chromium/src/+/df9b67eb9751650cebee13333ab125bb3f11df75

[xjz, 2017-03-22 00:31:54]

Description was changed from

==========
Media Remoting: Add disable button.

Add a disable button to media remoting UI to allow user stop remoting
and fall back to tab mirroring. This is to solve the accessiblity
regression caused by media remoting with lacking support for closed
captions.

BUG=700572
==========

to

==========
Media Remoting: Add disable button.

Add a disable button to media remoting UI to allow user stop remoting
and fall back to tab mirroring. This is to solve the accessiblity
regression caused by media remoting with lacking support for closed
captions.

BUG=700572
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[xjz, 2017-03-22 00:34:53]

Description was changed from

==========
Media Remoting: Add disable button.

Add a disable button to media remoting UI to allow user stop remoting
and fall back to tab mirroring. This is to solve the accessiblity
regression caused by media remoting with lacking support for closed
captions.

BUG=700572
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Media Remoting: Add disable button.

Add a disable button to media remoting UI to allow user stop remoting
and fall back to tab mirroring. This is to solve the accessiblity
regression caused by media remoting with lacking support for closed
captions.

BUG=700572
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[xjz, 2017-03-22 00:34:53]

xjz@chromium.org changed reviewers:
+ miu@chromium.org

[xjz, 2017-03-22 00:37:34]

PTAL. Thanks!

[miu, 2017-03-22 21:26:49]

lgtm % nits:

https://codereview.chromium.org/2767823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2767823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:1112:
MouseEventsListener(MediaRemotingDisableButtonElement& element)
nit: explicit

https://codereview.chromium.org/2767823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:1133: x >
clientRect->left() + clientRect->width() ||
nit: x > clientRect->right()

https://codereview.chromium.org/2767823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:1134: y >
clientRect->top() + clientRect->height())
nit: y > clientRect->bottom()

https://codereview.chromium.org/2767823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp (right):

https://codereview.chromium.org/2767823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp:220: IntRect
buttonRect(rect);
nit:

  const IntRect buttonRect(rect.center().x(),
                           rect.center().y()
                           mediaremotingDisableButtonWidth,
                           mediaremotingDisableButtonHeight);

...or maybe even just use an entirely functional style in your return statement:

  return paintMediaButton(
      paintInfo.context,
      IntRect(rect.center().x(),
              rect.center().y(),
              mediaremotingDisableButtonWidth,
              mediaremotingDisableButtonHeight),
      mediaremotingDisable);

[xjz, 2017-03-22 22:25:08]

xjz@chromium.org changed reviewers:
+ mlamouri@chromium.org

[xjz, 2017-03-22 22:25:09]

mlamouri@: This CL adds an overlay disable button on media element for media
remoting. The button is out of mediaControls (to avoid web sites blocking the
display). PTAL. Thanks!

https://codereview.chromium.org/2767823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2767823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:1112:
MouseEventsListener(MediaRemotingDisableButtonElement& element)
On 2017/03/22 21:26:48, miu wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2767823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:1133: x >
clientRect->left() + clientRect->width() ||
On 2017/03/22 21:26:48, miu wrote:
> nit: x > clientRect->right()

Done.

https://codereview.chromium.org/2767823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:1134: y >
clientRect->top() + clientRect->height())
On 2017/03/22 21:26:48, miu wrote:
> nit: y > clientRect->bottom()

Done.

https://codereview.chromium.org/2767823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp (right):

https://codereview.chromium.org/2767823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp:220: IntRect
buttonRect(rect);
On 2017/03/22 21:26:48, miu wrote:
> nit:
> 
>   const IntRect buttonRect(rect.center().x(),
>                            rect.center().y()
>                            mediaremotingDisableButtonWidth,
>                            mediaremotingDisableButtonHeight);
> 
> ...or maybe even just use an entirely functional style in your return
statement:
> 
>   return paintMediaButton(
>       paintInfo.context,
>       IntRect(rect.center().x(),
>               rect.center().y(),
>               mediaremotingDisableButtonWidth,
>               mediaremotingDisableButtonHeight),
>       mediaremotingDisable);

Done.

[mlamouri (slow - plz ping), 2017-03-23 16:02:29]

This is a large UI change: did you get any feedback from UX?

Also, it will have a large impact on the media code which makes me very
uncomfortable.

[xjz, 2017-03-23 18:07:56]

On 2017/03/23 16:02:29, mlamouri wrote:
> This is a large UI change: did you get any feedback from UX?
> 
> Also, it will have a large impact on the media code which makes me very
> uncomfortable.

Thanks mlamouri.
The proposal was sent to UX, but don't get reply yet.
The new button is supposed to not affect any other overlay elements/controls.

[miu, 2017-03-24 21:51:53]

On 2017/03/23 18:07:56, xjz wrote:
> On 2017/03/23 16:02:29, mlamouri wrote:
> > This is a large UI change: did you get any feedback from UX?

Yes. We've worked with several UX designers over the lifetime of this project.
:)

The procedure they prefer is for us to provide mocks for them to approve; and
then the next step, which we're on here, is to land the UI change. They want to
see our tentatively-approved UI in Canary so they can play around with the
feature, and suggest minor tweaks later.

For this CL: We are only adding a manual disable button, to resolve some
possible accessibility regression issues. This button only appears while Media
Remoting is active. So, this is a small UI change, not a large one.

> > Also, it will have a large impact on the media code which makes me very
> > uncomfortable.

I don't understand: Maybe the "large impact" here is that xjz@ is preparing for
a later change, where we're planning to move the "Remoting Interstitial" (e.g.,
src/media/remoting/iterstitial.cc) into Blink? This is actually in-line with
guidance from esprehn@, ojan@, and others. Meaning, they said they prefer that
page UI be done in Blink code, even if it is for a Chrome-specific feature. That
aside, we will need to do this to resolve a few other launch-blocking issues as
well: Accessibility/Screen-Reader issues, and a font-rendering bug on Win10
(http://crbug.com/697672).

> The proposal was sent to UX, but don't get reply yet.

We have had to recently switch UX designers, so the new one is ramping up right
now.

[xjz, 2017-04-01 23:45:46]

Description was changed from

==========
Media Remoting: Add disable button.

Add a disable button to media remoting UI to allow user stop remoting
and fall back to tab mirroring. This is to solve the accessiblity
regression caused by media remoting with lacking support for closed
captions.

BUG=700572
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Media Remoting: implement the new UI.

This CL implemented the new Media Remoting UI.
1. Add a disable button to allow user stop remoting and fall back to
tab mirroring. This is to solve the accessiblity regression caused by 
media remoting with lacking support for closed captions.
2. Render the cast icon and text message in blink to solve the font 
rendering issue and be consistent with the disable button.

The old interstitial (rendered as a video frame) will be removed in a
later CL. 

BUG=700572, 697672
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[xjz, 2017-04-01 23:45:46]

xjz@chromium.org changed reviewers:
+ kinuko@chromium.org, sandersd@chromium.org

[xjz, 2017-04-02 00:00:30]

Description was changed from

==========
Media Remoting: implement the new UI.

This CL implemented the new Media Remoting UI.
1. Add a disable button to allow user stop remoting and fall back to
tab mirroring. This is to solve the accessiblity regression caused by 
media remoting with lacking support for closed captions.
2. Render the cast icon and text message in blink to solve the font 
rendering issue and be consistent with the disable button.

The old interstitial (rendered as a video frame) will be removed in a
later CL. 

BUG=700572, 697672
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Media Remoting: Add interstitial elements to media element shadow dom.

This CL add the following elements to media element shadow dom when 
media remoting starts.
1. Add a disable button to allow user stop remoting and fall back to
tab mirroring. This is to solve the accessiblity regression caused by 
media remoting with lacking support for closed captions.
2. Add a background image, cast icon and a text message. These three 
are currently rendered as a video frame in media renderer. Move them
to media element to keep consistent with the disabel button and solve 
the font renderering issue on Windows 8/10.

The old interstitial (rendered as a video frame) will be removed in a
later CL. 

BUG=700572, 697672
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[xjz, 2017-04-02 00:07:33]

Modified the disable button as the UX designer suggested. Also added other
interstitial elements.
miu@: PTAL again. 
mlamouri@: PTAL changes to third_party/WebKit/Source/core/html/*.
kinuko@: PTAL other blink changes.
sandersd@: PTAL media/base and media/blink related changes. 
Thanks!

[miu, 2017-04-03 21:44:41]

Comments on PS3:

Are we sure we want to use the PNG format for the Cast icon? I was able to find
an SVG variant from a Google Image search, which would likely produce better
results (since the graphic can be resized). I would ask the UX designer which is
more appropriate, and to provide an SVG-format asset if that should be used
instead.

https://codereview.chromium.org/2767823002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp (right):

https://codereview.chromium.org/2767823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp:225:
IntRect(rect.center().x() - buttonWidth / 2,
Looks like there's a potential "one pixel off" result in these calculations for
certain inputs, due to the integer math. I think this should be:

  IntRect(rect.x() + (rect.width() - buttonWidth) / 2,
          rect.y() + (rect.height() - buttonHeight) / 2,
          ...)

[sandersd (OOO until July 31), 2017-04-03 21:48:05]

media/ lgtm.

[xjz, 2017-04-03 23:38:07]

On 2017/04/03 21:44:41, miu wrote:
> Comments on PS3:
> 
> Are we sure we want to use the PNG format for the Cast icon? I was able to
find
> an SVG variant from a Google Image search, which would likely produce better
> results (since the graphic can be resized). I would ask the UX designer which
is
> more appropriate, and to provide an SVG-format asset if that should be used
> instead.

This PNG is provided by the designer. I'll ask him whether he can provide an
SVG-format asset.

> 
>
https://codereview.chromium.org/2767823002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp (right):
> 
>
https://codereview.chromium.org/2767823002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp:225:
> IntRect(rect.center().x() - buttonWidth / 2,
> Looks like there's a potential "one pixel off" result in these calculations
for
> certain inputs, due to the integer math. I think this should be:
> 
>   IntRect(rect.x() + (rect.width() - buttonWidth) / 2,
>           rect.y() + (rect.height() - buttonHeight) / 2,
>           ...)

[xjz, 2017-04-03 23:39:52]

Addressed miu's comment in PS4. Also, as suggested by the UX designer, now keep
showing the media controls during media remoting. PTAL. Thanks!

https://codereview.chromium.org/2767823002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp (right):

https://codereview.chromium.org/2767823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp:225:
IntRect(rect.center().x() - buttonWidth / 2,
On 2017/04/03 21:44:41, miu wrote:
> Looks like there's a potential "one pixel off" result in these calculations
for
> certain inputs, due to the integer math. I think this should be:
> 
>   IntRect(rect.x() + (rect.width() - buttonWidth) / 2,
>           rect.y() + (rect.height() - buttonHeight) / 2,
>           ...)

Done.

[kinuko, 2017-04-04 03:18:05]

lgtm % nits

(A bit hand-wavy for UI related code but per your earlier comments I assume
you've gotten go sign from UX people for that part)

https://codereview.chromium.org/2767823002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp (right):

https://codereview.chromium.org/2767823002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp:61: static const
int MediaRemotingCastIconHeight = 72;
The naming looks inconsistent here and above, would be better to use the
consistent policy (old ones or new ones).

[mlamouri (slow - plz ping), 2017-04-04 13:19:44]

Could you move the implementation of the remoting interstitial to
HTMLVideoElement?

https://codereview.chromium.org/2767823002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/2767823002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:389: class
MediaRemotingDisableButtonElement final : public HTMLInputElement {
Can you create a MediaRemotingElements file instead?

[xjz, 2017-04-04 20:52:03]

Patchset #5 (id:80001) has been deleted

[xjz, 2017-04-04 20:56:27]

Thanks for reviewing. Addressed kinuko's and mlamouri's comments in PS#5. 
mlamouri: Moved MediaRemotingInterstitial to HTMLVideoElement. PTAL again.
Thanks!

https://codereview.chromium.org/2767823002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/2767823002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:389: class
MediaRemotingDisableButtonElement final : public HTMLInputElement {
On 2017/04/04 13:19:44, mlamouri wrote:
> Can you create a MediaRemotingElements file instead?

Done.

https://codereview.chromium.org/2767823002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp (right):

https://codereview.chromium.org/2767823002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp:61: static const
int MediaRemotingCastIconHeight = 72;
On 2017/04/04 03:18:05, kinuko wrote:
> The naming looks inconsistent here and above, would be better to use the
> consistent policy (old ones or new ones).

Done.

[xjz, 2017-04-04 22:23:42]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 22:24:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 22:29:09]

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

[commit-bot: I haz the power, 2017-04-04 22:29:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[xjz, 2017-04-04 23:35:52]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 23:36:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 03:03:16]

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

[commit-bot: I haz the power, 2017-04-05 03:03:17]

Dry run: This issue passed the CQ dry run.

[xjz, 2017-04-05 20:50:51]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 20:51:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 20:57:59]

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

[commit-bot: I haz the power, 2017-04-05 20:58:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mlamouri (slow - plz ping), 2017-04-07 13:18:41]

I had another pass but I'm afraid I still have a few comments.

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControls.css:148:
video::-internal-media-remoting-interstitial {
FWIW, this is not part of media controls. Let's ignore that for now but it will
have to be split out of mediaControls.css at some point. Adding a TODO about
this would be nice :)

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:622: if
(m_webMediaPlayer && !isMediaRemotingDisabled()) {
Instead of leaking the media remoting logic into HTMLMediaElement, could the
media remoting logic be adapted to handle this?

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:108: static void
assertShadowRootChildren(ShadowRoot&);
Could this be `protected`?

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:136: virtual bool
isMediaRemotingStarted() const { return false; }
Instead of making this a virtual on HTMLMediaElement, given that you use it in
only one place that checks for mediaElement.isHTMLVideoElement(), could you
simply do: `toHTMLVideoElement(mediaElement)->isMediaRemotingStarted()`?

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLVideoElement.cpp:512:
m_remotingInterstitial->hide();
Maybe you should call `mediaRemotingStopped()`.

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLVideoElement.h (right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLVideoElement.h:172: bool
m_mediaRemotingStarted : 1;
Could a `m_mediaRemotingStatus` be used with Disabled, NotStarted, Started as
the three possible values? You could also expose the media remoting status with
these three values instead of the two different methods.

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp
(right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:72:
document().addEventListener(EventTypeNames::click, m_listener, true);
Can't you use `defaultEventHandler()` from HTMLInputElement and check if the
event is a click? What would be the difference from what you are doing? Sounds
like it could reduce the boilerplate.

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:109:
setAttribute(srcAttr, posterUrl);
Stupid question but would it work to change
HTMLVideoElement::shouldDisplayPosterImage() to return true when doing media
remoting? It could save us from having this background image element.

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h (right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h:22: }
nit: can you move this to the cpp file?

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutMedia.cpp (right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:87:
child->node()->isMediaRemotingInterstitial()) {
I don't think you want this.

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:128: return true;
Can you merge this with the isTextTrackContainer() check above?

[xjz, 2017-04-07 23:07:02]

Addressed mlamouri's comments. PTAL again. Thanks!

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControls.css:148:
video::-internal-media-remoting-interstitial {
On 2017/04/07 13:18:32, mlamouri wrote:
> FWIW, this is not part of media controls. Let's ignore that for now but it
will
> have to be split out of mediaControls.css at some point. Adding a TODO about
> this would be nice :)

Done. Added TODO for now. Will do in a separate CL later. :)

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:622: if
(m_webMediaPlayer && !isMediaRemotingDisabled()) {
On 2017/04/07 13:18:32, mlamouri wrote:
> Instead of leaking the media remoting logic into HTMLMediaElement, could the
> media remoting logic be adapted to handle this?

Done. Removed the check. Yes, this is already handled by the media remoting
control logic.

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:108: static void
assertShadowRootChildren(ShadowRoot&);
On 2017/04/07 13:18:32, mlamouri wrote:
> Could this be `protected`?

Done. Moved it to 'protected'.

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:136: virtual bool
isMediaRemotingStarted() const { return false; }
On 2017/04/07 13:18:32, mlamouri wrote:
> Instead of making this a virtual on HTMLMediaElement, given that you use it in
> only one place that checks for mediaElement.isHTMLVideoElement(), could you
> simply do: `toHTMLVideoElement(mediaElement)->isMediaRemotingStarted()`?

Done.

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLVideoElement.cpp:512:
m_remotingInterstitial->hide();
On 2017/04/07 13:18:32, mlamouri wrote:
> Maybe you should call `mediaRemotingStopped()`.

Done.

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLVideoElement.h (right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLVideoElement.h:172: bool
m_mediaRemotingStarted : 1;
On 2017/04/07 13:18:33, mlamouri wrote:
> Could a `m_mediaRemotingStatus` be used with Disabled, NotStarted, Started as
> the three possible values? You could also expose the media remoting status
with
> these three values instead of the two different methods.

Good suggestion. Done. Thanks! :)

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp
(right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:72:
document().addEventListener(EventTypeNames::click, m_listener, true);
On 2017/04/07 13:18:34, mlamouri wrote:
> Can't you use `defaultEventHandler()` from HTMLInputElement and check if the
> event is a click? What would be the difference from what you are doing? Sounds
> like it could reduce the boilerplate.

Using 'defaultEventHandler()' does not work for websites that use custom
controls (like vimeo videos), because the mouse clicking events will be handled
by vimeo custom controls (which is on top of this disable button element). What
I am doing here is similar to what vimeo does, that to register an event
listener to the document, which can listen to the events no matter what layer
this element is. This listener will only be registered when media remoting
starts, and will be removed once it stops or the button is clicked.

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:109:
setAttribute(srcAttr, posterUrl);
On 2017/04/07 13:18:35, mlamouri wrote:
> Stupid question but would it work to change
> HTMLVideoElement::shouldDisplayPosterImage() to return true when doing media
> remoting? It could save us from having this background image element.

Yes, we can use this approach to show original poster image. The only reason I
didn't do this way because I don't know how to convert the poster image to
grayscale and get blurred in this way. So I added this element and did the
conversion using css.

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h (right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h:22: }
On 2017/04/07 13:18:36, mlamouri wrote:
> nit: can you move this to the cpp file?

Done.

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutMedia.cpp (right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:87:
child->node()->isMediaRemotingInterstitial()) {
On 2017/04/07 13:18:37, mlamouri wrote:
> I don't think you want this.

Done.

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutMedia.cpp:128: return true;
On 2017/04/07 13:18:37, mlamouri wrote:
> Can you merge this with the isTextTrackContainer() check above?

Done.

[xjz, 2017-04-07 23:07:13]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 23:07:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 23:12:10]

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

[commit-bot: I haz the power, 2017-04-07 23:12:11]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mlamouri (slow - plz ping), 2017-04-10 13:44:22]

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp
(right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:72:
document().addEventListener(EventTypeNames::click, m_listener, true);
On 2017/04/07 at 23:07:01, xjz wrote:
> On 2017/04/07 13:18:34, mlamouri wrote:
> > Can't you use `defaultEventHandler()` from HTMLInputElement and check if the
> > event is a click? What would be the difference from what you are doing?
Sounds
> > like it could reduce the boilerplate.
> 
> Using 'defaultEventHandler()' does not work for websites that use custom
controls (like vimeo videos), because the mouse clicking events will be handled
by vimeo custom controls (which is on top of this disable button element). What
I am doing here is similar to what vimeo does, that to register an event
listener to the document, which can listen to the events no matter what layer
this element is. This listener will only be registered when media remoting
starts, and will be removed once it stops or the button is clicked.

I agree that because the event is only there when in remoting mode, it's not
that bad but it's still not ideal as we are re-inventing the behaviour for
clicking which means that even if this button isn't visible but the user clicks
on top of it, it would do its action.

Did you try PreDispatchEventHandler?

Also, happy to look more into this by applying your CL but because of the big
Blink rename, I can't until you rebase :(

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLVideoElement.cpp:186: if
(m_remotingInterstitial)
Can you add a TODO explaining that ideally, the original poster should be
painted but for this, it would need to be painted in greyscale?

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp
(right):

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:65:
setType(InputTypeNames::text);
Why text instead of button?

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:133:
show();
Did you mean to just call show()?

Though, it sounds a bit weird that the poster image is checking with
m_interstitial whether it should be visible. It's its parent so it should likely
be notified, right?

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h (right):

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h:40: class
MediaRemotingBackgroundImageElement final : public HTMLInputElement {
I just realised this. Why is thin as HTMLInputElement and not an image element
or something similar?

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.cpp
(right):

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.cpp:19:
appendChild(m_MediaRemotingCastIcon);
deprecated-style-nit: it's m_mediaRemotingCastIcon. Though, since today, it's
media_remoting_cast_icon_ so it doesn't matter :)

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.cpp:66: if
(m_mediaRemotingBackgroundImage)
The null-check sounds useless given that the object has the lifetime of `this`.

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h
(right):

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h:37: bool
shouldShow() { return m_shown; }
nit: `const`

[xjz, 2017-04-11 04:00:56]

Patchset #10 (id:200001) has been deleted

[xjz, 2017-04-11 04:23:21]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 04:23:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xjz, 2017-04-11 04:29:45]

Thanks for reviewing. Addressed comments, and rebased. PTAL.

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp
(right):

https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:72:
document().addEventListener(EventTypeNames::click, m_listener, true);
On 2017/04/10 13:44:22, mlamouri wrote:
> On 2017/04/07 at 23:07:01, xjz wrote:
> > On 2017/04/07 13:18:34, mlamouri wrote:
> > > Can't you use `defaultEventHandler()` from HTMLInputElement and check if
the
> > > event is a click? What would be the difference from what you are doing?
> Sounds
> > > like it could reduce the boilerplate.
> > 
> > Using 'defaultEventHandler()' does not work for websites that use custom
> controls (like vimeo videos), because the mouse clicking events will be
handled
> by vimeo custom controls (which is on top of this disable button element).
What
> I am doing here is similar to what vimeo does, that to register an event
> listener to the document, which can listen to the events no matter what layer
> this element is. This listener will only be registered when media remoting
> starts, and will be removed once it stops or the button is clicked.
> 
> I agree that because the event is only there when in remoting mode, it's not
> that bad but it's still not ideal as we are re-inventing the behaviour for
> clicking which means that even if this button isn't visible but the user
clicks
> on top of it, it would do its action.
> 
> Did you try PreDispatchEventHandler?
> 
> Also, happy to look more into this by applying your CL but because of the big
> Blink rename, I can't until you rebase :(

I did try preDispatchEventHandler and it didn't work either. 
Rebased.

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLVideoElement.cpp:186: if
(m_remotingInterstitial)
On 2017/04/10 13:44:22, mlamouri wrote:
> Can you add a TODO explaining that ideally, the original poster should be
> painted but for this, it would need to be painted in greyscale?

Add comment. It's not TODO, but by design, we need to convert the poster image
to grayscale and blur it after.

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp
(right):

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:65:
setType(InputTypeNames::text);
On 2017/04/10 13:44:22, mlamouri wrote:
> Why text instead of button?

I did try button type at first, but the css didn't work properly for button
type. The border lines were not shown and the text was not in the center. So I
used text here because it functions and by design this button only has a text on
and no background png. Is there a difference between using button and using text
here? If not big difference, can we use text for now? And the style might get
changed later since the UX designer is waiting for this patch landed to check
for adjustments before M59. WDYT?

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:133:
show();
On 2017/04/10 13:44:22, mlamouri wrote:
> Did you mean to just call show()?
> 
> Though, it sounds a bit weird that the poster image is checking with
> m_interstitial whether it should be visible. It's its parent so it should
likely
> be notified, right?

Removed. Thanks! :)

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h (right):

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h:40: class
MediaRemotingBackgroundImageElement final : public HTMLInputElement {
On 2017/04/10 13:44:22, mlamouri wrote:
> I just realised this. Why is thin as HTMLInputElement and not an image element
> or something similar?

Changed it to a HTMLImageElement. Thanks for the suggestion.

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.cpp
(right):

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.cpp:19:
appendChild(m_MediaRemotingCastIcon);
On 2017/04/10 13:44:22, mlamouri wrote:
> deprecated-style-nit: it's m_mediaRemotingCastIcon. Though, since today, it's
> media_remoting_cast_icon_ so it doesn't matter :)

Done.

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.cpp:66: if
(m_mediaRemotingBackgroundImage)
On 2017/04/10 13:44:22, mlamouri wrote:
> The null-check sounds useless given that the object has the lifetime of
`this`.

Removed.

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h
(right):

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h:37: bool
shouldShow() { return m_shown; }
On 2017/04/10 13:44:22, mlamouri wrote:
> nit: `const` 

This function is removed now.

[commit-bot: I haz the power, 2017-04-11 06:02:55]

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

[commit-bot: I haz the power, 2017-04-11 06:02:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xjz, 2017-04-12 01:33:23]

Patchset #11 (id:240001) has been deleted

[xjz, 2017-04-12 01:33:54]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 01:34:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 04:14:05]

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

[commit-bot: I haz the power, 2017-04-12 04:14:07]

Dry run: This issue passed the CQ dry run.

[miu, 2017-04-12 20:45:50]

miu@chromium.org changed reviewers:
+ liberato@chromium.org

[miu, 2017-04-12 20:45:52]

liberato: Could you PTAL at the Blink changes? Ideally, we'd like to wrap this
up so mlamouri@ can LGTM when the globe turns back around halfway...

[liberato (no reviews please), 2017-04-12 21:57:44]

lg*m, minus some nits.  however, looking at this i realized that it's been far
too long since i've been active in this area.  lots of little things that i'm
forgetting to check, i'm sure.

thanks
-fl

https://codereview.chromium.org/2767823002/diff/260001/content/child/blink_pl...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/2767823002/diff/260001/content/child/blink_pl...
content/child/blink_platform_impl.cc:532: {"mediaremotingCast",
IDR_MEDIA_REMOTING_CAST_ICON, ui::SCALE_FACTOR_100P,
please alphabetize.

https://codereview.chromium.org/2767823002/diff/260001/media/base/media_obser...
File media/base/media_observer.h (right):

https://codereview.chromium.org/2767823002/diff/260001/media/base/media_obser...
media/base/media_observer.h:19: // possible. |is_rendered_remotely| indicates
whether to render the media
s/to render the media/the media is rendered/

https://codereview.chromium.org/2767823002/diff/260001/media/base/media_obser...
media/base/media_observer.h:21: // media pipeline are disabled.
s/are/should be/

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSValueKeywords.json5 (right):

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSValueKeywords.json5:671:
"media-remoting-cast-icon",
not sure if this should be renamed -internal.  i don't remember if that, by
itself, gives it magical properties to hide it from external modification. 
there's a way to do it, but i don't remember what it is.

worth a TODO.

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControls.css:168: -webkit-appearance:
media-remoting-background-image;
do these need to be in CSSValueKeywords.json5?

might not be needed if they don't need to be handled separately by the themed
painter... i don't remember.

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3642: // There can be
up to two children, either or both of the text
s/two/three/

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp (right):

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp:234: const int
button_width = std::min(rect.Width(), kMediaRemotingCastIconWidth);
what size box do you see here?  from the css, it looks like it's just the icon
size.  in that case, you might just be able to call PaintMediaButton, and not
memorize the desired size in kMediaRemotingCastIcon*.

the overlay button is 100%, which is why it needs to do extra math.

[xjz, 2017-04-12 23:59:26]

Patchset #12 (id:280001) has been deleted

[xjz, 2017-04-13 00:01:49]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 00:02:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xjz, 2017-04-13 00:08:48]

Thanks liberato@ for reviewing. Addressed your comments. Also, changed to use
HTMLDivElement instead of HTMLInputElement after an offline chatting with miu@.
PTAL. Thanks!

mlamouri@: PTAL. Need your RS. Thanks! :)

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp
(right):

https://codereview.chromium.org/2767823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:65:
setType(InputTypeNames::text);
On 2017/04/11 04:29:44, xjz wrote:
> On 2017/04/10 13:44:22, mlamouri wrote:
> > Why text instead of button?
> 
> I did try button type at first, but the css didn't work properly for button
> type. The border lines were not shown and the text was not in the center. So I
> used text here because it functions and by design this button only has a text
on
> and no background png. Is there a difference between using button and using
text
> here? If not big difference, can we use text for now? And the style might get
> changed later since the UX designer is waiting for this patch landed to check
> for adjustments before M59. WDYT? 

After a chat with miu@, we decide to use a div element for this button instead
of a HTMLInputElement. PTAL.

https://codereview.chromium.org/2767823002/diff/260001/content/child/blink_pl...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/2767823002/diff/260001/content/child/blink_pl...
content/child/blink_platform_impl.cc:532: {"mediaremotingCast",
IDR_MEDIA_REMOTING_CAST_ICON, ui::SCALE_FACTOR_100P,
On 2017/04/12 21:57:44, liberato wrote:
> please alphabetize.

Renamed it as mediaRemotingCastIcon

https://codereview.chromium.org/2767823002/diff/260001/media/base/media_obser...
File media/base/media_observer.h (right):

https://codereview.chromium.org/2767823002/diff/260001/media/base/media_obser...
media/base/media_observer.h:19: // possible. |is_rendered_remotely| indicates
whether to render the media
On 2017/04/12 21:57:44, liberato wrote:
> s/to render the media/the media is rendered/

Done.

https://codereview.chromium.org/2767823002/diff/260001/media/base/media_obser...
media/base/media_observer.h:21: // media pipeline are disabled.
On 2017/04/12 21:57:44, liberato wrote:
> s/are/should be/

Done.

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSValueKeywords.json5 (right):

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSValueKeywords.json5:671:
"media-remoting-cast-icon",
On 2017/04/12 21:57:44, liberato wrote:
> not sure if this should be renamed -internal.  i don't remember if that, by
> itself, gives it magical properties to hide it from external modification. 
> there's a way to do it, but i don't remember what it is.
> 
> worth a TODO.

Done. Changed it to -internal.

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControls.css:168: -webkit-appearance:
media-remoting-background-image;
On 2017/04/12 21:57:44, liberato wrote:
> do these need to be in CSSValueKeywords.json5?
> 
> might not be needed if they don't need to be handled separately by the themed
> painter... i don't remember.

Seems not necessary. Only the cast icon needs to be handled by the themed
painter. Now removed the -webkit-appearance.

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3642: // There can be
up to two children, either or both of the text
On 2017/04/12 21:57:44, liberato wrote:
> s/two/three/

Done. Also rewrite the comment. I missed this after rebasing. Thanks for
pointing this out. :)

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp (right):

https://codereview.chromium.org/2767823002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp:234: const int
button_width = std::min(rect.Width(), kMediaRemotingCastIconWidth);
On 2017/04/12 21:57:44, liberato wrote:
> what size box do you see here?  from the css, it looks like it's just the icon
> size.  in that case, you might just be able to call PaintMediaButton, and not
> memorize the desired size in kMediaRemotingCastIcon*.
> 
> the overlay button is 100%, which is why it needs to do extra math.

Done.

[miu, 2017-04-13 01:32:10]

PS12 lgtm % nit:

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLVideoElement.h (right):

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLVideoElement.h:136: MediaRemotingStatus
GetMediaRemotingStatus() const {
style nit: Inline accessors should be in lowercase:

  MediaRemotingStatus media_remoting_status() const {...}

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h (right):

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h:16: class
MediaRemotingDisableButtonElement final : public HTMLDivElement {
Nice. Yes, I think the DIV elements will be much lighter weight, since we really
only need elements to supply us layout, font-rendering, and border painting.

[commit-bot: I haz the power, 2017-04-13 02:16:09]

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

[commit-bot: I haz the power, 2017-04-13 02:16:11]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2017-04-13 17:20:52]

lgtm with comments applied.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControls.css:164: transition: opacity
.2s cubic-bezier (0.4, 0.0, 0.2, 1);
Sorry for the late comment: can you remove all the -webkit, -moz, -o properties?
Just keep 'transition' if Blink supports it. We don't intend for this file to
work cross-browser.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControls.css:232: transition: border .5s
ease-out;
ditto

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControls.css:236:
video::-internal-media-remoting-cast-icon:hover {
Tried locally: these do not seem to work.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLVideoElement.h (right):

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLVideoElement.h:57: enum
MediaRemotingStatus { kNotStarted, kStarted, kDisabled };
Can you make this an enum class?

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLVideoElement.h:136: MediaRemotingStatus
GetMediaRemotingStatus() const {
On 2017/04/13 at 01:32:10, miu wrote:
> style nit: Inline accessors should be in lowercase:
> 
>   MediaRemotingStatus media_remoting_status() const {...}

Unfortunately, this isn't true in Blink even after the new style:
https://docs.google.com/document/d/1ZQLiQKw9hCOyDDhDS4JhckhuiJmLqSbw9FLfySMyx...

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp
(right):

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:23: class
MediaRemotingDisableButtonElement::MouseEventsListener final
I looked more into this. The reason why you are not receiving the clicks is
because the remoting overlay is before the controls overlay in the DOM and the
controls overlay stay there. If you change the z-index higher than 1, it will
show up before the controls overlay. Because you use display: none when the
remoting overlay shouldn't show up, it should work fine.

Dropping this and use DefaultEventHandler() should significantly reduce the
complexity of this code.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:40:
DCHECK(event->type() == EventTypeNames::click);
DCHECK_EQ()?

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:54: true);
Why call removeEventListener()? Wouldn't this be called by DisableRemoting()
calling onHidden?

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:68:
AppendChild(text_);
Why not using setInnerText() and drop the Member<Text>?

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:99:
AppendChild(text_);
Same as above.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h (right):

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h:14: //
----------------------------
style: remove this here and below, add a quick documentation for all the classes
instead.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h:18: explicit
MediaRemotingDisableButtonElement(MediaRemotingInterstitial&);
I think this should be renamed. The name no longer matches what it is actually
doing.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h:22:
HTMLVideoElement& VideoElement() const;
nit: GetVideoElement()?

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h
(right):

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h:39:
HTMLVideoElement& VideoElement() const { return *video_element_; }
GetVideoElement()?

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h:49:
Member<MediaRemotingDisableButtonElement> disable_button_;
Rename this?

[xjz, 2017-04-13 19:10:48]

The CQ bit was checked by xjz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 19:11:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xjz, 2017-04-13 19:12:12]

Thanks mlamouri for reviewing. Addressed the comments.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControls.css:164: transition: opacity
.2s cubic-bezier (0.4, 0.0, 0.2, 1);
On 2017/04/13 17:20:51, mlamouri wrote:
> Sorry for the late comment: can you remove all the -webkit, -moz, -o
properties?
> Just keep 'transition' if Blink supports it. We don't intend for this file to
> work cross-browser.

Done.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControls.css:232: transition: border .5s
ease-out;
On 2017/04/13 17:20:51, mlamouri wrote:
> ditto

Done.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControls.css:236:
video::-internal-media-remoting-cast-icon:hover {
On 2017/04/13 17:20:51, mlamouri wrote:
> Tried locally: these do not seem to work.

Removed. The hover effect only works when remoting interstitial is the topmost
element (last child in the tree).

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLVideoElement.h (right):

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLVideoElement.h:57: enum
MediaRemotingStatus { kNotStarted, kStarted, kDisabled };
On 2017/04/13 17:20:51, mlamouri wrote:
> Can you make this an enum class?

Done.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLVideoElement.h:136: MediaRemotingStatus
GetMediaRemotingStatus() const {
On 2017/04/13 01:32:10, miu wrote:
> style nit: Inline accessors should be in lowercase:
> 
>   MediaRemotingStatus media_remoting_status() const {...}

Kept it unchanged according to the Blink style.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLVideoElement.h:136: MediaRemotingStatus
GetMediaRemotingStatus() const {
On 2017/04/13 17:20:51, mlamouri wrote:
> On 2017/04/13 at 01:32:10, miu wrote:
> > style nit: Inline accessors should be in lowercase:
> > 
> >   MediaRemotingStatus media_remoting_status() const {...}
> 
> Unfortunately, this isn't true in Blink even after the new style:
>
https://docs.google.com/document/d/1ZQLiQKw9hCOyDDhDS4JhckhuiJmLqSbw9FLfySMyx...

Thanks for clarification.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp
(right):

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:23: class
MediaRemotingDisableButtonElement::MouseEventsListener final
On 2017/04/13 17:20:51, mlamouri wrote:
> I looked more into this. The reason why you are not receiving the clicks is
> because the remoting overlay is before the controls overlay in the DOM and the
> controls overlay stay there. If you change the z-index higher than 1, it will
> show up before the controls overlay. Because you use display: none when the
> remoting overlay shouldn't show up, it should work fine.
> 
> Dropping this and use DefaultEventHandler() should significantly reduce the
> complexity of this code.

This approach seems not work with vimeo videos. It looks like vimeo also
registers a document event listener so that it can receive the clicks and pause
the video. I agree that by using DefaultEventHandler() can significantly reduce
the complexity. But for now, since this approach is the only approach that I
know that functions well, I'll leave it in. I can always merge a fix CL back
once we find an alternate way. sgty?

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:40:
DCHECK(event->type() == EventTypeNames::click);
On 2017/04/13 17:20:51, mlamouri wrote:
> DCHECK_EQ()?

Done.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:54: true);
On 2017/04/13 17:20:51, mlamouri wrote:
> Why call removeEventListener()? Wouldn't this be called by DisableRemoting()
> calling onHidden?

Done. Removed the call.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:68:
AppendChild(text_);
On 2017/04/13 17:20:51, mlamouri wrote:
> Why not using setInnerText() and drop the Member<Text>?

Done.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:99:
AppendChild(text_);
On 2017/04/13 17:20:51, mlamouri wrote:
> Same as above.

Done.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h (right):

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h:14: //
----------------------------
On 2017/04/13 17:20:51, mlamouri wrote:
> style: remove this here and below, add a quick documentation for all the
classes
> instead.

Done.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h:16: class
MediaRemotingDisableButtonElement final : public HTMLDivElement {
On 2017/04/13 01:32:10, miu wrote:
> Nice. Yes, I think the DIV elements will be much lighter weight, since we
really
> only need elements to supply us layout, font-rendering, and border painting.

Thanks for the suggestion. :)

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h:18: explicit
MediaRemotingDisableButtonElement(MediaRemotingInterstitial&);
On 2017/04/13 17:20:51, mlamouri wrote:
> I think this should be renamed. The name no longer matches what it is actually
> doing.

Done. Renamed it to MediaRemotingExitButtonElement.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h:22:
HTMLVideoElement& VideoElement() const;
On 2017/04/13 17:20:51, mlamouri wrote:
> nit: GetVideoElement()?

Done.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h
(right):

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h:39:
HTMLVideoElement& VideoElement() const { return *video_element_; }
On 2017/04/13 17:20:51, mlamouri wrote:
> GetVideoElement()?

Done.

https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h:49:
Member<MediaRemotingDisableButtonElement> disable_button_;
On 2017/04/13 17:20:51, mlamouri wrote:
> Rename this?

Done.

[commit-bot: I haz the power, 2017-04-13 19:16:00]

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

[commit-bot: I haz the power, 2017-04-13 19:16:02]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xjz, 2017-04-13 21:38:53]

The CQ bit was checked by xjz@chromium.org

[xjz, 2017-04-13 21:38:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org, kinuko@chromium.org, miu@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2767823002/#ps340001
(title: "Rebased.")

[commit-bot: I haz the power, 2017-04-13 21:39:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 23:28:31]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1492119533982750,
"parent_rev": "314052191919e73f2ed0c7b8014c842103d5688c", "commit_rev":
"df9b67eb9751650cebee13333ab125bb3f11df75"}

[commit-bot: I haz the power, 2017-04-13 23:29:21]

Description was changed from

==========
Media Remoting: Add interstitial elements to media element shadow dom.

This CL add the following elements to media element shadow dom when 
media remoting starts.
1. Add a disable button to allow user stop remoting and fall back to
tab mirroring. This is to solve the accessiblity regression caused by 
media remoting with lacking support for closed captions.
2. Add a background image, cast icon and a text message. These three 
are currently rendered as a video frame in media renderer. Move them
to media element to keep consistent with the disabel button and solve 
the font renderering issue on Windows 8/10.

The old interstitial (rendered as a video frame) will be removed in a
later CL. 

BUG=700572, 697672
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Media Remoting: Add interstitial elements to media element shadow dom.

This CL add the following elements to media element shadow dom when 
media remoting starts.
1. Add a disable button to allow user stop remoting and fall back to
tab mirroring. This is to solve the accessiblity regression caused by 
media remoting with lacking support for closed captions.
2. Add a background image, cast icon and a text message. These three 
are currently rendered as a video frame in media renderer. Move them
to media element to keep consistent with the disabel button and solve 
the font renderering issue on Windows 8/10.

The old interstitial (rendered as a video frame) will be removed in a
later CL. 

BUG=700572, 697672
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2767823002
Cr-Commit-Position: refs/heads/master@{#464604}
Committed:
https://chromium.googlesource.com/chromium/src/+/df9b67eb9751650cebee13333ab1...
==========

[commit-bot: I haz the power, 2017-04-13 23:29:23]

Committed patchset #14 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/df9b67eb9751650cebee13333ab1...