MediaRouter: Cache MediaRoutesObserver queries to prevent ext wake-ups

MediaRouter: Cache MediaRoutesObserver queries to prevent ext wake-ups

Resolved a performance issue where the Media Router component extension
was being woken-up each time a new tab was opened. The solution is to
cache route list updates so that the registration of new observers does
not force the extension to wake-up to report on something we can cache
browser-side. (This is the same as what is already being done for the
MediaSinksObservers.)

BUG=621246
TEST=Ran chrome with --enable-logging=stderr --vmodule=media_router_mojo_impl=1 to confirm extension wake-ups do not happen when opening new tabs.

Review-Url: https://codereview.chromium.org/2735493002
Cr-Commit-Position: refs/heads/master@{#454769}
Committed: https://chromium.googlesource.com/chromium/src/+/34e86af9f3ebe792b7681053eca4c08b27eca52d

[miu, 2017-03-04 01:50:29]

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

[commit-bot: I haz the power, 2017-03-04 01:50:39]

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

[miu, 2017-03-04 01:53:54]

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

[miu, 2017-03-04 01:53:56]

wez: PTAL.

[Wez, 2017-03-04 02:07:32]

https://codereview.chromium.org/2735493002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2735493002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:473: bool new_query =
false;
nit: is_new_query

https://codereview.chromium.org/2735493002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:506: // observer is
still registered. Otherwise, punt.
nit: What does "punt" mean here? Perhaps rephrase to something like "If the
query has no cached result, or the observer is gone, then there is nothing to do
here."

https://codereview.chromium.org/2735493002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right):

https://codereview.chromium.org/2735493002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router_mojo_impl.h:196: bool
has_cached_result = false;
nit: Since you have two cached-data fields below this seems fine, but would it
be possible to restructure things to use base::Optional<>..?

https://codereview.chromium.org/2735493002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router_mojo_impl.h:246: void
NotifyOfExistingRoutesAfterRegistration(
This is a confusing name, IMO - I'd suggest either calling it
NotifyOfExistingRoutes and having the comment explain that the observer must be
registered, or calling it NotifyOfExistingRoutesIfRegistered, since it's
actually conditional on the observer _still_ being registered.

Or, call it FinishRegisterMediaRoutesOberver, since that's in effect what it's
doing; though that would raise the question of why this is the only form of
observer that needs a separate step for that

[miu, 2017-03-04 02:24:39]

https://codereview.chromium.org/2735493002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2735493002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:473: bool new_query =
false;
On 2017/03/04 02:07:32, Wez wrote:
> nit: is_new_query

Done.

https://codereview.chromium.org/2735493002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:506: // observer is
still registered. Otherwise, punt.
On 2017/03/04 02:07:32, Wez wrote:
> nit: What does "punt" mean here? Perhaps rephrase to something like "If the
> query has no cached result, or the observer is gone, then there is nothing to
do
> here."

Done.

https://codereview.chromium.org/2735493002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right):

https://codereview.chromium.org/2735493002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router_mojo_impl.h:196: bool
has_cached_result = false;
On 2017/03/04 02:07:32, Wez wrote:
> nit: Since you have two cached-data fields below this seems fine, but would it
> be possible to restructure things to use base::Optional<>..?

Done.

https://codereview.chromium.org/2735493002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/mojo/media_router_mojo_impl.h:246: void
NotifyOfExistingRoutesAfterRegistration(
On 2017/03/04 02:07:32, Wez wrote:
> This is a confusing name, IMO - I'd suggest either calling it
> NotifyOfExistingRoutes and having the comment explain that the observer must
be
> registered, or calling it NotifyOfExistingRoutesIfRegistered, since it's
> actually conditional on the observer _still_ being registered.
> 
> Or, call it FinishRegisterMediaRoutesOberver, since that's in effect what it's
> doing; though that would raise the question of why this is the only form of
> observer that needs a separate step for that

Done.

[Wez, 2017-03-04 08:29:42]

The CQ bit was checked by wez@chromium.org

[Wez, 2017-03-04 08:29:42]

lgtm

[commit-bot: I haz the power, 2017-03-04 08:29:46]

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-03-04 09:09:13]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488616182240150,
"parent_rev": "b7d82790bc5ff733f55f92e2b0ecba0fa0c04931", "commit_rev":
"34e86af9f3ebe792b7681053eca4c08b27eca52d"}

[commit-bot: I haz the power, 2017-03-04 09:09:47]

Description was changed from

==========
MediaRouter: Cache MediaRoutesObserver queries to prevent ext wake-ups

Resolved a performance issue where the Media Router component extension
was being woken-up each time a new tab was opened. The solution is to
cache route list updates so that the registration of new observers does
not force the extension to wake-up to report on something we can cache
browser-side. (This is the same as what is already being done for the
MediaSinksObservers.)

BUG=621246
TEST=Ran chrome with --enable-logging=stderr --vmodule=media_router_mojo_impl=1
to confirm extension wake-ups do not happen when opening new tabs.
==========

to

==========
MediaRouter: Cache MediaRoutesObserver queries to prevent ext wake-ups

Resolved a performance issue where the Media Router component extension
was being woken-up each time a new tab was opened. The solution is to
cache route list updates so that the registration of new observers does
not force the extension to wake-up to report on something we can cache
browser-side. (This is the same as what is already being done for the
MediaSinksObservers.)

BUG=621246
TEST=Ran chrome with --enable-logging=stderr --vmodule=media_router_mojo_impl=1
to confirm extension wake-ups do not happen when opening new tabs.

Review-Url: https://codereview.chromium.org/2735493002
Cr-Commit-Position: refs/heads/master@{#454769}
Committed:
https://chromium.googlesource.com/chromium/src/+/34e86af9f3ebe792b7681053eca4...
==========

[commit-bot: I haz the power, 2017-03-04 09:09:48]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/34e86af9f3ebe792b7681053eca4...