[Media Router] Factor extension-related logic out of MediaRouterMojoImpl

[Media Router] Factor extension-related logic out of MediaRouterMojoImpl

We take the component-extension-waking and request-queueing logic out of
MediaRouterMojoImpl and put it in its own class, EventPageRequestManager. In a
future patch the part of MediaRouterMojoImpl that interfaces with the request
manager will be further refactored into its own class, as described in the
design doc below.

Currently MediaRouterMojoImpl is the only client of the request manager, but the
MediaRouteController will also use it in the future (see design doc).

Design doc:
https://docs.google.com/document/d/1PEcNLc9TTaRbeC1jZwVX7E3Rm3bJrqV8049cKXQ7Yho/edit

BUG=737320
BUG=739516
Review-Url: https://codereview.chromium.org/2949933002
Cr-Commit-Position: refs/heads/master@{#484688}
Committed: https://chromium.googlesource.com/chromium/src/+/0e3adbf3d7c3a0436a5d4007505fd3590489049e

[takumif, 2017-06-20 22:43:31]

Description was changed from

==========
[Media Router] Facto extension-related logic out of MediaRouterMojoImpl

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

to

==========
[Media Router] Factor extension-related logic out of MediaRouterMojoImpl

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

[takumif, 2017-06-27 22:29:59]

Description was changed from

==========
[Media Router] Factor extension-related logic out of MediaRouterMojoImpl

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

to

==========
[Media Router] Factor extension-related logic out of MediaRouterMojoImpl

We take the component-extension-waking and request-queueing logic out of
MediaRouterMojoImpl and put it in its own class, EventPageRequestManager. In a
future patch the part of MediaRouterMojoImpl that interfaces with the request
manager will be further refactored into its own class, as described in the
design doc below.

Currently MediaRouterMojoImpl is the only client of the request manager, but the
MediaRouteController will also use it in the future (see design doc).

Design doc:
https://docs.google.com/document/d/1PEcNLc9TTaRbeC1jZwVX7E3Rm3bJrqV8049cKXQ7Y...

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

[takumif, 2017-06-28 00:19:08]

Description was changed from

==========
[Media Router] Factor extension-related logic out of MediaRouterMojoImpl

We take the component-extension-waking and request-queueing logic out of
MediaRouterMojoImpl and put it in its own class, EventPageRequestManager. In a
future patch the part of MediaRouterMojoImpl that interfaces with the request
manager will be further refactored into its own class, as described in the
design doc below.

Currently MediaRouterMojoImpl is the only client of the request manager, but the
MediaRouteController will also use it in the future (see design doc).

Design doc:
https://docs.google.com/document/d/1PEcNLc9TTaRbeC1jZwVX7E3Rm3bJrqV8049cKXQ7Y...

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

to

==========
[Media Router] Factor extension-related logic out of MediaRouterMojoImpl

We take the component-extension-waking and request-queueing logic out of
MediaRouterMojoImpl and put it in its own class, EventPageRequestManager. In a
future patch the part of MediaRouterMojoImpl that interfaces with the request
manager will be further refactored into its own class, as described in the
design doc below.

Currently MediaRouterMojoImpl is the only client of the request manager, but the
MediaRouteController will also use it in the future (see design doc).

Design doc:
https://docs.google.com/document/d/1PEcNLc9TTaRbeC1jZwVX7E3Rm3bJrqV8049cKXQ7Y...

BUG=737320
==========

[takumif, 2017-06-28 00:22:15]

Description was changed from

==========
[Media Router] Factor extension-related logic out of MediaRouterMojoImpl

We take the component-extension-waking and request-queueing logic out of
MediaRouterMojoImpl and put it in its own class, EventPageRequestManager. In a
future patch the part of MediaRouterMojoImpl that interfaces with the request
manager will be further refactored into its own class, as described in the
design doc below.

Currently MediaRouterMojoImpl is the only client of the request manager, but the
MediaRouteController will also use it in the future (see design doc).

Design doc:
https://docs.google.com/document/d/1PEcNLc9TTaRbeC1jZwVX7E3Rm3bJrqV8049cKXQ7Y...

BUG=737320
==========

to

==========
[Media Router] Factor extension-related logic out of MediaRouterMojoImpl

We take the component-extension-waking and request-queueing logic out of
MediaRouterMojoImpl and put it in its own class, EventPageRequestManager. In a
future patch the part of MediaRouterMojoImpl that interfaces with the request
manager will be further refactored into its own class, as described in the
design doc below.

Currently MediaRouterMojoImpl is the only client of the request manager, but the
MediaRouteController will also use it in the future (see design doc).

Design doc:
https://docs.google.com/document/d/1PEcNLc9TTaRbeC1jZwVX7E3Rm3bJrqV8049cKXQ7Y...

BUG=737320
==========

[takumif, 2017-06-28 00:25:36]

takumif@chromium.org changed reviewers:
+ imcheng@chromium.org, zhaobin@chromium.org

[takumif, 2017-06-28 00:25:37]

Please take a look, thanks!

[imcheng, 2017-06-30 00:17:39]

I took a first pass at this patch and didn't look at the unit tests. Some of my
comments are just questions clarifying what the overall picture will look like
after the refactoring.

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/event_page_request_manager.cc (right):

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/event_page_request_manager.cc:81: void
EventPageRequestManager::EnqueueRequest(base::OnceClosure closure) {
s/closure/request for consistency.

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/event_page_request_manager.h (right):

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/event_page_request_manager.h:88: bool
IsEventPageSuspended();
const?

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/event_page_request_manager.h:104: // Id of the
component extension. Used for managing its suspend/wake state
s/Id/ID

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/event_page_request_manager.h:123:
MediaRouteProviderWakeReason current_wake_reason_;
nit: set to MediaRouteProviderWakeReason::TOTAL_COUNT here.

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/event_page_request_manager_unittest.cc (right):

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/event_page_request_manager_unittest.cc:55: class
MockRequest {
Can you use base::MockCallback?
https://cs.chromium.org/chromium/src/base/test/mock_callback.h?q=mockcallback...

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:113:
event_page_request_manager_->SetExtensionId(extension.id());
I think this method will need to move to the proxy since it is desktop specific.

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:224: const
std::string& MediaRouterMojoImpl::media_route_provider_extension_id()
As a TODO we should remove this method and have the caller get the ID from
desktop specific object.

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:272:
event_page_request_manager_->SetWakeReason(
Will the pattern of SetWakeReason+RunOrDefer be replaced by a invocation of a
MediaRouteProvider mojo ptr in a future patch?

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:444:
event_page_request_manager_->RunOrDefer(
Seems like WakeReason could be part of the RunOrDefer API; it would be great if
we can get it to work with MediaRouteController too.

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right):

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.h:401: std::string
instance_id_;
Is the plan to move instance_id_ / provider_version_was_recorded_ to the proxy
object once it is implemented?

[takumif, 2017-07-01 01:14:56]

Patchset #8 (id:140001) has been deleted

[takumif, 2017-07-01 01:16:45]

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/event_page_request_manager.cc (right):

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/event_page_request_manager.cc:81: void
EventPageRequestManager::EnqueueRequest(base::OnceClosure closure) {
On 2017/06/30 00:17:38, imcheng wrote:
> s/closure/request for consistency.

Done.

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/event_page_request_manager.h (right):

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/event_page_request_manager.h:88: bool
IsEventPageSuspended();
On 2017/06/30 00:17:38, imcheng wrote:
> const?

Done.

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/event_page_request_manager.h:104: // Id of the
component extension. Used for managing its suspend/wake state
On 2017/06/30 00:17:38, imcheng wrote:
> s/Id/ID

Done.

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/event_page_request_manager.h:123:
MediaRouteProviderWakeReason current_wake_reason_;
On 2017/06/30 00:17:38, imcheng wrote:
> nit: set to MediaRouteProviderWakeReason::TOTAL_COUNT here.

Done.

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/event_page_request_manager_unittest.cc (right):

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/event_page_request_manager_unittest.cc:55: class
MockRequest {
On 2017/06/30 00:17:38, imcheng wrote:
> Can you use base::MockCallback?
>
https://cs.chromium.org/chromium/src/base/test/mock_callback.h?q=mockcallback...

Done.

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:113:
event_page_request_manager_->SetExtensionId(extension.id());
On 2017/06/30 00:17:39, imcheng wrote:
> I think this method will need to move to the proxy since it is desktop
specific.

I agree.

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:224: const
std::string& MediaRouterMojoImpl::media_route_provider_extension_id()
On 2017/06/30 00:17:39, imcheng wrote:
> As a TODO we should remove this method and have the caller get the ID from
> desktop specific object.

Right, I added a TODO in the .h to remove this method.

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:272:
event_page_request_manager_->SetWakeReason(
On 2017/06/30 00:17:39, imcheng wrote:
> Will the pattern of SetWakeReason+RunOrDefer be replaced by a invocation of a
> MediaRouteProvider mojo ptr in a future patch?

Yes, all the references to event_page_request_manager_ and the Do* methods will
move to the proxy, and MRMojoImpl will be using the proxy via the
MediaRouteProvider interface.

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:444:
event_page_request_manager_->RunOrDefer(
On 2017/06/30 00:17:39, imcheng wrote:
> Seems like WakeReason could be part of the RunOrDefer API; it would be great
if
> we can get it to work with MediaRouteController too.

Added wake reason as a param to this method. This makes sense if we're requiring
the wake reason to be set whenever RunOrDefer() is called. We can add enum
values for the various route controller commands as well.

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right):

https://codereview.chromium.org/2949933002/diff/120001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.h:401: std::string
instance_id_;
On 2017/06/30 00:17:39, imcheng wrote:
> Is the plan to move instance_id_ / provider_version_was_recorded_ to the proxy
> object once it is implemented?

Yes, given those are used in BindToRequest() and RegisterMRP() which will be
moved to the proxy object, it'd make sense to do so.

[imcheng, 2017-07-05 19:07:54]

lgtm

https://codereview.chromium.org/2949933002/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_test.h (right):

https://codereview.chromium.org/2949933002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_test.h:10: #include <vector>
<vector> not needed? Since we aren't using vector directly.

[takumif, 2017-07-05 19:48:15]

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

[commit-bot: I haz the power, 2017-07-05 19:48:28]

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-07-05 20:04:39]

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

[commit-bot: I haz the power, 2017-07-05 20:04:40]

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

[takumif, 2017-07-05 21:38:39]

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

[commit-bot: I haz the power, 2017-07-05 21:39:08]

Dry run: CQ is trying da patch.

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

[takumif, 2017-07-05 22:04:22]

Description was changed from

==========
[Media Router] Factor extension-related logic out of MediaRouterMojoImpl

We take the component-extension-waking and request-queueing logic out of
MediaRouterMojoImpl and put it in its own class, EventPageRequestManager. In a
future patch the part of MediaRouterMojoImpl that interfaces with the request
manager will be further refactored into its own class, as described in the
design doc below.

Currently MediaRouterMojoImpl is the only client of the request manager, but the
MediaRouteController will also use it in the future (see design doc).

Design doc:
https://docs.google.com/document/d/1PEcNLc9TTaRbeC1jZwVX7E3Rm3bJrqV8049cKXQ7Y...

BUG=737320
==========

to

==========
[Media Router] Factor extension-related logic out of MediaRouterMojoImpl

We take the component-extension-waking and request-queueing logic out of
MediaRouterMojoImpl and put it in its own class, EventPageRequestManager. In a
future patch the part of MediaRouterMojoImpl that interfaces with the request
manager will be further refactored into its own class, as described in the
design doc below.

Currently MediaRouterMojoImpl is the only client of the request manager, but the
MediaRouteController will also use it in the future (see design doc).

Design doc:
https://docs.google.com/document/d/1PEcNLc9TTaRbeC1jZwVX7E3Rm3bJrqV8049cKXQ7Y...

BUG=737320
BUG=739516
==========

[commit-bot: I haz the power, 2017-07-05 22:48:13]

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

[commit-bot: I haz the power, 2017-07-05 22:48:14]

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

[takumif, 2017-07-05 23:05:01]

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

[commit-bot: I haz the power, 2017-07-05 23:05:16]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-06 00:09:55]

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

[commit-bot: I haz the power, 2017-07-06 00:09:56]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-07-06 01:24:21]

lgtm

https://codereview.chromium.org/2949933002/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_test.h (right):

https://codereview.chromium.org/2949933002/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_test.h:8: #include <memory>
nit: #include seems not needed.

[takumif, 2017-07-06 17:15:34]

Thanks for the reviews!

https://codereview.chromium.org/2949933002/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_test.h (right):

https://codereview.chromium.org/2949933002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_test.h:10: #include <vector>
On 2017/07/05 19:07:54, imcheng wrote:
> <vector> not needed? Since we aren't using vector directly.

Done.

https://codereview.chromium.org/2949933002/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_test.h (right):

https://codereview.chromium.org/2949933002/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_test.h:8: #include <memory>
On 2017/07/06 01:24:21, zhaobin wrote:
> nit: #include seems not needed.

Done.

[takumif, 2017-07-06 17:15:39]

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

[commit-bot: I haz the power, 2017-07-06 17:15:56]

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-07-06 18:28:53]

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

[commit-bot: I haz the power, 2017-07-06 18:28:55]

Dry run: This issue passed the CQ dry run.

[takumif, 2017-07-06 18:33:21]

The CQ bit was checked by takumif@chromium.org

[takumif, 2017-07-06 18:33:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
imcheng@chromium.org, zhaobin@chromium.org
Link to the patchset: https://codereview.chromium.org/2949933002/#ps200001
(title: "Remove includes")

[commit-bot: I haz the power, 2017-07-06 18:33:35]

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-07-06 18:37:55]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1499366001800050,
"parent_rev": "36969c47fb5943856ceb8e76dbc65c7df662f0b5", "commit_rev":
"3856d6e855a4c9f36cd44dfaa8d6f0f440d0339a"}

[commit-bot: I haz the power, 2017-07-06 18:38:06]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1499366001800050,
"parent_rev": "b0ec3b08df8a12d01b0f37553578d976559dd600", "commit_rev":
"db67b54d1a091b8bd3379120b872004f3d77a6b7"}

[commit-bot: I haz the power, 2017-07-06 18:38:26]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1499366001800050,
"parent_rev": "4772c8e6b053dfb70cfc21db41ca46b07eb31027", "commit_rev":
"0e3adbf3d7c3a0436a5d4007505fd3590489049e"}

[commit-bot: I haz the power, 2017-07-06 18:38:40]

Description was changed from

==========
[Media Router] Factor extension-related logic out of MediaRouterMojoImpl

We take the component-extension-waking and request-queueing logic out of
MediaRouterMojoImpl and put it in its own class, EventPageRequestManager. In a
future patch the part of MediaRouterMojoImpl that interfaces with the request
manager will be further refactored into its own class, as described in the
design doc below.

Currently MediaRouterMojoImpl is the only client of the request manager, but the
MediaRouteController will also use it in the future (see design doc).

Design doc:
https://docs.google.com/document/d/1PEcNLc9TTaRbeC1jZwVX7E3Rm3bJrqV8049cKXQ7Y...

BUG=737320
BUG=739516
==========

to

==========
[Media Router] Factor extension-related logic out of MediaRouterMojoImpl

We take the component-extension-waking and request-queueing logic out of
MediaRouterMojoImpl and put it in its own class, EventPageRequestManager. In a
future patch the part of MediaRouterMojoImpl that interfaces with the request
manager will be further refactored into its own class, as described in the
design doc below.

Currently MediaRouterMojoImpl is the only client of the request manager, but the
MediaRouteController will also use it in the future (see design doc).

Design doc:
https://docs.google.com/document/d/1PEcNLc9TTaRbeC1jZwVX7E3Rm3bJrqV8049cKXQ7Y...

BUG=737320
BUG=739516

Review-Url: https://codereview.chromium.org/2949933002
Cr-Commit-Position: refs/heads/master@{#484688}
Committed:
https://chromium.googlesource.com/chromium/src/+/0e3adbf3d7c3a0436a5d4007505f...
==========

[commit-bot: I haz the power, 2017-07-06 18:38:42]

Committed patchset #10 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/0e3adbf3d7c3a0436a5d4007505f...