Media Remoting Clean-up: Less-redundant naming, style consistency, etc.

Media Remoting Clean-up: Less-redundant naming, style consistency, etc.

Now that most of the implementation is complete, this change revisits
our naming and namespacing strategy. An obvious issue is that all our
code had "remote" or "remoting" in its class/variable names, which is
redundant. Also, some code was put in a media::remoting namespace, while
other code was in the media namespace with "remoting" in the class name.
Finally, some code was in subdirectories, but the code base did not turn
out to be so large to warrant that.

This change places all code in the src/media/remoting dir in the
media::remoting namespace, makes some minor style fixes, and tweaks
comments around touched lines of code. The following classes have been
renamed:

1. RemoteRendererImpl --> CourierRenderer because it is an impl of
media::Renderer that maps all operations and callbacks into two-way RPC
messaging, to give the local media pipeline the illusion of having a
local Renderer, but with rendering actually occurring remotely.

2. RemoteDemuxerStreamAdapter --> DemuxerStreamAdapter: Removed
redundant "Remote" from the name since this class is disambiguated by
being in the media::remoting namespace.

3. RemotingSourceImpl --> SharedSession because it is an impl of
media::mojom::RemotingSource that is shared by multiple "clients."

4. RemotingSinkObsever --> SinkAvailabilityObserver, to be more-specific
about its purpose.

5. RemotingRendererFactory --> AdaptiveRendererFactory because this
RendererFactory "wraps" the normal RendererFactory to create either
local or remoting renderers depending on the current situation.

6. RemotingRendererController --> RendererController: Removed redundant
"Remoting" from the name because this class is disambiguated by being
in the media::remoting namespace.

7. Miscellaneous similar renames in "fake" classes and other testing-
only code.

Note: The RemotingCdm* classes were not changed because there is current
WIP to merge-in more implementation. Renaming there will occur in a
later change.

BUG=643964
Review-Url: https://codereview.chromium.org/2643253003
Cr-Commit-Position: refs/heads/master@{#445880}
Committed: https://chromium.googlesource.com/chromium/src/+/9f7788ef57ae0e0bf11a81f027512b9798983561

[miu, 2017-01-20 23:13:01]

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

[commit-bot: I haz the power, 2017-01-20 23:13:18]

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

[miu, 2017-01-20 23:14:40]

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

[miu, 2017-01-20 23:14:41]

xjz: PTAL. I would suggest just skimming over everything, to spot-check things.
I basically did a rename and a `git cl format`, and moved everything into the
media::remoting namespace. However, there are also the non-renaming changes
noted below:

https://codereview.chromium.org/2643253003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/2643253003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2769:
RenderFrameImpl::CreateRemotingRendererController() {
Note: I just in-lined this where it was used (because there was only one call
point).

https://codereview.chromium.org/2643253003/diff/1/media/remoting/courier_rend...
File media/remoting/courier_renderer.cc (right):

https://codereview.chromium.org/2643253003/diff/1/media/remoting/courier_rend...
media/remoting/courier_renderer.cc:623: PipelineStatistics stats;
Note: This RPC-->Local struct conversion should have been in proto_utils.cc, so
I moved it there (and added a unit test).

https://codereview.chromium.org/2643253003/diff/1/media/remoting/demuxer_stre...
File media/remoting/demuxer_stream_adapter.cc (left):

https://codereview.chromium.org/2643253003/diff/1/media/remoting/demuxer_stre...
media/remoting/demuxer_stream_adapter.cc:283: // TODO(erickung): consider
sending updated Audio/Video decoder config to
Removed this TODO comment because we did this recently (the controller gets this
info from PipelineMetadata updates).

https://codereview.chromium.org/2643253003/diff/1/media/remoting/proto_utils.cc
File media/remoting/proto_utils.cc (left):

https://codereview.chromium.org/2643253003/diff/1/media/remoting/proto_utils....
media/remoting/proto_utils.cc:123: const scoped_refptr<::media::DecoderBuffer>&
decoder_buffer,
Changed 1st arg to be just a plain const-ref.

https://codereview.chromium.org/2643253003/diff/1/media/remoting/proto_utils.cc
File media/remoting/proto_utils.cc (right):

https://codereview.chromium.org/2643253003/diff/1/media/remoting/proto_utils....
media/remoting/proto_utils.cc:355: stats->video_keyframe_distance_average =
base::TimeDelta::Max();
This new "video_keyframe_distance" field was added recently, but not accounted
for in the media/remoting code. My added unit test will catch any future
new/changed fields so this doesn't happen again.

https://codereview.chromium.org/2643253003/diff/1/media/remoting/proto_utils_...
File media/remoting/proto_utils_unittest.cc (right):

https://codereview.chromium.org/2643253003/diff/1/media/remoting/proto_utils_...
media/remoting/proto_utils_unittest.cc:152: // TODO(miu): Tests for all other
conversion functions.
We need to make sure all conversion functions have unit tests. They are so
trivial to write, but very important for making sure our code doesn't break when
new fields are added, or existing fields are changed.

[commit-bot: I haz the power, 2017-01-21 02:50:46]

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

[commit-bot: I haz the power, 2017-01-21 02:50:47]

Dry run: This issue passed the CQ dry run.

[xjz, 2017-01-21 06:12:34]

Looks great!

https://codereview.chromium.org/2643253003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/2643253003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2769:
RenderFrameImpl::CreateRemotingRendererController() {
On 2017/01/20 23:14:41, miu wrote:
> Note: I just in-lined this where it was used (because there was only one call
> point).

Sounds good.

https://codereview.chromium.org/2643253003/diff/1/media/remoting/courier_rend...
File media/remoting/courier_renderer.cc (right):

https://codereview.chromium.org/2643253003/diff/1/media/remoting/courier_rend...
media/remoting/courier_renderer.cc:623: PipelineStatistics stats;
On 2017/01/20 23:14:41, miu wrote:
> Note: This RPC-->Local struct conversion should have been in proto_utils.cc,
so
> I moved it there (and added a unit test).

Acknowledged.

https://codereview.chromium.org/2643253003/diff/1/media/remoting/demuxer_stre...
File media/remoting/demuxer_stream_adapter.cc (left):

https://codereview.chromium.org/2643253003/diff/1/media/remoting/demuxer_stre...
media/remoting/demuxer_stream_adapter.cc:283: // TODO(erickung): consider
sending updated Audio/Video decoder config to
On 2017/01/20 23:14:41, miu wrote:
> Removed this TODO comment because we did this recently (the controller gets
this
> info from PipelineMetadata updates).

The controller now only get the update of natural_size, not other config change.

https://codereview.chromium.org/2643253003/diff/1/media/remoting/proto_utils.cc
File media/remoting/proto_utils.cc (left):

https://codereview.chromium.org/2643253003/diff/1/media/remoting/proto_utils....
media/remoting/proto_utils.cc:123: const scoped_refptr<::media::DecoderBuffer>&
decoder_buffer,
On 2017/01/20 23:14:41, miu wrote:
> Changed 1st arg to be just a plain const-ref.

Acknowledged.

https://codereview.chromium.org/2643253003/diff/1/media/remoting/proto_utils.cc
File media/remoting/proto_utils.cc (right):

https://codereview.chromium.org/2643253003/diff/1/media/remoting/proto_utils....
media/remoting/proto_utils.cc:355: stats->video_keyframe_distance_average =
base::TimeDelta::Max();
On 2017/01/20 23:14:41, miu wrote:
> This new "video_keyframe_distance" field was added recently, but not accounted
> for in the media/remoting code. My added unit test will catch any future
> new/changed fields so this doesn't happen again.

Good!

https://codereview.chromium.org/2643253003/diff/1/media/remoting/proto_utils_...
File media/remoting/proto_utils_unittest.cc (right):

https://codereview.chromium.org/2643253003/diff/1/media/remoting/proto_utils_...
media/remoting/proto_utils_unittest.cc:152: // TODO(miu): Tests for all other
conversion functions.
On 2017/01/20 23:14:41, miu wrote:
> We need to make sure all conversion functions have unit tests. They are so
> trivial to write, but very important for making sure our code doesn't break
when
> new fields are added, or existing fields are changed.

Agree.

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/courier_...
File media/remoting/courier_renderer.h (right):

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/courier_...
media/remoting/courier_renderer.h:44: class CourierRenderer : public Renderer {
naming nit: How about RendererWrapper or ProxyRenderer?

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

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/remoting...
media/remoting/remoting_cdm_context.h:17: // both the CDM and CdmContext
interfaces.
This is required by media owner. The following is xhwang@'s comment on my
original CL: "The purpose that we have CdmContext is to isolate the media player
(e.g. renderer) from accessing
the CDM directly. Therefore, it's more desirable that we cast CdmContext to a
RemotingCdmContext here, and get GetRemotingSource() there, instead of getting
the CDM from the context, which defies the purpose of CdmContext."

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/remoting...
media/remoting/remoting_cdm_context.h:27: RemotingCdm* cdm() const { return
remoting_cdm_; }
This accessor can access the CDM directly. I am OK with this, but it seems
doesn't follow the purpose of CdmContext as commented above. Since the only
thing we want from CDM is the RemotingSource, it sounds good to just have a
GetSession() function. WDYT?

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/shared_s...
File media/remoting/shared_session.h (right):

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/shared_s...
media/remoting/shared_session.h:144: // Handles dispatching of incoming and
outgoing RPC messages.
nit: Should we remove this TODO?

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/situatio...
File media/remoting/situational_renderer_factory.h (right):

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/situatio...
media/remoting/situational_renderer_factory.h:16: class
SituationalRendererFactory : public RendererFactory {
naming nit: How about AdaptiveRendererFactory?

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/user_exp...
File media/remoting/user_experience_controller.h (right):

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/user_exp...
media/remoting/user_experience_controller.h:27: class UserExperienceController
final : public SharedSession::Client,
naming nit: I still prefer RendererController. The main function of this class
is to control the switching of renderer.

[miu, 2017-01-23 20:57:37]

Naming is hard. I've taken awhile to consider your comments, made some changes,
but discussed some of them further:

https://codereview.chromium.org/2643253003/diff/1/media/remoting/demuxer_stre...
File media/remoting/demuxer_stream_adapter.cc (left):

https://codereview.chromium.org/2643253003/diff/1/media/remoting/demuxer_stre...
media/remoting/demuxer_stream_adapter.cc:283: // TODO(erickung): consider
sending updated Audio/Video decoder config to
On 2017/01/21 06:12:33, xjz wrote:
> On 2017/01/20 23:14:41, miu wrote:
> > Removed this TODO comment because we did this recently (the controller gets
> this
> > info from PipelineMetadata updates).
> 
> The controller now only get the update of natural_size, not other config
change.

Ok. I put this TODO back and explained further.

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/courier_...
File media/remoting/courier_renderer.h (right):

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/courier_...
media/remoting/courier_renderer.h:44: class CourierRenderer : public Renderer {
On 2017/01/21 06:12:34, xjz wrote:
> naming nit: How about RendererWrapper or ProxyRenderer? 

RendererWrapper doesn't seem right, since this class isn't wrapping anything
(well, perhaps in a loose sense, it's "wrapping" a renderer impl on another
device).

I had considered ProxyRenderer, but I didn't want that to be confused with the
normal "Proxy/Delegate" pattern and have people think there was another Renderer
impl somewhere in this code that this class "proxies" to.

Ultimately, after consulting a thesaurus, I settled on "courier" because that
means "entity that passes messages back-and-forth," which is exactly what this
impl does.

I'm open to other suggestions, of course. :)

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

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/remoting...
media/remoting/remoting_cdm_context.h:17: // both the CDM and CdmContext
interfaces.
On 2017/01/21 06:12:34, xjz wrote:
> This is required by media owner. The following is xhwang@'s comment on my
> original CL: "The purpose that we have CdmContext is to isolate the media
player
> (e.g. renderer) from accessing
> the CDM directly. Therefore, it's more desirable that we cast CdmContext to a
> RemotingCdmContext here, and get GetRemotingSource() there, instead of getting
> the CDM from the context, which defies the purpose of CdmContext."

Interesting point. I looked into this more, and I think we can still merge the
classes *if* we get rid of the cdm() (or GetRemotingSource()) accessor. Instead
of the accessor, we could provide a new static function in SharedSession:

  // If CdmContext represents a remoting implementation, return a pointer to the
SharedSession it
  // is using. Otherwise, return nullptr.
  SharedSession* GetForCdmContext(CdmContext* context);

Then, in the controller (or renderer), there is no direct pointer to the Cdm.
All they would have is a CdmContext* (and no way to downcast it to the merged
impl class).

I think that would satisfy all requirements, and allow us to get rid of this
extra class that is only owned by RemotingCdm and points back to it.

WDYT? (For a later CL; not this one.)

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/remoting...
media/remoting/remoting_cdm_context.h:27: RemotingCdm* cdm() const { return
remoting_cdm_; }
On 2017/01/21 06:12:34, xjz wrote:
> This accessor can access the CDM directly. I am OK with this, but it seems
> doesn't follow the purpose of CdmContext as commented above. Since the only
> thing we want from CDM is the RemotingSource, it sounds good to just have a
> GetSession() function. WDYT?

For now, I put this back to a SharedSession getter. (See comment above for other
details.)

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/shared_s...
File media/remoting/shared_session.h (right):

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/shared_s...
media/remoting/shared_session.h:144: // Handles dispatching of incoming and
outgoing RPC messages.
On 2017/01/21 06:12:34, xjz wrote:
> nit: Should we remove this TODO?

Done. Yeah, I suppose the receiver code depends on a separate RpcBroker. So
we'll never be able to do this.

(FWIW, we should move the receiver impl into the Chromium tree so we don't have
to structure our code around code in external projects. That's not maintainable
in the long-term.)

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/situatio...
File media/remoting/situational_renderer_factory.h (right):

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/situatio...
media/remoting/situational_renderer_factory.h:16: class
SituationalRendererFactory : public RendererFactory {
On 2017/01/21 06:12:34, xjz wrote:
> naming nit: How about AdaptiveRendererFactory?

Hmm...I had a hard time naming this class. Could you explain why you like
Adaptive better?

Other names I had considered (do you prefer any of these?):

1. ConditionalRendererFactory: because it creates different renderers based on
conditions

2. AdjuvantRendererFactory: because it is an extra actor that provides
supplemental functionality over DefaultRendererFactory

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/user_exp...
File media/remoting/user_experience_controller.h (right):

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/user_exp...
media/remoting/user_experience_controller.h:27: class UserExperienceController
final : public SharedSession::Client,
On 2017/01/21 06:12:34, xjz wrote:
> naming nit: I still prefer RendererController. The main function of this class
> is to control the switching of renderer.

Done. Re-thought this one for awhile, and I agree: My naming was about the goal,
not the targeted action; and this class doesn't control the user experience, but
instead acts to achieve a good one.

[xjz, 2017-01-23 23:08:14]

LGTM % some typos and nits.

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/courier_...
File media/remoting/courier_renderer.h (right):

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/courier_...
media/remoting/courier_renderer.h:44: class CourierRenderer : public Renderer {
On 2017/01/23 20:57:36, miu wrote:
> On 2017/01/21 06:12:34, xjz wrote:
> > naming nit: How about RendererWrapper or ProxyRenderer? 
> 
> RendererWrapper doesn't seem right, since this class isn't wrapping anything
> (well, perhaps in a loose sense, it's "wrapping" a renderer impl on another
> device).
> 
> I had considered ProxyRenderer, but I didn't want that to be confused with the
> normal "Proxy/Delegate" pattern and have people think there was another
Renderer
> impl somewhere in this code that this class "proxies" to.
> 
> Ultimately, after consulting a thesaurus, I settled on "courier" because that
> means "entity that passes messages back-and-forth," which is exactly what this
> impl does.
> 
> I'm open to other suggestions, of course. :)

ProxyRenderer or DelegateRenderer sgtm because this class does act as a
proxy/delegate to the renderer on receiver. But CourierRenderer also makes sense
in that this renderer dispatches contents and RPC messages to/from receiver.  I
am not good at naming, so it's up to you whether to change it or not. :)

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

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/remoting...
media/remoting/remoting_cdm_context.h:17: // both the CDM and CdmContext
interfaces.
On 2017/01/23 20:57:36, miu wrote:
> On 2017/01/21 06:12:34, xjz wrote:
> > This is required by media owner. The following is xhwang@'s comment on my
> > original CL: "The purpose that we have CdmContext is to isolate the media
> player
> > (e.g. renderer) from accessing
> > the CDM directly. Therefore, it's more desirable that we cast CdmContext to
a
> > RemotingCdmContext here, and get GetRemotingSource() there, instead of
getting
> > the CDM from the context, which defies the purpose of CdmContext."
> 
> Interesting point. I looked into this more, and I think we can still merge the
> classes *if* we get rid of the cdm() (or GetRemotingSource()) accessor.
Instead
> of the accessor, we could provide a new static function in SharedSession:
> 
>   // If CdmContext represents a remoting implementation, return a pointer to
the
> SharedSession it
>   // is using. Otherwise, return nullptr.
>   SharedSession* GetForCdmContext(CdmContext* context);
> 
> Then, in the controller (or renderer), there is no direct pointer to the Cdm.
> All they would have is a CdmContext* (and no way to downcast it to the merged
> impl class).
> 
> I think that would satisfy all requirements, and allow us to get rid of this
> extra class that is only owned by RemotingCdm and points back to it.
> 
> WDYT? (For a later CL; not this one.)

sgtm.

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/situatio...
File media/remoting/situational_renderer_factory.h (right):

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/situatio...
media/remoting/situational_renderer_factory.h:16: class
SituationalRendererFactory : public RendererFactory {
On 2017/01/23 20:57:37, miu wrote:
> On 2017/01/21 06:12:34, xjz wrote:
> > naming nit: How about AdaptiveRendererFactory?
> 
> Hmm...I had a hard time naming this class. Could you explain why you like
> Adaptive better?
> 
> Other names I had considered (do you prefer any of these?):
> 
> 1. ConditionalRendererFactory: because it creates different renderers based on
> conditions
> 
> 2. AdjuvantRendererFactory: because it is an extra actor that provides
> supplemental functionality over DefaultRendererFactory

I hesitated between AdaptiveRendererFactory and ConditionalRendererFactory, and
chose AdaptiveRendererFactory because it adaptively creates different renderers
when condition changes. Conditional/SitualtionalRenderer sounds a little like it
only creates renderer on a kind of condition/situation. But still, I am not good
at naming, so leave this to you to decide whether change it or not. :)

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

https://codereview.chromium.org/2643253003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:276: #include
"media/remoting/user_experience_controller.h"    // nogncheck
Do you mean renderer_controller.h now?

https://codereview.chromium.org/2643253003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:2819: using RemotingController =
media::remoting::UserExperienceController;
s/UserExperienceController/RendererController

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/courier_...
File media/remoting/courier_renderer.h (right):

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/courier_...
media/remoting/courier_renderer.h:41: // While the media content is rendered
remotely, the CourieRenderer emits
s/CourieRenderer/CourierRenderer

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/courier_...
media/remoting/courier_renderer.h:79: base::WeakPtr<CourierRenderer>
remote_renderer_impl,
nit: s/remote_renderer_impl/renderer or self as used in above methods.

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/courier_...
File media/remoting/courier_renderer_unittest.cc (right):

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/courier_...
media/remoting/courier_renderer_unittest.cc:280:
std::unique_ptr<CourierRenderer> remote_renderer_impl_;
naming nit: courier_renderer_ or maybe just renderer_?

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/renderer...
File media/remoting/renderer_controller.h (right):

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/renderer...
media/remoting/renderer_controller.h:5: #ifndef
MEDIA_REMOTING_USER_EXPERIENCE_CONTROLLER_H_
s/USER_EXPERIENCE/RENDERER, and in the below codes.

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/renderer...
media/remoting/renderer_controller.h:33: // SharedSession::Client implemenation.
s/implemenation/implementation.

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/renderer...
media/remoting/renderer_controller.h:219: #endif  //
MEDIA_REMOTING_USER_EXPERIENCE_CONTROLLER_H_
ditto: s/USER_EXPERIENCE/RENDERER

[miu, 2017-01-24 00:18:48]

Description was changed from

==========
Media Remoting Clean-up: Less-reduntant naming, style consistency, etc.

Now that most of the implementation is complete, this change revisits
our naming and namespacing strategy. An obvious issue is that all our
code had "remote" or "remoting" in its class/variable names, which is
redundant. Also, some code was put in a media::remoting namespace, while
other code was in the media namespace with "remoting" in the class name.
Finally, some code was in subdirectories, but the code base did not turn
out to be so large to warrant that.

This change places all code in the src/media/remoting dir in the
media::remoting namespace, makes some minor style fixes, and tweaks
comments around touched lines of code. The following classes have been
renamed:

1. RemoteRendererImpl --> CourierRenderer because it is an impl of
media::Renderer that maps all operations and callbacks into two-way RPC
messaging, to give the local media pipeline the illusion of having a
local Renderer, but with rendering actually occurring remotely.

2. RemoteDemuxerStreamAdapter --> DemuxerStreamAdapter: Removed
redundant "Remote" from the name since this class is disambiguated by
being in the media::remoting namespace.

3. RemotingSourceImpl --> SharedSession because it is an impl of
media::mojom::RemotingSource that is shared by multiple "clients."

4. RemotingSinkObsever --> SinkAvailabilityObserver, to be more-specific
about its purpose.

5. RemotingRendererFactory --> SituationalRendererFactory because this
RendererFactory "wraps" the normal RendererFactory to create either
local or remoting renderers depending on the current situation.

6. RemotingRendererController --> UserExperienceController because its
focus is on whether/when to activate remoting, given specific user
input/interaction signals; and it controls the UI shown to the user; and
it works with signals to/from the blink HTMLMediaElement implementation.

7. Miscellaneous similar renames in "fake" classes and other testing-
only code.

Note: The RemotingCdm* classes were not changed because there is current
WIP to merge-in more implementation. Renaming there will occur in a
later change.

BUG=643964
==========

to

==========
Media Remoting Clean-up: Less-reduntant naming, style consistency, etc.

Now that most of the implementation is complete, this change revisits
our naming and namespacing strategy. An obvious issue is that all our
code had "remote" or "remoting" in its class/variable names, which is
redundant. Also, some code was put in a media::remoting namespace, while
other code was in the media namespace with "remoting" in the class name.
Finally, some code was in subdirectories, but the code base did not turn
out to be so large to warrant that.

This change places all code in the src/media/remoting dir in the
media::remoting namespace, makes some minor style fixes, and tweaks
comments around touched lines of code. The following classes have been
renamed:

1. RemoteRendererImpl --> CourierRenderer because it is an impl of
media::Renderer that maps all operations and callbacks into two-way RPC
messaging, to give the local media pipeline the illusion of having a
local Renderer, but with rendering actually occurring remotely.

2. RemoteDemuxerStreamAdapter --> DemuxerStreamAdapter: Removed
redundant "Remote" from the name since this class is disambiguated by
being in the media::remoting namespace.

3. RemotingSourceImpl --> SharedSession because it is an impl of
media::mojom::RemotingSource that is shared by multiple "clients."

4. RemotingSinkObsever --> SinkAvailabilityObserver, to be more-specific
about its purpose.

5. RemotingRendererFactory --> AdaptiveRendererFactory because this
RendererFactory "wraps" the normal RendererFactory to create either
local or remoting renderers depending on the current situation.

6. RemotingRendererController --> RendererController: Removed redundant
"Remoting" from the name because this class is disambiguated by being
in the media::remoting namespace.

7. Miscellaneous similar renames in "fake" classes and other testing-
only code.

Note: The RemotingCdm* classes were not changed because there is current
WIP to merge-in more implementation. Renaming there will occur in a
later change.

BUG=643964
==========

[miu, 2017-01-24 00:19:40]

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/courier_...
File media/remoting/courier_renderer.h (right):

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/courier_...
media/remoting/courier_renderer.h:44: class CourierRenderer : public Renderer {
On 2017/01/23 23:08:13, xjz wrote:
> On 2017/01/23 20:57:36, miu wrote:
> > On 2017/01/21 06:12:34, xjz wrote:
> > > naming nit: How about RendererWrapper or ProxyRenderer? 
> > 
> > RendererWrapper doesn't seem right, since this class isn't wrapping anything
> > (well, perhaps in a loose sense, it's "wrapping" a renderer impl on another
> > device).
> > 
> > I had considered ProxyRenderer, but I didn't want that to be confused with
the
> > normal "Proxy/Delegate" pattern and have people think there was another
> Renderer
> > impl somewhere in this code that this class "proxies" to.
> > 
> > Ultimately, after consulting a thesaurus, I settled on "courier" because
that
> > means "entity that passes messages back-and-forth," which is exactly what
this
> > impl does.
> > 
> > I'm open to other suggestions, of course. :)
> 
> ProxyRenderer or DelegateRenderer sgtm because this class does act as a
> proxy/delegate to the renderer on receiver. But CourierRenderer also makes
sense
> in that this renderer dispatches contents and RPC messages to/from receiver. 
I
> am not good at naming, so it's up to you whether to change it or not. :)

Agreed that ProxyRenderer would also work. The reason I like CourierRenderer is
because it's even more specific (and for prior reasons stated).

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/situatio...
File media/remoting/situational_renderer_factory.h (right):

https://codereview.chromium.org/2643253003/diff/20001/media/remoting/situatio...
media/remoting/situational_renderer_factory.h:16: class
SituationalRendererFactory : public RendererFactory {
On 2017/01/23 23:08:13, xjz wrote:
> On 2017/01/23 20:57:37, miu wrote:
> > On 2017/01/21 06:12:34, xjz wrote:
> > > naming nit: How about AdaptiveRendererFactory?
> > 
> > Hmm...I had a hard time naming this class. Could you explain why you like
> > Adaptive better?
> > 
> > Other names I had considered (do you prefer any of these?):
> > 
> > 1. ConditionalRendererFactory: because it creates different renderers based
on
> > conditions
> > 
> > 2. AdjuvantRendererFactory: because it is an extra actor that provides
> > supplemental functionality over DefaultRendererFactory
> 
> I hesitated between AdaptiveRendererFactory and ConditionalRendererFactory,
and
> chose AdaptiveRendererFactory because it adaptively creates different
renderers
> when condition changes. Conditional/SitualtionalRenderer sounds a little like
it
> only creates renderer on a kind of condition/situation. But still, I am not
good
> at naming, so leave this to you to decide whether change it or not. :)  

Done. I suppose since the class owns the controller (which makes the decision),
you can extend that to mean the factory adapts.

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

https://codereview.chromium.org/2643253003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:276: #include
"media/remoting/user_experience_controller.h"    // nogncheck
On 2017/01/23 23:08:13, xjz wrote:
> Do you mean renderer_controller.h now?

Done. Whoops! Forgot to update this file.

https://codereview.chromium.org/2643253003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:2819: using RemotingController =
media::remoting::UserExperienceController;
On 2017/01/23 23:08:13, xjz wrote:
> s/UserExperienceController/RendererController

Done.

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/courier_...
File media/remoting/courier_renderer.h (right):

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/courier_...
media/remoting/courier_renderer.h:41: // While the media content is rendered
remotely, the CourieRenderer emits
On 2017/01/23 23:08:13, xjz wrote:
> s/CourieRenderer/CourierRenderer

Done.

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/courier_...
media/remoting/courier_renderer.h:79: base::WeakPtr<CourierRenderer>
remote_renderer_impl,
On 2017/01/23 23:08:13, xjz wrote:
> nit: s/remote_renderer_impl/renderer or self as used in above methods.

Done. And, did a bunch of other search-and-replace for this in the unittest.cc
file too.

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/courier_...
File media/remoting/courier_renderer_unittest.cc (right):

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/courier_...
media/remoting/courier_renderer_unittest.cc:280:
std::unique_ptr<CourierRenderer> remote_renderer_impl_;
On 2017/01/23 23:08:14, xjz wrote:
> naming nit: courier_renderer_ or maybe just renderer_?

Done.

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/renderer...
File media/remoting/renderer_controller.h (right):

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/renderer...
media/remoting/renderer_controller.h:5: #ifndef
MEDIA_REMOTING_USER_EXPERIENCE_CONTROLLER_H_
On 2017/01/23 23:08:14, xjz wrote:
> s/USER_EXPERIENCE/RENDERER, and in the below codes.

Done.

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/renderer...
media/remoting/renderer_controller.h:33: // SharedSession::Client implemenation.
On 2017/01/23 23:08:14, xjz wrote:
> s/implemenation/implementation.

Done.

https://codereview.chromium.org/2643253003/diff/40001/media/remoting/renderer...
media/remoting/renderer_controller.h:219: #endif  //
MEDIA_REMOTING_USER_EXPERIENCE_CONTROLLER_H_
On 2017/01/23 23:08:14, xjz wrote:
> ditto: s/USER_EXPERIENCE/RENDERER

Done.

[miu, 2017-01-24 00:23:50]

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

[miu, 2017-01-24 00:23:51]

nick: PTAL (OWNERS) at changes to content/renderer/render_frame_impl.cc. These
are all due to class renames in media/remoting. I also got rid of the
called-from-one-place CreateRemotingRendererController() method.

[miu, 2017-01-24 00:23:57]

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

[commit-bot: I haz the power, 2017-01-24 00:25: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, 2017-01-24 00:28:22]

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

[commit-bot: I haz the power, 2017-01-24 00:28:23]

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

[ncarter (slow), 2017-01-24 18:12:06]

Description was changed from

==========
Media Remoting Clean-up: Less-reduntant naming, style consistency, etc.

Now that most of the implementation is complete, this change revisits
our naming and namespacing strategy. An obvious issue is that all our
code had "remote" or "remoting" in its class/variable names, which is
redundant. Also, some code was put in a media::remoting namespace, while
other code was in the media namespace with "remoting" in the class name.
Finally, some code was in subdirectories, but the code base did not turn
out to be so large to warrant that.

This change places all code in the src/media/remoting dir in the
media::remoting namespace, makes some minor style fixes, and tweaks
comments around touched lines of code. The following classes have been
renamed:

1. RemoteRendererImpl --> CourierRenderer because it is an impl of
media::Renderer that maps all operations and callbacks into two-way RPC
messaging, to give the local media pipeline the illusion of having a
local Renderer, but with rendering actually occurring remotely.

2. RemoteDemuxerStreamAdapter --> DemuxerStreamAdapter: Removed
redundant "Remote" from the name since this class is disambiguated by
being in the media::remoting namespace.

3. RemotingSourceImpl --> SharedSession because it is an impl of
media::mojom::RemotingSource that is shared by multiple "clients."

4. RemotingSinkObsever --> SinkAvailabilityObserver, to be more-specific
about its purpose.

5. RemotingRendererFactory --> AdaptiveRendererFactory because this
RendererFactory "wraps" the normal RendererFactory to create either
local or remoting renderers depending on the current situation.

6. RemotingRendererController --> RendererController: Removed redundant
"Remoting" from the name because this class is disambiguated by being
in the media::remoting namespace.

7. Miscellaneous similar renames in "fake" classes and other testing-
only code.

Note: The RemotingCdm* classes were not changed because there is current
WIP to merge-in more implementation. Renaming there will occur in a
later change.

BUG=643964
==========

to

==========
Media Remoting Clean-up: Less-redundant naming, style consistency, etc.

Now that most of the implementation is complete, this change revisits
our naming and namespacing strategy. An obvious issue is that all our
code had "remote" or "remoting" in its class/variable names, which is
redundant. Also, some code was put in a media::remoting namespace, while
other code was in the media namespace with "remoting" in the class name.
Finally, some code was in subdirectories, but the code base did not turn
out to be so large to warrant that.

This change places all code in the src/media/remoting dir in the
media::remoting namespace, makes some minor style fixes, and tweaks
comments around touched lines of code. The following classes have been
renamed:

1. RemoteRendererImpl --> CourierRenderer because it is an impl of
media::Renderer that maps all operations and callbacks into two-way RPC
messaging, to give the local media pipeline the illusion of having a
local Renderer, but with rendering actually occurring remotely.

2. RemoteDemuxerStreamAdapter --> DemuxerStreamAdapter: Removed
redundant "Remote" from the name since this class is disambiguated by
being in the media::remoting namespace.

3. RemotingSourceImpl --> SharedSession because it is an impl of
media::mojom::RemotingSource that is shared by multiple "clients."

4. RemotingSinkObsever --> SinkAvailabilityObserver, to be more-specific
about its purpose.

5. RemotingRendererFactory --> AdaptiveRendererFactory because this
RendererFactory "wraps" the normal RendererFactory to create either
local or remoting renderers depending on the current situation.

6. RemotingRendererController --> RendererController: Removed redundant
"Remoting" from the name because this class is disambiguated by being
in the media::remoting namespace.

7. Miscellaneous similar renames in "fake" classes and other testing-
only code.

Note: The RemotingCdm* classes were not changed because there is current
WIP to merge-in more implementation. Renaming there will occur in a
later change.

BUG=643964
==========

[ncarter (slow), 2017-01-24 18:12:20]

content lgtm

[miu, 2017-01-24 20:55:47]

The CQ bit was checked by miu@chromium.org

[miu, 2017-01-24 20:55:48]

The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org,
nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2643253003/#ps80001
(title: "REBASE")

[commit-bot: I haz the power, 2017-01-24 20:56:09]

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

[commit-bot: I haz the power, 2017-01-24 21:33:40]

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

[commit-bot: I haz the power, 2017-01-24 21:33:41]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[miu, 2017-01-24 23:36:51]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2017-01-24 23:37:29]

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

[commit-bot: I haz the power, 2017-01-24 23:45:34]

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

[commit-bot: I haz the power, 2017-01-24 23:45:35]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[miu, 2017-01-24 23:55:09]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2017-01-24 23:55:50]

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

[commit-bot: I haz the power, 2017-01-25 00:46:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485302109435200,
"parent_rev": "94a7540e73efd7ba5e99436188c0d00562657b5c", "commit_rev":
"9f7788ef57ae0e0bf11a81f027512b9798983561"}

[commit-bot: I haz the power, 2017-01-25 00:46:54]

Description was changed from

==========
Media Remoting Clean-up: Less-redundant naming, style consistency, etc.

Now that most of the implementation is complete, this change revisits
our naming and namespacing strategy. An obvious issue is that all our
code had "remote" or "remoting" in its class/variable names, which is
redundant. Also, some code was put in a media::remoting namespace, while
other code was in the media namespace with "remoting" in the class name.
Finally, some code was in subdirectories, but the code base did not turn
out to be so large to warrant that.

This change places all code in the src/media/remoting dir in the
media::remoting namespace, makes some minor style fixes, and tweaks
comments around touched lines of code. The following classes have been
renamed:

1. RemoteRendererImpl --> CourierRenderer because it is an impl of
media::Renderer that maps all operations and callbacks into two-way RPC
messaging, to give the local media pipeline the illusion of having a
local Renderer, but with rendering actually occurring remotely.

2. RemoteDemuxerStreamAdapter --> DemuxerStreamAdapter: Removed
redundant "Remote" from the name since this class is disambiguated by
being in the media::remoting namespace.

3. RemotingSourceImpl --> SharedSession because it is an impl of
media::mojom::RemotingSource that is shared by multiple "clients."

4. RemotingSinkObsever --> SinkAvailabilityObserver, to be more-specific
about its purpose.

5. RemotingRendererFactory --> AdaptiveRendererFactory because this
RendererFactory "wraps" the normal RendererFactory to create either
local or remoting renderers depending on the current situation.

6. RemotingRendererController --> RendererController: Removed redundant
"Remoting" from the name because this class is disambiguated by being
in the media::remoting namespace.

7. Miscellaneous similar renames in "fake" classes and other testing-
only code.

Note: The RemotingCdm* classes were not changed because there is current
WIP to merge-in more implementation. Renaming there will occur in a
later change.

BUG=643964
==========

to

==========
Media Remoting Clean-up: Less-redundant naming, style consistency, etc.

Now that most of the implementation is complete, this change revisits
our naming and namespacing strategy. An obvious issue is that all our
code had "remote" or "remoting" in its class/variable names, which is
redundant. Also, some code was put in a media::remoting namespace, while
other code was in the media namespace with "remoting" in the class name.
Finally, some code was in subdirectories, but the code base did not turn
out to be so large to warrant that.

This change places all code in the src/media/remoting dir in the
media::remoting namespace, makes some minor style fixes, and tweaks
comments around touched lines of code. The following classes have been
renamed:

1. RemoteRendererImpl --> CourierRenderer because it is an impl of
media::Renderer that maps all operations and callbacks into two-way RPC
messaging, to give the local media pipeline the illusion of having a
local Renderer, but with rendering actually occurring remotely.

2. RemoteDemuxerStreamAdapter --> DemuxerStreamAdapter: Removed
redundant "Remote" from the name since this class is disambiguated by
being in the media::remoting namespace.

3. RemotingSourceImpl --> SharedSession because it is an impl of
media::mojom::RemotingSource that is shared by multiple "clients."

4. RemotingSinkObsever --> SinkAvailabilityObserver, to be more-specific
about its purpose.

5. RemotingRendererFactory --> AdaptiveRendererFactory because this
RendererFactory "wraps" the normal RendererFactory to create either
local or remoting renderers depending on the current situation.

6. RemotingRendererController --> RendererController: Removed redundant
"Remoting" from the name because this class is disambiguated by being
in the media::remoting namespace.

7. Miscellaneous similar renames in "fake" classes and other testing-
only code.

Note: The RemotingCdm* classes were not changed because there is current
WIP to merge-in more implementation. Renaming there will occur in a
later change.

BUG=643964

Review-Url: https://codereview.chromium.org/2643253003
Cr-Commit-Position: refs/heads/master@{#445880}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f7788ef57ae0e0bf11a81f02751...
==========

[commit-bot: I haz the power, 2017-01-25 00:46:58]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9f7788ef57ae0e0bf11a81f02751...