WIP - Add EME

WIP - Add EME

[miu, 2016-10-08 01:06:14]

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

[miu, 2016-10-08 01:06:16]

Overall, as I mentioned face-to-face today, I think we should split-up
RemotingController into separate classes. I explained a bit in the comments
below.

Also, note some of these comments may or may not apply, depending on how
breaking-up RemotingController goes...

https://codereview.chromium.org/2406483002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2406483002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:6354: #if BUILDFLAG(ENABLE_MEDIA_REMOTING)
This needs to go inside the #if...#endif section above, since
PepperCdmWrapperImpl won't work if !defined(ENABLE_PEPPER_CDMS).

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
File media/remoting/remoting_cdm.cc (right):

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_cdm.cc:18: void RemotingCdm::Create(
This function is not necessary. Clients can just invoke the constructor
directly.

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
File media/remoting/remoting_cdm.h (right):

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_cdm.h:36: std::unique_ptr<RemotingController>
remoting_controller() {
naming: This isn't an accessor method. So, I'd suggest naming it:
PassRemotingController().

However, I'm not sure you *should* be passing the ownership here. Maybe this
should be an accessor method only? The reason is that RemotingCdm will still
want to send/receive messages and Start/Stop the remoting session, and it needs
a reference to the RemotingController to do that. Also, see comments in
remoting_controller.cc below.

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
File media/remoting/remoting_cdm_factory.cc (right):

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_cdm_factory.cc:55:
RemotingCdmFactory::CreateRemotingController() {
Folks are probably going to suggest this code go into RenderFrameImpl, just like
in the last CL.

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_cdm_factory.cc:70: const SessionMessageCB&
session_message_cb,
Not your problem, but this spaghetti mess of callbacks should be cleaned-up at
some point (i.e., replaced with an interface). Maybe file a TODO bug and assign
it to xwhang@ or sandersd@?

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_cdm_factory.cc:75: std::unique_ptr<RemotingController>
remoting_controller =
It's okay (and probably better) to create the RemotingController in the ctor. By
creating it earlier, it has a chance to receive the OnSinkAvailable()/Gone()
call earlier, and that means we'll know sooner whether to create a remoting CDM
or a local one.

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_cdm_factory.cc:77:
remoting_controller->ShouldCreateRemotingCdm(base::Bind(
This will crash: base::Passed(&remoting_controller) will execute and invalidate
the std::unique_ptr before it is used as the "this" pointer to the
ShouldCreateRemotingCdm() method call.

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
File media/remoting/remoting_controller.cc (right):

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_controller.cc:104: if (auto* cdm =
RemotingCdm::From(cdm_context)) {
Possible edge-case issue: What if a remoting session was already started? Should
we stop it first, before using the delegate's session? Or, if it shouldn't be
possible, perhaps we need a CHECK() here? It's also possible a CHECK() could be
wrong: Maybe instead we should log an error and abort all remoting without
crashing the process?

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_controller.cc:267: cdm_check_cb_.Run(false);
What if the sink becomes available later? Say, in 500 ms?

I think there's a simpler solution here: How about you *always* call
remoter_->Start() and then run the |cdm_check_cb_| in OnStarted() or
OnStartFailed()? It doesn't matter whether the sink is available or not, the
browser process will always give you back an appropriate response. :)

  if (is_remoting_) {
    // Uh oh! What do we do if we were already remoting?!?!?!?
    return;
  }

  if (cdm_check_cb_.is_null()) {
    cdm_check_cb_ = cb;
    remoter_->Start();
  } else {
    // Uh oh! Maybe we need to support multiple CDM instances using the remoting
session? Would a list of CB's be appropriate?
  }

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
File media/remoting/remoting_controller.h (right):

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_controller.h:22: // This class does the following:
This comment should be next to the class declaration, not the namespace
delcaration.

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_controller.h:30: class RemotingController final : public
MediaObserver,
I think you should consider splitting this class up: The reason is that the
control logic for starting/ending a session around a CDM is significantly
different than that for a WMPI/Renderer, and it seems like we're mixing-up the
internal state and workflow a bit too much. I'll e-mail you some ideas I have
around this.

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_controller.h:62: // The callback used to tell which CDM
should be created.
This comment isn't really necessary. But, the method needs a comment that's
similar to this. How about:

  // Query whether to create a Media Remoting CDM. |cb| could be run
synchronously or asynchronously, depending on whether the required information
is available now or later.

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_controller.h:81: return false;
If this is the RemotingController owned by the CDM factory, I think you would
want it to return |is_remoting_failed_| here? If that's right, then the comment
is misleading and should probably be deleted.

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_controller.h:102: bool is_remoting_ = false;
This is now "sort of" true because it will be false while a remoting session has
been started for the CDM. (This is one of the reasons I'm proposing we split-up
this class.)

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_controller.h:108: bool is_remoting_cdm_ = false;
This isn't necessary, because this can always be determined from other data
members. You could make an inline helper method for it instead, such as:

  bool is_remoting() const {
    if (cdm_remoting_controller_)
      return cdm_remoting_controller_->is_remoting_;
    return is_remoting_;
  }

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_controller.h:110: // Indicates that remoting was stopped
after CDM is created remotely. In this
Suggested wording:

// Indicates that the video stack cannot continue operation. For example, if a
remoting session involving CDM content was stopped, there is no way to continue
playback because the CDM is required but is no longer available.

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_controller.h:112: bool is_remoting_failed_ = false;
naming: How about has_terminated_permanently_.

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_controller.h:112: bool is_remoting_failed_ = false;
Actually, do you need this bool? Wouldn't it always be equal to:

  (audio_decoder_config_.is_encrypted() ||
   video_decoder_config_.is_encrypted()) &&
  !cdm_remoting_controller_

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_controller.h:131: CdmCheckCallback cdm_check_cb_;
It seems like this is a "run once" callback. Is that right? If so, you should
reset this callback (e.g., using base::ResetAndReturn()) each place where you
Run() it, since it would be catastrophic if it were called twice.

https://codereview.chromium.org/2406483002/diff/20001/media/remoting/remoting...
media/remoting/remoting_controller.h:137: std::unique_ptr<RemotingController>
cdm_remoting_controller_;
It doesn't seem that you should be taking ownership of the other
RemotingController here. Perhaps let RemotingCDM always own it, and use a
WeakPtr here? (This is another reason I think we should split-up this class.)

[xjz, 2016-10-20 21:03:32]

Patchset #3 (id:40001) has been deleted

[xjz, 2016-10-20 21:14:52]

Patchset #3 (id:60001) has been deleted

[xjz, 2016-10-20 21:22:53]

Patchset #3 (id:80001) has been deleted

[xjz, 2016-10-20 21:25:28]

Split-up the RemotingController and addressed comments. PTAL.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/content/renderer...
File content/renderer/render_frame_impl.cc (right):

https://chromiumcodereview.appspot.com/2406483002/diff/20001/content/renderer...
content/renderer/render_frame_impl.cc:6354: #if BUILDFLAG(ENABLE_MEDIA_REMOTING)
On 2016/10/08 01:06:15, miu wrote:
> This needs to go inside the #if...#endif section above, since
> PepperCdmWrapperImpl won't work if !defined(ENABLE_PEPPER_CDMS).

Done.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
File media/remoting/remoting_cdm.cc (right):

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_cdm.cc:18: void RemotingCdm::Create(
On 2016/10/08 01:06:16, miu wrote:
> This function is not necessary. Clients can just invoke the constructor
> directly.

Done.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
File media/remoting/remoting_cdm.h (right):

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_cdm.h:36: std::unique_ptr<RemotingController>
remoting_controller() {
On 2016/10/08 01:06:16, miu wrote:
> naming: This isn't an accessor method. So, I'd suggest naming it:
> PassRemotingController().
> 
> However, I'm not sure you *should* be passing the ownership here. Maybe this
> should be an accessor method only? The reason is that RemotingCdm will still
> want to send/receive messages and Start/Stop the remoting session, and it
needs
> a reference to the RemotingController to do that. Also, see comments in
> remoting_controller.cc below.

Not applicable.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
File media/remoting/remoting_cdm_factory.cc (right):

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_cdm_factory.cc:55:
RemotingCdmFactory::CreateRemotingController() {
On 2016/10/08 01:06:16, miu wrote:
> Folks are probably going to suggest this code go into RenderFrameImpl, just
like
> in the last CL.

This is a different case. The RendererFactory is created one for each media
element. But the CdmFactory is one for each render frame. So we need to create
one remoting session for each CDM.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_cdm_factory.cc:70: const SessionMessageCB&
session_message_cb,
On 2016/10/08 01:06:16, miu wrote:
> Not your problem, but this spaghetti mess of callbacks should be cleaned-up at
> some point (i.e., replaced with an interface). Maybe file a TODO bug and
assign
> it to xwhang@ or sandersd@?

Done.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_cdm_factory.cc:75: std::unique_ptr<RemotingController>
remoting_controller =
On 2016/10/08 01:06:16, miu wrote:
> It's okay (and probably better) to create the RemotingController in the ctor.
By
> creating it earlier, it has a chance to receive the OnSinkAvailable()/Gone()
> call earlier, and that means we'll know sooner whether to create a remoting
CDM
> or a local one.

Please see my reply above. This is created one for each CDM.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_cdm_factory.cc:77:
remoting_controller->ShouldCreateRemotingCdm(base::Bind(
On 2016/10/08 01:06:16, miu wrote:
> This will crash: base::Passed(&remoting_controller) will execute and
invalidate
> the std::unique_ptr before it is used as the "this" pointer to the
> ShouldCreateRemotingCdm() method call.

Done.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
File media/remoting/remoting_controller.cc (right):

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_controller.cc:104: if (auto* cdm =
RemotingCdm::From(cdm_context)) {
On 2016/10/08 01:06:16, miu wrote:
> Possible edge-case issue: What if a remoting session was already started?
Should
> we stop it first, before using the delegate's session? Or, if it shouldn't be
> possible, perhaps we need a CHECK() here? It's also possible a CHECK() could
be
> wrong: Maybe instead we should log an error and abort all remoting without
> crashing the process?

This seems not a problem now. If a remoting session was already started, after
this call, it will be stopped and we'll switch back to local renderer.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_controller.cc:267: cdm_check_cb_.Run(false);
On 2016/10/08 01:06:16, miu wrote:
> What if the sink becomes available later? Say, in 500 ms?
> 
> I think there's a simpler solution here: How about you *always* call
> remoter_->Start() and then run the |cdm_check_cb_| in OnStarted() or
> OnStartFailed()? It doesn't matter whether the sink is available or not, the
> browser process will always give you back an appropriate response. :)
> 
>   if (is_remoting_) {
>     // Uh oh! What do we do if we were already remoting?!?!?!?
>     return;
>   }
> 
>   if (cdm_check_cb_.is_null()) {
>     cdm_check_cb_ = cb;
>     remoter_->Start();
>   } else {
>     // Uh oh! Maybe we need to support multiple CDM instances using the
remoting
> session? Would a list of CB's be appropriate?
>   }

Can we just consider simple case? If the sink is not available when CDM is
created, we'll just go with local CDM. And each remoting session is only for one
CDM. For multiple CDM, we'll create multiple remoting sessions.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
File media/remoting/remoting_controller.h (right):

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_controller.h:22: // This class does the following:
On 2016/10/08 01:06:16, miu wrote:
> This comment should be next to the class declaration, not the namespace
> delcaration.

Done.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_controller.h:30: class RemotingController final : public
MediaObserver,
On 2016/10/08 01:06:16, miu wrote:
> I think you should consider splitting this class up: The reason is that the
> control logic for starting/ending a session around a CDM is significantly
> different than that for a WMPI/Renderer, and it seems like we're mixing-up the
> internal state and workflow a bit too much. I'll e-mail you some ideas I have
> around this.

Done.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_controller.h:62: // The callback used to tell which CDM
should be created.
On 2016/10/08 01:06:16, miu wrote:
> This comment isn't really necessary. But, the method needs a comment that's
> similar to this. How about:
> 
>   // Query whether to create a Media Remoting CDM. |cb| could be run
> synchronously or asynchronously, depending on whether the required information
> is available now or later.

Done.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_controller.h:81: return false;
On 2016/10/08 01:06:16, miu wrote:
> If this is the RemotingController owned by the CDM factory, I think you would
> want it to return |is_remoting_failed_| here? If that's right, then the
comment
> is misleading and should probably be deleted.

Not applicable.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_controller.h:102: bool is_remoting_ = false;
On 2016/10/08 01:06:16, miu wrote:
> This is now "sort of" true because it will be false while a remoting session
has
> been started for the CDM. (This is one of the reasons I'm proposing we
split-up
> this class.)

Not applicable.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_controller.h:108: bool is_remoting_cdm_ = false;
On 2016/10/08 01:06:16, miu wrote:
> This isn't necessary, because this can always be determined from other data
> members. You could make an inline helper method for it instead, such as:
> 
>   bool is_remoting() const {
>     if (cdm_remoting_controller_)
>       return cdm_remoting_controller_->is_remoting_;
>     return is_remoting_;
>   }

Not applicable.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_controller.h:110: // Indicates that remoting was stopped
after CDM is created remotely. In this
On 2016/10/08 01:06:16, miu wrote:
> Suggested wording:
> 
> // Indicates that the video stack cannot continue operation. For example, if a
> remoting session involving CDM content was stopped, there is no way to
continue
> playback because the CDM is required but is no longer available.

Add this comment to RemotingSessionState::SESSION_PERMANENTLY_STOPPED.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_controller.h:112: bool is_remoting_failed_ = false;
On 2016/10/08 01:06:16, miu wrote:
> Actually, do you need this bool? Wouldn't it always be equal to:
> 
>   (audio_decoder_config_.is_encrypted() ||
>    video_decoder_config_.is_encrypted()) &&
>   !cdm_remoting_controller_

Not applicable.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_controller.h:112: bool is_remoting_failed_ = false;
On 2016/10/08 01:06:16, miu wrote:
> naming: How about has_terminated_permanently_.

Not applicable.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_controller.h:131: CdmCheckCallback cdm_check_cb_;
On 2016/10/08 01:06:16, miu wrote:
> It seems like this is a "run once" callback. Is that right? If so, you should
> reset this callback (e.g., using base::ResetAndReturn()) each place where you
> Run() it, since it would be catastrophic if it were called twice.

Done.

https://chromiumcodereview.appspot.com/2406483002/diff/20001/media/remoting/r...
media/remoting/remoting_controller.h:137: std::unique_ptr<RemotingController>
cdm_remoting_controller_;
On 2016/10/08 01:06:16, miu wrote:
> It doesn't seem that you should be taking ownership of the other
> RemotingController here. Perhaps let RemotingCDM always own it, and use a
> WeakPtr here? (This is another reason I think we should split-up this class.)

Not applicable.

[miu, 2016-10-25 04:21:27]

Looks great. Please adjust the change description and include other OWNERS as
needed (no need to re-open a separate code review, IMHO).

Comments for PS3 (there are a lot, but they're mostly just little things):

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

https://codereview.chromium.org/2406483002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2694: return
base::MakeUnique<media::RemotingRendererController>(
nit: Looks like you were trying to do this:

  return base::MakeUnique<media::RemotingRendererController(
      make_scoped_refptr(new media::RemotingSourceImpl(
          std::move(remoting_source_request), std::move(remoter))));

https://codereview.chromium.org/2406483002/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2406483002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.h:1073: // remoting from/to local playback.
nit: This line can go at the end of the line above it.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_cdm.cc (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm.cc:28: NOTIMPLEMENTED();
nit: DCHECK(remoting_controller_); so we don't have to worry about whether its
null in the rest of the code.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_cdm.h (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm.h:32: scoped_refptr<RemotingSourceImpl>
GetRemotingSource();
General scoped_refptr usage note: The return type of a direct accessor should be
the raw pointer (RemotingSourceImpl*) instead. This lets the caller decide
whether to increment the ref-count by storing the raw pointer into a
scoped_refptr.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm.h:61: std::unique_ptr<RemotingCdmController>
remoting_controller_;
const

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_cdm_controller.cc (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_controller.cc:16: :
remoting_source_(remoting_source) {
nit: Use std::move() here to avoid an unnecessary increment/dercrement on the
ref-count.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_controller.cc:21:
remoting_source_->RemoveClient(this);
DCHECK(thread_checker_...);

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_controller.cc:27:
base::ResetAndReturn(&cdm_check_cb_).Run(success);
DCHECK(!cdm_check_cb_.is_null()) before this line to make sure OnStarted() is
not called multiple times.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_controller.cc:51: cdm_check_cb_ = cb;
DCHECK(cdm_check_cb_.is_null()) before this line to make sure there are no other
queries in-flight. Or, is it reasonable for there to be? If so, |cdm_check_cb_|
would need to be a vector<CdbCheckCallback>.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_cdm_controller.h (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_controller.h:49: scoped_refptr<RemotingSourceImpl>
remoting_source_;
nit: const (and move up to be the first data member, since this is injected at
construction time).

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_cdm_factory.cc (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_factory.cc:30: new RemotingCdm(key_system,
security_origin, cdm_config, session_message_cb,
It's always a little strange to see a "new Foo()" by itself. Could you write a
comment explaining what takes ownership of RemotingCdm?

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_factory.cc:49: remoter_factory_(remoter_factory) {}
Please DCHECK that |default_cdm_factory_| and |remoter_factory_| are not null.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_factory.cc:65: std::move(remoting_source_impl));
ditto here: Consider using make_scoped_refptr() to simplify a little.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_cdm_factory.h (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_factory.h:17:
RemotingCdmFactory(std::unique_ptr<CdmFactory> default_cdm_factory,
Please add a comment that |remoter_factory| should outlive RemotingCdmFactory.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_factory.h:33: std::unique_ptr<CdmFactory>
default_cdm_factory_;
These should be const.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_renderer_controller.cc (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:21:
remoting_source_->RemoveClient(this);
Above this line: DCHECK(thread_checker_.CalledOnValidThread());

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:30: switch_renderer_cb_.Run();
For safety, you might want to guard this:

  if (!switch_renderer_cb_.is_null())
    switch_renderer_cb_.Run();

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:43: bool can_start_remoting =
can_start_remoting_;
naming nits: The class data member |can_start_remoting_| could mean a lot of
things. How about |session_can_remote_| to be more precise?

Also, consider naming this local variable |session_could_remote| (past tense),
to make the if-statement below more readable. Also, const please. ;)

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:82:
remoting_source_->AddClient(this);
Suggest adding this helpful comment for readability:

  remoting_source_->AddClient(this);  // Calls OnSessionStateChanged().

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:104: return;
I think you should remove the early return here. Let UpdateAndMaybeSwitch()
decide what to do (and maybe it would stop an existing remoting session?)

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:106: if (has_video_) {
Before this line, add:

  is_encrypted_ = false;

(To make sure it will become false if it was true before, but now both tracks
have stopped using encryption.)

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:166: if (has_video_ &&
!IsVideoCodecSupported())
Before this, I think you should add (this will simplify the code in
UpdateAndMaybeSwitch()):

  if (switch_renderer_cb_.is_null())
    return false;
  if (!has_audio_ && !has_video_)
    return false;

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:171: return
remoting_source_->state() == RemotingSessionState::SESSION_STARTED;
Consider adding a comment like: "Explicitly ignoring |is_fullscreen_| since all
EME content should trigger the start of remoting immediately, and not later
after switching into fullscreen. This is required by current technical
limitations."

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:183: return;
If you take my suggestions in ShouldBeRemoting() (above), then you don't need
either of these early-return statements. The code below will always start or
stop remoting, or do nothing, as appropriate.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_renderer_controller.h (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
Since remoting_renderer_controller.h/.cc are modifications of the [now deleted]
remoting_controller.h/.cc, consider uploading your next patch set with a lower
similarity (maybe `git cl upload --similarity=20` would work?) so that these new
files show up as a diff from the old files.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.h:41: using SwitchRendererCallback =
base::Callback<void()>;
This is the same as base::Closure, so consider removing this typedef and just
using base::Closure.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.h:48: // Used to query whether to
create Media Remoting Renderer.
To help those unfamiliar with this code: s/Used to query/Used by
RemotingRendererFactory to query/

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.h:51: // Used to query whether the
remoting session is permanently terminated.
ditto: (Used by RemotingRendererFactory)

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.h:65: // Indicates if this media
element or its ancestor enters full screen.
s/if/whether/

and

s/enters/is in/

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.h:69: bool should_be_remoting_ =
false;
naming: Based on the way this variable is used, it seems like it should be named
|remote_rendering_started_|. (and to avoid confusion between this variable and
the ShouldBeRemoting() helper method)

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.h:86: // This will be replaced by
the remoting source in RemotingCdmController
For clarification: "This is initially the RemotingSourceImpl passed to the ctor,
and might be replaced with a different instance later if OnSetCdm() is called."

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_renderer_controller_unittest.cc (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller_unittest.cc:154:
remoting_source_impl->OnSinkAvailable();
You should add a call to RunUntilIdle() and EXPECT_FALSE(...) after each of the
method calls. While we know RemotingSourceImpl won't post any tasks, the test
shouldn't know this. In other words, this test code should give
RemotingSourceImpl a chance to do the wrong thing. ;)

  remoting_controller_->SetSwitchRendererCallback(base::Bind(
      &RemotingRendererControllerTest::ToggleRenderer, base::Unretained(this)));
  RunUntilIdle();
  EXPECT_FALSE(is_remoting_);
  remoting_source_impl->OnSinkAvailable();
  RunUntilIdle();
  EXPECT_FALSE(is_remoting_);
  remoting_controller_->OnEnteredFullscreen();
  RunUntilIdle();
  EXPECT_FALSE(is_remoting_);
  remoting_controller_->OnMetadataChanged(defaultMetadata());
  RunUntilIdle();
  EXPECT_TRUE(is_remoting_);

...and the same thing in the StartFailed test.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_renderer_factory.cc (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_factory.cc:26:
remoting_controller_->IsTerminated())) {
Seems like this should be:

  if (remoting_controller_ &&
      remoting_controller_->IsRemoting() &&
      !remoting_controller_->IsTerminated()) {

Rather than having two separate IsRemoting() and IsTerminated() methods, you
might just combine them and rename to something like IsRenderingRemotely().

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_source_impl.cc (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:20:
UpdateAndNotifyState(RemotingSessionState::SESSION_PERMANENTLY_STOPPED);
Instead of this, consider calling Shutdown() so that remoter_->Stop() is called
if needed.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:41: if (state_ ==
RemotingSessionState::SESSION_STARTED) {
Or if in the SESSION_STARTING state too, right?

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:42:
remoter_->Stop(mojom::RemotingStopReason::ROUTE_TERMINATED);
Actually, you shouldn't need to call Stop() here. OnSinkGone() implies
OnStopped() will be called soon. You could add a helpful comment here like:
"Remoting is being shut down from the other end."

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:49: if (!is_sink_available_ ||
clients_.empty() ||
The "!is_sink_available_" shouldn't be needed. (There would be a consistency
problem if OnStarted() were called after OnSinkGone().)

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:54:
remoter_->Stop(mojom::RemotingStopReason::ROUTE_TERMINATED);
The reason should be LOCAL_PLAYBACK since either there are no
RemotingRenderers/CDMs or this current session is permanently stopped.
(ROUTE_TERMINATED was meant for shutting down remoting via the Media Router
dialog.)

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:70: state_ = is_sink_available_ ?
RemotingSessionState::SESSION_CAN_START
This should just be "state_ = SESSION_UNAVAILABLE" because is_sink_available_
should always be false at this point. Also, if you look at the possible
RemotingStartFailReasons, you would not want to set "state_ = SESSION_CAN_START"
until OnSinkAvailable() gets called again.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:78: RemotingSessionState state =
is_sink_available_
This should just be "state_ = SESSION_UNAVAILABLE" because there will be a later
call to OnSinkAvailable(), and that's where you can set "state_ =
SESSION_CAN_START".

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:146: if (clients_.empty() && state_ ==
RemotingSessionState::SESSION_STARTED) {
Or if in the SESSION_STARTING state too, right?

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:153: if (state_ ==
RemotingSessionState::SESSION_STARTED)
ditto: We want to call Stop() if SESSION_STARTING.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_source_impl.h (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.h:13: namespace base {
Please remove this forward declaration and instead #include
"base/threading/thread_checker.h" The RemotingSourceImpl class declaration below
is only compiling because one of the #includes above is pulling-in the full
declaration of ThreadChecker for you. ;)

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.h:42: // 3) Any client requests to
permanently terminated the session.
s/terminated/terminate/

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.h:44: class RemotingSourceImpl final
Another helpful thing to add to the class-level comment:

  // This class is ref-counted because, in some cases, an instance will have
shared ownership between RemotingRendererController and RemotingCdmController.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.h:51: class Client {
style nit: Type declarations have to go at the top (just after "public:").

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.h:99: mojom::RemoterPtr remoter_;
const please

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.h:106: bool is_sink_available_ = false;
You can remove this boolean. See comments in .cc file. After making changes in
the .cc file, I think you'll notice that |is_sink_available_| is no longer used.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.h:109: std::set<Client*> clients_;
Use std::vector<Client*> instead (saves memory and CPU runtime overhead because
we expect only one or two clients).

[xjz, 2016-10-26 20:54:07]

Patchset #4 (id:120001) has been deleted

[xjz, 2016-10-26 22:00:28]

Thanks for detailed comments!

Addressed your comments in PS#4. Also added tests on EME case. PTAL.

I don't know how to bring the watch list back for public review in this CL. So I
closed this CL and opened a new CL for public review:
https://codereview.chromium.org/2457563002/.
PS#1 in the new CL is identical to PS#4 here. 
Thanks!

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

https://codereview.chromium.org/2406483002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2694: return
base::MakeUnique<media::RemotingRendererController>(
On 2016/10/25 04:21:25, miu wrote:
> nit: Looks like you were trying to do this:
> 
>   return base::MakeUnique<media::RemotingRendererController(
>       make_scoped_refptr(new media::RemotingSourceImpl(
>           std::move(remoting_source_request), std::move(remoter))));

Done.

https://codereview.chromium.org/2406483002/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2406483002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.h:1073: // remoting from/to local playback.
On 2016/10/25 04:21:25, miu wrote:
> nit: This line can go at the end of the line above it.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_cdm.cc (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm.cc:28: NOTIMPLEMENTED();
On 2016/10/25 04:21:25, miu wrote:
> nit: DCHECK(remoting_controller_); so we don't have to worry about whether its
> null in the rest of the code.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_cdm.h (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm.h:32: scoped_refptr<RemotingSourceImpl>
GetRemotingSource();
On 2016/10/25 04:21:25, miu wrote:
> General scoped_refptr usage note: The return type of a direct accessor should
be
> the raw pointer (RemotingSourceImpl*) instead. This lets the caller decide
> whether to increment the ref-count by storing the raw pointer into a
> scoped_refptr.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm.h:61: std::unique_ptr<RemotingCdmController>
remoting_controller_;
On 2016/10/25 04:21:25, miu wrote:
> const

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_cdm_controller.cc (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_controller.cc:16: :
remoting_source_(remoting_source) {
On 2016/10/25 04:21:25, miu wrote:
> nit: Use std::move() here to avoid an unnecessary increment/dercrement on the
> ref-count.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_controller.cc:21:
remoting_source_->RemoveClient(this);
On 2016/10/25 04:21:25, miu wrote:
> DCHECK(thread_checker_...);

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_controller.cc:27:
base::ResetAndReturn(&cdm_check_cb_).Run(success);
On 2016/10/25 04:21:25, miu wrote:
> DCHECK(!cdm_check_cb_.is_null()) before this line to make sure OnStarted() is
> not called multiple times.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_controller.cc:51: cdm_check_cb_ = cb;
On 2016/10/25 04:21:25, miu wrote:
> DCHECK(cdm_check_cb_.is_null()) before this line to make sure there are no
other
> queries in-flight. Or, is it reasonable for there to be? If so,
|cdm_check_cb_|
> would need to be a vector<CdbCheckCallback>.

Done with DCHECK. There should be no other queries in-flight.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_cdm_controller.h (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_controller.h:49: scoped_refptr<RemotingSourceImpl>
remoting_source_;
On 2016/10/25 04:21:25, miu wrote:
> nit: const (and move up to be the first data member, since this is injected at
> construction time).

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_cdm_factory.cc (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_factory.cc:30: new RemotingCdm(key_system,
security_origin, cdm_config, session_message_cb,
On 2016/10/25 04:21:25, miu wrote:
> It's always a little strange to see a "new Foo()" by itself. Could you write a
> comment explaining what takes ownership of RemotingCdm?

Oh, yes, this is wrong. I made this mistake while cleaning up some debugging
codes. Removed it.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_factory.cc:49: remoter_factory_(remoter_factory) {}
On 2016/10/25 04:21:25, miu wrote:
> Please DCHECK that |default_cdm_factory_| and |remoter_factory_| are not null.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_factory.cc:65: std::move(remoting_source_impl));
On 2016/10/25 04:21:25, miu wrote:
> ditto here: Consider using make_scoped_refptr() to simplify a little.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_cdm_factory.h (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_factory.h:17:
RemotingCdmFactory(std::unique_ptr<CdmFactory> default_cdm_factory,
On 2016/10/25 04:21:26, miu wrote:
> Please add a comment that |remoter_factory| should outlive RemotingCdmFactory.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_cdm_factory.h:33: std::unique_ptr<CdmFactory>
default_cdm_factory_;
On 2016/10/25 04:21:26, miu wrote:
> These should be const.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_renderer_controller.cc (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:21:
remoting_source_->RemoveClient(this);
On 2016/10/25 04:21:26, miu wrote:
> Above this line: DCHECK(thread_checker_.CalledOnValidThread());

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:30: switch_renderer_cb_.Run();
On 2016/10/25 04:21:26, miu wrote:
> For safety, you might want to guard this:
> 
>   if (!switch_renderer_cb_.is_null())
>     switch_renderer_cb_.Run();

This callback should never be null here. Added DCHECK.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:43: bool can_start_remoting =
can_start_remoting_;
On 2016/10/25 04:21:26, miu wrote:
> naming nits: The class data member |can_start_remoting_| could mean a lot of
> things. How about |session_can_remote_| to be more precise?
> 
> Also, consider naming this local variable |session_could_remote| (past tense),
> to make the if-statement below more readable. Also, const please. ;)

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:82:
remoting_source_->AddClient(this);
On 2016/10/25 04:21:26, miu wrote:
> Suggest adding this helpful comment for readability:
> 
>   remoting_source_->AddClient(this);  // Calls OnSessionStateChanged().

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:104: return;
On 2016/10/25 04:21:26, miu wrote:
> I think you should remove the early return here. Let UpdateAndMaybeSwitch()
> decide what to do (and maybe it would stop an existing remoting session?)

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:106: if (has_video_) {
On 2016/10/25 04:21:26, miu wrote:
> Before this line, add:
> 
>   is_encrypted_ = false;
> 
> (To make sure it will become false if it was true before, but now both tracks
> have stopped using encryption.)

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:166: if (has_video_ &&
!IsVideoCodecSupported())
On 2016/10/25 04:21:26, miu wrote:
> Before this, I think you should add (this will simplify the code in
> UpdateAndMaybeSwitch()):
> 
>   if (switch_renderer_cb_.is_null())
>     return false;
>   if (!has_audio_ && !has_video_)
>     return false;

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:171: return
remoting_source_->state() == RemotingSessionState::SESSION_STARTED;
On 2016/10/25 04:21:26, miu wrote:
> Consider adding a comment like: "Explicitly ignoring |is_fullscreen_| since
all
> EME content should trigger the start of remoting immediately, and not later
> after switching into fullscreen. This is required by current technical
> limitations."

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.cc:183: return;
On 2016/10/25 04:21:26, miu wrote:
> If you take my suggestions in ShouldBeRemoting() (above), then you don't need
> either of these early-return statements. The code below will always start or
> stop remoting, or do nothing, as appropriate.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_renderer_controller.h (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2016/10/25 04:21:26, miu wrote:
> Since remoting_renderer_controller.h/.cc are modifications of the [now
deleted]
> remoting_controller.h/.cc, consider uploading your next patch set with a lower
> similarity (maybe `git cl upload --similarity=20` would work?) so that these
new
> files show up as a diff from the old files.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.h:41: using SwitchRendererCallback =
base::Callback<void()>;
On 2016/10/25 04:21:26, miu wrote:
> This is the same as base::Closure, so consider removing this typedef and just
> using base::Closure.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.h:48: // Used to query whether to
create Media Remoting Renderer.
On 2016/10/25 04:21:26, miu wrote:
> To help those unfamiliar with this code: s/Used to query/Used by
> RemotingRendererFactory to query/

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.h:51: // Used to query whether the
remoting session is permanently terminated.
On 2016/10/25 04:21:26, miu wrote:
> ditto: (Used by RemotingRendererFactory)

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.h:65: // Indicates if this media
element or its ancestor enters full screen.
On 2016/10/25 04:21:26, miu wrote:
> s/if/whether/
> 
> and
> 
> s/enters/is in/

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.h:69: bool should_be_remoting_ =
false;
On 2016/10/25 04:21:26, miu wrote:
> naming: Based on the way this variable is used, it seems like it should be
named
> |remote_rendering_started_|. (and to avoid confusion between this variable and
> the ShouldBeRemoting() helper method)

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller.h:86: // This will be replaced by
the remoting source in RemotingCdmController
On 2016/10/25 04:21:26, miu wrote:
> For clarification: "This is initially the RemotingSourceImpl passed to the
ctor,
> and might be replaced with a different instance later if OnSetCdm() is
called."

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_renderer_controller_unittest.cc (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_controller_unittest.cc:154:
remoting_source_impl->OnSinkAvailable();
On 2016/10/25 04:21:26, miu wrote:
> You should add a call to RunUntilIdle() and EXPECT_FALSE(...) after each of
the
> method calls. While we know RemotingSourceImpl won't post any tasks, the test
> shouldn't know this. In other words, this test code should give
> RemotingSourceImpl a chance to do the wrong thing. ;)
> 
>   remoting_controller_->SetSwitchRendererCallback(base::Bind(
>       &RemotingRendererControllerTest::ToggleRenderer,
base::Unretained(this)));
>   RunUntilIdle();
>   EXPECT_FALSE(is_remoting_);
>   remoting_source_impl->OnSinkAvailable();
>   RunUntilIdle();
>   EXPECT_FALSE(is_remoting_);
>   remoting_controller_->OnEnteredFullscreen();
>   RunUntilIdle();
>   EXPECT_FALSE(is_remoting_);
>   remoting_controller_->OnMetadataChanged(defaultMetadata());
>   RunUntilIdle();
>   EXPECT_TRUE(is_remoting_);
> 
> ...and the same thing in the StartFailed test.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_renderer_factory.cc (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_renderer_factory.cc:26:
remoting_controller_->IsTerminated())) {
On 2016/10/25 04:21:26, miu wrote:
> Seems like this should be:
> 
>   if (remoting_controller_ &&
>       remoting_controller_->IsRemoting() &&
>       !remoting_controller_->IsTerminated()) {
> 
> Rather than having two separate IsRemoting() and IsTerminated() methods, you
> might just combine them and rename to something like IsRenderingRemotely().

Renamed IsRemoting() to IsRenderingRemotely(). I originally thought that we
should switch to remoting renderer if remoting session is started or is
permanently terminated. On a second thought, I think for the latter, we may just
create a separate renderer to only render that failure page (with apacible's
change). So now I separated the two cases. WDYT?

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_source_impl.cc (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:20:
UpdateAndNotifyState(RemotingSessionState::SESSION_PERMANENTLY_STOPPED);
On 2016/10/25 04:21:27, miu wrote:
> Instead of this, consider calling Shutdown() so that remoter_->Stop() is
called
> if needed.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:41: if (state_ ==
RemotingSessionState::SESSION_STARTED) {
On 2016/10/25 04:21:27, miu wrote:
> Or if in the SESSION_STARTING state too, right?

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:42:
remoter_->Stop(mojom::RemotingStopReason::ROUTE_TERMINATED);
On 2016/10/25 04:21:27, miu wrote:
> Actually, you shouldn't need to call Stop() here. OnSinkGone() implies
> OnStopped() will be called soon. You could add a helpful comment here like:
> "Remoting is being shut down from the other end."

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:49: if (!is_sink_available_ ||
clients_.empty() ||
On 2016/10/25 04:21:26, miu wrote:
> The "!is_sink_available_" shouldn't be needed. (There would be a consistency
> problem if OnStarted() were called after OnSinkGone().)

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:54:
remoter_->Stop(mojom::RemotingStopReason::ROUTE_TERMINATED);
On 2016/10/25 04:21:27, miu wrote:
> The reason should be LOCAL_PLAYBACK since either there are no
> RemotingRenderers/CDMs or this current session is permanently stopped.
> (ROUTE_TERMINATED was meant for shutting down remoting via the Media Router
> dialog.)

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:70: state_ = is_sink_available_ ?
RemotingSessionState::SESSION_CAN_START
On 2016/10/25 04:21:27, miu wrote:
> This should just be "state_ = SESSION_UNAVAILABLE" because is_sink_available_
> should always be false at this point. Also, if you look at the possible
> RemotingStartFailReasons, you would not want to set "state_ =
SESSION_CAN_START"
> until OnSinkAvailable() gets called again.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:78: RemotingSessionState state =
is_sink_available_
On 2016/10/25 04:21:27, miu wrote:
> This should just be "state_ = SESSION_UNAVAILABLE" because there will be a
later
> call to OnSinkAvailable(), and that's where you can set "state_ =
> SESSION_CAN_START".

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:146: if (clients_.empty() && state_ ==
RemotingSessionState::SESSION_STARTED) {
On 2016/10/25 04:21:27, miu wrote:
> Or if in the SESSION_STARTING state too, right?

My original thought was to call Stop() in OnStarted() when session is in
SESSION_STARTING state and there is no client, or ShutDown() is called, or
StopRemoting() is called, and is in the SESSION_STARTING state. Now changed to
call Stop() immediately when any of these happen. This might be more readable.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.cc:153: if (state_ ==
RemotingSessionState::SESSION_STARTED)
On 2016/10/25 04:21:27, miu wrote:
> ditto: We want to call Stop() if SESSION_STARTING.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
File media/remoting/remoting_source_impl.h (right):

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.h:13: namespace base {
On 2016/10/25 04:21:27, miu wrote:
> Please remove this forward declaration and instead #include
> "base/threading/thread_checker.h" The RemotingSourceImpl class declaration
below
> is only compiling because one of the #includes above is pulling-in the full
> declaration of ThreadChecker for you. ;)

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.h:42: // 3) Any client requests to
permanently terminated the session.
On 2016/10/25 04:21:27, miu wrote:
> s/terminated/terminate/

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.h:44: class RemotingSourceImpl final
On 2016/10/25 04:21:27, miu wrote:
> Another helpful thing to add to the class-level comment:
> 
>   // This class is ref-counted because, in some cases, an instance will have
> shared ownership between RemotingRendererController and RemotingCdmController.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.h:51: class Client {
On 2016/10/25 04:21:27, miu wrote:
> style nit: Type declarations have to go at the top (just after "public:").

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.h:99: mojom::RemoterPtr remoter_;
On 2016/10/25 04:21:27, miu wrote:
> const please

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.h:106: bool is_sink_available_ = false;
On 2016/10/25 04:21:27, miu wrote:
> You can remove this boolean. See comments in .cc file. After making changes in
> the .cc file, I think you'll notice that |is_sink_available_| is no longer
used.

Done.

https://codereview.chromium.org/2406483002/diff/100001/media/remoting/remotin...
media/remoting/remoting_source_impl.h:109: std::set<Client*> clients_;
On 2016/10/25 04:21:27, miu wrote:
> Use std::vector<Client*> instead (saves memory and CPU runtime overhead
because
> we expect only one or two clients).

Done.