WIP - WebMediaPlayer switch media renderer.

The CL adds: 
1. The notification to WebMediaPlayer when html video is in fullscreen
playing back but it is not the fullscreen element.
2. The switching logic for media renderer in WebMediaPlayerImpl.

[xjz, 2016-08-03 17:01:27]

Description was changed from

==========
Notify WebMediaPlayerImpl when html video enters fullscreen.

BUG=
==========

to

==========
Notify WebMediaPlayer when html video enters fullscreen while the 
fullscreen element is not HTMLMediaElement.
==========

[liberato (no reviews please), 2016-08-03 21:54:16]

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

[liberato (no reviews please), 2016-08-03 21:54:17]

what you're doing seems reasonable, but there are plenty of subtleties that
perhaps i'm missing.

https://codereview.chromium.org/2204673004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2204673004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FullscreenController.cpp:91: if
(!isHTMLMediaElement(element)) {
could early-out on isRunningLayoutTest here, rather than in the loop.

actually, there's a bunch of code that's identical between the enter / exit
cases.  maybe some helper:

HTMLMediaElement* FullscreenController::findChildMediaElement(element)

also, what is the desired behavior if
 - more than one media element exists under |element|?
 - a media element is not visible in the sense of css display:none?

https://codereview.chromium.org/2204673004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FullscreenController.cpp:95: if
(isHTMLMediaElement(collectElement)) {
if (!is)... continue; will save {} nesting.

https://codereview.chromium.org/2204673004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FullscreenController.cpp:99:
videoElement->webMediaPlayer()->enteredFullscreen();
entered/exitedFullscreen shouldn't be called for an ancestor case. it will
trigger an optimization on android that will break standards conformance,
unfortunately.  my current project is to fix that.

i recommend adding something like ancestorEntered/ExitedFullscreen() to
WebMediaPlayer.  whatever processing fullscreen does for cast will have to
happen in both the ancestor and self cases, but most of the existing non-cast
logic will happen only in the self case.

[xjz, 2016-08-04 02:28:16]

liberato@: Thanks for your comments! Addressed them in Patch# 4.

https://codereview.chromium.org/2204673004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2204673004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FullscreenController.cpp:91: if
(!isHTMLMediaElement(element)) {
On 2016/08/03 21:54:17, liberato wrote:
> could early-out on isRunningLayoutTest here, rather than in the loop.
> 
> actually, there's a bunch of code that's identical between the enter / exit
> cases.  maybe some helper:
> 
> HTMLMediaElement* FullscreenController::findChildMediaElement(element)
> 
> also, what is the desired behavior if
>  - more than one media element exists under |element|?
>  - a media element is not visible in the sense of css display:none?

Done with the helper. 
Yes, it could have multiple media elements under this |element| or/and invisible
elements. The desired behavior here is to notify each media element that its
parent goes to fullscreen.

https://codereview.chromium.org/2204673004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FullscreenController.cpp:95: if
(isHTMLMediaElement(collectElement)) {
On 2016/08/03 21:54:17, liberato wrote:
> if (!is)... continue; will save {} nesting.

Done.

https://codereview.chromium.org/2204673004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FullscreenController.cpp:99:
videoElement->webMediaPlayer()->enteredFullscreen();
On 2016/08/03 21:54:17, liberato wrote:
> entered/exitedFullscreen shouldn't be called for an ancestor case. it will
> trigger an optimization on android that will break standards conformance,
> unfortunately.  my current project is to fix that.
> 
> i recommend adding something like ancestorEntered/ExitedFullscreen() to
> WebMediaPlayer.  whatever processing fullscreen does for cast will have to
> happen in both the ancestor and self cases, but most of the existing non-cast
> logic will happen only in the self case.

I agree. Now add WebMediaPlayer::updateMediaInFullscreen() for this purpose, and
keep the original non-cast logic.

[miu, 2016-08-09 19:22:43]

This looks good to me. Please loop-in owners of the WebKit code.

[xjz, 2016-08-19 01:26:07]

xjz@chromium.org changed reviewers:
- liberato@chromium.org

[xjz, 2016-08-19 01:29:11]

Description was changed from

==========
Notify WebMediaPlayer when html video enters fullscreen while the 
fullscreen element is not HTMLMediaElement.
==========

to

==========
The CL adds: 
1. The notification to WebMediaPlayer when html video is in fullscreen
playing back but it is not the fullscreen element.
2. The switching logic for media renderer in WebMediaPlayerImpl.
==========

[liberato (no reviews please), 2016-08-29 16:49:11]

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

[liberato (no reviews please), 2016-08-29 16:49:12]

thanks
-fl

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1760: void
WebMediaPlayerImpl::OnSinkAvailable() {
would it be clearer if these things were pulled out into another class?

for example, could wmpi check someNewCastStateThing_.IsMediaRemoting()?  it
could chain calls to videoEnteredFullscreen over to someNewCastStateThing, too. 
OnSink* could be moved over.

it might make things more cumbersome, but it depends a lot on how much code is
going to be added.  it also depends on where the callbacks are set up, etc.

not sure if it would make things clearer, or just end up with a bunch of chained
methods from wmpi => new thing without really removing much code.

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1776: // Switch media renderer between local
playing back and remoting media renderer
please move this to the .h (good comment, by the way).

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1779: void
WebMediaPlayerImpl::SwitchMediaRenderer() {
maybe take the new value of is_media_remoting_ as an argument, and have it take
responsibility for setting it.

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1782: pipeline_controller_.Resume();
how does this switch to remote playback?

is there there logic going to go into CreateRenderer to check
is_media_remoting_?

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:235: void videoExitedFullscreen() override;
these names are a little confusing with respect to entered/exitedFullscreen. 
'Video' doesn't really add anything to differentiate them.

maybe elementOrContainerEnteredFullscreen()?  then again, that's not terribly
descriptive of what's going on, either.  becamePrimaryContent?

the particular check used in blink right now is heuristic, and might change
later.  better to choose a name that fits the intention, than the current
implementation.

also, please make the documentation very clear about what these do.  include a
contrast with enteredFullscreen, so that somebody i the future will be able to
pick which one they care about.  probably better in WMP rather than here.

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:358: void OnSinkAvailable();
does anybody call these?

https://codereview.chromium.org/2204673004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/2204673004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:827: if
((htmlMediaElement.clientWidth() >= document.domWindow()->innerWidth()) &&
(htmlMediaElement.clientHeight() >= document.domWindow()->innerHeight()))
please consolidate this check with the one in HTMLMediaElement.

https://codereview.chromium.org/2204673004/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2204673004/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaPlayer.h:207: virtual void
videoExitedFullscreen() {}
please move these to be near entered/exitedFullscreen (here and wmpi).

[mark a. foltz, 2016-09-06 21:30:24]

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

[mark a. foltz, 2016-09-06 21:30:38]

Will look if I have time (don't block on my review).

[xjz, 2016-09-09 23:13:05]

xjz@chromium.org changed reviewers:
- mfoltz@chromium.org

[xjz, 2016-09-09 23:13:07]

Thanks for all the feedbacks. Still working on this. :)

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1760: void
WebMediaPlayerImpl::OnSinkAvailable() {
On 2016/08/29 16:49:12, liberato (ooo) wrote:
> would it be clearer if these things were pulled out into another class?
> 
> for example, could wmpi check someNewCastStateThing_.IsMediaRemoting()?  it
> could chain calls to videoEnteredFullscreen over to someNewCastStateThing,
too. 
> OnSink* could be moved over.
> 
> it might make things more cumbersome, but it depends a lot on how much code is
> going to be added.  it also depends on where the callbacks are set up, etc.
> 
> not sure if it would make things clearer, or just end up with a bunch of
chained
> methods from wmpi => new thing without really removing much code.

Yes. Done.

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1776: // Switch media renderer between local
playing back and remoting media renderer
On 2016/08/29 16:49:12, liberato (ooo) wrote:
> please move this to the .h (good comment, by the way).

Done.

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1779: void
WebMediaPlayerImpl::SwitchMediaRenderer() {
On 2016/08/29 16:49:12, liberato (ooo) wrote:
> maybe take the new value of is_media_remoting_ as an argument, and have it
take
> responsibility for setting it.

Done.

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1782: pipeline_controller_.Resume();
On 2016/08/29 16:49:12, liberato (ooo) wrote:
> how does this switch to remote playback?
> 
> is there there logic going to go into CreateRenderer to check
> is_media_remoting_?

Yes. The implementation will be in the upcoming CL. Added comment for this.

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:235: void videoExitedFullscreen() override;
On 2016/08/29 16:49:12, liberato (ooo) wrote:
> these names are a little confusing with respect to entered/exitedFullscreen. 
> 'Video' doesn't really add anything to differentiate them.
> 
> maybe elementOrContainerEnteredFullscreen()?  then again, that's not terribly
> descriptive of what's going on, either.  becamePrimaryContent?
> 
> the particular check used in blink right now is heuristic, and might change
> later.  better to choose a name that fits the intention, than the current
> implementation.
> 
> also, please make the documentation very clear about what these do.  include a
> contrast with enteredFullscreen, so that somebody i the future will be able to
> pick which one they care about.  probably better in WMP rather than here.

Changed the names to ancestorEntered/exitedFullscreen().

https://codereview.chromium.org/2204673004/diff/100001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:358: void OnSinkAvailable();
On 2016/08/29 16:49:12, liberato (ooo) wrote:
> does anybody call these?

This is called by Remoter, which is not implemented in this CL.

https://codereview.chromium.org/2204673004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/2204673004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:827: if
((htmlMediaElement.clientWidth() >= document.domWindow()->innerWidth()) &&
(htmlMediaElement.clientHeight() >= document.domWindow()->innerHeight()))
On 2016/08/29 16:49:12, liberato (ooo) wrote:
> please consolidate this check with the one in HTMLMediaElement.

Done.

https://codereview.chromium.org/2204673004/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2204673004/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaPlayer.h:207: virtual void
videoExitedFullscreen() {}
On 2016/08/29 16:49:12, liberato (ooo) wrote:
> please move these to be near entered/exitedFullscreen (here and wmpi).

Done.

[xjz, 2016-09-09 23:13:36]

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

[miu, 2016-09-14 20:59:27]

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

[miu, 2016-09-14 20:59:27]

https://codereview.chromium.org/2204673004/diff/140001/media/base/pipeline_im...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2204673004/diff/140001/media/base/pipeline_im...
media/base/pipeline_impl.cc:569: AudioDecoderConfig
PipelineImpl::RendererWrapper::GetAudioDecoderConfig()
base::Optional<AudioDecoderConfig>
PipelineImpl::RendererWrapper::GetAudioDecoderConfig() const {
  base::AutoLock autolock(&lock_);
  if (the var was set)
    return audio_decoder_config_;
  else
    return base::nullopt;
}

[xjz, 2016-09-19 18:53:36]

xjz@chromium.org changed reviewers:
- liberato@chromium.org

[xjz, 2016-09-27 22:58:51]

Patchset #13 (id:240001) has been deleted

[xjz, 2016-09-28 00:31:59]

Patchset #13 (id:260001) has been deleted

[miu, 2016-09-28 01:33:32]

Looks great!

At a high level (described in more detail later):

1. I think we should rename RendererController to MediaPlayerController since
it's controlling a player and not a media::Renderer.

2. I think we should add a MediaPlayer interface that WMPI implements, instead
of using the SwitchRenderer callback scheme.

https://codereview.chromium.org/2204673004/diff/280001/media/base/renderer_co...
File media/base/renderer_controller.h (right):

https://codereview.chromium.org/2204673004/diff/280001/media/base/renderer_co...
media/base/renderer_controller.h:15: // This class is an observer of
media::WebMediaPlayerImpl and can trigger a
Don't mention WebMediaPlayerImpl in the comments because that's not a concept
known to src/media/base code.

https://codereview.chromium.org/2204673004/diff/280001/media/base/renderer_co...
media/base/renderer_controller.h:17: class RendererController {
naming: Now that I've seen the code, I think this should be named
MediaPlayerController. It is receiving signals from the player and then telling
the player when to switch. It never interacts directly with a media::Renderer
instance.

https://codereview.chromium.org/2204673004/diff/280001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2204673004/diff/280001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:272: renderer_controller_ = nullptr;
Along the lines of other comments: Call controller_->SetMediaPlayer(nullptr)
here to notify the controller that it cannot reference the MediaPlayer anymore.

https://codereview.chromium.org/2204673004/diff/280001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1108:
renderer_controller_->OnSetDecoderConfig(
naming nit: If this could be called multiple times, let's name this
OnDecoderConfigChanged().

https://codereview.chromium.org/2204673004/diff/280001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2204673004/diff/280001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:220: // Ask for the renderer to be restarted
(destructed and recreated).
Suggestion:

  // Restart the player/pipeline as soon as possible. This will destroy the
current Renderer, if any, and create a new one via the RendererFactory; and then
seek to resume playback at the current position.

https://codereview.chromium.org/2204673004/diff/280001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:221: void ScheduleRestart();
Thinking about how apacible's work will also integrate with WMPI, I really think
we should create a MediaPlayer interface that WMPI implements (rather than just
using a callback for ScheduleRestart()).

Granted, with your change, there will just be one ScheduleRestart() method in
this interface, so it seems overkill. However, thinking forward, I'm pretty sure
the UI component will also need a few things. For example:

  class MediaPlayer {
    virtual void ScheduleRestart() = 0;

    // Later, for apacible's change:
    // virtual gfx::Image GetPosterImage() = 0;
  };

Then, in the controller, SetSwitchRenderCallback() becomes:

  virtual void SetMediaPlayer(MediaPlayer* player) = 0;

Then, in the WMPI ctor:

  controller->SetMediaPlayer(this);

WDYT?

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
File media/remoting/remoting_controller.cc (right):

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:68: remoter_->Start();
Thinking aloud: Maybe this method should just set use_remoting_renderer_ = false
and wait for other signals (that will call Update()) to set it back to true.
From the user perspective, this would require them to, say, leave and re-enter
fullscreen mode before we attempt to start Remoting again.

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:84: // TODO(xjz): Not implemented. Will
add in up-coming change.
Note: Please use the NOTIMPLEMENTED() logging macro here. It'll log a warning to
remind us.

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:98: audio_decoder_config_ = audio_config;
Please add DCHECK(task_runner->Belongs()) here.

BTW--Nice that you found a way to do this on the main thread! :)

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:154: // TODO(xjz): The swithcing logic for
encrypted content will be added in a
typo: s/swithcing/switching/

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:156: if
(video_decoder_config_.is_encrypted())
Instead of returning early, I'd suggest you add it to the "should" boolean
below.

  bool should_be_remoting = ... && !video_decoder_config_.is_encrypted();

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:159: bool should_remoting =
is_sink_available_ && is_fullscreen_ &&
naming: should_be_remoting

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:169: if (!switch_renderer_cb_.is_null())
In the "should" boolean above, I'd suggest adding:

  bool should_be_remoting = ... && !switch_renderer_cb.is_null();

(So that we never try to call remoter_->Start() if the callback is missing.)

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
File media/remoting/remoting_controller.h (right):

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.h:16: // 2) Gets info from
media::WebMediaPlayerImpl;
s/media::WebMediaPlayerImpl/media::MediaPlayer/

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.h:52: // Re-calculates
|use_remoting_renderer_|.
Suggestion: // Determines whether to enter or leave Remoting mode and switches
if necessary.

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.h:62: bool use_remoting_renderer_;
naming: Looked at everywhere this is being used. It doesn't seem that it means
"should be remoting" anymore. Instead, it looks like it means "is remoting." So,
I'd suggest naming it something like: is_remoting_ or remoting_is_started_.

[miu, 2016-09-28 07:23:09]

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
File media/remoting/remoting_controller.cc (right):

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:18:
remoter_factory->Create(binding_.CreateInterfacePtrAndBind(),
This issue came up on Eric's change: We should be doing the mojo binding on the
IO thread instead of the main thread. This is important because we have objects
on two different threads (the main thread and the media thread) that want to
make and receive IPC calls through the mojo interface. There's also a concern
that the main thread can block for long periods of time (e.g., if the page runs
JavaScript in an infinite loop).

So, for all calls to |remoter_|, we'll need to:

  io_thread_task_runner_->PostTask(...remoter_->MethodCall() here...);

...and, all methods in this class that implement media::mojom::RemotingSource
will get called on the IO thread. Other than OnMessageFromSink(), the method
calls will have to trampoline back to the main thread to call your Update()
method.

...and, finally, at destruction time, the |binding_.Unbind()| will have to occur
on the IO thread.

[xhwang, 2016-09-28 23:27:24]

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

[xhwang, 2016-09-28 23:27:25]

I did review the CL. Just commenting on miu's comments.

Naming: We only use "MediaPlayer" in media/blink (as in WebMediaPlayer), and we
try to keep the concept of "media player" in the blink layer. That's why in
media/base you don't see "media player" anywhere (except for Android where we
have MediaPlayer Android API :( ). I don't have any better name on top of my
head though.

Threading: I am not sure how things should work and might be missing something.
Are you concerned about remoting being blocked by tasks on the main thread?
Where should those tasks run? If on media thread, we should be able to bind the
service to media thread directly, so that messages will be forwarded from IO to
media thread automatically.

Here's some statement on threading that might be related.
"Making a method call doesn't involve a thread hop, but receiving it on a thread
other than the IO thread does involve a thread hop."
https://www.chromium.org/developers/design-documents/mojo/chrome-ipc-to-mojo-...

[xjz, 2016-09-28 23:47:50]

Patchset #14 (id:300001) has been deleted

[xjz, 2016-09-29 22:47:36]

Patchset #14 (id:320001) has been deleted

[xjz, 2016-09-29 22:54:22]

Addressed comments. PTAL.

https://codereview.chromium.org/2204673004/diff/280001/media/base/renderer_co...
File media/base/renderer_controller.h (right):

https://codereview.chromium.org/2204673004/diff/280001/media/base/renderer_co...
media/base/renderer_controller.h:15: // This class is an observer of
media::WebMediaPlayerImpl and can trigger a
On 2016/09/28 01:33:31, miu wrote:
> Don't mention WebMediaPlayerImpl in the comments because that's not a concept
> known to src/media/base code.

Done.

https://codereview.chromium.org/2204673004/diff/280001/media/base/renderer_co...
media/base/renderer_controller.h:17: class RendererController {
On 2016/09/28 01:33:31, miu wrote:
> naming: Now that I've seen the code, I think this should be named
> MediaPlayerController. It is receiving signals from the player and then
telling
> the player when to switch. It never interacts directly with a media::Renderer
> instance.

Renamed it as  MediaPlayerObserver, since it could be a pure observer if the
SwitchRendererCallback is not set. Does that make sense? Feel free to suggest
better names. :)

https://codereview.chromium.org/2204673004/diff/280001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2204673004/diff/280001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:272: renderer_controller_ = nullptr;
On 2016/09/28 01:33:31, miu wrote:
> Along the lines of other comments: Call controller_->SetMediaPlayer(nullptr)
> here to notify the controller that it cannot reference the MediaPlayer
anymore.

Removed this line.

https://codereview.chromium.org/2204673004/diff/280001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1108:
renderer_controller_->OnSetDecoderConfig(
On 2016/09/28 01:33:31, miu wrote:
> naming nit: If this could be called multiple times, let's name this
> OnDecoderConfigChanged().

Done.

https://codereview.chromium.org/2204673004/diff/280001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2204673004/diff/280001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:220: // Ask for the renderer to be restarted
(destructed and recreated).
On 2016/09/28 01:33:31, miu wrote:
> Suggestion:
> 
>   // Restart the player/pipeline as soon as possible. This will destroy the
> current Renderer, if any, and create a new one via the RendererFactory; and
then
> seek to resume playback at the current position.

Done.

https://codereview.chromium.org/2204673004/diff/280001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:221: void ScheduleRestart();
On 2016/09/28 01:33:32, miu wrote:
> Thinking about how apacible's work will also integrate with WMPI, I really
think
> we should create a MediaPlayer interface that WMPI implements (rather than
just
> using a callback for ScheduleRestart()).
> 
> Granted, with your change, there will just be one ScheduleRestart() method in
> this interface, so it seems overkill. However, thinking forward, I'm pretty
sure
> the UI component will also need a few things. For example:
> 
>   class MediaPlayer {
>     virtual void ScheduleRestart() = 0;
> 
>     // Later, for apacible's change:
>     // virtual gfx::Image GetPosterImage() = 0;
>   };
> 
> Then, in the controller, SetSwitchRenderCallback() becomes:
> 
>   virtual void SetMediaPlayer(MediaPlayer* player) = 0;
> 
> Then, in the WMPI ctor:
> 
>   controller->SetMediaPlayer(this);
> 
> WDYT?

As discussed offline, we may be able to pass info through MediaPlayerObserver
for apacible's change. And as I was told, the ScheduleRestart() will be
refactored and moved to PipelineController. So for now continue with the
callback, and could refactor it later if necessary.

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
File media/remoting/remoting_controller.cc (right):

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:18:
remoter_factory->Create(binding_.CreateInterfacePtrAndBind(),
On 2016/09/28 07:23:09, miu wrote:
> This issue came up on Eric's change: We should be doing the mojo binding on
the
> IO thread instead of the main thread. This is important because we have
objects
> on two different threads (the main thread and the media thread) that want to
> make and receive IPC calls through the mojo interface. There's also a concern
> that the main thread can block for long periods of time (e.g., if the page
runs
> JavaScript in an infinite loop).
> 
> So, for all calls to |remoter_|, we'll need to:
> 
>   io_thread_task_runner_->PostTask(...remoter_->MethodCall() here...);
> 
> ...and, all methods in this class that implement media::mojom::RemotingSource
> will get called on the IO thread. Other than OnMessageFromSink(), the method
> calls will have to trampoline back to the main thread to call your Update()
> method.
> 
> ...and, finally, at destruction time, the |binding_.Unbind()| will have to
occur
> on the IO thread.

As discussed offline, keep this on main thread for now.

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:68: remoter_->Start();
On 2016/09/28 01:33:32, miu wrote:
> Thinking aloud: Maybe this method should just set use_remoting_renderer_ =
false
> and wait for other signals (that will call Update()) to set it back to true.
> From the user perspective, this would require them to, say, leave and re-enter
> fullscreen mode before we attempt to start Remoting again.

Done.

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:84: // TODO(xjz): Not implemented. Will
add in up-coming change.
On 2016/09/28 01:33:32, miu wrote:
> Note: Please use the NOTIMPLEMENTED() logging macro here. It'll log a warning
to
> remind us.

Done.

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:98: audio_decoder_config_ = audio_config;
On 2016/09/28 01:33:32, miu wrote:
> Please add DCHECK(task_runner->Belongs()) here.
> 
> BTW--Nice that you found a way to do this on the main thread! :)

Done.

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:154: // TODO(xjz): The swithcing logic for
encrypted content will be added in a
On 2016/09/28 01:33:32, miu wrote:
> typo: s/swithcing/switching/

Done.

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:156: if
(video_decoder_config_.is_encrypted())
On 2016/09/28 01:33:32, miu wrote:
> Instead of returning early, I'd suggest you add it to the "should" boolean
> below.
> 
>   bool should_be_remoting = ... && !video_decoder_config_.is_encrypted();

Done.

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:159: bool should_remoting =
is_sink_available_ && is_fullscreen_ &&
On 2016/09/28 01:33:32, miu wrote:
> naming: should_be_remoting

Done.

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.cc:169: if (!switch_renderer_cb_.is_null())
On 2016/09/28 01:33:32, miu wrote:
> In the "should" boolean above, I'd suggest adding:
> 
>   bool should_be_remoting = ... && !switch_renderer_cb.is_null();
> 
> (So that we never try to call remoter_->Start() if the callback is missing.)

Done.

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
File media/remoting/remoting_controller.h (right):

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.h:16: // 2) Gets info from
media::WebMediaPlayerImpl;
On 2016/09/28 01:33:32, miu wrote:
> s/media::WebMediaPlayerImpl/media::MediaPlayer/

Not applicable.

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.h:52: // Re-calculates
|use_remoting_renderer_|.
On 2016/09/28 01:33:32, miu wrote:
> Suggestion: // Determines whether to enter or leave Remoting mode and switches
> if necessary.

Done.

https://codereview.chromium.org/2204673004/diff/280001/media/remoting/remotin...
media/remoting/remoting_controller.h:62: bool use_remoting_renderer_;
On 2016/09/28 01:33:32, miu wrote:
> naming: Looked at everywhere this is being used. It doesn't seem that it means
> "should be remoting" anymore. Instead, it looks like it means "is remoting."
So,
> I'd suggest naming it something like: is_remoting_ or remoting_is_started_.

Renamed "is_remoting_".

[miu, 2016-09-30 08:12:11]

IMHO, we're ready to start the code review process w/ the other stakeholders.

https://codereview.chromium.org/2204673004/diff/340001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2204673004/diff/340001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2712: // TODO(miu): In a soon-upcoming
change, call GetRemoterFactory()->Create() to
Looks like you can remove this TODO comment now. ;)

https://codereview.chromium.org/2204673004/diff/340001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/2204673004/diff/340001/media/BUILD.gn#newcode603
media/BUILD.gn:603: "//media/base",
Is this still needed?

https://codereview.chromium.org/2204673004/diff/340001/media/base/mediaplayer...
File media/base/mediaplayer_observer.h (right):

https://codereview.chromium.org/2204673004/diff/340001/media/base/mediaplayer...
media/base/mediaplayer_observer.h:36: virtual void SetSwitchRenderCallback(const
SwitchRendererCallback& cb) = 0;
Hmm...Everything except this looks like an "observed event of interest." Is
there a simple way to not have this method in this observer interface? Maybe
this SetSwitchRendererCallback() can be called from render_frame_impl.cc?

https://codereview.chromium.org/2204673004/diff/340001/media/base/mediaplayer...
media/base/mediaplayer_observer.h:39:
DISALLOW_COPY_AND_ASSIGN(MediaPlayerObserver);
This doesn't belong in a pure virtual interface, since there's no reason to
restrict copying of a class that can't be instantiated.

https://codereview.chromium.org/2204673004/diff/340001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2204673004/diff/340001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1105: if (observer_)
style: Multi-line then-clause needs braces around it.

https://codereview.chromium.org/2204673004/diff/340001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2204673004/diff/340001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:560: // Outlives this class. This observer may
trigger restarting pipeline to
This comment may need adjustment, depending on the other comment.

https://codereview.chromium.org/2204673004/diff/340001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:562: MediaPlayerObserver* observer_;
nit: const please.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
File media/remoting/remoting_controller.cc (right):

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller.cc:177: // TODO(xjz): Send RPC message to
notify ChromeCast.
We shouldn't be talking about Chromecast in Chromium code. This implementation
could be used by any remote device that supports media remoting with the RPC
scheme being implemented in src/media/remoting.

That said, I don't think you need the TODO comment here. By running the
|switch_renderer_cb_|, the RemotingRenderer will be shut down and it can send
the RPC itself.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
File media/remoting/remoting_controller.h (right):

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller.h:16: // 2) Gets info from
media::WebMediaPlayerImpl;
For #2, WMPI is not known in media/remoting code. Suggestion:

  2) Monitors player events as a MediaPlayerObserver.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller.h:25: explicit
RemotingController(mojom::RemoterFactory* remoter_factory);
This looks like you're injecting a RemoterFactory as a dependency. So, please
comment that |remoter_factory| is just being used to create a Remoter and need
not remain valid after the constructor returns.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller.h:46: bool is_remoting();
Simple accessors should be defined inline (and const):

  bool is_remoting() const { return is_remoting_; }

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
File media/remoting/remoting_controller_unittest.cc (right):

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller_unittest.cc:42:
FakeRemoter(mojom::RemotingSourcePtr source, bool enable_remoting)
naming nit: Instead of |enable_remoting|, this is really controlling whether
starting will fail.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller_unittest.cc:49: source_->OnStarted();
Because OnStarted() is called later, on a reply IPC, consider posting a task
instead of calling OnStarted() directly.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller_unittest.cc:51:
source_->OnStartFailed(mojom::RemotingStartFailReason::ROUTE_TERMINATED);
ditto here: post task

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller_unittest.cc:61:
source_->OnStopped(mojom::RemotingStopReason::LOCAL_PLAYBACK);
ditto here: post task

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller_unittest.cc:75: FakeRemoterFactory(bool
enable_remoting)
explicit

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
File media/remoting/remoting_renderer_factory.h (right):

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_renderer_factory.h:39: std::unique_ptr<RendererFactory>
default_renderer_factory_;
Please const both of these.

https://codereview.chromium.org/2204673004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/2204673004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:795: // Notify WMPI if
the video is loaded in fullscreen or its ancestor is full screen element.
Blink coding style generally leans on the side of "no comments" so consider
removing this one. Same goes for comments in the other files.

If you decide to keep it, don't mention WMPI since this code doesn't know what a
WMPI is (but it does know what a WebMediaPlayer is).

[xjz, 2016-09-30 17:06:34]

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

[commit-bot: I haz the power, 2016-09-30 17:07:06]

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, 2016-09-30 17:11:14]

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

[commit-bot: I haz the power, 2016-09-30 17:11:16]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xjz, 2016-09-30 20:23:14]

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

[commit-bot: I haz the power, 2016-09-30 20:23:44]

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

[xjz, 2016-09-30 20:27:44]

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

[commit-bot: I haz the power, 2016-09-30 20:28:18]

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, 2016-09-30 20:33:13]

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

[commit-bot: I haz the power, 2016-09-30 20:33:14]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xjz, 2016-09-30 21:10:29]

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

[commit-bot: I haz the power, 2016-09-30 21:11:05]

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, 2016-09-30 21:26:09]

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

[commit-bot: I haz the power, 2016-09-30 21:26:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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, 2016-09-30 21:48:34]

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

[commit-bot: I haz the power, 2016-09-30 21:49:15]

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, 2016-09-30 22:03:04]

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

[commit-bot: I haz the power, 2016-09-30 22:03:05]

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

[xjz, 2016-10-01 00:02:56]

Patchset #16 (id:380001) has been deleted

[xjz, 2016-10-01 00:03:18]

Patchset #15 (id:360001) has been deleted

[xjz, 2016-10-01 00:28:42]

Addressed miu's comments. Thanks all for the reviewing. Opened a new issue for
public code review. https://codereview.chromium.org/2389473002

https://codereview.chromium.org/2204673004/diff/340001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2204673004/diff/340001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2712: // TODO(miu): In a soon-upcoming
change, call GetRemoterFactory()->Create() to
On 2016/09/30 08:12:10, miu wrote:
> Looks like you can remove this TODO comment now. ;)

Done.

https://codereview.chromium.org/2204673004/diff/340001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/2204673004/diff/340001/media/BUILD.gn#newcode603
media/BUILD.gn:603: "//media/base",
On 2016/09/30 08:12:10, miu wrote:
> Is this still needed?

Removed.

https://codereview.chromium.org/2204673004/diff/340001/media/base/mediaplayer...
File media/base/mediaplayer_observer.h (right):

https://codereview.chromium.org/2204673004/diff/340001/media/base/mediaplayer...
media/base/mediaplayer_observer.h:36: virtual void SetSwitchRenderCallback(const
SwitchRendererCallback& cb) = 0;
On 2016/09/30 08:12:10, miu wrote:
> Hmm...Everything except this looks like an "observed event of interest." Is
> there a simple way to not have this method in this observer interface? Maybe
> this SetSwitchRendererCallback() can be called from render_frame_impl.cc?

Done.

https://codereview.chromium.org/2204673004/diff/340001/media/base/mediaplayer...
media/base/mediaplayer_observer.h:39:
DISALLOW_COPY_AND_ASSIGN(MediaPlayerObserver);
On 2016/09/30 08:12:10, miu wrote:
> This doesn't belong in a pure virtual interface, since there's no reason to
> restrict copying of a class that can't be instantiated.

Removed.

https://codereview.chromium.org/2204673004/diff/340001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2204673004/diff/340001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1105: if (observer_)
On 2016/09/30 08:12:10, miu wrote:
> style: Multi-line then-clause needs braces around it.

Done.

https://codereview.chromium.org/2204673004/diff/340001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2204673004/diff/340001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:560: // Outlives this class. This observer may
trigger restarting pipeline to
On 2016/09/30 08:12:10, miu wrote:
> This comment may need adjustment, depending on the other comment.

Done.

https://codereview.chromium.org/2204673004/diff/340001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:562: MediaPlayerObserver* observer_;
On 2016/09/30 08:12:10, miu wrote:
> nit: const please.

Done.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
File media/remoting/remoting_controller.cc (right):

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller.cc:177: // TODO(xjz): Send RPC message to
notify ChromeCast.
On 2016/09/30 08:12:10, miu wrote:
> We shouldn't be talking about Chromecast in Chromium code. This implementation
> could be used by any remote device that supports media remoting with the RPC
> scheme being implemented in src/media/remoting.
> 
> That said, I don't think you need the TODO comment here. By running the
> |switch_renderer_cb_|, the RemotingRenderer will be shut down and it can send
> the RPC itself.

Done.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
File media/remoting/remoting_controller.h (right):

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller.h:16: // 2) Gets info from
media::WebMediaPlayerImpl;
On 2016/09/30 08:12:10, miu wrote:
> For #2, WMPI is not known in media/remoting code. Suggestion:
> 
>   2) Monitors player events as a MediaPlayerObserver.

Done.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller.h:25: explicit
RemotingController(mojom::RemoterFactory* remoter_factory);
On 2016/09/30 08:12:10, miu wrote:
> This looks like you're injecting a RemoterFactory as a dependency. So, please
> comment that |remoter_factory| is just being used to create a Remoter and need
> not remain valid after the constructor returns.

Done.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller.h:46: bool is_remoting();
On 2016/09/30 08:12:10, miu wrote:
> Simple accessors should be defined inline (and const):
> 
>   bool is_remoting() const { return is_remoting_; }

Done.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
File media/remoting/remoting_controller_unittest.cc (right):

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller_unittest.cc:42:
FakeRemoter(mojom::RemotingSourcePtr source, bool enable_remoting)
On 2016/09/30 08:12:10, miu wrote:
> naming nit: Instead of |enable_remoting|, this is really controlling whether
> starting will fail.

Renamed as |start_will_fail|.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller_unittest.cc:49: source_->OnStarted();
On 2016/09/30 08:12:10, miu wrote:
> Because OnStarted() is called later, on a reply IPC, consider posting a task
> instead of calling OnStarted() directly.

Done.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller_unittest.cc:51:
source_->OnStartFailed(mojom::RemotingStartFailReason::ROUTE_TERMINATED);
On 2016/09/30 08:12:10, miu wrote:
> ditto here: post task

Done.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller_unittest.cc:61:
source_->OnStopped(mojom::RemotingStopReason::LOCAL_PLAYBACK);
On 2016/09/30 08:12:10, miu wrote:
> ditto here: post task

Done.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_controller_unittest.cc:75: FakeRemoterFactory(bool
enable_remoting)
On 2016/09/30 08:12:10, miu wrote:
> explicit

Done.

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
File media/remoting/remoting_renderer_factory.h (right):

https://codereview.chromium.org/2204673004/diff/340001/media/remoting/remotin...
media/remoting/remoting_renderer_factory.h:39: std::unique_ptr<RendererFactory>
default_renderer_factory_;
On 2016/09/30 08:12:11, miu wrote:
> Please const both of these.

Done.

https://codereview.chromium.org/2204673004/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/2204673004/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:795: // Notify WMPI if
the video is loaded in fullscreen or its ancestor is full screen element.
On 2016/09/30 08:12:11, miu wrote:
> Blink coding style generally leans on the side of "no comments" so consider
> removing this one. Same goes for comments in the other files.
> 
> If you decide to keep it, don't mention WMPI since this code doesn't know what
a
> WMPI is (but it does know what a WebMediaPlayer is).

Done. Removed comments.

[xjz, 2016-10-01 04:08:44]

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

[commit-bot: I haz the power, 2016-10-01 04:08:57]

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, 2016-10-01 04:35:50]

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

[commit-bot: I haz the power, 2016-10-01 04:35:51]

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_...)