Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2332693003:
Show media router toolbar icon ephemerally for active local routes and issues (Closed)
DescriptionShow media router toolbar icon ephemerally for active local routes and issues
This CL is part 2 of 3 CLs to implement ephemeral toolbar icon for media router:
1. Implement MediaRouterActionController [1]
2. Show ephemeral icon for active local routes and issues (this CL)
3. Show ephemeral icon for open dialog
These CLs are a redesign of a previous CL [2], which has been reverted for its bugs. Since the contents of this CL are similar to those of [2], the first patch in this CL is from [2] for comparison.
This CL makes the media router toolbar icon show temporarily whenever there is an active local media route or an issue.
This CL also replaces the "Remove icon" option in the action context menu with "Always show icon" which can be checked. When this option is unchecked, the icon is neither on the toolbar nor in the overflow menu, and is only shown ephemerally in the situations mentioned above.
We replace the word "popup" with "dialog" wherever possible in MediaRouterAction, to stay consistent with the rest of Media Router files.
We also change the default position of the component actions on the toolbar from the leftmost to the rightmost (same behavior as extension actions).
Major differences between the original CL [2] and this CL is that now the MediaRouterAction is no longer responsible for determining whether it should stay shown or be hidden. Instead, showing/hiding of the icon is done by the MediaRouterActionController. Also the unit tests for the ephemeral icon has been moved to the unit tests for MediaRouterActionController (in [1]). MediaRouterDialogController-related changes will be in part 3.
[1] https://codereview.chromium.org/2294973002/
[2] https://codereview.chromium.org/2155293002/
BUG=594577
Committed: https://crrev.com/b22235a16193be6cfa6e199a5c7a2427a7b505e6
Cr-Commit-Position: refs/heads/master@{#420217}
Patch Set 1 : CL 2155293002 (mod rebase merge conflicts) #Patch Set 2 #
Total comments: 30
Patch Set 3 : Address Devlin's comments #
Total comments: 28
Patch Set 4 : Address Derek and Mike's comments #
Total comments: 1
Patch Set 5 : Modify ToolbarActionsModel #
Total comments: 9
Patch Set 6 : Address Devlin's comments #
Total comments: 6
Patch Set 7 : Address Mike's comments #
Total comments: 8
Patch Set 8 : Modify MediaRouterContextualMenu #Patch Set 9 : Free memory in MRUIServiceFactoryUnitTest #
Total comments: 2
Patch Set 10 : Address Derek's comment #Messages
Total messages: 72 (43 generated)
Description was changed from ========== CL 2155293002 BUG= ========== to ========== Show media router toolbar icon ephemerally for active local routes and issues This CL is part 2 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController [1] 2. Show ephemeral icon for active local routes and issues (this CL) 3. Show ephemeral icon for open dialog These CLs are a redesign of a previous CL [2], which has been reverted for its bugs. Since the contents of this CL are similar to those of [2], the first patch in this CL is from [2] for comparison. This CL makes the media router toolbar icon show temporarily whenever there is an active local media route or an issue. This CL also replaces the "Remove icon" option in the action context menu with "Always show icon" which can be checked. When this option is unchecked, the icon is neither on the toolbar nor in the overflow menu, and is only shown ephemerally in the situations mentioned above. We replace the word "popup" with "dialog" wherever possible in MediaRouterAction, to stay consistent with the rest of Media Router files. We also change the default position of the component actions on the toolbar from the leftmost to the rightmost (same behavior as extension actions). Major differences between the original CL [2] and this CL is that now the MediaRouterAction is no longer responsible for determining whether it should stay shown or be hidden. Instead, showing/hiding of the icon is done by the MediaRouterActionController. Also the unit tests for the ephemeral icon has been moved to the unit tests for MediaRouterActionController (in [1]). MediaRouterDialogController-related changes will be in part 3. [1] https://codereview.chromium.org/2294973002/ [2] https://codereview.chromium.org/2155293002/ BUG=594577 ==========
Patchset #2 (id:20001) has been deleted
takumif@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org
This is the CL part 2 for implementing the ephemeral toolbar icon. This instantiates MRActionController and updates the behavior of MRAction/ContexualMenu. Please take a look, thanks!
mfoltz@chromium.org changed reviewers: + apacible@chromium.org - mfoltz@chromium.org
takumif@chromium.org changed reviewers: + mfoltz@chromium.org, rdevlin.cronin@chromium.org - apacible@chromium.org
takumif@chromium.org changed reviewers: + grt@chromium.org, msw@chromium.org
mfoltz@chromium.org changed reviewers: - mfoltz@chromium.org
takumif@chromium.org changed reviewers: + mfoltz@chromium.org
+imcheng@ for MR files +mfoltz@ for MR files +rdcronin@ for toolbar files +msw@ for toolbar files +grt@ for chrome_command_ids.h Please take a look, thank you!
my rubberstamp from 2155293002 applies equally here; lgtm for chrome/app/chrome_command_ids.h.
https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:101: media_router::MediaRouterUIService::Get(profile); Would this same effect be achieved by KeyedServiceBasefactory::ServiceIsCreatedWithContext()? https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:55: virtual void InitializeComponentActionControllers(Profile* profile); add function comment https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.h:83: void SetAlwaysShowActionPref(bool always_show); Is this used?
I'd like to be able to land this by the feature freeze next Friday, so early reviews would be appreciated. Please take a look, thank you! https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:101: media_router::MediaRouterUIService::Get(profile); On 2016/09/15 21:19:17, Devlin wrote: > Would this same effect be achieved by > KeyedServiceBasefactory::ServiceIsCreatedWithContext()? Yes, using ServiceIsCreatedWithContext() (didn't know about it!) and ServiceIsNULLWhileTesting() instead would work. I'll put those in the MRUIServiceFactory once part 1 lands and that file gets created (I can't put it in that CL since we can't instantiate MRUIService until this CL lands). https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:55: virtual void InitializeComponentActionControllers(Profile* profile); On 2016/09/15 21:19:17, Devlin wrote: > add function comment Replacing this with ServiceIsCreatedWithBrowserContext(). https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.h:83: void SetAlwaysShowActionPref(bool always_show); On 2016/09/15 21:19:17, Devlin wrote: > Is this used? Yes, by the ContextualMenu.
I have some questions about how the different objects interact with MediaRouterAction for the ephemeral / preference usecase. https://codereview.chromium.org/2332693003/diff/40001/chrome/app/media_router... File chrome/app/media_router_strings.grdp (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/app/media_router... chrome/app/media_router_strings.grdp:69: <message name="IDS_MEDIA_ROUTER_ALWAYS_SHOW_TOOLBAR_ACTION" desc="Title of a menu item which, on click, toggles whether or not the Media Router action icon is always shown. When toggled off, the icon is shown only when the Media Router dialog is open or if there is an active local Media Route."> or if there is an issue. https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:55: virtual void InitializeComponentActionControllers(Profile* profile); This doesn't need to be virtual unless you are overriding it in the mock version? https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:80: // Update the button in case the pressed state is out of sync with dialog nit: blank line before comments https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:83: if (GetAlwaysShowActionPref()) question: So if I added the icon by preference (i.e., not ephemerally) then OnToolbarActionsBarAnimationEnded won't get called, hence the need to call UpdateDialogState here? Why would that be the case? https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:190: if (GetMediaRouterDialogController()->IsShowingMediaRouterDialog()) Is it possible for this to return false, and do we need to call OnDialogHidden() in that case? (Or just call UpdateDialogState() here)? https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:198: delegate_->OnPopupClosed(); Will |has_dialog_| always be in sync with |delegate_|'s state? https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:244: DCHECK(delegate_); nit: this DCHECK not needed as the methods that you call already have them. https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:55: content::WebContents* new_contents, nit: please fix indent https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:69: controller_->SetMediaRouterAction(GetWeakPtr()); Is this needed if you don't call SetDelegate a second time? See comment below. https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:344: base::AutoReset<bool> disable_animations( Please add comments on why this is needed? https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:364: action()->SetDelegate(mock_delegate.get()); You changed the test to call SetDelegate() during SetUp(). Is this still needed? https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:102: MediaRouterContextualMenu menu(browser(), action()); Could you add a test for the new option? https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:34: MediaRouterUIBrowserTest() {} : action_controller_(nullptr) https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:213: return ToolbarActionsModel::Get(browser_->profile()) I feel MediaRouterContextualMenu has to jump through several hoops to read/write the pref... I wonder if we can just have MediaRouterContextualMenu talk to ComponentMigrationHelper directly? https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_contextual_menu.h (right): https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_contextual_menu.h:37: Browser* browser_; Browser* const https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_contextual_menu.h:39: MediaRouterAction* action_; MediaRouterAction* const
Mostly lg; just minor comments/nits. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:7: #include <string> nit: maybe not needed (included in header) https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:83: if (GetAlwaysShowActionPref()) nit/q: would it be safe to just always call UpdateDialogState here? (if so, inline UpdateDialogState in RegisterWithDialogController?) https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:197: DCHECK(delegate_); nit: no need to dcheck immediately before deref https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:205: DCHECK(delegate_); nit: no need to dcheck immediately before deref https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:10: #include "base/command_line.h" nit: remove (unused) https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:54: void ActiveTabChanged(content::WebContents* old_contents, nit: comment "// MediaRouterAction:" https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:55: content::WebContents* new_contents, nit: fix indent (git cl format) https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:58: // This would be null if |controller_| hasn't been set. Is this override event necessary? It looks like MediaRouterAction::ActiveTabChanged / MediaRouterAction::UpdatePopupState handles a null value returned from GetMediaRouterDialogController() by just bailing. Seems like we don't need to check this any earlier and thus don't need this override. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:73: MediaRouterDialogControllerImpl* GetMediaRouterDialogController() optional ditto nit: comment "// MediaRouterAction:" and remove the blank line before GetPlatformDelegate() https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:137: MediaRouterActionController* action_controller_; nit: = nullptr; https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:167: EphemeralToolbarIcon) { nit: fits on line above (git cl format)
Patchset #4 (id:80001) has been deleted
Thank you for the comments. Please take a look, thanks! https://codereview.chromium.org/2332693003/diff/40001/chrome/app/media_router... File chrome/app/media_router_strings.grdp (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/app/media_router... chrome/app/media_router_strings.grdp:69: <message name="IDS_MEDIA_ROUTER_ALWAYS_SHOW_TOOLBAR_ACTION" desc="Title of a menu item which, on click, toggles whether or not the Media Router action icon is always shown. When toggled off, the icon is shown only when the Media Router dialog is open or if there is an active local Media Route."> On 2016/09/16 21:47:48, imcheng wrote: > or if there is an issue. Changed. https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:83: if (GetAlwaysShowActionPref()) On 2016/09/16 21:47:48, imcheng wrote: > question: So if I added the icon by preference (i.e., not ephemerally) then > OnToolbarActionsBarAnimationEnded won't get called, hence the need to call > UpdateDialogState here? Why would that be the case? Actually, OnToolbarActionsBarAnimationEnded, OnActiveTabCahged, and the calls from MRDialogControllerImpl cover all the cases, so this is not necessary. Removing. https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:190: if (GetMediaRouterDialogController()->IsShowingMediaRouterDialog()) On 2016/09/16 21:47:48, imcheng wrote: > Is it possible for this to return false, and do we need to call OnDialogHidden() > in that case? (Or just call UpdateDialogState() here)? This can be false in cases such as receiving an issue while the dialog is not open. This callback's purpose is to depress the newly created action, and we don't have to worry about calling OnDialogHidden() because it comes un-depressed. https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:198: delegate_->OnPopupClosed(); On 2016/09/16 21:47:49, imcheng wrote: > Will |has_dialog_| always be in sync with |delegate_|'s state? It should be. We only call OnPopupShown/Closed() when we change |has_dialog_|. https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:244: DCHECK(delegate_); On 2016/09/16 21:47:49, imcheng wrote: > nit: this DCHECK not needed as the methods that you call already have them. Removed. https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:55: content::WebContents* new_contents, On 2016/09/16 21:47:49, imcheng wrote: > nit: please fix indent Done. https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:69: controller_->SetMediaRouterAction(GetWeakPtr()); On 2016/09/16 21:47:49, imcheng wrote: > Is this needed if you don't call SetDelegate a second time? See comment below. No, this is not necessary. Removing. https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:344: base::AutoReset<bool> disable_animations( On 2016/09/16 21:47:49, imcheng wrote: > Please add comments on why this is needed? This is no longer necessary. Removed. https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:364: action()->SetDelegate(mock_delegate.get()); On 2016/09/16 21:47:49, imcheng wrote: > You changed the test to call SetDelegate() during SetUp(). Is this still needed? Removed the first call. https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:102: MediaRouterContextualMenu menu(browser(), action()); On 2016/09/16 21:47:49, imcheng wrote: > Could you add a test for the new option? Added a test. https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/2332693003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:34: MediaRouterUIBrowserTest() {} On 2016/09/16 21:47:49, imcheng wrote: > : action_controller_(nullptr) Set to nullptr inline below. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:7: #include <string> On 2016/09/16 23:02:43, msw vacation 9-19 and 9-20 wrote: > nit: maybe not needed (included in header) Removed. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:83: if (GetAlwaysShowActionPref()) On 2016/09/16 23:02:43, msw vacation 9-19 and 9-20 wrote: > nit/q: would it be safe to just always call UpdateDialogState here? > (if so, inline UpdateDialogState in RegisterWithDialogController?) Removing UpdateDialogState as it's no longer necessary. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:197: DCHECK(delegate_); On 2016/09/16 23:02:43, msw vacation 9-19 and 9-20 wrote: > nit: no need to dcheck immediately before deref Removed. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:205: DCHECK(delegate_); On 2016/09/16 23:02:43, msw vacation 9-19 and 9-20 wrote: > nit: no need to dcheck immediately before deref Removed. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:213: return ToolbarActionsModel::Get(browser_->profile()) On 2016/09/16 21:47:49, imcheng wrote: > I feel MediaRouterContextualMenu has to jump through several hoops to read/write > the pref... I wonder if we can just have MediaRouterContextualMenu talk to > ComponentMigrationHelper directly? Moved the methods to MediaRouterContextualMenu. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:10: #include "base/command_line.h" On 2016/09/16 23:02:43, msw vacation 9-19 and 9-20 wrote: > nit: remove (unused) Done. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:54: void ActiveTabChanged(content::WebContents* old_contents, On 2016/09/16 23:02:43, msw vacation 9-19 and 9-20 wrote: > nit: comment "// MediaRouterAction:" Done. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:55: content::WebContents* new_contents, On 2016/09/16 23:02:43, msw vacation 9-19 and 9-20 wrote: > nit: fix indent (git cl format) Done. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:58: // This would be null if |controller_| hasn't been set. On 2016/09/16 23:02:43, msw vacation 9-19 and 9-20 wrote: > Is this override event necessary? It looks like > MediaRouterAction::ActiveTabChanged / MediaRouterAction::UpdatePopupState > handles a null value returned from GetMediaRouterDialogController() by just > bailing. Seems like we don't need to check this any earlier and thus don't need > this override. UpdatePopupState() actually uses the return value of GetMediaRouterDialogController() without checking for nullptr, so it's necessary. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:73: MediaRouterDialogControllerImpl* GetMediaRouterDialogController() On 2016/09/16 23:02:43, msw vacation 9-19 and 9-20 wrote: > optional ditto nit: comment "// MediaRouterAction:" and remove the blank line > before GetPlatformDelegate() Done. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_contextual_menu.h (right): https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_contextual_menu.h:37: Browser* browser_; On 2016/09/16 21:47:49, imcheng wrote: > Browser* const Done. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_contextual_menu.h:39: MediaRouterAction* action_; On 2016/09/16 21:47:49, imcheng wrote: > MediaRouterAction* const Removed action_. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:137: MediaRouterActionController* action_controller_; On 2016/09/16 23:02:43, msw vacation 9-19 and 9-20 wrote: > nit: = nullptr; Done. https://codereview.chromium.org/2332693003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:167: EphemeralToolbarIcon) { On 2016/09/16 23:02:43, msw vacation 9-19 and 9-20 wrote: > nit: fits on line above (git cl format) Done. https://codereview.chromium.org/2332693003/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service.cc (right): https://codereview.chromium.org/2332693003/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service.cc:14: : action_controller_(profile) {} Cannot DCHECK here because it makes MediaRouterDisabledPolicyTest.MediaRouterDisabled fail.
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right): https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/toolbar_actions_model.cc:279: if (use_redesign_) { There were some browsertest failures due to DCHECK(use_redesign_) failing in ToolbarActionsModel. This is caused by MRActionController calling ToolbarActionsModel::HasComponentAction() as a result of a pref change by ComponentToolbarActionsFactory::HandleComponentMigrations(). I think putting this here is better than setting extensions::FeatureSwitch::extension_action_redesign() in all the failing tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service_factory.cc (right): https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service_factory.cc:36: use_in_tests = true; Drive-by: this is a little unconventional (and is also never reset, which is bad for globals in unittests). What about using SetTestingFactoryAndUse() instead? https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc (right): https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:129: // Whether the option is checked should refrect the pref. s/refrect/reflect? https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right): https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/toolbar_actions_model.cc:279: if (use_redesign_) { On 2016/09/20 18:56:34, takumif wrote: > There were some browsertest failures due to DCHECK(use_redesign_) failing in > ToolbarActionsModel. This is caused by MRActionController calling > ToolbarActionsModel::HasComponentAction() as a result of a pref change by > ComponentToolbarActionsFactory::HandleComponentMigrations(). > > I think putting this here is better than setting > extensions::FeatureSwitch::extension_action_redesign() in all the failing tests. Overall, this seems reasonable, but which tests were failing? extension_action_redesign should default to true, unless the test is overriding it... https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:130: ToolbarActionView* toolbar_action_view_; nit: init these members, too?
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.
Thanks for the comments! https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service_factory.cc (right): https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service_factory.cc:36: use_in_tests = true; On 2016/09/20 21:55:48, Devlin wrote: > Drive-by: this is a little unconventional (and is also never reset, which is bad > for globals in unittests). What about using SetTestingFactoryAndUse() instead? Changed to calling BuildServiceInstanceFor() directly. https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc (right): https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:129: // Whether the option is checked should refrect the pref. On 2016/09/20 21:55:48, Devlin wrote: > s/refrect/reflect? Oops. Fixed. https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right): https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/toolbar_actions_model.cc:279: if (use_redesign_) { On 2016/09/20 21:55:48, Devlin wrote: > On 2016/09/20 18:56:34, takumif wrote: > > There were some browsertest failures due to DCHECK(use_redesign_) failing in > > ToolbarActionsModel. This is caused by MRActionController calling > > ToolbarActionsModel::HasComponentAction() as a result of a pref change by > > ComponentToolbarActionsFactory::HandleComponentMigrations(). > > > > I think putting this here is better than setting > > extensions::FeatureSwitch::extension_action_redesign() in all the failing > tests. > > Overall, this seems reasonable, but which tests were failing? > extension_action_redesign should default to true, unless the test is overriding > it... This shows the tests that were failing. They seem to be legacy tests: https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel... https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/2332693003/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:130: ToolbarActionView* toolbar_action_view_; On 2016/09/20 21:55:48, Devlin wrote: > nit: init these members, too? Done.
c/b/ui/ lgtm with minor nits https://codereview.chromium.org/2332693003/diff/140001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2332693003/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:145: DCHECK(delegate_); nit: no need to dcheck right before deref https://codereview.chromium.org/2332693003/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:178: // Depress the action if the dialog is shown. nit: just call UpdateDialogState()? https://codereview.chromium.org/2332693003/diff/140001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc (right): https://codereview.chromium.org/2332693003/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:123: std::unique_ptr<ToolbarActionsModel> toolbar_actions_model( nit: just make a stack local instance and remove #include <memory>? ToolbarActionsModel toolbar_actions_model(browser()->profile(), nullptr); MediaRouterContextualMenu menu(browser(), &toolbar_actions_model); ...
Thank you for reviewing! https://codereview.chromium.org/2332693003/diff/140001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2332693003/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:145: DCHECK(delegate_); On 2016/09/21 15:40:59, msw wrote: > nit: no need to dcheck right before deref Removed. https://codereview.chromium.org/2332693003/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:178: // Depress the action if the dialog is shown. On 2016/09/21 15:40:59, msw wrote: > nit: just call UpdateDialogState()? Done. https://codereview.chromium.org/2332693003/diff/140001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc (right): https://codereview.chromium.org/2332693003/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:123: std::unique_ptr<ToolbarActionsModel> toolbar_actions_model( On 2016/09/21 15:40:59, msw wrote: > nit: just make a stack local instance and remove #include <memory>? > ToolbarActionsModel toolbar_actions_model(browser()->profile(), nullptr); > MediaRouterContextualMenu menu(browser(), &toolbar_actions_model); > ... Done.
lgtm https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:42: toolbar_actions_model_(toolbar_actions_model) { It looks like we only ever use this for the ComponentMigrationHelper - could we just use that instead?
lgtm
https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:42: toolbar_actions_model_(toolbar_actions_model) { On 2016/09/21 17:15:12, Devlin wrote: > It looks like we only ever use this for the ComponentMigrationHelper - could we > just use that instead? This is to be able to inject a non-NULL ToolbarActionsModel in test. I would have used TestingProfile::Builder::AddTestingFactory(), except the profile is already instantiated in MediaRouterContextualMenuUnitTest.
https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:42: toolbar_actions_model_(toolbar_actions_model) { On 2016/09/21 17:27:15, takumif wrote: > On 2016/09/21 17:15:12, Devlin wrote: > > It looks like we only ever use this for the ComponentMigrationHelper - could > we > > just use that instead? > > This is to be able to inject a non-NULL ToolbarActionsModel in test. I would > have used TestingProfile::Builder::AddTestingFactory(), except the profile is > already instantiated in MediaRouterContextualMenuUnitTest. What uses it that needs it to be non-null, rather than passing in the non-null ComponentMigrationHelper? It looks like the helper is all that's used here.
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...
https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:42: toolbar_actions_model_(toolbar_actions_model) { On 2016/09/21 17:32:57, Devlin wrote: > On 2016/09/21 17:27:15, takumif wrote: > > On 2016/09/21 17:15:12, Devlin wrote: > > > It looks like we only ever use this for the ComponentMigrationHelper - could > > we > > > just use that instead? > > > > This is to be able to inject a non-NULL ToolbarActionsModel in test. I would > > have used TestingProfile::Builder::AddTestingFactory(), except the profile is > > already instantiated in MediaRouterContextualMenuUnitTest. > > What uses it that needs it to be non-null, rather than passing in the non-null > ComponentMigrationHelper? It looks like the helper is all that's used here. Right, it's only used for the migration helper. But if the ContextualMenu were to have a ComponentMigrationHelper* property, it'd have to call ToolbarActionsModel::Get(profile)->component_migration_helper() in the ctor, which wouldn't work in tests.
https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:42: toolbar_actions_model_(toolbar_actions_model) { On 2016/09/21 17:45:45, takumif wrote: > On 2016/09/21 17:32:57, Devlin wrote: > > On 2016/09/21 17:27:15, takumif wrote: > > > On 2016/09/21 17:15:12, Devlin wrote: > > > > It looks like we only ever use this for the ComponentMigrationHelper - > could > > > we > > > > just use that instead? > > > > > > This is to be able to inject a non-NULL ToolbarActionsModel in test. I would > > > have used TestingProfile::Builder::AddTestingFactory(), except the profile > is > > > already instantiated in MediaRouterContextualMenuUnitTest. > > > > What uses it that needs it to be non-null, rather than passing in the non-null > > ComponentMigrationHelper? It looks like the helper is all that's used here. > > Right, it's only used for the migration helper. But if the ContextualMenu were > to have a ComponentMigrationHelper* property, it'd have to call > > ToolbarActionsModel::Get(profile)->component_migration_helper() > > in the ctor, which wouldn't work in tests. Why? We have a ctor explicitly for injecting dependencies, so this could just become MediaRouterContextualMenu(Browser* browser, ComponentMigrationHelper* component_migration_helper) : browser_(browser), migration_helper_(helper) {} The "real" ctor already calls ToolbarActionsModel::Get(), so there's on difference there.
https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:42: toolbar_actions_model_(toolbar_actions_model) { On 2016/09/21 17:45:45, takumif wrote: > On 2016/09/21 17:32:57, Devlin wrote: > > On 2016/09/21 17:27:15, takumif wrote: > > > On 2016/09/21 17:15:12, Devlin wrote: > > > > It looks like we only ever use this for the ComponentMigrationHelper - > could > > > we > > > > just use that instead? > > > > > > This is to be able to inject a non-NULL ToolbarActionsModel in test. I would > > > have used TestingProfile::Builder::AddTestingFactory(), except the profile > is > > > already instantiated in MediaRouterContextualMenuUnitTest. > > > > What uses it that needs it to be non-null, rather than passing in the non-null > > ComponentMigrationHelper? It looks like the helper is all that's used here. > > Right, it's only used for the migration helper. But if the ContextualMenu were > to have a ComponentMigrationHelper* property, it'd have to call > > ToolbarActionsModel::Get(profile)->component_migration_helper() > > in the ctor, which wouldn't work in tests. I was thinking that if the real ctor gets called in tests (which happens whenever MRAction is instantiated) and it had to call ToolbarActionsModel::Get(profile)->component_migration_helper() with null ToolbarActionsModel, then that wouldn't work. But I can make it check if the ToolbarActionsModel is null, and set ComponentMigrationHelper* only if the ToolbarActionsModel is non-null. Would that be better?
https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:42: toolbar_actions_model_(toolbar_actions_model) { On 2016/09/21 17:59:16, takumif wrote: > On 2016/09/21 17:45:45, takumif wrote: > > On 2016/09/21 17:32:57, Devlin wrote: > > > On 2016/09/21 17:27:15, takumif wrote: > > > > On 2016/09/21 17:15:12, Devlin wrote: > > > > > It looks like we only ever use this for the ComponentMigrationHelper - > > could > > > > we > > > > > just use that instead? > > > > > > > > This is to be able to inject a non-NULL ToolbarActionsModel in test. I > would > > > > have used TestingProfile::Builder::AddTestingFactory(), except the profile > > is > > > > already instantiated in MediaRouterContextualMenuUnitTest. > > > > > > What uses it that needs it to be non-null, rather than passing in the > non-null > > > ComponentMigrationHelper? It looks like the helper is all that's used here. > > > > Right, it's only used for the migration helper. But if the ContextualMenu were > > to have a ComponentMigrationHelper* property, it'd have to call > > > > ToolbarActionsModel::Get(profile)->component_migration_helper() > > > > in the ctor, which wouldn't work in tests. > > I was thinking that if the real ctor gets called in tests (which happens > whenever MRAction is instantiated) and it had to call > > ToolbarActionsModel::Get(profile)->component_migration_helper() > > with null ToolbarActionsModel, then that wouldn't work. But I can make it check > if the ToolbarActionsModel is null, and set ComponentMigrationHelper* only if > the ToolbarActionsModel is non-null. Would that be better? So these are tests that construct a MediaRouterContextualMenu via the main ctor, but then don't trigger any of the functionality that uses ComponentActionHelper, is that correct? That seems a little fragile to me. Ideally, we'd be able to DCHECK that component actions helper is never null, because otherwise an unrelated code change (e.g. calling GetAlwaysShowActionPref()) will cause a failure. Which tests do this currently?
https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2332693003/diff/160001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:42: toolbar_actions_model_(toolbar_actions_model) { On 2016/09/21 18:17:30, Devlin wrote: > On 2016/09/21 17:59:16, takumif wrote: > > On 2016/09/21 17:45:45, takumif wrote: > > > On 2016/09/21 17:32:57, Devlin wrote: > > > > On 2016/09/21 17:27:15, takumif wrote: > > > > > On 2016/09/21 17:15:12, Devlin wrote: > > > > > > It looks like we only ever use this for the ComponentMigrationHelper - > > > could > > > > > we > > > > > > just use that instead? > > > > > > > > > > This is to be able to inject a non-NULL ToolbarActionsModel in test. I > > would > > > > > have used TestingProfile::Builder::AddTestingFactory(), except the > profile > > > is > > > > > already instantiated in MediaRouterContextualMenuUnitTest. > > > > > > > > What uses it that needs it to be non-null, rather than passing in the > > non-null > > > > ComponentMigrationHelper? It looks like the helper is all that's used > here. > > > > > > Right, it's only used for the migration helper. But if the ContextualMenu > were > > > to have a ComponentMigrationHelper* property, it'd have to call > > > > > > ToolbarActionsModel::Get(profile)->component_migration_helper() > > > > > > in the ctor, which wouldn't work in tests. > > > > I was thinking that if the real ctor gets called in tests (which happens > > whenever MRAction is instantiated) and it had to call > > > > ToolbarActionsModel::Get(profile)->component_migration_helper() > > > > with null ToolbarActionsModel, then that wouldn't work. But I can make it > check > > if the ToolbarActionsModel is null, and set ComponentMigrationHelper* only if > > the ToolbarActionsModel is non-null. Would that be better? > > So these are tests that construct a MediaRouterContextualMenu via the main ctor, > but then don't trigger any of the functionality that uses ComponentActionHelper, > is that correct? > > That seems a little fragile to me. Ideally, we'd be able to DCHECK that > component actions helper is never null, because otherwise an unrelated code > change (e.g. calling GetAlwaysShowActionPref()) will cause a failure. > > Which tests do this currently? Sorry, I'd mistakenly thought that both the MRContextualMenu and MRAction unit tests were failing, but it turns out MRAction tests were fine, thanks to extensions::extension_action_test_util::CreateToolbarModelForProfile(). This works for MRContextualMenu tests as well, so injecting ToolbarActionsModel/ComponentMigrationHelper is no longer necessary.
nice, 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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
Thank you everyone for the timely reviews, I really appreciate it! Derek, could you check media_router_ui_service_factory_unittest.cc one last time? I made a change due to a memory leak. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % 1 comment https://codereview.chromium.org/2332693003/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service_factory_unittest.cc (right): https://codereview.chromium.org/2332693003/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service_factory_unittest.cc:50: MediaRouterUIService* service = static_cast<MediaRouterUIService*>( nit: You can use unique_ptr<> so you don't have to delete manually.
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...
Thanks for reviewing! https://codereview.chromium.org/2332693003/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_router_ui_service_factory_unittest.cc (right): https://codereview.chromium.org/2332693003/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_router_ui_service_factory_unittest.cc:50: MediaRouterUIService* service = static_cast<MediaRouterUIService*>( On 2016/09/21 23:45:38, imcheng wrote: > nit: You can use unique_ptr<> so you don't have to delete manually. Done.
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 grt@chromium.org, msw@chromium.org, rdevlin.cronin@chromium.org, imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2332693003/#ps220001 (title: "Address Derek's comment")
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 ========== Show media router toolbar icon ephemerally for active local routes and issues This CL is part 2 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController [1] 2. Show ephemeral icon for active local routes and issues (this CL) 3. Show ephemeral icon for open dialog These CLs are a redesign of a previous CL [2], which has been reverted for its bugs. Since the contents of this CL are similar to those of [2], the first patch in this CL is from [2] for comparison. This CL makes the media router toolbar icon show temporarily whenever there is an active local media route or an issue. This CL also replaces the "Remove icon" option in the action context menu with "Always show icon" which can be checked. When this option is unchecked, the icon is neither on the toolbar nor in the overflow menu, and is only shown ephemerally in the situations mentioned above. We replace the word "popup" with "dialog" wherever possible in MediaRouterAction, to stay consistent with the rest of Media Router files. We also change the default position of the component actions on the toolbar from the leftmost to the rightmost (same behavior as extension actions). Major differences between the original CL [2] and this CL is that now the MediaRouterAction is no longer responsible for determining whether it should stay shown or be hidden. Instead, showing/hiding of the icon is done by the MediaRouterActionController. Also the unit tests for the ephemeral icon has been moved to the unit tests for MediaRouterActionController (in [1]). MediaRouterDialogController-related changes will be in part 3. [1] https://codereview.chromium.org/2294973002/ [2] https://codereview.chromium.org/2155293002/ BUG=594577 ========== to ========== Show media router toolbar icon ephemerally for active local routes and issues This CL is part 2 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController [1] 2. Show ephemeral icon for active local routes and issues (this CL) 3. Show ephemeral icon for open dialog These CLs are a redesign of a previous CL [2], which has been reverted for its bugs. Since the contents of this CL are similar to those of [2], the first patch in this CL is from [2] for comparison. This CL makes the media router toolbar icon show temporarily whenever there is an active local media route or an issue. This CL also replaces the "Remove icon" option in the action context menu with "Always show icon" which can be checked. When this option is unchecked, the icon is neither on the toolbar nor in the overflow menu, and is only shown ephemerally in the situations mentioned above. We replace the word "popup" with "dialog" wherever possible in MediaRouterAction, to stay consistent with the rest of Media Router files. We also change the default position of the component actions on the toolbar from the leftmost to the rightmost (same behavior as extension actions). Major differences between the original CL [2] and this CL is that now the MediaRouterAction is no longer responsible for determining whether it should stay shown or be hidden. Instead, showing/hiding of the icon is done by the MediaRouterActionController. Also the unit tests for the ephemeral icon has been moved to the unit tests for MediaRouterActionController (in [1]). MediaRouterDialogController-related changes will be in part 3. [1] https://codereview.chromium.org/2294973002/ [2] https://codereview.chromium.org/2155293002/ BUG=594577 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Show media router toolbar icon ephemerally for active local routes and issues This CL is part 2 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController [1] 2. Show ephemeral icon for active local routes and issues (this CL) 3. Show ephemeral icon for open dialog These CLs are a redesign of a previous CL [2], which has been reverted for its bugs. Since the contents of this CL are similar to those of [2], the first patch in this CL is from [2] for comparison. This CL makes the media router toolbar icon show temporarily whenever there is an active local media route or an issue. This CL also replaces the "Remove icon" option in the action context menu with "Always show icon" which can be checked. When this option is unchecked, the icon is neither on the toolbar nor in the overflow menu, and is only shown ephemerally in the situations mentioned above. We replace the word "popup" with "dialog" wherever possible in MediaRouterAction, to stay consistent with the rest of Media Router files. We also change the default position of the component actions on the toolbar from the leftmost to the rightmost (same behavior as extension actions). Major differences between the original CL [2] and this CL is that now the MediaRouterAction is no longer responsible for determining whether it should stay shown or be hidden. Instead, showing/hiding of the icon is done by the MediaRouterActionController. Also the unit tests for the ephemeral icon has been moved to the unit tests for MediaRouterActionController (in [1]). MediaRouterDialogController-related changes will be in part 3. [1] https://codereview.chromium.org/2294973002/ [2] https://codereview.chromium.org/2155293002/ BUG=594577 ========== to ========== Show media router toolbar icon ephemerally for active local routes and issues This CL is part 2 of 3 CLs to implement ephemeral toolbar icon for media router: 1. Implement MediaRouterActionController [1] 2. Show ephemeral icon for active local routes and issues (this CL) 3. Show ephemeral icon for open dialog These CLs are a redesign of a previous CL [2], which has been reverted for its bugs. Since the contents of this CL are similar to those of [2], the first patch in this CL is from [2] for comparison. This CL makes the media router toolbar icon show temporarily whenever there is an active local media route or an issue. This CL also replaces the "Remove icon" option in the action context menu with "Always show icon" which can be checked. When this option is unchecked, the icon is neither on the toolbar nor in the overflow menu, and is only shown ephemerally in the situations mentioned above. We replace the word "popup" with "dialog" wherever possible in MediaRouterAction, to stay consistent with the rest of Media Router files. We also change the default position of the component actions on the toolbar from the leftmost to the rightmost (same behavior as extension actions). Major differences between the original CL [2] and this CL is that now the MediaRouterAction is no longer responsible for determining whether it should stay shown or be hidden. Instead, showing/hiding of the icon is done by the MediaRouterActionController. Also the unit tests for the ephemeral icon has been moved to the unit tests for MediaRouterActionController (in [1]). MediaRouterDialogController-related changes will be in part 3. [1] https://codereview.chromium.org/2294973002/ [2] https://codereview.chromium.org/2155293002/ BUG=594577 Committed: https://crrev.com/b22235a16193be6cfa6e199a5c7a2427a7b505e6 Cr-Commit-Position: refs/heads/master@{#420217} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/b22235a16193be6cfa6e199a5c7a2427a7b505e6 Cr-Commit-Position: refs/heads/master@{#420217} |
