Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2294973002:
Create MediaRouterActionController and MediaRouterUIService (Closed)
|
Created:
4 years, 3 months ago by takumif Modified:
4 years, 3 months ago CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, media-router+watch_chromium.org, miu+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate MediaRouterActionController and MediaRouterUIService
This CL is part 1 of 3 CLs to implement ephemeral toolbar icon for media router:
1. Implement MediaRouterActionController (this CL)
2. Show ephemeral icon for active local routes and issues [1]
3. Show ephemeral icon for open dialog
The icon will be shown when the "always show icon" setting is on, or there is a local media route, an unresolved issue, or an open dialog. The MediaRouterActionController will be responsible for telling the ToolbarActionsModel to show/hide the MR icon.
MediaRouterUIService is a KeyedService (a per-profile instance) that owns MediaRouterActionController.
These CLs are a redesign of a previous CL [2], which has been reverted for its bugs.
[1] https://codereview.chromium.org/2332693003/
[2] https://codereview.chromium.org/2155293002/
BUG=594577
Committed: https://crrev.com/be86872a218127fdf92001a1f69e044239a058b4
Cr-Commit-Position: refs/heads/master@{#419581}
Patch Set 1 #
Total comments: 51
Patch Set 2 : Address Derek and Mark's comments #
Total comments: 35
Patch Set 3 : Add MediaRouterUIService (KeyedService) #
Total comments: 40
Patch Set 4 : Rebase #Patch Set 5 : Address Derek and Mark's comments #
Total comments: 19
Patch Set 6 : Address Devlin's comments #
Total comments: 31
Patch Set 7 : Address Mike and Devlin's comments #Patch Set 8 : Address comments #Patch Set 9 : Modify BUILD.gn files, rebase #Messages
Total messages: 62 (32 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== . BUG= ========== to ========== Create MediaRouterActionController This CL is part 1 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController (this CL) 2. Show ephemeral icon for active local routes and issues 3. Show ephemeral icon for open dialog The MediaRouterActionController will be responsible for telling the ToolbarActionsModel to show/hide the MR icon. The icon will be shown when the "always show icon" setting is on or there is a local media route, an unresolved issue, or an open dialog. MediaRouterActionController will also notify MediaRouterAction (which will be an observer for MediaRouterActionController) when its icon should be updated. BUG=594577 ==========
Description was changed from ========== Create MediaRouterActionController This CL is part 1 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController (this CL) 2. Show ephemeral icon for active local routes and issues 3. Show ephemeral icon for open dialog The MediaRouterActionController will be responsible for telling the ToolbarActionsModel to show/hide the MR icon. The icon will be shown when the "always show icon" setting is on or there is a local media route, an unresolved issue, or an open dialog. MediaRouterActionController will also notify MediaRouterAction (which will be an observer for MediaRouterActionController) when its icon should be updated. BUG=594577 ========== to ========== Create MediaRouterActionController This CL is part 1 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController (this CL) 2. Show ephemeral icon for active local routes and issues 3. Show ephemeral icon for open dialog The MediaRouterActionController will be responsible for telling the ToolbarActionsModel to show/hide the MR icon. The icon will be shown when the "always show icon" setting is on or there is a local media route, an unresolved issue, or an open dialog. MediaRouterActionController will also notify MediaRouterAction (which will be an observer for MediaRouterActionController) when its icon should be updated. These CLs are a redesign of a previous CL [1], which has been reverted for its bugs. [1] https://codereview.chromium.org/2155293002/ BUG=594577 ==========
Description was changed from ========== Create MediaRouterActionController This CL is part 1 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController (this CL) 2. Show ephemeral icon for active local routes and issues 3. Show ephemeral icon for open dialog The MediaRouterActionController will be responsible for telling the ToolbarActionsModel to show/hide the MR icon. The icon will be shown when the "always show icon" setting is on or there is a local media route, an unresolved issue, or an open dialog. MediaRouterActionController will also notify MediaRouterAction (which will be an observer for MediaRouterActionController) when its icon should be updated. These CLs are a redesign of a previous CL [1], which has been reverted for its bugs. [1] https://codereview.chromium.org/2155293002/ BUG=594577 ========== to ========== Create MediaRouterActionController This CL is part 1 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController (this CL) 2. Show ephemeral icon for active local routes and issues 3. Show ephemeral icon for open dialog The icon will be shown when the "always show icon" setting is on, or there is a local media route, an unresolved issue, or an open dialog. The MediaRouterActionController will be responsible for telling the ToolbarActionsModel to show/hide the MR icon. MediaRouterActionController will also notify MediaRouterAction (which will be an observer for MediaRouterActionController) when its icon should be updated. These CLs are a redesign of a previous CL [1], which has been reverted for its bugs. [1] https://codereview.chromium.org/2155293002/ BUG=594577 ==========
takumif@chromium.org changed reviewers: + anthonyvd@chromium.org, mfoltz@chromium.org, msw@chromium.org
+mfoltz@ for MR-related files +msw@ for adding three files to c/b/ui/toolbar/ +anthonyvd@ for c/b/profiles/profile.cc Please take a look, thank you!
imcheng@chromium.org changed reviewers: + imcheng@chromium.org
Drive-by with some high level comments. I am not a fan of introducing ui/toolbar dependency to the MediaRouter API, but I might not have full context here since I wasn't following the previous CL. I wonder if we can make MediaRouterActionController a browser context keyed service and have it instantiated in ui/toolbar code. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:32: issue_.reset(issue ? new media_router::Issue(*issue) : nullptr); Unless something changed, we don't actually need to store the entire Issue object in this class; we only need to know whether there is an Issue currently. In crrev.com/2176613003 I changed |issue_| to a boolean to avoid copy construction. It would be good to do it here. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:77: DCHECK(delegate_); Move this DCHECK to ctor. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:91: DCHECK(profile_); Move this DCHECK to ctor. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:23: // There should be one instance of this class per profile. If it were really the case, would it be better to make this a KeyedService? 1) Define a MediaRouterActionControllerFactory class that extends BrowserContextKeyedServiceFactory. 2) The factory class would DependOn MediaRouterFactory That way, you wouldn't need to declare a new method on MediaRouter and add a dependency to ui/toolbar. Instead we would treat MediaRouterActionController as just another observer of MediaRouter. In terms of instantiation, I don't think it makes sense for it to be done during MediaRouterBase::Initialize(). Instead it should be done by whoever uses MediaRouterActionController (which I would guess is something in ui/toolbar?) https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:30: virtual void OnIssueUpdated(const media_router::Issue* issue); Should these be pure virtual methods, i.e. Observer should be an interface? Actually, will we have observers other than |delegate_| (which seemed to have been special cased in the .cc)? https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:71: Profile* profile_; Profile* const profile_; https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:75: extensions::ComponentMigrationHelper::ComponentActionDelegate* delegate_; ...ComponentActionDelegate* const delegate_;
On 2016/08/31 05:39:49, imcheng wrote: > Drive-by with some high level comments. > > I am not a fan of introducing ui/toolbar dependency to the MediaRouter API, but > I might not have full context here since I wasn't following the previous CL. I > wonder if we can make MediaRouterActionController a browser context keyed > service and have it instantiated in ui/toolbar code. > > https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... > File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): > > https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... > chrome/browser/ui/toolbar/media_router_action_controller.cc:32: > issue_.reset(issue ? new media_router::Issue(*issue) : nullptr); > Unless something changed, we don't actually need to store the entire Issue > object in this class; we only need to know whether there is an Issue currently. > In crrev.com/2176613003 I changed |issue_| to a boolean to avoid copy > construction. It would be good to do it here. > > https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... > chrome/browser/ui/toolbar/media_router_action_controller.cc:77: > DCHECK(delegate_); > Move this DCHECK to ctor. > > https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... > chrome/browser/ui/toolbar/media_router_action_controller.cc:91: > DCHECK(profile_); > Move this DCHECK to ctor. > > https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... > File chrome/browser/ui/toolbar/media_router_action_controller.h (right): > > https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... > chrome/browser/ui/toolbar/media_router_action_controller.h:23: // There should > be one instance of this class per profile. > If it were really the case, would it be better to make this a KeyedService? > > 1) Define a MediaRouterActionControllerFactory class that extends > BrowserContextKeyedServiceFactory. > 2) The factory class would DependOn MediaRouterFactory > > That way, you wouldn't need to declare a new method on MediaRouter and add a > dependency to ui/toolbar. Instead we would treat MediaRouterActionController as > just another observer of MediaRouter. > > In terms of instantiation, I don't think it makes sense for it to be done during > MediaRouterBase::Initialize(). Instead it should be done by whoever uses > MediaRouterActionController (which I would guess is something in ui/toolbar?) > > https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... > chrome/browser/ui/toolbar/media_router_action_controller.h:30: virtual void > OnIssueUpdated(const media_router::Issue* issue); > Should these be pure virtual methods, i.e. Observer should be an interface? > > Actually, will we have observers other than |delegate_| (which seemed to have > been special cased in the .cc)? > > https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... > chrome/browser/ui/toolbar/media_router_action_controller.h:71: Profile* > profile_; > Profile* const profile_; > > https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... > chrome/browser/ui/toolbar/media_router_action_controller.h:75: > extensions::ComponentMigrationHelper::ComponentActionDelegate* delegate_; > ...ComponentActionDelegate* const delegate_; I talked to Devlin about making one of the toolbar classes own the MediaRouterActionController, but he was against making the toolbar own anything specific to a component action. I suggested making changes to existing component action-related classes like ComponentMigrationHelper, but he didn't like those ideas either. Mark and I considered making MRActionController a KeyedService of its own, but decided against it as it'd create some overhead, and there could be push back against registering a new KeyedService.
c/b/profiles/profile.cc lgtm
We should come to consensus on the ownership bit. Ownership by the MR is a little hacky. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_base.cc (right): https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/media_router_base.cc:130: action_controller_.reset(); Can this TODO be done now? https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_base.h (right): https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/media_router_base.h:101: std::unique_ptr<MediaRouterActionController> action_controller_; Please add a TODO to find an owner for this object in chrome/browser/ui. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:16: void MediaRouterActionController::Observer::OnLocalRouteUpdated( Should Observer be pure virtual? https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:58: bool MediaRouterActionController::HasLocalRoute() const { What calls this? https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:62: media_router::Issue* MediaRouterActionController::GetIssue() const { Ditto https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:81: if (ShouldAlwaysShowIcon() || has_local_display_route_ || issue_) { 1) Consider having a private IsActionEnabled() method to compute this. 2) You need to ask the component migration helper if the user had the Cast extension installed, or set the initial value of the pref appropriately. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:92: return profile_->GetPrefs()->GetBoolean( If the pref value changes through sync, how does this class get notified? https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:5: #ifndef CHROME_BROWSER_UI_TOOLBAR_MEDIA_ROUTER_ACTION_CONTROLLER_H_ I wonder if we should put this in its own folder, chrome/browser/ui/media_router. It doesn't seem related to the implementation of the toolbar, it's specific to the media router UX. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:23: // There should be one instance of this class per profile. On 2016/08/31 at 05:39:48, imcheng wrote: > If it were really the case, would it be better to make this a KeyedService? > > 1) Define a MediaRouterActionControllerFactory class that extends BrowserContextKeyedServiceFactory. > 2) The factory class would DependOn MediaRouterFactory Takumi and I discussed this. This code didn't seem worth the boilerplate and overhead of instantiating another KeyedService. > That way, you wouldn't need to declare a new method on MediaRouter and add a dependency to ui/toolbar. Instead we would treat MediaRouterActionController as just another observer of MediaRouter. > > In terms of instantiation, I don't think it makes sense for it to be done during MediaRouterBase::Initialize(). Instead it should be done by whoever uses MediaRouterActionController (which I would guess is something in ui/toolbar?) It would make most sense for something in chrome/browser/ui to own this. The toolbar owners have pushed back on adding a dependency from the toolbar on this code. At this point the likely path long term is to add a chrome/browser/ui/media_router folder and an ownership path from something else in browser/ui. If we continue to get push back we can go the keyed service route, I suppose. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:28: class Observer { I don't see any observers implemented in this change. Who is observing this class? Can you document this if it's required? https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:30: virtual void OnIssueUpdated(const media_router::Issue* issue); Pass by const reference https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:31: virtual void OnLocalRouteUpdated(bool has_local_route); Bikeshed: slightly prefer an API OnNumLocalRoutesChanged(size_t num_routes) This makes it clear the meaning of what is being passed in the argument. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:35: Profile* profile, media_router::MediaRouter* router); I'm not a huge fan of the circular dependency between MediaRouter and this class. Derek, is there a different way we can construct observers that are owned by the MediaRouter? https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:48: void ToggleActionVisibilityPreference(); This API is error prone: - You have to check the state of the pref before calling it to know what it is going to do. - If you accidentally call it twice it will create hard to find bugs. Better to have SetAlwaysShowActionPref() and RemoveAlwaysShowActionPref(). https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:53: media_router::Issue* GetIssue() const; If a caller wants to find the current issue should they instead register an observer/delegate for OnIssueUpdated? https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:59: friend class MediaRouterActionControllerUnitTest; The pattern I've seen is to use FRIEND_TEST_ALL_PREFIXES to friend individual test cases instead of friending the base class. But I could go either way on it. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:63: void UpdateActionVisibility(); Bikeshed: 1. This may be a no-op if the action is already in the toolbar. 2. The action may not be visible if it's in the overflow. Suggest updating documentation and tweaking the name. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:67: bool ShouldAlwaysShowIcon(); Nit: Please use 'Action' consistently to refer to the widget in the toolbar. The icon is a property of the action :) Here, maybe GetAlwaysShowActionPref() https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:74: // toolbar. Please document ownership & why delegate_ going to outlive this object. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:75: extensions::ComponentMigrationHelper::ComponentActionDelegate* delegate_; component_action_delegate_
Derek and Mark, thanks for your comments. I believe the things we still have to discuss are: - Is it okay for MediaRouterBase to own a MRActionController for now? - Who should own the MRActionController in the long run (or should it be a KeyedService)? - Should MRActionController go in c/b/ui/toolbar/ or some other directory? https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_base.cc (right): https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/media_router_base.cc:130: action_controller_.reset(); On 2016/09/02 22:19:09, mark a. foltz wrote: > Can this TODO be done now? We need to change MRAction to use MRActionController before we do that, which I plan to do in another CL. I'll do this TODO there. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_base.h (right): https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/media_router_base.h:101: std::unique_ptr<MediaRouterActionController> action_controller_; On 2016/09/02 22:19:09, mark a. foltz wrote: > Please add a TODO to find an owner for this object in chrome/browser/ui. Done. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:16: void MediaRouterActionController::Observer::OnLocalRouteUpdated( On 2016/09/02 22:19:09, mark a. foltz wrote: > Should Observer be pure virtual? Done. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:32: issue_.reset(issue ? new media_router::Issue(*issue) : nullptr); On 2016/08/31 05:39:48, imcheng wrote: > Unless something changed, we don't actually need to store the entire Issue > object in this class; we only need to know whether there is an Issue currently. > In crrev.com/2176613003 I changed |issue_| to a boolean to avoid copy > construction. It would be good to do it here. I can make MRActionController store a boolean and issue severity and pass those on to the MRAction, but those changes would largely overlap with your changes in the CL you mentioned. Perhaps it'd be better to make those changes after your CL has landed? https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:58: bool MediaRouterActionController::HasLocalRoute() const { On 2016/09/02 22:19:09, mark a. foltz wrote: > What calls this? This is called by MRAction when it is initialized, to show the correct icon image. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:62: media_router::Issue* MediaRouterActionController::GetIssue() const { On 2016/09/02 22:19:09, mark a. foltz wrote: > Ditto Also MRAction. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:77: DCHECK(delegate_); On 2016/08/31 05:39:48, imcheng wrote: > Move this DCHECK to ctor. Done. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:81: if (ShouldAlwaysShowIcon() || has_local_display_route_ || issue_) { On 2016/09/02 22:19:09, mark a. foltz wrote: > 1) Consider having a private IsActionEnabled() method to compute this. > > 2) You need to ask the component migration helper if the user had the Cast > extension installed, or set the initial value of the pref appropriately. > > > 1) Done. 2) Added the change to ComponentMigrationHelper to modify the pref to this patch. MRActionController can observe the change through PrefChangeRegistrar. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:91: DCHECK(profile_); On 2016/08/31 05:39:48, imcheng wrote: > Move this DCHECK to ctor. Done. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:92: return profile_->GetPrefs()->GetBoolean( On 2016/09/02 22:19:09, mark a. foltz wrote: > If the pref value changes through sync, how does this class get notified? I've added a PrefChangeRegistrar to observe changes to the pref, and a unit test for it. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:28: class Observer { On 2016/09/02 22:19:09, mark a. foltz wrote: > I don't see any observers implemented in this change. Who is observing this > class? > > Can you document this if it's required? It's for MRAction to observe MRActionController. Added a comment. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:30: virtual void OnIssueUpdated(const media_router::Issue* issue); On 2016/08/31 05:39:49, imcheng wrote: > Should these be pure virtual methods, i.e. Observer should be an interface? > > Actually, will we have observers other than |delegate_| (which seemed to have > been special cased in the .cc)? MediaRouterActions will be observing |this|, rather than separately observing issues and routes. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:30: virtual void OnIssueUpdated(const media_router::Issue* issue); On 2016/09/02 22:19:10, mark a. foltz wrote: > Pass by const reference I'd like to be able to pass a nullptr to indicate the lack of issues. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:31: virtual void OnLocalRouteUpdated(bool has_local_route); On 2016/09/02 22:19:10, mark a. foltz wrote: > Bikeshed: slightly prefer an API > > OnNumLocalRoutesChanged(size_t num_routes) > > This makes it clear the meaning of what is being passed in the argument. Do we expect to ever have more than one local route? Also, MRAction only needs to know whether there exists a local route. Would it be better to name it something like OnLocalRoutePresenceChanged()? https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:48: void ToggleActionVisibilityPreference(); On 2016/09/02 22:19:09, mark a. foltz wrote: > This API is error prone: > - You have to check the state of the pref before calling it to know what it is > going to do. > - If you accidentally call it twice it will create hard to find bugs. > > Better to have SetAlwaysShowActionPref() and RemoveAlwaysShowActionPref(). I've changed it to SetAlwaysShowActionPref(bool always_show). Is it still preferable to have two separate methods instead? https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:53: media_router::Issue* GetIssue() const; On 2016/09/02 22:19:09, mark a. foltz wrote: > If a caller wants to find the current issue should they instead register an > observer/delegate for OnIssueUpdated? When a MRAction is instantiated, it needs to know whether there already is an issue / local route, so it calls HasLocalRoute() and GetIssue(). https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:59: friend class MediaRouterActionControllerUnitTest; On 2016/09/02 22:19:09, mark a. foltz wrote: > The pattern I've seen is to use FRIEND_TEST_ALL_PREFIXES to friend individual > test cases instead of friending the base class. But I could go either way on > it. I wanted to give the base class access to private attributes, but changed the test so that's no longer necessary and removed this line. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:63: void UpdateActionVisibility(); On 2016/09/02 22:19:09, mark a. foltz wrote: > Bikeshed: > 1. This may be a no-op if the action is already in the toolbar. > 2. The action may not be visible if it's in the overflow. > > Suggest updating documentation and tweaking the name. Done. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:67: bool ShouldAlwaysShowIcon(); On 2016/09/02 22:19:09, mark a. foltz wrote: > Nit: > Please use 'Action' consistently to refer to the widget in the toolbar. > The icon is a property of the action :) > > Here, maybe GetAlwaysShowActionPref() Done. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:71: Profile* profile_; On 2016/08/31 05:39:49, imcheng wrote: > Profile* const profile_; Done. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:74: // toolbar. On 2016/09/02 22:19:09, mark a. foltz wrote: > Please document ownership & why delegate_ going to outlive this object. Done. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:75: extensions::ComponentMigrationHelper::ComponentActionDelegate* delegate_; On 2016/09/02 22:19:09, mark a. foltz wrote: > component_action_delegate_ Done. https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:75: extensions::ComponentMigrationHelper::ComponentActionDelegate* delegate_; On 2016/08/31 05:39:49, imcheng wrote: > ...ComponentActionDelegate* const delegate_; Done.
My take on the ownership model:
I think ComponentActionDelegate interface makes sense (and it should be moved
out of ComponentMigrationHelper into some common location), as it can also be
used to allow a component to control whether its action gets shown according to
its own logic. To use it though we need to guarantee that ToolbarActionsModel
outlives the objects that access it via the CAD interface. The most
straightforward way IMO is to have ToolbarActionsModel own these objects (like
how it does with ComponentMigrationHelper today). This also draws a parallel to
how ToolbarActionsBar owns the action instances of various features.
And then there is the issue of how MediaRouterAction obtains a reference to
MediaRouterActionController to register itself. I suppose it is possible by
jumping through the ToolbarActions{Bar,Model} hoop, but it seems a bit
convoluted. I'd rather have MediaRouterActions register their own
MediaRoutesObserver/IssuesObserver with MediaRouter than share them with
MediaRouterActionController; I don't think it will incur too much performance
penalty but it should hopefully makes things simpler.
WDYT?
https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action_controller.h (right):
https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_controller.h:35: Profile* profile,
media_router::MediaRouter* router);
On 2016/09/02 22:19:09, mark a. foltz wrote:
> I'm not a huge fan of the circular dependency between MediaRouter and this
> class. Derek, is there a different way we can construct observers that are
> owned by the MediaRouter?
I would suggest not having MediaRouter depend on MediaRouterActionController. To
me it doesn't really make sense for MediaRouter to own it, since in general
MediaRouter is not aware of UI specific logic.
On 2016/09/08 20:49:31, imcheng wrote:
> My take on the ownership model:
>
> I think ComponentActionDelegate interface makes sense (and it should be moved
> out of ComponentMigrationHelper into some common location), as it can also be
> used to allow a component to control whether its action gets shown according
to
> its own logic. To use it though we need to guarantee that ToolbarActionsModel
> outlives the objects that access it via the CAD interface. The most
> straightforward way IMO is to have ToolbarActionsModel own these objects (like
> how it does with ComponentMigrationHelper today). This also draws a parallel
to
> how ToolbarActionsBar owns the action instances of various features.
>
> And then there is the issue of how MediaRouterAction obtains a reference to
> MediaRouterActionController to register itself. I suppose it is possible by
> jumping through the ToolbarActions{Bar,Model} hoop, but it seems a bit
> convoluted. I'd rather have MediaRouterActions register their own
> MediaRoutesObserver/IssuesObserver with MediaRouter than share them with
> MediaRouterActionController; I don't think it will incur too much performance
> penalty but it should hopefully makes things simpler.
>
> WDYT?
>
>
https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb...
> File chrome/browser/ui/toolbar/media_router_action_controller.h (right):
>
>
https://codereview.chromium.org/2294973002/diff/20001/chrome/browser/ui/toolb...
> chrome/browser/ui/toolbar/media_router_action_controller.h:35: Profile*
profile,
> media_router::MediaRouter* router);
> On 2016/09/02 22:19:09, mark a. foltz wrote:
> > I'm not a huge fan of the circular dependency between MediaRouter and this
> > class. Derek, is there a different way we can construct observers that are
> > owned by the MediaRouter?
>
> I would suggest not having MediaRouter depend on MediaRouterActionController.
To
> me it doesn't really make sense for MediaRouter to own it, since in general
> MediaRouter is not aware of UI specific logic.
The reason I wanted to make MRAction register with MRActionController is that
when we dynamically create a MRAction to show the icon, it needs to know if
there already exists a local route / issue so that it can update its icon. In my
current implementation the MRActionController keeps track of those two.
I suggested to Devlin making ToolbarActionsModel own an object with some name
like ComponentActionsHelper that owns ComponentMigrationHelper and
MRActionController, but he didn't like the idea of component action specific
additions. And in this case, the issue of how to give MRAction a reference to
MRActionController remains.
MRAction is instantiated by ComponentToolbarActionsFactory, which is a
singleton. So if we can make ComponentToolbarActionsFactory own
MRActionController, then we can pass in a reference to MRActionController when
we instantiate MRAction. In that case it'd probably make sense to make
ToolbarActionsModel own ComponentToolbarActionsFactory, so that
MRActionController doesn't outlive ToolbarActionsModel. the
ComponentToolbarActionsFactory doesn't have to outlive ToolbarActionsModel
either.
If that doesn't work or gets a push back, maybe we should make
MRActionController a KeyedService. In that case it'd be easy for MRAction to
reach it.
Getting close; comments on everything but the unit tests. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/component_migration_helper.cc (right): https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/component_migration_helper.cc:96: if (component_action_pref) { Just so I understand - this code is here to ensure that users with the extension or a pre-existing |component_action_pref| force prefs::kMediaRouterAlwaysShowActionIcon to true. I think this makes sense; we want the "checked" status of the context menu item to reflect that the action will remain in the toolbar. However, since it's a syncable pref, this will effectively enable the action across all of the user's profiles (even if they only had the extension on one of them). I think that's okay as well; I consider this to be an extension of the toolbar model, which is also synced. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/component_migration_helper.cc:99: pref_service_->SetBoolean(prefs::kMediaRouterAlwaysShowActionIcon, Can this only be set if the current value is not already true? I believe this will write prefs to disk. Suggest factoring out a function to check-and-set the pref. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/component_migration_helper.cc:116: if (component_action_id == If the feature is disabled, we should not be creating the action at all, IIUC. I would be okay with leaving the existing pref value for the next time the MR is re-enabled. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/component_migration_helper.cc:151: if (component_action_id == Please check-and-set the pref. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:17: router, MediaRouterFactory::GetApiForBrowserContext(profile) https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:39: MediaRouterActionController::~MediaRouterActionController() {} Will the pref_change_registar_ dtor unregister our callback? https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:63: profile_->GetPrefs()->SetBoolean(prefs::kMediaRouterAlwaysShowActionIcon, Check the implementation of SetBoolean() to see if setting the same value is a no-op. Otherwise, check the current value then set if different. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:87: const std::string& action_id = Super nit: Use a using declaration or type alias instead of a variable allocation. http://en.cppreference.com/w/cpp/language/type_alias https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:105: return GetAlwaysShowActionPref() || has_local_display_route_ || issue_; You're not actually calling anything on |issue_|; could it be a boolean has_issue_ ? https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:13: #include "chrome/browser/extensions/component_migration_helper.h" Can you forward declare ComponentMigrationHelper? https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:15: #include "chrome/browser/media/router/media_router_factory.h" Is this used? Or can you forward declare MediaRouter? https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:17: #include "chrome/browser/profiles/profile.h" Can you forward declare Profile? https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:20: class MediaRouterActionControllerUnitTest; This seems to be unused https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:22: // Controller for MediaRouterAction that determines when to show and hide the Please document threading assumptions, i.e. this class should only be used on the UI thread. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:24: // There should be one instance of this class per profile. Sorry for the churn, but my current thinking is we should bite the bullet and make a ProfileKeyed service that owns the MRAC. That way we can keep a cleaner layering of UI and MR code. e.g. MediaRouterUIService ---(owns)---> MRAC MediaRouterUIService ---(depends)---> ToolbarActionsModel MediaRouterUIService ---(depends)---> MediaRouter For now, it will just have the MRAC, but if there are other aspects to the MR UI model that are profile keyed, they can be owned by that service as well. What do you think Derek? Are there other profile keyed objects that would belong there? https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:30: class Observer { This does not seem to be used (yet). Is there a separate patch to MediaRouterAction coming? Design: Without seeing the code that implements this observer, it's hard to evaluate the API. I might have an API that more closely mirrors the intended states of the MRA; that puts the logic that maps the state of the MR to the state of the MRA into the controller, and means the MRA doesn't have to depend directly on MR objects. E.g. OnWarningIssue() OnSevereIssue() OnIssueResolved() // Some route is connected OnRouteConnected() // All routes are terminated OnRoutesTerminated() https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:38: MediaRouterActionController( Normally |router| and |component_action_delegate| would be looked up from |profile|. If this ctor is for tests-only to inject dependencies, please comment it as such. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:55: // Also updates the presence of the action if necessary. Nit: wrap comment to previous line https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:58: bool HasLocalRoute() const; Should this be part of the Observer API? If this is to initialize the state of the action, you could have this call Observer::OnLocalRouteUpdated (or the equivalent) when an observer is first added. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:61: media_router::Issue* GetIssue() const; Similar comments to |HasLocalRoute()|. https://codereview.chromium.org/2294973002/diff/40001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/2294973002/diff/40001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:1561: 'browser/ui/toolbar/media_router_action_controller.cc', FYI, I think all the .gypi in chrome/ have just been deleted.
See inline: > The reason I wanted to make MRAction register with MRActionController is that > when we dynamically create a MRAction to show the icon, it needs to know if > there already exists a local route / issue so that it can update its icon. In my > current implementation the MRActionController keeps track of those two. > Chatted offline about this. When a new IssuesObserver / MediaRoutesObserver with MediaRouter, MediaRouter should invoke the observer methods with the current issue / routes. That is how MediaRouterAction works today. Since routes are returned asynchronously, I can think of a couple ways to improve this (though outside of scope of this patch): - Cache routes in MediaRouter. When a MediaRoutesObserver is registered, invoke it with cached routes synchronously. - (This is probably better) Add an API in MediaRouter to get the current list of routes synchronously. > I suggested to Devlin making ToolbarActionsModel own an object with some name > like ComponentActionsHelper that owns ComponentMigrationHelper and > MRActionController, but he didn't like the idea of component action specific > additions. And in this case, the issue of how to give MRAction a reference to > MRActionController remains. > > MRAction is instantiated by ComponentToolbarActionsFactory, which is a > singleton. So if we can make ComponentToolbarActionsFactory own > MRActionController, then we can pass in a reference to MRActionController when > we instantiate MRAction. In that case it'd probably make sense to make > ToolbarActionsModel own ComponentToolbarActionsFactory, so that > MRActionController doesn't outlive ToolbarActionsModel. the > ComponentToolbarActionsFactory doesn't have to outlive ToolbarActionsModel > either. > > If that doesn't work or gets a push back, maybe we should make > MRActionController a KeyedService. In that case it'd be easy for MRAction to > reach it.
https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:24: // There should be one instance of this class per profile. On 2016/09/09 17:48:28, mark a. foltz wrote: > Sorry for the churn, but my current thinking is we should bite the bullet and > make a ProfileKeyed service that owns the MRAC. That way we can keep a cleaner > layering of UI and MR code. > > e.g. > > MediaRouterUIService ---(owns)---> MRAC > MediaRouterUIService ---(depends)---> ToolbarActionsModel > MediaRouterUIService ---(depends)---> MediaRouter > > For now, it will just have the MRAC, but if there are other aspects to the MR UI > model that are profile keyed, they can be owned by that service as well. > > What do you think Derek? Are there other profile keyed objects that would > belong there? I think that makes sense given that we face opposition in introducing dependency in ToolbarActionsModel. It is another way to express dependency / lifetime guarantee of TAM and MR, but it does isolate the ToolbarActionsModel keyed service from dependency to MR, which I suppose is beneficial in some ways.
On 2016/09/09 at 17:58:54, imcheng wrote: > See inline: > > > The reason I wanted to make MRAction register with MRActionController is that > > when we dynamically create a MRAction to show the icon, it needs to know if > > there already exists a local route / issue so that it can update its icon. In my > > current implementation the MRActionController keeps track of those two. > > > > Chatted offline about this. When a new IssuesObserver / MediaRoutesObserver with > MediaRouter, MediaRouter should invoke the observer methods with the current > issue / routes. That is how MediaRouterAction works today. Since routes are > returned asynchronously, I can think of a couple ways to improve this (though > outside of scope of this patch): > - Cache routes in MediaRouter. When a MediaRoutesObserver is registered, invoke > it with cached routes synchronously. > - (This is probably better) Add an API in MediaRouter to get the current list > of routes synchronously. I would prefer the former design to adding a polling mechanism to MR: - Getting the current list of routes could be expensive if the cache is not populated. - In the current design, each observer can keep track of its own state as needed. - If we end up with lots of observers having the same route lists, we can consider an API to pull from a shared cache. But it should not call through to the underlying MRPs.
Description was changed from ========== Create MediaRouterActionController This CL is part 1 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController (this CL) 2. Show ephemeral icon for active local routes and issues 3. Show ephemeral icon for open dialog The icon will be shown when the "always show icon" setting is on, or there is a local media route, an unresolved issue, or an open dialog. The MediaRouterActionController will be responsible for telling the ToolbarActionsModel to show/hide the MR icon. MediaRouterActionController will also notify MediaRouterAction (which will be an observer for MediaRouterActionController) when its icon should be updated. These CLs are a redesign of a previous CL [1], which has been reverted for its bugs. [1] https://codereview.chromium.org/2155293002/ BUG=594577 ========== to ========== Create MediaRouterActionController and MediaRouterUIService This CL is part 1 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController (this CL) 2. Show ephemeral icon for active local routes and issues 3. Show ephemeral icon for open dialog The icon will be shown when the "always show icon" setting is on, or there is a local media route, an unresolved issue, or an open dialog. The MediaRouterActionController will be responsible for telling the ToolbarActionsModel to show/hide the MR icon. MediaRouterUIService is a KeyedService (a per-profile instance) that owns MediaRouterActionController. These CLs are a redesign of a previous CL [1], which has been reverted for its bugs. [1] https://codereview.chromium.org/2155293002/ BUG=594577 ==========
Patchset #3 (id:60001) has been deleted
As per our discussion, I've added MediaRouterUIService, a KeyedService that owns MRActionController. The plan is to have ComponentToolbarActionsFactory call Get() on MRUIService to ensure it's instantiated. I realized that prefs::kToolbarMigratedComponentActionStatus and prefs::kMediaRouterAlwaysShowActionIcon were both for whether to show the icon, so I got rid of prefs::kMediaRouterAlwaysShowActionIcon. MRAction can call on ComponentMigrationHelper's methods to get/set prefs::kToolbarMigratedComponentActionStatus, which the MRContexualMenu was already doing before. MRAC can also call on ComponentMigrationHelper methods. I've removed the Observer class for MRAC and reverted to MRAction being an IssuesObserver and MediaRoutesObserver of its own, so that it doesn't have to be aware of MRAC. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:17: router, On 2016/09/09 17:48:28, mark a. foltz wrote: > MediaRouterFactory::GetApiForBrowserContext(profile) Done. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:39: MediaRouterActionController::~MediaRouterActionController() {} On 2016/09/09 17:48:28, mark a. foltz wrote: > Will the pref_change_registar_ dtor unregister our callback? Yes, it does. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:63: profile_->GetPrefs()->SetBoolean(prefs::kMediaRouterAlwaysShowActionIcon, On 2016/09/09 17:48:28, mark a. foltz wrote: > Check the implementation of SetBoolean() to see if setting the same value is a > no-op. Otherwise, check the current value then set if different. Yes they do check for equality before setting it. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:87: const std::string& action_id = On 2016/09/09 17:48:28, mark a. foltz wrote: > Super nit: Use a using declaration or type alias instead of a variable > allocation. > > http://en.cppreference.com/w/cpp/language/type_alias Sorry, I'm not sure what you mean. ComponentToolbarActionsFactory::kMediaRouterActionId is not a type, so it wouldn't apply? https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:105: return GetAlwaysShowActionPref() || has_local_display_route_ || issue_; On 2016/09/09 17:48:28, mark a. foltz wrote: > You're not actually calling anything on |issue_|; could it be a boolean > has_issue_ ? Changed to has_issue_. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:13: #include "chrome/browser/extensions/component_migration_helper.h" On 2016/09/09 17:48:29, mark a. foltz wrote: > Can you forward declare ComponentMigrationHelper? No, because it includes declaration for ComponentActionDelegate. Perhaps ComponentActionDelegate should be in its own file? https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:15: #include "chrome/browser/media/router/media_router_factory.h" On 2016/09/09 17:48:29, mark a. foltz wrote: > Is this used? Or can you forward declare MediaRouter? It's not used here. Removed. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:17: #include "chrome/browser/profiles/profile.h" On 2016/09/09 17:48:29, mark a. foltz wrote: > Can you forward declare Profile? Done. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:20: class MediaRouterActionControllerUnitTest; On 2016/09/09 17:48:29, mark a. foltz wrote: > This seems to be unused Removed. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:22: // Controller for MediaRouterAction that determines when to show and hide the On 2016/09/09 17:48:29, mark a. foltz wrote: > Please document threading assumptions, i.e. this class should only be used on > the UI thread. Done. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:38: MediaRouterActionController( On 2016/09/09 17:48:28, mark a. foltz wrote: > Normally |router| and |component_action_delegate| would be looked up from > |profile|. > If this ctor is for tests-only to inject dependencies, please comment it as > such. Done. https://codereview.chromium.org/2294973002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:55: // Also updates the presence of the action if necessary. On 2016/09/09 17:48:29, mark a. foltz wrote: > Nit: wrap comment to previous line Done. https://codereview.chromium.org/2294973002/diff/40001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/2294973002/diff/40001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:1561: 'browser/ui/toolbar/media_router_action_controller.cc', On 2016/09/09 17:48:29, mark a. foltz wrote: > FYI, I think all the .gypi in chrome/ have just been deleted. Edited BUILD.gn files instead.
LGTM This is much cleaner, thanks. Minor comments on naming and style. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/component_migration_helper.cc (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/component_migration_helper.cc:142: bool ComponentMigrationHelper::GetComponentActionPref( - Please refactor @L80 to call this method. - DCHECK() the success value from GetBoolean as is done above. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/component_migration_helper.cc:148: migration_pref->GetBoolean(component_action_id, &component_action_pref); What does this return if there is no pre-existing value for |component_action_id| in |migration_pref|? https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/BUILD.gn (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/BUILD.gn:38: "media_router_ui_service.cc", File a cleanup bug and add a TODO(crbug.com/XXX) to migrate this service into chrome/browser/ui. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_ui_service_factory.cc (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_ui_service_factory.cc:46: return context; This will mean a separate services gets instantiated for an incognito profile spawned from a regular profile. This seems fine as the media router (which is the same across profiles) should advertise the identical set of routes to observers. I do have a question about pref behavior. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:38: pref_change_registrar_.Init(profile->GetPrefs()); I wonder if changing the pref in incognito affects the pref in the parent profile. If not, do we hide the checkbox? Hmmm. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:69: ComponentToolbarActionsFactory::kMediaRouterActionId; Can you write using ComponentToolbarActionsFactory::kMediaRouterActionId; and kMediaRouterActionId below. (Maybe the compiler will optimize out the pointer you're allocating; unsure.) https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:14: class TestComponentActionDelegate Nit: This class reads like a "fake" to me, so FakeComponentActionDelegate. http://stackoverflow.com/questions/346372/whats-the-difference-between-faking... https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:21: if (action_id == ComponentToolbarActionsFactory::kMediaRouterActionId) { using ComponentToolbarActionsFactory::kMediaRouterActionId at the file level (or declare a file level constant) https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:120: const media_router::Issue fake_issue_; Nit: It's implied that data declared in a test is test data, so not strictly necessary to call it "fake." https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:165: EXPECT_FALSE(ActionExists()); Also test removing route and verify that action no longer exists. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:168: TEST_F(MediaRouterActionControllerUnitTest, ObserveAlwaysShowPrefChange) { Also test changing the pref to false while there is a route/issue to cover one more code path.
https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/component_migration_helper.h (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/component_migration_helper.h:113: void SetComponentActionPref(const std::string& component_action_id, Why is SetComponentActionPref public? I don't see any new callsites introduced in this patch? https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_ui_service.h (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_ui_service.h:29: std::unique_ptr<MediaRouterActionController> action_controller_; nit: This doesn't need to be a unique_ptr. If you are changing this to just the plain type, then GetActionController() can be renamed action_controller(). https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:25: extensions::ComponentMigrationHelper::ComponentActionDelegate* nit: I think we should extract ComponentActionDelegate out of ComponentMigrationHelper, now that it is no longer specific to CMH. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:80: bool MediaRouterActionController::IsActionEnabled() { I think this method makes more sense if it is named ShouldEnableAction() https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:81: return component_migration_helper_->GetComponentActionPref( nit: as a small optimization, we can reorder the statement to check has_local_display_route_ and has_issue_ first. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:29: // Constructor for injecting dependencies in tests. nit: blank line before comment https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:30: MediaRouterActionController( nit: consider moving this ctor to private and adding the test class as friends. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:54: // Returns ture if the Media Router action should be present on the toolbar s/ture/|true| https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:56: bool IsActionEnabled(); const
Patchset #4 (id:100001) has been deleted
Patchset #5 (id:140001) has been deleted
Thanks for the comments! https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/component_migration_helper.cc (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/component_migration_helper.cc:142: bool ComponentMigrationHelper::GetComponentActionPref( On 2016/09/13 18:23:04, mark a. foltz wrote: > - Please refactor @L80 to call this method. > - DCHECK() the success value from GetBoolean as is done above. > Done. DCHECKing here means component actions can only call this when the pref exists for the action, and that should be okay (at least for MRA) since MRUIService wouldn't be instantiated if MR is disabled, and OnFeatureDisabled() (which removes prefs) only gets called when starting up ToolbarActionsModel. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/component_migration_helper.cc:148: migration_pref->GetBoolean(component_action_id, &component_action_pref); On 2016/09/13 18:23:04, mark a. foltz wrote: > What does this return if there is no pre-existing value for > |component_action_id| in |migration_pref|? GetBoolean returns false and not touch |component_action_pref| in that case. https://cs.chromium.org/chromium/src/base/values.cc?sq=package:chromium&rcl=1... https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/component_migration_helper.h (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/component_migration_helper.h:113: void SetComponentActionPref(const std::string& component_action_id, On 2016/09/13 23:13:09, imcheng wrote: > Why is SetComponentActionPref public? I don't see any new callsites introduced > in this patch? It's for MRAction to call, which is in the other patch. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/BUILD.gn (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/BUILD.gn:38: "media_router_ui_service.cc", On 2016/09/13 18:23:05, mark a. foltz wrote: > File a cleanup bug and add a TODO(crbug.com/XXX) to migrate this service into > chrome/browser/ui. Done. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_ui_service.h (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_ui_service.h:29: std::unique_ptr<MediaRouterActionController> action_controller_; On 2016/09/13 23:13:09, imcheng wrote: > nit: This doesn't need to be a unique_ptr. If you are changing this to just the > plain type, then GetActionController() can be renamed action_controller(). Done. Removing GetActionController() for now, as MRAction no longer needs it. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_ui_service_factory.cc (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router_ui_service_factory.cc:46: return context; On 2016/09/13 18:23:05, mark a. foltz wrote: > This will mean a separate services gets instantiated for an incognito profile > spawned from a regular profile. This seems fine as the media router (which is > the same across profiles) should advertise the identical set of routes to > observers. > > I do have a question about pref behavior. ToolbarActionsModel separates normal and incognito profiles, so it probably makes sense to do so here as well. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:25: extensions::ComponentMigrationHelper::ComponentActionDelegate* On 2016/09/13 23:13:10, imcheng wrote: > nit: I think we should extract ComponentActionDelegate out of > ComponentMigrationHelper, now that it is no longer specific to CMH. I'm not sure where that'd go. ComponentMigrationHelper is in c/b/extensions, but ComponentActionDelegate doesn't really have to do with extensions. Maybe c/b/ui/toolbar is better? The extraction would also touch ToolbarActionsModel, etc. Maybe it can happen in its own CL? https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:38: pref_change_registrar_.Init(profile->GetPrefs()); On 2016/09/13 18:23:05, mark a. foltz wrote: > I wonder if changing the pref in incognito affects the pref in the parent > profile. > If not, do we hide the checkbox? Hmmm. Pref changes made while in incognito do persist. Is that what we want? If not, we'd either need a separate pref dictionary for incognito, or not let the icon persist in incognito, either of which would require changes to ComponentMigrationHelper. Or let it share the pref but not show the context menu option, as you mentioned. Showing/hiding of the ephemeral icon is synced in real-time between incognito and normal tabs as well. My understanding is that this happens because the MR instance is shared between incognito and normal profiles. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:69: ComponentToolbarActionsFactory::kMediaRouterActionId; On 2016/09/13 18:23:05, mark a. foltz wrote: > Can you write > > using ComponentToolbarActionsFactory::kMediaRouterActionId; > > and > > kMediaRouterActionId below. > > (Maybe the compiler will optimize out the pointer you're allocating; unsure.) using ComponentToolbarActionsFactory::kMediaRouterActionId; gives a compilation error (can't use "using" for a class member). Making it a file-level const reference in an unnamed namespace. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:80: bool MediaRouterActionController::IsActionEnabled() { On 2016/09/13 23:13:10, imcheng wrote: > I think this method makes more sense if it is named ShouldEnableAction() Renamed. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.cc:81: return component_migration_helper_->GetComponentActionPref( On 2016/09/13 23:13:10, imcheng wrote: > nit: as a small optimization, we can reorder the statement to check > has_local_display_route_ and has_issue_ first. Done. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:29: // Constructor for injecting dependencies in tests. On 2016/09/13 23:13:10, imcheng wrote: > nit: blank line before comment Moving the ctor to private. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:30: MediaRouterActionController( On 2016/09/13 23:13:10, imcheng wrote: > nit: consider moving this ctor to private and adding the test class as friends. Done. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:54: // Returns ture if the Media Router action should be present on the toolbar On 2016/09/13 23:13:10, imcheng wrote: > s/ture/|true| Oops. Replaced. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller.h:56: bool IsActionEnabled(); On 2016/09/13 23:13:10, imcheng wrote: > const Done. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc (right): https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:14: class TestComponentActionDelegate On 2016/09/13 18:23:05, mark a. foltz wrote: > Nit: This class reads like a "fake" to me, so FakeComponentActionDelegate. > > http://stackoverflow.com/questions/346372/whats-the-difference-between-faking... > > Done. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:21: if (action_id == ComponentToolbarActionsFactory::kMediaRouterActionId) { On 2016/09/13 18:23:05, mark a. foltz wrote: > using ComponentToolbarActionsFactory::kMediaRouterActionId at the file level (or > declare a file level constant) using ComponentToolbarActionsFactory::kMediaRouterActionId; gives me "using declaration cannot refer to class member" error. Declared it as a const in an unnamed namespace. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:120: const media_router::Issue fake_issue_; On 2016/09/13 18:23:05, mark a. foltz wrote: > Nit: It's implied that data declared in a test is test data, so not strictly > necessary to call it "fake." Okay. Renaming. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:165: EXPECT_FALSE(ActionExists()); On 2016/09/13 18:23:05, mark a. foltz wrote: > Also test removing route and verify that action no longer exists. Done. https://codereview.chromium.org/2294973002/diff/80001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:168: TEST_F(MediaRouterActionControllerUnitTest, ObserveAlwaysShowPrefChange) { On 2016/09/13 18:23:05, mark a. foltz wrote: > Also test changing the pref to false while there is a route/issue to cover one > more code path. Done.
lgtm
Description was changed from ========== Create MediaRouterActionController and MediaRouterUIService This CL is part 1 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController (this CL) 2. Show ephemeral icon for active local routes and issues 3. Show ephemeral icon for open dialog The icon will be shown when the "always show icon" setting is on, or there is a local media route, an unresolved issue, or an open dialog. The MediaRouterActionController will be responsible for telling the ToolbarActionsModel to show/hide the MR icon. MediaRouterUIService is a KeyedService (a per-profile instance) that owns MediaRouterActionController. These CLs are a redesign of a previous CL [1], which has been reverted for its bugs. [1] https://codereview.chromium.org/2155293002/ BUG=594577 ========== to ========== Create MediaRouterActionController and MediaRouterUIService This CL is part 1 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController (this CL) 2. Show ephemeral icon for active local routes and issues [1] 3. Show ephemeral icon for open dialog The icon will be shown when the "always show icon" setting is on, or there is a local media route, an unresolved issue, or an open dialog. The MediaRouterActionController will be responsible for telling the ToolbarActionsModel to show/hide the MR icon. MediaRouterUIService is a KeyedService (a per-profile instance) that owns MediaRouterActionController. These CLs are a redesign of a previous CL [2], which has been reverted for its bugs. [1] https://codereview.chromium.org/2332693003/ [2] https://codereview.chromium.org/2155293002/ BUG=594577 ==========
takumif@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
+rdcronin@ This is part 1 of ephemeral MR icon redesign. MRActionController calls ComponentMigrationHelper::Get/SetComponentActionPref() and ToolbarActionsModel::Has/Add/RemoveComponentAction(). ComponentToolbarActionsFactory will call MRUIService::Get() to ensure the MRActionController is instantiated. That happens in part 2 [1]. Please take a look, thanks! [1] https://codereview.chromium.org/2332693003/
https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/component_migration_helper.cc (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/component_migration_helper.cc:85: } else if (extension_was_installed) { Note: this is a behavior change, where previously we would check for extension installation even if the pref was false (but existed). Is this change intentional? https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/component_migration_helper.h (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/component_migration_helper.h:114: void SetComponentActionPref(const std::string& component_action_id, nit: this is only used by tests, right? Do we need to make it public (or can we make it public through a ForTesting() wrapper)? https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service.h (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service.h:27: MediaRouterActionController action_controller_; I'm a bit confused about why we have a service that owns a controller, rather than having the controller itself be a keyed service. Is there more functionality that will be in this class in the future? https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service_factory_unittest.cc (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service_factory_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Does this test add any value over generic KeyedService tests? https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:79: if (component_action_delegate_->HasComponentAction(kMediaRouterActionId)) nit: else { if (x) { } } is identical to else if (x) { } https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.h:65: // toolbar. It is a KeyedService and outlives |this|. nit: omit "It is a KeyedService", since that's not relevant to this class. https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:28: if (action_id == kMediaRouterActionId) { When would this not == kMediaRouterActionId?
Thanks for the comments. I'd like to land this CL and part 2 [1] before the feature freeze next Friday, so early reviews would be appreciated. Thank you! [1] https://codereview.chromium.org/2332693003/ https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/component_migration_helper.cc (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/component_migration_helper.cc:85: } else if (extension_was_installed) { On 2016/09/15 21:08:41, Devlin wrote: > Note: this is a behavior change, where previously we would check for extension > installation even if the pref was false (but existed). Is this change > intentional? I believe there's no behavior change. If the pref existed and was false, |component_action_pref| would be false but |has_component_action_pref| should still have been true. https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/component_migration_helper.h (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/component_migration_helper.h:114: void SetComponentActionPref(const std::string& component_action_id, On 2016/09/15 21:08:41, Devlin wrote: > nit: this is only used by tests, right? Do we need to make it public (or can we > make it public through a ForTesting() wrapper)? This will be used by MRAction (http://crrev.com/2332693003) for toggling the "always show" option. https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service.h (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service.h:27: MediaRouterActionController action_controller_; On 2016/09/15 21:08:41, Devlin wrote: > I'm a bit confused about why we have a service that owns a controller, rather > than having the controller itself be a keyed service. Is there more > functionality that will be in this class in the future? Yes, the plan is to be able to add other MR UI related objects that should persist. https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service_factory_unittest.cc (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service_factory_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/09/15 21:08:42, Devlin wrote: > Does this test add any value over generic KeyedService tests? It serves as a sanity check to ensure that the service can be instantiated properly. I couldn't find a test that already does that for us. Is there one? https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:79: if (component_action_delegate_->HasComponentAction(kMediaRouterActionId)) On 2016/09/15 21:08:42, Devlin wrote: > nit: > else { > if (x) { > } > } > > is identical to > > else if (x) { > > } Fixed. https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.h:65: // toolbar. It is a KeyedService and outlives |this|. On 2016/09/15 21:08:42, Devlin wrote: > nit: omit "It is a KeyedService", since that's not relevant to this class. Done. https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:28: if (action_id == kMediaRouterActionId) { On 2016/09/15 21:08:42, Devlin wrote: > When would this not == kMediaRouterActionId? Right now, it should always equal kMediaRouterActionId. But I didn't want to flip the has_media_router_action_ boolean without checking the ID, and I didn't want to DCHECK that the ID is kMediaRouterActionId, either. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/component_migration_helper.cc (right): https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/component_migration_helper.cc:143: migration_pref->GetBoolean(component_action_id, &component_action_pref); I can't DCHECK here because the MRActionController may want to call this before ToolbarActionsModel::OnReady() sets the pref for MRAction. I think it would make sense for this method to return false in that case. The DCHECK here was to detect pref corruption. Is that something we should generally DCHECK for?
Nice test; c/b/ui mostly lg, just minor comments. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:7: #include "chrome/browser/extensions/component_migration_helper.h" nit: remove (redundant with header) https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:9: #include "chrome/browser/profiles/profile.h" nit: remove if kept in header https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:17: const char (&kMediaRouterActionId)[] = nit/q: why not just inline |ComponentToolbarActionsFactory::kMediaRouterActionId| below? Comment on why this is needed. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:20: } nit: "} // namespace" https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.h:8: #include <memory> remove https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.h:9: #include <string> remove https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.h:15: #include "chrome/browser/profiles/profile.h" nit: remove and rely on fwd decl (or remove the fwd decl) https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.h:73: bool has_issue_; optional nit: init bools here instead of in the initializer list. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc (right): https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:16: const char (&kMediaRouterActionId)[] = ditto: inline or comment https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:44: return false; optional nit: consider either: DCHECK_EQ(kMediaRouterActionId, action_id); return has_media_router_action_; or perhaps: return action_id == kMediaRouterActionId && has_media_router_action_; https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:48: bool has_media_router_action_; ditto optional nit: inline = false here; nix initializer list
lgtm with nits https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service_factory_unittest.cc (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service_factory_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/09/16 21:04:05, takumif wrote: > On 2016/09/15 21:08:42, Devlin wrote: > > Does this test add any value over generic KeyedService tests? > > It serves as a sanity check to ensure that the service can be instantiated > properly. I couldn't find a test that already does that for us. Is there one? This service doesn't do anything different from every other keyed service, right? If so, I'd say we should rely on KeyedService tests. It looks like those are a little sparse, so if you wanted to add a few more there, that'd be great (but maybe out of the scope of this CL), but I don't think we should add these type of tests for every keyed service we make. https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:28: if (action_id == kMediaRouterActionId) { On 2016/09/16 21:04:05, takumif wrote: > On 2016/09/15 21:08:42, Devlin wrote: > > When would this not == kMediaRouterActionId? > > Right now, it should always equal kMediaRouterActionId. But I didn't want to > flip the has_media_router_action_ boolean without checking the ID, and I didn't > want to DCHECK that the ID is kMediaRouterActionId, either. ASSERT_EQ(kMediaRouterActionId, action_id);? https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/component_migration_helper.cc (right): https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/component_migration_helper.cc:143: migration_pref->GetBoolean(component_action_id, &component_action_pref); On 2016/09/16 21:04:05, takumif wrote: > I can't DCHECK here because the MRActionController may want to call this before > ToolbarActionsModel::OnReady() sets the pref for MRAction. I think it would make > sense for this method to return false in that case. > > The DCHECK here was to detect pref corruption. Is that something we should > generally DCHECK for? The DCHECK was actually because it was always supposed to be set. The comment "// This shouldn't fail, but can in the cases of pref corruption" was explaining why it couldn't instead be a CHECK. If this can legitimately happen, this is fine. But it's worth documenting. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service_factory.cc (right): https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service_factory.cc:21: // GetServiceForBrowserContext returns a KeyedService hence the static_cast<> nit: I don't think this comment adds any additional information that looking at the method signature wouldn't. I'd remove it. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:79: ->HasComponentAction(kMediaRouterActionId)) { was this git cl format'd?
Patchset #7 (id:200001) has been deleted
Patchset #8 (id:240001) has been deleted
Patchset #7 (id:220001) has been deleted
Thank you so much for reviewing! https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:28: if (action_id == kMediaRouterActionId) { On 2016/09/16 22:37:32, Devlin wrote: > On 2016/09/16 21:04:05, takumif wrote: > > On 2016/09/15 21:08:42, Devlin wrote: > > > When would this not == kMediaRouterActionId? > > > > Right now, it should always equal kMediaRouterActionId. But I didn't want to > > flip the has_media_router_action_ boolean without checking the ID, and I > didn't > > want to DCHECK that the ID is kMediaRouterActionId, either. > > ASSERT_EQ(kMediaRouterActionId, action_id);? It just feels a bit odd to me to enforce something that the actual ToolbarActionsModel wouldn't. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/component_migration_helper.cc (right): https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/component_migration_helper.cc:143: migration_pref->GetBoolean(component_action_id, &component_action_pref); On 2016/09/16 22:37:32, Devlin wrote: > On 2016/09/16 21:04:05, takumif wrote: > > I can't DCHECK here because the MRActionController may want to call this > before > > ToolbarActionsModel::OnReady() sets the pref for MRAction. I think it would > make > > sense for this method to return false in that case. > > > > The DCHECK here was to detect pref corruption. Is that something we should > > generally DCHECK for? > > The DCHECK was actually because it was always supposed to be set. The comment > "// This shouldn't fail, but can in the cases of pref corruption" was explaining > why it couldn't instead be a CHECK. > > If this can legitimately happen, this is fine. But it's worth documenting. I see. Yes, it happens when MRActionController observes media route changes before ToolbarActionsModel is ready. Added a comment saying that GetBoolean() might not modify the boolean. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service_factory.cc (right): https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service_factory.cc:21: // GetServiceForBrowserContext returns a KeyedService hence the static_cast<> On 2016/09/16 22:37:32, Devlin wrote: > nit: I don't think this comment adds any additional information that looking at > the method signature wouldn't. I'd remove it. Done. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:7: #include "chrome/browser/extensions/component_migration_helper.h" On 2016/09/16 22:22:25, msw vacation 9-19 and 9-20 wrote: > nit: remove (redundant with header) Done. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:9: #include "chrome/browser/profiles/profile.h" On 2016/09/16 22:22:25, msw vacation 9-19 and 9-20 wrote: > nit: remove if kept in header Done. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:17: const char (&kMediaRouterActionId)[] = On 2016/09/16 22:22:25, msw vacation 9-19 and 9-20 wrote: > nit/q: why not just inline > |ComponentToolbarActionsFactory::kMediaRouterActionId| below? Comment on why > this is needed. This is just to give it a shorter name. Is there a better way to do this? (By inline, do you mean to use the full variable name everywhere?) This line also doesn't pass cpplint, and I'm not sure if it's worth // NOLINT. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:20: } On 2016/09/16 22:22:25, msw vacation 9-19 and 9-20 wrote: > nit: "} // namespace" Done. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:79: ->HasComponentAction(kMediaRouterActionId)) { On 2016/09/16 22:37:32, Devlin wrote: > was this git cl format'd? No. Will use git cl format. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.h (right): https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.h:8: #include <memory> On 2016/09/16 22:22:26, msw vacation 9-19 and 9-20 wrote: > remove Done. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.h:9: #include <string> On 2016/09/16 22:22:26, msw vacation 9-19 and 9-20 wrote: > remove Done. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.h:15: #include "chrome/browser/profiles/profile.h" On 2016/09/16 22:22:25, msw vacation 9-19 and 9-20 wrote: > nit: remove and rely on fwd decl (or remove the fwd decl) Removed the forward declaration. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.h:73: bool has_issue_; On 2016/09/16 22:22:26, msw vacation 9-19 and 9-20 wrote: > optional nit: init bools here instead of in the initializer list. Done. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc (right): https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:16: const char (&kMediaRouterActionId)[] = On 2016/09/16 22:22:26, msw vacation 9-19 and 9-20 wrote: > ditto: inline or comment (repeated) This is just to give it a shorter name. Is there a better way to do this? (By inline, do you mean to use the full variable name everywhere?) This line also doesn't pass cpplint, and I'm not sure if it's worth // NOLINT. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:44: return false; On 2016/09/16 22:22:26, msw vacation 9-19 and 9-20 wrote: > optional nit: consider either: > DCHECK_EQ(kMediaRouterActionId, action_id); > return has_media_router_action_; > or perhaps: > return action_id == kMediaRouterActionId && has_media_router_action_; Done. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:48: bool has_media_router_action_; On 2016/09/16 22:22:26, msw vacation 9-19 and 9-20 wrote: > ditto optional nit: inline = false here; nix initializer list Done.
https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:17: const char (&kMediaRouterActionId)[] = On 2016/09/17 00:24:30, takumif wrote: > On 2016/09/16 22:22:25, msw vacation 9-19 and 9-20 wrote: > > nit/q: why not just inline > > |ComponentToolbarActionsFactory::kMediaRouterActionId| below? Comment on why > > this is needed. > > This is just to give it a shorter name. Is there a better way to do this? (By > inline, do you mean to use the full variable name everywhere?) This line also > doesn't pass cpplint, and I'm not sure if it's worth // NOLINT. I'd just use the full name everywhere, or re-use / add a helper like MediaRouterAction::GetId(). Perhaps someone else knows a better pattern to alias a const char array...
https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:28: if (action_id == kMediaRouterActionId) { On 2016/09/17 00:24:29, takumif wrote: > On 2016/09/16 22:37:32, Devlin wrote: > > On 2016/09/16 21:04:05, takumif wrote: > > > On 2016/09/15 21:08:42, Devlin wrote: > > > > When would this not == kMediaRouterActionId? > > > > > > Right now, it should always equal kMediaRouterActionId. But I didn't want to > > > flip the has_media_router_action_ boolean without checking the ID, and I > > didn't > > > want to DCHECK that the ID is kMediaRouterActionId, either. > > > > ASSERT_EQ(kMediaRouterActionId, action_id);? > > It just feels a bit odd to me to enforce something that the actual > ToolbarActionsModel wouldn't. You should enforce it in the test, because we wouldn't expect any other calls here, right? In fact, since this is the only component action anywhere (especially in this test), passing any other id could be indicative of a failure.
https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc (right): https://codereview.chromium.org/2294973002/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:28: if (action_id == kMediaRouterActionId) { On 2016/09/17 00:37:31, Devlin wrote: > On 2016/09/17 00:24:29, takumif wrote: > > On 2016/09/16 22:37:32, Devlin wrote: > > > On 2016/09/16 21:04:05, takumif wrote: > > > > On 2016/09/15 21:08:42, Devlin wrote: > > > > > When would this not == kMediaRouterActionId? > > > > > > > > Right now, it should always equal kMediaRouterActionId. But I didn't want > to > > > > flip the has_media_router_action_ boolean without checking the ID, and I > > > didn't > > > > want to DCHECK that the ID is kMediaRouterActionId, either. > > > > > > ASSERT_EQ(kMediaRouterActionId, action_id);? > > > > It just feels a bit odd to me to enforce something that the actual > > ToolbarActionsModel wouldn't. > > You should enforce it in the test, because we wouldn't expect any other calls > here, right? In fact, since this is the only component action anywhere > (especially in this test), passing any other id could be indicative of a > failure. Makes sense. Using EXPECT_EQ. https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_controller.cc (right): https://codereview.chromium.org/2294973002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_controller.cc:17: const char (&kMediaRouterActionId)[] = On 2016/09/17 00:37:11, msw vacation 9-19 and 9-20 wrote: > On 2016/09/17 00:24:30, takumif wrote: > > On 2016/09/16 22:22:25, msw vacation 9-19 and 9-20 wrote: > > > nit/q: why not just inline > > > |ComponentToolbarActionsFactory::kMediaRouterActionId| below? Comment on why > > > this is needed. > > > > This is just to give it a shorter name. Is there a better way to do this? (By > > inline, do you mean to use the full variable name everywhere?) This line also > > doesn't pass cpplint, and I'm not sure if it's worth // NOLINT. > > I'd just use the full name everywhere, or re-use / add a helper like > MediaRouterAction::GetId(). Perhaps someone else knows a better pattern to alias > a const char array... Using the full name.
lgtm
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #9 (id:300001) has been deleted
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by takumif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from anthonyvd@chromium.org, mfoltz@chromium.org, imcheng@chromium.org, rdevlin.cronin@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2294973002/#ps320001 (title: "Modify BUILD.gn files, rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Create MediaRouterActionController and MediaRouterUIService This CL is part 1 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController (this CL) 2. Show ephemeral icon for active local routes and issues [1] 3. Show ephemeral icon for open dialog The icon will be shown when the "always show icon" setting is on, or there is a local media route, an unresolved issue, or an open dialog. The MediaRouterActionController will be responsible for telling the ToolbarActionsModel to show/hide the MR icon. MediaRouterUIService is a KeyedService (a per-profile instance) that owns MediaRouterActionController. These CLs are a redesign of a previous CL [2], which has been reverted for its bugs. [1] https://codereview.chromium.org/2332693003/ [2] https://codereview.chromium.org/2155293002/ BUG=594577 ========== to ========== Create MediaRouterActionController and MediaRouterUIService This CL is part 1 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController (this CL) 2. Show ephemeral icon for active local routes and issues [1] 3. Show ephemeral icon for open dialog The icon will be shown when the "always show icon" setting is on, or there is a local media route, an unresolved issue, or an open dialog. The MediaRouterActionController will be responsible for telling the ToolbarActionsModel to show/hide the MR icon. MediaRouterUIService is a KeyedService (a per-profile instance) that owns MediaRouterActionController. These CLs are a redesign of a previous CL [2], which has been reverted for its bugs. [1] https://codereview.chromium.org/2332693003/ [2] https://codereview.chromium.org/2155293002/ BUG=594577 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Create MediaRouterActionController and MediaRouterUIService This CL is part 1 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController (this CL) 2. Show ephemeral icon for active local routes and issues [1] 3. Show ephemeral icon for open dialog The icon will be shown when the "always show icon" setting is on, or there is a local media route, an unresolved issue, or an open dialog. The MediaRouterActionController will be responsible for telling the ToolbarActionsModel to show/hide the MR icon. MediaRouterUIService is a KeyedService (a per-profile instance) that owns MediaRouterActionController. These CLs are a redesign of a previous CL [2], which has been reverted for its bugs. [1] https://codereview.chromium.org/2332693003/ [2] https://codereview.chromium.org/2155293002/ BUG=594577 ========== to ========== Create MediaRouterActionController and MediaRouterUIService This CL is part 1 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController (this CL) 2. Show ephemeral icon for active local routes and issues [1] 3. Show ephemeral icon for open dialog The icon will be shown when the "always show icon" setting is on, or there is a local media route, an unresolved issue, or an open dialog. The MediaRouterActionController will be responsible for telling the ToolbarActionsModel to show/hide the MR icon. MediaRouterUIService is a KeyedService (a per-profile instance) that owns MediaRouterActionController. These CLs are a redesign of a previous CL [2], which has been reverted for its bugs. [1] https://codereview.chromium.org/2332693003/ [2] https://codereview.chromium.org/2155293002/ BUG=594577 Committed: https://crrev.com/be86872a218127fdf92001a1f69e044239a058b4 Cr-Commit-Position: refs/heads/master@{#419581} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/be86872a218127fdf92001a1f69e044239a058b4 Cr-Commit-Position: refs/heads/master@{#419581} |
