Rename AdaptiveRendererFactory

Rename AdaptiveRendererFactory


The introduction of the RendererFactorySelector allows us to choose
between different types of renderers after WMPI has been created.

The AdaptiveRendererFactory currently takes a RendererFactory* as a
construction parameter. When creating a Renderer, it chooses between
using the passed factory, or creating a CourierRenderer, for remoting.

This CL moves the decision logic from the AdaptiveRendererFactory to
the RendererFactorySelector, and renames the ARF to the
CourierRendererFactory.


BUG=716154
TEST= added UTs

Review-Url: https://codereview.chromium.org/2826883002
Cr-Commit-Position: refs/heads/master@{#469585}
Committed: https://chromium.googlesource.com/chromium/src/+/16d9d7d9ca1db157277bb1170eda38fac52eed7c

[tguilbert, 2017-04-19 03:07:51]

Patchset #2 (id:20001) has been deleted

[tguilbert, 2017-04-27 20:30:37]

Description was changed from

==========
Rename AdaptiveRendererFactory

//TODO(tguilbert)

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

to

==========
Rename AdaptiveRendererFactory


The introduction of the RendererFactorySelector allows us to choose
between different types of renderers after WMPI has been created.

The AdaptiveRendererFactory currently takes a RendererFactory* as a
construction parameter. When creating a Renderer, it chooses between
using the passed factory, or creating a CourierRenderer, for remoting.

This CL moves the decision logic from the AdaptiveRendererFactory to
the RendererFactorySelector, and renames the ARF to the
CourierRendererFactory.


BUG=716154
TEST= added UTs
==========

[tguilbert, 2017-04-27 20:44:43]

Patchset #5 (id:100001) has been deleted

[tguilbert, 2017-04-27 20:52:16]

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

[tguilbert, 2017-04-27 20:52:16]

PTAL!

miu@ do you have a suggested set of tests to run as a basic sanity test?

Thank you!
Thomas

[tguilbert, 2017-05-01 18:14:56]

On 2017/04/27 20:52:16, tguilbert wrote:
> PTAL!
> 
> miu@ do you have a suggested set of tests to run as a basic sanity test?
> 
> Thank you!
> Thomas

Ping :)

[miu, 2017-05-02 22:40:04]

Apologies for the delay.

Regarding your testing question: We have browser-level tests in the works, but
otherwise there are no unit tests to test libcontent integration with the
RendererFactorySelector and multiple RendererFactories. Media remoting itself
has thorough unit testing in media_unittests and is also covered in
pipeline_integration_tests. So, for this change, just confirming that
MediaRemoting activates/deactivates, with correct local video rendering would be
sufficient.

Comments on PS5:

https://codereview.chromium.org/2826883002/diff/120001/media/base/renderer_fa...
File media/base/renderer_factory_selector.h (right):

https://codereview.chromium.org/2826883002/diff/120001/media/base/renderer_fa...
media/base/renderer_factory_selector.h:61: UseCourierCB use_courier_cb_;
nit: When I see this used in the code, it feels like this member is a bool, but
we know it's not. Up to you, but consider changing the name. I myself can't come
up with any great ones though. Maybe: query_to_use_courier_cb_ or
remoting_is_active_cb_ or some such?

https://codereview.chromium.org/2826883002/diff/120001/media/base/renderer_fa...
media/base/renderer_factory_selector.h:63: bool current_factory_needs_update_ =
true;
Can we simplify by getting rid of |current_factory_| and
|current_factory_needs_update_|? Meaning, just have GetCurrentFactory()
determine which pointer to return each time it's called. Unless I'm mistaken,
GetCurrentFactory() isn't being called so frequently that we need this pointer
caching mechanism.

https://codereview.chromium.org/2826883002/diff/120001/media/remoting/courier...
File media/remoting/courier_renderer_factory.cc (right):

https://codereview.chromium.org/2826883002/diff/120001/media/remoting/courier...
media/remoting/courier_renderer_factory.cc:27: VLOG(1) << "Create Remoting
renderer.";
nit: We could get rid of this VLOG now. Or, if you want to keep it for debugging
purposes, it should probably be in the selector class now (with VLOGging to
indicate when other renderer types are being created).

https://codereview.chromium.org/2826883002/diff/120001/media/remoting/courier...
File media/remoting/courier_renderer_factory.h (right):

https://codereview.chromium.org/2826883002/diff/120001/media/remoting/courier...
media/remoting/courier_renderer_factory.h:17:
CourierRendererFactory(std::unique_ptr<RendererController> controller);
Please add 'explicit' keyword for 1-arg ctor.

https://codereview.chromium.org/2826883002/diff/120001/media/remoting/courier...
media/remoting/courier_renderer_factory.h:27: // Indicates whether or not we
should emit a CourierRenderer, based off of
nit: For those unfamiliar with the larger system, can we word this comment as
something like, "Called by RendererFactorySelector to determine when to create
CourierRenderers. CourierRenderers are created when activating media remoting."

[tguilbert, 2017-05-03 01:32:05]

Patchset #6 (id:140001) has been deleted

[tguilbert, 2017-05-03 01:32:28]

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

[commit-bot: I haz the power, 2017-05-03 01:33:30]

Dry run: CQ is trying da patch.

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

[tguilbert, 2017-05-03 01:34:25]

Addressed comments. Thanks!

https://codereview.chromium.org/2826883002/diff/120001/media/base/renderer_fa...
File media/base/renderer_factory_selector.h (right):

https://codereview.chromium.org/2826883002/diff/120001/media/base/renderer_fa...
media/base/renderer_factory_selector.h:61: UseCourierCB use_courier_cb_;
On 2017/05/02 22:40:04, miu wrote:
> nit: When I see this used in the code, it feels like this member is a bool,
but
> we know it's not. Up to you, but consider changing the name. I myself can't
come
> up with any great ones though. Maybe: query_to_use_courier_cb_ or
> remoting_is_active_cb_ or some such?

I am using QueryIsRemotingActiveCB for now. I also considered
QueryHasRemotingStartedCB. LMK if you have any other ideas/preferences.

https://codereview.chromium.org/2826883002/diff/120001/media/base/renderer_fa...
media/base/renderer_factory_selector.h:63: bool current_factory_needs_update_ =
true;
On 2017/05/02 22:40:04, miu wrote:
> Can we simplify by getting rid of |current_factory_| and
> |current_factory_needs_update_|? Meaning, just have GetCurrentFactory()
> determine which pointer to return each time it's called. Unless I'm mistaken,
> GetCurrentFactory() isn't being called so frequently that we need this pointer
> caching mechanism.

You are right, this was probably excessive in the first place. Done.

https://codereview.chromium.org/2826883002/diff/120001/media/remoting/courier...
File media/remoting/courier_renderer_factory.cc (right):

https://codereview.chromium.org/2826883002/diff/120001/media/remoting/courier...
media/remoting/courier_renderer_factory.cc:27: VLOG(1) << "Create Remoting
renderer.";
On 2017/05/02 22:40:04, miu wrote:
> nit: We could get rid of this VLOG now. Or, if you want to keep it for
debugging
> purposes, it should probably be in the selector class now (with VLOGging to
> indicate when other renderer types are being created).

Done.

https://codereview.chromium.org/2826883002/diff/120001/media/remoting/courier...
File media/remoting/courier_renderer_factory.h (right):

https://codereview.chromium.org/2826883002/diff/120001/media/remoting/courier...
media/remoting/courier_renderer_factory.h:17:
CourierRendererFactory(std::unique_ptr<RendererController> controller);
On 2017/05/02 22:40:04, miu wrote:
> Please add 'explicit' keyword for 1-arg ctor.

Done.

https://codereview.chromium.org/2826883002/diff/120001/media/remoting/courier...
media/remoting/courier_renderer_factory.h:27: // Indicates whether or not we
should emit a CourierRenderer, based off of
On 2017/05/02 22:40:04, miu wrote:
> nit: For those unfamiliar with the larger system, can we word this comment as
> something like, "Called by RendererFactorySelector to determine when to create
> CourierRenderers. CourierRenderers are created when activating media
remoting."

I changed the method name to match the name of the CB in the RFS. I can also
change it to "HasRemotingStarted" if there is a difference between the "Active"
and "Started" remoting state, or if a distinction might appear in the future.

[commit-bot: I haz the power, 2017-05-03 03:54:42]

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

[commit-bot: I haz the power, 2017-05-03 03:54:43]

Dry run: This issue passed the CQ dry run.

[miu, 2017-05-03 21:40:35]

PS6 lgtm. Thanks! :)

[tguilbert, 2017-05-03 21:49:41]

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

[tguilbert, 2017-05-03 21:49:42]

Thank you for the feedback :)

nick@, can you OWNERS review RenderFrameImpl?

Thanks!

[ncarter (slow), 2017-05-03 23:19:15]

On 2017/05/03 21:49:42, tguilbert wrote:
> Thank you for the feedback :)
> 
> nick@, can you OWNERS review RenderFrameImpl?
> 
> Thanks!

lgtm

[tguilbert, 2017-05-05 01:27:58]

The CQ bit was checked by tguilbert@chromium.org

[commit-bot: I haz the power, 2017-05-05 01:28:21]

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-05-05 03:24:48]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493947678076070,
"parent_rev": "9b503962a017a8d69adfdae9300684c299922640", "commit_rev":
"16d9d7d9ca1db157277bb1170eda38fac52eed7c"}

[commit-bot: I haz the power, 2017-05-05 03:25:27]

Description was changed from

==========
Rename AdaptiveRendererFactory


The introduction of the RendererFactorySelector allows us to choose
between different types of renderers after WMPI has been created.

The AdaptiveRendererFactory currently takes a RendererFactory* as a
construction parameter. When creating a Renderer, it chooses between
using the passed factory, or creating a CourierRenderer, for remoting.

This CL moves the decision logic from the AdaptiveRendererFactory to
the RendererFactorySelector, and renames the ARF to the
CourierRendererFactory.


BUG=716154
TEST= added UTs
==========

to

==========
Rename AdaptiveRendererFactory


The introduction of the RendererFactorySelector allows us to choose
between different types of renderers after WMPI has been created.

The AdaptiveRendererFactory currently takes a RendererFactory* as a
construction parameter. When creating a Renderer, it chooses between
using the passed factory, or creating a CourierRenderer, for remoting.

This CL moves the decision logic from the AdaptiveRendererFactory to
the RendererFactorySelector, and renames the ARF to the
CourierRendererFactory.


BUG=716154
TEST= added UTs

Review-Url: https://codereview.chromium.org/2826883002
Cr-Commit-Position: refs/heads/master@{#469585}
Committed:
https://chromium.googlesource.com/chromium/src/+/16d9d7d9ca1db157277bb1170eda...
==========

[commit-bot: I haz the power, 2017-05-05 03:25:28]

Committed patchset #6 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/16d9d7d9ca1db157277bb1170eda...