New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 739516 link

Starred by 3 users
Status: Fixed
Owner:
taku...@chromium.org
Closed: Jul 2017
Cc:
█ zhaobin@chromium.org
mfo...@chromium.org
btolsch@chromium.org
powerb@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

[MediaRouter] MediaRouter.Provider.WakeReason metric not recorded accurately

Project Member Reported by imch...@chromium.org, Jul 5 2017 
MediaRouterMojoImpl calls SetWakeReason on every call that needs to be relayed to the MR extension -- regardless of whether the extension needs to be woken up at that point (i.e., because it is already awake). To fix this, we should only call SetWakeReason if a call is going to request waking up the extension.
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 6 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0e3adbf3d7c3a0436a5d4007505fd3590489049e

commit 0e3adbf3d7c3a0436a5d4007505fd3590489049e
Author: takumif <takumif@chromium.org>
Date: Thu Jul 06 18:38:25 2017

[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}

[modify] https://crrev.com/0e3adbf3d7c3a0436a5d4007505fd3590489049e/chrome/browser/media/router/BUILD.gn
[add] https://crrev.com/0e3adbf3d7c3a0436a5d4007505fd3590489049e/chrome/browser/media/router/event_page_request_manager.cc
[add] https://crrev.com/0e3adbf3d7c3a0436a5d4007505fd3590489049e/chrome/browser/media/router/event_page_request_manager.h
[add] https://crrev.com/0e3adbf3d7c3a0436a5d4007505fd3590489049e/chrome/browser/media/router/event_page_request_manager_factory.cc
[add] https://crrev.com/0e3adbf3d7c3a0436a5d4007505fd3590489049e/chrome/browser/media/router/event_page_request_manager_factory.h
[add] https://crrev.com/0e3adbf3d7c3a0436a5d4007505fd3590489049e/chrome/browser/media/router/event_page_request_manager_unittest.cc
[modify] https://crrev.com/0e3adbf3d7c3a0436a5d4007505fd3590489049e/chrome/browser/media/router/media_router_factory.cc
[modify] https://crrev.com/0e3adbf3d7c3a0436a5d4007505fd3590489049e/chrome/browser/media/router/mojo/media_router_mojo_impl.cc
[modify] https://crrev.com/0e3adbf3d7c3a0436a5d4007505fd3590489049e/chrome/browser/media/router/mojo/media_router_mojo_impl.h
[modify] https://crrev.com/0e3adbf3d7c3a0436a5d4007505fd3590489049e/chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc
[modify] https://crrev.com/0e3adbf3d7c3a0436a5d4007505fd3590489049e/chrome/browser/media/router/mojo/media_router_mojo_test.cc
[modify] https://crrev.com/0e3adbf3d7c3a0436a5d4007505fd3590489049e/chrome/browser/media/router/mojo/media_router_mojo_test.h
[modify] https://crrev.com/0e3adbf3d7c3a0436a5d4007505fd3590489049e/chrome/test/BUILD.gn

Comment 2 by taku...@chromium.org, Jul 6 2017

Status: Fixed (was: Assigned)

Comment 3 by mfo...@chromium.org, Jul 10 2017 

The original logic records only the first wake reason each time the extension is woken up.  Subsequent wake reasons are ignored.  So I believe the metric should be accurate.

Comment 4 by taku...@chromium.org, Jul 10 2017 

I believe the issue was that |current_wake_reason_| could have been updated before the extension was suspended, so it might not have been the actual wake reason.

Comment 5 by mfo...@chromium.org, Jul 10 2017 

Yeah but if the extension is already awake then it shouldn't be called at all?

Anyway, if there was a lurking bug, then thanks for fixing it.

Comment 6 by imch...@chromium.org, Jul 10 2017 

The previous code always called SetWakeReason before calling RunOrDefer, even if the extension is already awake.
►

Sign in to add a comment