Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2155293002:
Show the Cast toolbar icon ephemerally when Cast is in use (Closed)
|
Created:
4 years, 5 months ago by takumif Modified:
4 years, 4 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, media-router+watch_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow the Cast toolbar icon ephemerally when Cast is in use
This CL changes the behavior of the Cast/Media Router toolbar component
action icon. Currently, the only way to add the icon is by installing
the Cast extension from the Web Store. After this change, the icon will
be displayed temporarily whenever the Media Router dialog is open, or there
is an active local media route.
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).
TBR=grt@chromium.org
BUG=594577
Committed: https://crrev.com/de716ca8b6cffb4c7ae30990b3288b3499d28a4c
Cr-Commit-Position: refs/heads/master@{#412280}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address Devlin's comments #Patch Set 3 : Rebase #
Total comments: 79
Patch Set 4 : Address Mike and Devlin's comments #Patch Set 5 : Rebase #
Total comments: 29
Patch Set 6 : Address Mike and Devlin's comments #
Total comments: 2
Patch Set 7 : Address Mike's comment #Patch Set 8 : Rebase #
Total comments: 12
Patch Set 9 : Address Devlin and Jennifer's comments #Patch Set 10 : Bug fix and refactoring #
Total comments: 6
Patch Set 11 : Address Devlin's comments #Patch Set 12 : Rebase #Patch Set 13 : Address Devlin's comment, rename Popup to Dialog in MRA #
Total comments: 6
Patch Set 14 : Address Devlin's comments #Patch Set 15 : Address Mark's comment, trybot failures, rebase #Messages
Total messages: 72 (35 generated)
takumif@chromium.org changed reviewers: + anthonyvd@chromium.org, mfoltz@chromium.org, msw@chromium.org, rdevlin.cronin@chromium.org
Please take a look, thank you! anthonyvd@ for c/b/profiles/profile.cc rdevlin.cronin@ for c/b/extensions/component_migration_helper.cc and *toolbar* except for *media_router* mfoltz@ for *media_router* msw@ for *media_router*
https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/m... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/m... chrome/browser/ui/toolbar/media_router_action_unittest.cc:40: class MockToolbarActionsModel : public ToolbarActionsModel { Why do we need to override this rather than going through a realistic flow? https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/t... File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right): https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.h:289: ToolbarActionViewController* GetComponentActionForId( The toolbar actions bar should be as agnostic as possible about the type of action its showing. Why do we need this? https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/browser_actions_container.h:155: ToolbarActionView* GetComponentViewForId(const std::string& id); Same here - browser actions container has no business knowing about component vs non-component views.
https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/m... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/m... chrome/browser/ui/toolbar/media_router_action_unittest.cc:40: class MockToolbarActionsModel : public ToolbarActionsModel { On 2016/07/19 22:56:25, Devlin wrote: > Why do we need to override this rather than going through a realistic flow? I was getting an error with animating/drawing the toolbar that I wasn't in the actual build, so I wanted to bypass that. https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/t... File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right): https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.h:289: ToolbarActionViewController* GetComponentActionForId( On 2016/07/19 22:56:25, Devlin wrote: > The toolbar actions bar should be as agnostic as possible about the type of > action its showing. Why do we need this? We want to add/remove the Media Router icon to the toolbar whenever the MR popup is shown/hidden etc, and with the current toolbar the ToolbarActionViewController and ToolbarActionView for the MR action get destroyed whenever the icon is removed, and I wanted to make them persist. I'm not sure if it would make more sense to instantiate them every time we're adding the icon to the toolbar.
https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/m... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/m... chrome/browser/ui/toolbar/media_router_action_unittest.cc:40: class MockToolbarActionsModel : public ToolbarActionsModel { On 2016/07/20 23:27:18, takumif wrote: > On 2016/07/19 22:56:25, Devlin wrote: > > Why do we need to override this rather than going through a realistic flow? > > I was getting an error with animating/drawing the toolbar that I wasn't in the > actual build, so I wanted to bypass that. animation can be surpressed using ToolbarActionsBar::disable_animations_for_testing_. https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/t... File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right): https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.h:289: ToolbarActionViewController* GetComponentActionForId( On 2016/07/20 23:27:18, takumif wrote: > On 2016/07/19 22:56:25, Devlin wrote: > > The toolbar actions bar should be as agnostic as possible about the type of > > action its showing. Why do we need this? > > We want to add/remove the Media Router icon to the toolbar whenever the MR popup > is shown/hidden etc, and with the current toolbar the > ToolbarActionViewController and ToolbarActionView for the MR action get > destroyed whenever the icon is removed, and I wanted to make them persist. I'm > not sure if it would make more sense to instantiate them every time we're adding > the icon to the toolbar. What work does the MediaRouterAction do that makes it so expensive to construct? It looks pretty simple to me. Remember that this only happens on user action, so it's never going to be hit hundreds or thousands of times a minute.
https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/m... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/m... chrome/browser/ui/toolbar/media_router_action_unittest.cc:40: class MockToolbarActionsModel : public ToolbarActionsModel { On 2016/07/20 23:31:16, Devlin wrote: > On 2016/07/20 23:27:18, takumif wrote: > > On 2016/07/19 22:56:25, Devlin wrote: > > > Why do we need to override this rather than going through a realistic flow? > > > > I was getting an error with animating/drawing the toolbar that I wasn't in the > > actual build, so I wanted to bypass that. > > animation can be surpressed using > ToolbarActionsBar::disable_animations_for_testing_. Thanks. I'll try that https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/t... File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right): https://codereview.chromium.org/2155293002/diff/1/chrome/browser/ui/toolbar/t... chrome/browser/ui/toolbar/toolbar_actions_bar.h:289: ToolbarActionViewController* GetComponentActionForId( On 2016/07/20 23:31:16, Devlin wrote: > On 2016/07/20 23:27:18, takumif wrote: > > On 2016/07/19 22:56:25, Devlin wrote: > > > The toolbar actions bar should be as agnostic as possible about the type of > > > action its showing. Why do we need this? > > > > We want to add/remove the Media Router icon to the toolbar whenever the MR > popup > > is shown/hidden etc, and with the current toolbar the > > ToolbarActionViewController and ToolbarActionView for the MR action get > > destroyed whenever the icon is removed, and I wanted to make them persist. I'm > > not sure if it would make more sense to instantiate them every time we're > adding > > the icon to the toolbar. > > What work does the MediaRouterAction do that makes it so expensive to construct? > It looks pretty simple to me. Remember that this only happens on user action, > so it's never going to be hit hundreds or thousands of times a minute. That's true. It just felt a bit weird to me to destroy/instantiate every time the user switched tabs or closed the dialog, but no it's not expensive. I'll try it out. Thanks for the comment.
c/b/profiles/profile.cc lgtm
takumif@chromium.org changed reviewers: - anthonyvd@chromium.org
Please take a look, thank you!
https://codereview.chromium.org/2155293002/diff/40001/chrome/app/media_router... File chrome/app/media_router_strings.grdp (right): https://codereview.chromium.org/2155293002/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, removes the Media Router Action icon from the toolbar."> nit: update "removes" https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/component_migration_helper.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/component_migration_helper.cc:139: ComponentToolbarActionsFactory::kMediaRouterActionId) nit: add curly braces. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:60: bool IsComponentActionId(const std::string& id); nit: make this static https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.cc (left): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:116: OnPopupHidden(); Why is this no longer called? https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:189: SetVisibilityPreference(!browser_->profile()->GetPrefs()-> optional nit: use the IsEphemeral (perhaps renamed) helper here. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:223: if (model->HasComponentAction(GetId()) && Shouldn't this also check IsShowingMediaRouterDialog? (I'm not sure if the dialog is the same as the popup...) https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:232: if (GetMediaRouterDialogController()->IsShowingMediaRouterDialog()) { nit: curlies not needed. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:266: if (!IsEphemeral() && controller->IsShowingMediaRouterDialog()) { nit: curlies not needed. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.h:78: void SetVisibilityPreference(bool always_show); This is only called from ToggleVisibilityPreference, inline there? https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.h:84: // For accessing from tests nit: add a trailing period. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.h:112: bool IsEphemeral(); nit: consider "ShouldAlwaysShowIcon", "GetAlwaysShowIconPreference", or similar; simplify comment. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:359: EXPECT_CALL(*mock_delegate, OnPopupClosed()).Times(1); nit: EXPECT_FALSE(dialog_controller_->IsShowingMediaRouterDialog());? https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:378: WebContents* initiator_ = nit: no trailing underscore for function-local identifiers. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:382: MediaRouterDialogControllerImpl* dialog_controller_ = ditto nit: no trailing underscore for function-local identifiers. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:387: // Show the popup. The icon should become visible nit: trailing periods here and in comments below. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:102: PrefService* pref_service = browser_->profile()->GetPrefs(); optional nit: move to IDC_MEDIA_ROUTER_CLOUD_SERVICES_TOGGLE case with curlies. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:109: if (action_) Should this DCHECK? Is it possible to toggle on when it's not around? https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:574: if (observers_.HasObserver(observer)) Is something removing itself twice now? Can you fix that instead? https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:682: if (ComponentToolbarActionsFactory::GetInstance() Is this check actually necessary? Wouldn't IsBeingUpgraded just return false, achieving the same effect without this new check? https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model.cc (left): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.cc:346: new_index = is_component ? 0 : visible_icon_count(); nit: note this tangential behavior change in the CL description. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.cc:334: void ToolbarActionsModel::AddItem(const ToolbarItem& item, bool is_component) { nit: remove unused param |is_component| and cleanup callsites. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (left): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:271: action_->OnPopupShown(); Why doesn't CreateMediaRouterDialog still call OnPopupShown? (is the call in OnDialogNavigated replacing this for some reason?) https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:57: MediaRouterDialogDelegate( nit: explicit. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:109: DCHECK(size); optional nit: inline definition above, like other functions. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:336: ShowMediaRouterAction(); Why doesn't the MediaRouterDialogController base class own |action_| and handle its management? It seems like that's not dependent upon any WebUI impl details. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:345: if (IsShowingMediaRouterDialog() && !action_) Can the user actually change tabs while the dialog is showing? Or is the dialog somehow shown/hidden when switching tabs and left somewhat modal to the tab? https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:47: bool ShowMediaRouterDialog() override; nit: order immediately before IsShowingMediaRouterDialog (with the other MediaRouterDialogController override); ditto with the impl in the cc. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:50: void ActiveTabChanged(content::WebContents* old_contents, optional nit: order immediately after MediaRouterDialogController overrides. (keep overrides together, and keep class-specific functions together) (reorder UpdateMaxDialogSize and OnDialogClosed after SetMediaRouterAction)
toolbar stuff is looking a lot better; glad to see the complexity go down! https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:214: if (model->HasComponentAction(GetId())) isn't this the same as else if (model->HasComponentAction(GetID())) { ... } ? https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:233: delegate_->OnPopupShown(true); This behavior, or rather the motivation, isn't clear to me. Can you add a comment? https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:280: web_contents); indentation was right before. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:149: bool ActionIsVisible() { "Visible" could mean different things here, since we can also overflow actions in the wrench menu. Let's go with something like "ActionIsPresent()", "ActionExists()", "ActionIsInToolbarModel()", etc. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:370: ToolbarActionsBar::disable_animations_for_testing_ = true; base::AutoReset<bool> disable_animations(&ToolbarActionsBar::disable_animations_for_testing_, true); https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_contextual_menu.h (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_contextual_menu.h:36: base::WeakPtr<MediaRouterAction> action_; MediaRouterAction owns this object, right? Why do we need a weak_ptr? https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:574: if (observers_.HasObserver(observer)) On 2016/07/26 19:59:31, msw wrote: > Is something removing itself twice now? Can you fix that instead? +1 https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:682: if (ComponentToolbarActionsFactory::GetInstance() On 2016/07/26 19:59:31, msw wrote: > Is this check actually necessary? Wouldn't IsBeingUpgraded just return false, > achieving the same effect without this new check? Yes, it would. Please remove this check. ToolbarActionsBar should try to remain relatively agnostic about extensions vs components. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model.cc (left): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.cc:346: new_index = is_component ? 0 : visible_icon_count(); FYI, this also breaks behavior we had in place for stars. That's probably no longer as important given stars isn't bundled with chrome, but it might be worth discussing. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:1311: TEST_F(ToolbarActionsModelUnitTest, ComponentExtensionsAddedToRight) { If we do remove this functionality, isn't this testing the same thing as NewExtensionsAreVisible?
Patchset #4 (id:60001) has been deleted
Thank you for the comments, Devlin and Mike. I've addressed all the points regarding MR, but had questions for the ToolbarActionsBar and ToolbarActionsModel. Devlin, could you take a look at my comments on those files in Patch 3? Thank you! https://codereview.chromium.org/2155293002/diff/40001/chrome/app/media_router... File chrome/app/media_router_strings.grdp (right): https://codereview.chromium.org/2155293002/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, removes the Media Router Action icon from the toolbar."> On 2016/07/26 19:59:30, msw wrote: > nit: update "removes" Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/component_migration_helper.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/component_migration_helper.cc:139: ComponentToolbarActionsFactory::kMediaRouterActionId) On 2016/07/26 19:59:31, msw wrote: > nit: add curly braces. Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:60: bool IsComponentActionId(const std::string& id); On 2016/07/26 19:59:31, msw wrote: > nit: make this static Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.cc (left): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:116: OnPopupHidden(); On 2016/07/26 19:59:31, msw wrote: > Why is this no longer called? MediaRouterDialogControllerImpl::OnDialogClosed also calls OnPopupHidden, so this is redundant. Also this is only called when the popup is closed through the toolbar, whereas OnDialogClosed is always called. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:189: SetVisibilityPreference(!browser_->profile()->GetPrefs()-> On 2016/07/26 19:59:31, msw wrote: > optional nit: use the IsEphemeral (perhaps renamed) helper here. Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:214: if (model->HasComponentAction(GetId())) On 2016/07/26 21:59:02, Devlin wrote: > isn't this the same as > else if (model->HasComponentAction(GetID())) { > ... > } > ? Yes, it is. Corrected. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:223: if (model->HasComponentAction(GetId()) && On 2016/07/26 19:59:31, msw wrote: > Shouldn't this also check IsShowingMediaRouterDialog? > (I'm not sure if the dialog is the same as the popup...) I could replace this with a call to MRAction::UpdateVisibility from MRDialogControllerImpl::Reset, so I did that. Yes, the dialog is same as the popup. MR uses the term dialog and the toolbar uses popup. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:232: if (GetMediaRouterDialogController()->IsShowingMediaRouterDialog()) { On 2016/07/26 19:59:31, msw wrote: > nit: curlies not needed. Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:233: delegate_->OnPopupShown(true); On 2016/07/26 21:59:02, Devlin wrote: > This behavior, or rather the motivation, isn't clear to me. Can you add a > comment? Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:266: if (!IsEphemeral() && controller->IsShowingMediaRouterDialog()) { On 2016/07/26 19:59:31, msw wrote: > nit: curlies not needed. Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.cc:280: web_contents); On 2016/07/26 21:59:02, Devlin wrote: > indentation was right before. Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.h:78: void SetVisibilityPreference(bool always_show); On 2016/07/26 19:59:31, msw wrote: > This is only called from ToggleVisibilityPreference, inline there? Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.h:84: // For accessing from tests On 2016/07/26 19:59:31, msw wrote: > nit: add a trailing period. Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action.h:112: bool IsEphemeral(); On 2016/07/26 19:59:31, msw wrote: > nit: consider "ShouldAlwaysShowIcon", "GetAlwaysShowIconPreference", or similar; > simplify comment. Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:149: bool ActionIsVisible() { On 2016/07/26 21:59:02, Devlin wrote: > "Visible" could mean different things here, since we can also overflow actions > in the wrench menu. Let's go with something like "ActionIsPresent()", > "ActionExists()", "ActionIsInToolbarModel()", etc. Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:359: EXPECT_CALL(*mock_delegate, OnPopupClosed()).Times(1); On 2016/07/26 19:59:31, msw wrote: > nit: EXPECT_FALSE(dialog_controller_->IsShowingMediaRouterDialog());? Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:370: ToolbarActionsBar::disable_animations_for_testing_ = true; On 2016/07/26 21:59:02, Devlin wrote: > base::AutoReset<bool> > disable_animations(&ToolbarActionsBar::disable_animations_for_testing_, true); Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:378: WebContents* initiator_ = On 2016/07/26 19:59:31, msw wrote: > nit: no trailing underscore for function-local identifiers. Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:382: MediaRouterDialogControllerImpl* dialog_controller_ = On 2016/07/26 19:59:31, msw wrote: > ditto nit: no trailing underscore for function-local identifiers. Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_action_unittest.cc:387: // Show the popup. The icon should become visible On 2016/07/26 19:59:31, msw wrote: > nit: trailing periods here and in comments below. Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:102: PrefService* pref_service = browser_->profile()->GetPrefs(); On 2016/07/26 19:59:31, msw wrote: > optional nit: move to IDC_MEDIA_ROUTER_CLOUD_SERVICES_TOGGLE case with curlies. Undid this change (not sure what you meant with curlies). https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:109: if (action_) On 2016/07/26 19:59:31, msw wrote: > Should this DCHECK? Is it possible to toggle on when it's not around? The action owns the contextual menu, so no it shouldn't be possible. Changed to DCHECK. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_contextual_menu.h (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_contextual_menu.h:36: base::WeakPtr<MediaRouterAction> action_; On 2016/07/26 21:59:02, Devlin wrote: > MediaRouterAction owns this object, right? Why do we need a weak_ptr? Changing to a normal pointer. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:574: if (observers_.HasObserver(observer)) On 2016/07/26 21:59:03, Devlin wrote: > On 2016/07/26 19:59:31, msw wrote: > > Is something removing itself twice now? Can you fix that instead? > > +1 Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:682: if (ComponentToolbarActionsFactory::GetInstance() On 2016/07/26 21:59:02, Devlin wrote: > On 2016/07/26 19:59:31, msw wrote: > > Is this check actually necessary? Wouldn't IsBeingUpgraded just return false, > > achieving the same effect without this new check? > > Yes, it would. Please remove this check. ToolbarActionsBar should try to > remain relatively agnostic about extensions vs components. Okay. In MRActionUnitTest, runtime_data() here is set to null (which I believe is MockExtensionSystem::runtime_data()). Devlin, what would be a good way to deal with that? https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model.cc (left): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.cc:346: new_index = is_component ? 0 : visible_icon_count(); On 2016/07/26 21:59:03, Devlin wrote: > FYI, this also breaks behavior we had in place for stars. That's probably no > longer as important given stars isn't bundled with chrome, but it might be worth > discussing. I wasn't aware that there are/were component actions in development other than MR. Was this behavior implemented specifically for Stars? https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (left): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:271: action_->OnPopupShown(); On 2016/07/26 19:59:32, msw wrote: > Why doesn't CreateMediaRouterDialog still call OnPopupShown? > (is the call in OnDialogNavigated replacing this for some reason?) The only thing MediaRouterAction::OnPopupShown does right now is to call ToolbarActionView::OnPopupShown, which depresses the icon. I had to remove OnPopupShown from here due to an issue in which the depression shadow appears in a wrong place if we're animating the icon into existence at the same time. That case is dealt with in MediaRouterAction::OnToolbarActionsBarAnimationEnded, and the case in which the icon is already on the toolbar is taken care of by OnPopupShown in OnDialogNavigated. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:57: MediaRouterDialogDelegate( On 2016/07/26 19:59:32, msw wrote: > nit: explicit. Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:109: DCHECK(size); On 2016/07/26 19:59:32, msw wrote: > optional nit: inline definition above, like other functions. Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:336: ShowMediaRouterAction(); On 2016/07/26 19:59:32, msw wrote: > Why doesn't the MediaRouterDialogController base class own |action_| and handle > its management? It seems like that's not dependent upon any WebUI impl details. The base class contains logic that's shared across desktop and Android, and the Impl class contains desktop-only logic. Since the toolbar action is desktop specific, we keep it here. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:345: if (IsShowingMediaRouterDialog() && !action_) On 2016/07/26 19:59:32, msw wrote: > Can the user actually change tabs while the dialog is showing? Or is the dialog > somehow shown/hidden when switching tabs and left somewhat modal to the tab? Yes, the user can switch tabs while the dialog is open, and have the dialog open in one tab but not in another, just like the print preview dialog. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:47: bool ShowMediaRouterDialog() override; On 2016/07/26 19:59:32, msw wrote: > nit: order immediately before IsShowingMediaRouterDialog (with the other > MediaRouterDialogController override); ditto with the impl in the cc. Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:50: void ActiveTabChanged(content::WebContents* old_contents, On 2016/07/26 19:59:32, msw wrote: > optional nit: order immediately after MediaRouterDialogController overrides. > (keep overrides together, and keep class-specific functions together) > (reorder UpdateMaxDialogSize and OnDialogClosed after SetMediaRouterAction) Done.
lgtm with nits and modulo MRActionUnitTest/runtime_data() stuff. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.cc:334: void ToolbarActionsModel::AddItem(const ToolbarItem& item, bool is_component) { On 2016/07/26 19:59:32, msw wrote: > nit: remove unused param |is_component| and cleanup callsites. ping; please don't leave unused code around. https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:114: pref_service = browser_->profile()->GetPrefs(); nit: use curly braces around this to declare pref_service in this case: case IDC_MEDIA_ROUTER_CLOUD_SERVICES_TOGGLE: { PrefService* pref_service = browser_->profile()->GetPrefs(); ... } (otherwise, please init |pref_service| to nullptr above) https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:48: nit: remove blank line
https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:682: if (ComponentToolbarActionsFactory::GetInstance() On 2016/07/28 20:04:11, takumif wrote: > On 2016/07/26 21:59:02, Devlin wrote: > > On 2016/07/26 19:59:31, msw wrote: > > > Is this check actually necessary? Wouldn't IsBeingUpgraded just return > false, > > > achieving the same effect without this new check? > > > > Yes, it would. Please remove this check. ToolbarActionsBar should try to > > remain relatively agnostic about extensions vs components. > > Okay. In MRActionUnitTest, runtime_data() here is set to null (which I believe > is MockExtensionSystem::runtime_data()). Devlin, what would be a good way to > deal with that? Call CreateExtensionService() on TestExtensionSystem. See other classes that use CreateToolbarModelForProfile() for examples. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model.cc (left): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.cc:346: new_index = is_component ? 0 : visible_icon_count(); On 2016/07/28 20:04:11, takumif wrote: > On 2016/07/26 21:59:03, Devlin wrote: > > FYI, this also breaks behavior we had in place for stars. That's probably no > > longer as important given stars isn't bundled with chrome, but it might be > worth > > discussing. > > I wasn't aware that there are/were component actions in development other than > MR. Was this behavior implemented specifically for Stars? Yes, this was just for stars, so I think we're probably okay on that front. At the very least though, we should mention this change in the CL description. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:1311: TEST_F(ToolbarActionsModelUnitTest, ComponentExtensionsAddedToRight) { On 2016/07/26 21:59:03, Devlin wrote: > If we do remove this functionality, isn't this testing the same thing as > NewExtensionsAreVisible? ping https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:59: // Returns whether the given ID belongs to a component action end in a '.' https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:195: if (!delegate_) Document when this can happen. https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:205: if (!model->HasComponentAction(GetId())) Thinking about this. In theory, the ToolbarActionsBar should represent the state of the ToolbarActionsModel % a few ordering concerns (e.g. popped out actions). When could this action exist, but not be present in the model? https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:214: if (delegate_) { again, document when this can be false. https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:252: if (delegate_) { ditto https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:80: void OnToolbarActionsBarAnimationEnded() override; Put inherited methods together within the same visibility. https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:368: TEST_F(MediaRouterActionUnitTest, EphemeralIcon) { What about also testing the perma-shown icon? https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:377: // Create a reference to initiator contents. I don't follow this comment. What's an initiator contents? What are we creating? https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:96: controller_.get()->OnDialogClosed(); This .get() is unnecessary. https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:208: action_->OnPopupHidden(); It seems a little messy to have so much two-way communication between MediaRouterDialogControllerImpl and MediaRouterAction. Can we make the action only respond to input from the controller, rather than needing to query it so frequently? Or make the action only observe the controller, rather than needing the controller to inform the action? https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:357: ComponentToolbarActionsFactory::kMediaRouterActionId)) { indentation seems off here.
Description was changed from ========== Show the Cast toolbar icon ephemerally when Cast is in use This CL changes the behavior of the Cast/Media Router toolbar component action icon. Currently, the only way to add the icon is by installing the Cast extension from the Web Store. After this change, the icon will be displayed temporarily whenever the Media Router dialog is open, or there is an active local media route. 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. BUG=594577 ========== to ========== Show the Cast toolbar icon ephemerally when Cast is in use This CL changes the behavior of the Cast/Media Router toolbar component action icon. Currently, the only way to add the icon is by installing the Cast extension from the Web Store. After this change, the icon will be displayed temporarily whenever the Media Router dialog is open, or there is an active local media route. 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 also change the default position of the component actions on the toolbar from the leftmost to the rightmost (same behavior as extension actions). BUG=594577 ==========
Description was changed from ========== Show the Cast toolbar icon ephemerally when Cast is in use This CL changes the behavior of the Cast/Media Router toolbar component action icon. Currently, the only way to add the icon is by installing the Cast extension from the Web Store. After this change, the icon will be displayed temporarily whenever the Media Router dialog is open, or there is an active local media route. 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 also change the default position of the component actions on the toolbar from the leftmost to the rightmost (same behavior as extension actions). BUG=594577 ========== to ========== Show the Cast toolbar icon ephemerally when Cast is in use This CL changes the behavior of the Cast/Media Router toolbar component action icon. Currently, the only way to add the icon is by installing the Cast extension from the Web Store. After this change, the icon will be displayed temporarily whenever the Media Router dialog is open, or there is an active local media route. 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 also change the default position of the component actions on the toolbar from the leftmost to the rightmost (same behavior as extension actions). BUG=594577 ==========
Patchset #6 (id:120001) has been deleted
takumif@chromium.org changed reviewers: - mfoltz@chromium.org
takumif@chromium.org changed reviewers: + apacible@chromium.org, cpu@chromium.org
+apacible@: Please take a look at media_router_strings.grdp (and optionally the rest of media router files). +cpu@: Please take a look at chrome_command_ids.h. msw@ and rdevlin.cronin@, thanks for your comments again. Mike, I should've addressed all your comments. Please let me know if not. Devlin, I had a few questions regarding your comments on media_router_action.cc and media_router_dialog_controller_impl.cc. Please take a look. Thank you all! https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_bar.cc:682: if (ComponentToolbarActionsFactory::GetInstance() On 2016/07/29 19:10:22, Devlin wrote: > On 2016/07/28 20:04:11, takumif wrote: > > On 2016/07/26 21:59:02, Devlin wrote: > > > On 2016/07/26 19:59:31, msw wrote: > > > > Is this check actually necessary? Wouldn't IsBeingUpgraded just return > > false, > > > > achieving the same effect without this new check? > > > > > > Yes, it would. Please remove this check. ToolbarActionsBar should try to > > > remain relatively agnostic about extensions vs components. > > > > Okay. In MRActionUnitTest, runtime_data() here is set to null (which I believe > > is MockExtensionSystem::runtime_data()). Devlin, what would be a good way to > > deal with that? > > Call CreateExtensionService() on TestExtensionSystem. See other classes that > use CreateToolbarModelForProfile() for examples. Thanks, did that. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model.cc (left): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.cc:346: new_index = is_component ? 0 : visible_icon_count(); On 2016/07/29 19:10:22, Devlin wrote: > On 2016/07/28 20:04:11, takumif wrote: > > On 2016/07/26 21:59:03, Devlin wrote: > > > FYI, this also breaks behavior we had in place for stars. That's probably > no > > > longer as important given stars isn't bundled with chrome, but it might be > > worth > > > discussing. > > > > I wasn't aware that there are/were component actions in development other than > > MR. Was this behavior implemented specifically for Stars? > > Yes, this was just for stars, so I think we're probably okay on that front. At > the very least though, we should mention this change in the CL description. Done changing the description. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model.cc:334: void ToolbarActionsModel::AddItem(const ToolbarItem& item, bool is_component) { On 2016/07/29 03:12:15, msw wrote: > On 2016/07/26 19:59:32, msw wrote: > > nit: remove unused param |is_component| and cleanup callsites. > > ping; please don't leave unused code around. Done. https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right): https://codereview.chromium.org/2155293002/diff/40001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:1311: TEST_F(ToolbarActionsModelUnitTest, ComponentExtensionsAddedToRight) { On 2016/07/29 19:10:22, Devlin wrote: > On 2016/07/26 21:59:03, Devlin wrote: > > If we do remove this functionality, isn't this testing the same thing as > > NewExtensionsAreVisible? > > ping Removing the test. https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:59: // Returns whether the given ID belongs to a component action On 2016/07/29 19:10:22, Devlin wrote: > end in a '.' Done. https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:195: if (!delegate_) On 2016/07/29 19:10:22, Devlin wrote: > Document when this can happen. This should only happen in tests. Would it make more sense to override these methods in the TestMediaRouterAction class and check for |delegate_| there (and replace the ones here with DCHECKs)? Something like: void TestMediaRouterAction::UpdateVisibility() override { if (!delegate_) return; MediaRouterAction::UpdateVisibility(); } https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:205: if (!model->HasComponentAction(GetId())) On 2016/07/29 19:10:22, Devlin wrote: > Thinking about this. In theory, the ToolbarActionsBar should represent the > state of the ToolbarActionsModel % a few ordering concerns (e.g. popped out > actions). When could this action exist, but not be present in the model? Right, now that I reverted the changes to the ToolbarActionsBar, the action should exist iff it's present in the model (except for in tests). That means this wouldn't get called. Changing the method and its name to take care of just removing the action. https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:214: if (delegate_) { On 2016/07/29 19:10:22, Devlin wrote: > again, document when this can be false. Please see the comment at line 195 https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:252: if (delegate_) { On 2016/07/29 19:10:22, Devlin wrote: > ditto Please see line 195 https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.h (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.h:80: void OnToolbarActionsBarAnimationEnded() override; On 2016/07/29 19:10:22, Devlin wrote: > Put inherited methods together within the same visibility. Done. https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:368: TEST_F(MediaRouterActionUnitTest, EphemeralIcon) { On 2016/07/29 19:10:22, Devlin wrote: > What about also testing the perma-shown icon? Created. https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action_unittest.cc:377: // Create a reference to initiator contents. On 2016/07/29 19:10:22, Devlin wrote: > I don't follow this comment. What's an initiator contents? What are we > creating? It's web contents for which the dialog is being initialized. Removing the comment as it's not really adding info. https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:114: pref_service = browser_->profile()->GetPrefs(); On 2016/07/29 03:12:15, msw wrote: > nit: use curly braces around this to declare pref_service in this case: > case IDC_MEDIA_ROUTER_CLOUD_SERVICES_TOGGLE: { > PrefService* pref_service = browser_->profile()->GetPrefs(); > ... > } > (otherwise, please init |pref_service| to nullptr above) Done. https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:96: controller_.get()->OnDialogClosed(); On 2016/07/29 19:10:23, Devlin wrote: > This .get() is unnecessary. Done. https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:208: action_->OnPopupHidden(); On 2016/07/29 19:10:22, Devlin wrote: > It seems a little messy to have so much two-way communication between > MediaRouterDialogControllerImpl and MediaRouterAction. Can we make the action > only respond to input from the controller, rather than needing to query it so > frequently? Or make the action only observe the controller, rather than needing > the controller to inform the action? I got rid of a call to MediaRouterAction::UpdateVisibility. Now the action methods we call from the controller are OnPopupHidden and OnPopupShown. The controller methods we call from the action are SetMediaRouterAction, IsShowingMediaRouterDialog, ShowMediaRouterDialog, and HideMediaRouterDialog. We might be able to get rid of the calls to IsShowingMediaRouterDialog by keeping a boolean for whether the popup is shown, and updating it from OnPopupShown/Hidden and when there are tab switches etc. Would that be better than calling IsShowingMediaRouterDialog? I'm not sure what you mean by make the action observe the controller. Do you mean making an observer interface for the controller, and making the controller keep a list of observers that it calls on events? https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:357: ComponentToolbarActionsFactory::kMediaRouterActionId)) { On 2016/07/29 19:10:22, Devlin wrote: > indentation seems off here. Fixed. https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h:48: On 2016/07/29 03:12:15, msw wrote: > nit: remove blank line Done.
lgtm with a nit. I don't think cpu@ still does chromium reviews often, you should seek an alternate reviewer. https://codereview.chromium.org/2155293002/diff/140001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2155293002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:113: PrefService* pref_service; nit: init the value here, by combining this with the statement below, like: PrefService* pref_service = browser_->profile()->GetPrefs();
takumif@chromium.org changed reviewers: - cpu@chromium.org
takumif@chromium.org changed reviewers: + grt@chromium.org
+grt@: Please take a look at chrome/app/chrome_command_ids.h. Thank you! https://codereview.chromium.org/2155293002/diff/140001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2155293002/diff/140001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:113: PrefService* pref_service; On 2016/08/02 17:37:41, msw wrote: > nit: init the value here, by combining this with the statement below, like: > PrefService* pref_service = browser_->profile()->GetPrefs(); Done.
https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:195: if (!delegate_) On 2016/08/02 04:58:41, takumif wrote: > On 2016/07/29 19:10:22, Devlin wrote: > > Document when this can happen. > > This should only happen in tests. Would it make more sense to override these > methods in the TestMediaRouterAction class and check for |delegate_| there (and > replace the ones here with DCHECKs)? Something like: > > void TestMediaRouterAction::UpdateVisibility() override { > if (!delegate_) > return; > MediaRouterAction::UpdateVisibility(); > } Ideally, I think my preference would be to create a TestToolbarActionViewDelegate() with stub implementations for most of the methods. Is that feasible? https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:208: action_->OnPopupHidden(); On 2016/08/02 04:58:42, takumif wrote: > On 2016/07/29 19:10:22, Devlin wrote: > > It seems a little messy to have so much two-way communication between > > MediaRouterDialogControllerImpl and MediaRouterAction. Can we make the action > > only respond to input from the controller, rather than needing to query it so > > frequently? Or make the action only observe the controller, rather than > needing > > the controller to inform the action? > > I got rid of a call to MediaRouterAction::UpdateVisibility. > > Now the action methods we call from the controller are OnPopupHidden and > OnPopupShown. > > The controller methods we call from the action are SetMediaRouterAction, > IsShowingMediaRouterDialog, ShowMediaRouterDialog, and HideMediaRouterDialog. > > We might be able to get rid of the calls to IsShowingMediaRouterDialog by > keeping a boolean for whether the popup is shown, and updating it from > OnPopupShown/Hidden and when there are tab switches etc. Would that be better > than calling IsShowingMediaRouterDialog? > > I'm not sure what you mean by make the action observe the controller. Do you > mean making an observer interface for the controller, and making the controller > keep a list of observers that it calls on events? At a high level, it's good to avoid two-way communication like this in code, because it tightly couples everything. It makes it more difficult to test, and generally leads to code that is also more difficult to understand. If there's a way to have at least one of these classes be unaware of the other, it's preferable. An Observer or Delegate implementation is often used towards that end - the observer posts events without knowledge of its consumers, and a delegate is unaware of the owner's logic; they are each self-contained implementations. Those aren't the only solutions; they just happen to be used commonly. All that said, I'm not an OWNER of media router-specific code (just toolbar and extension stuff), and this looks better than it was, so I'll leave it up to Jennifer whether this should be changed further. :) https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/component_migration_helper.cc (right): https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/component_migration_helper.cc:140: pref_service_->SetBoolean(prefs::kMediaRouterAlwaysShowActionIcon, Thinking about this, this probably warrants a comment. Something like: // We treat installation of the cast extension as a signal to permanently // show the icon in the toolbar. https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:60: static bool IsComponentActionId(const std::string& id); Is this used? https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:187: // Depress the action if the dialog is shown, release it otherwise. What if the animation was unrelated? Does it matter? https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:276: model->HasComponentAction(GetId())) { Here still, when would this object exist without a presence in the model? https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.h (right): https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.h:21: void SetMediaRouterAction(MediaRouterAction* action); Is there a reason to not pass this in in the ctor?
I'll take a closer look at the MR files after rdcronin@'s comments are addressed. https://codereview.chromium.org/2155293002/diff/180001/chrome/app/media_route... File chrome/app/media_router_strings.grdp (right): https://codereview.chromium.org/2155293002/diff/180001/chrome/app/media_route... 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 the Media Router action icon is always shown or shown only when there is an active local Media Route or a Media Router popup."> I had to read this a couple times to understand where the sentence splits. How about: "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."
Patchset #9 (id:200001) has been deleted
takumif@chromium.org changed reviewers: - msw@chromium.org
https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2155293002/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:195: if (!delegate_) On 2016/08/02 20:18:51, Devlin wrote: > On 2016/08/02 04:58:41, takumif wrote: > > On 2016/07/29 19:10:22, Devlin wrote: > > > Document when this can happen. > > > > This should only happen in tests. Would it make more sense to override these > > methods in the TestMediaRouterAction class and check for |delegate_| there > (and > > replace the ones here with DCHECKs)? Something like: > > > > void TestMediaRouterAction::UpdateVisibility() override { > > if (!delegate_) > > return; > > MediaRouterAction::UpdateVisibility(); > > } > > Ideally, I think my preference would be to create a > TestToolbarActionViewDelegate() with stub implementations for most of the > methods. Is that feasible? We do have a mock delegate, and since it wasn't always being set, I changed that. Now the dialog controller is null, and this one can't really be mocked without the web content present, so I overrode this method (now renamed to MaybeRemoveAction) in the TestMediaRouterAction to check for the presence of dialog controller there. https://codereview.chromium.org/2155293002/diff/180001/chrome/app/media_route... File chrome/app/media_router_strings.grdp (right): https://codereview.chromium.org/2155293002/diff/180001/chrome/app/media_route... 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 the Media Router action icon is always shown or shown only when there is an active local Media Route or a Media Router popup."> On 2016/08/02 21:22:20, apacible wrote: > I had to read this a couple times to understand where the sentence splits. > > How about: "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." Done. https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/component_migration_helper.cc (right): https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/component_migration_helper.cc:140: pref_service_->SetBoolean(prefs::kMediaRouterAlwaysShowActionIcon, On 2016/08/02 20:18:51, Devlin wrote: > Thinking about this, this probably warrants a comment. Something like: > // We treat installation of the cast extension as a signal to permanently > // show the icon in the toolbar. Done. https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right): https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:60: static bool IsComponentActionId(const std::string& id); On 2016/08/02 20:18:51, Devlin wrote: > Is this used? No. Removing. https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:187: // Depress the action if the dialog is shown, release it otherwise. On 2016/08/02 20:18:51, Devlin wrote: > What if the animation was unrelated? Does it matter? In that case this does nothing, as the icon would stay (un)depressed. https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:276: model->HasComponentAction(GetId())) { On 2016/08/02 20:18:51, Devlin wrote: > Here still, when would this object exist without a presence in the model? This was also only for tests. Changed the tests and removed this. https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_contextual_menu.h (right): https://codereview.chromium.org/2155293002/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_contextual_menu.h:21: void SetMediaRouterAction(MediaRouterAction* action); On 2016/08/02 20:18:51, Devlin wrote: > Is there a reason to not pass this in in the ctor? No. Putting it in the constructor.
Patchset #10 (id:240001) has been deleted
chrome/app/chrome_command_ids.h rslgtm
lgtm
https://codereview.chromium.org/2155293002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/component_migration_helper.cc (right): https://codereview.chromium.org/2155293002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/component_migration_helper.cc:144: MediaRouterAction::SetVisibilityPreference(pref_service_, true); nit: it's a little weird that other places use the pref directly, but this one calls into MediaRouterAction. I'd just use the pref so we avoid the dependency on MediaRouterAction. https://codereview.chromium.org/2155293002/diff/260001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2155293002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:240: delegate_->OnPopupShown(true); Calling OnPopupShown()/Hidden() so much like this makes me worried. Right now, the logic contained in there is pretty limited, but there's no guarantee it would be forever. Can we make this more deterministic, and make sure we don't call Shown() twice without calling Hidden(), etc?
takumif@chromium.org changed reviewers: - grt@chromium.org
https://codereview.chromium.org/2155293002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/component_migration_helper.cc (right): https://codereview.chromium.org/2155293002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/component_migration_helper.cc:144: MediaRouterAction::SetVisibilityPreference(pref_service_, true); On 2016/08/09 23:02:20, Devlin wrote: > nit: it's a little weird that other places use the pref directly, but this one > calls into MediaRouterAction. I'd just use the pref so we avoid the dependency > on MediaRouterAction. Done. https://codereview.chromium.org/2155293002/diff/260001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2155293002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:240: delegate_->OnPopupShown(true); On 2016/08/09 23:02:20, Devlin wrote: > Calling OnPopupShown()/Hidden() so much like this makes me worried. Right now, > the logic contained in there is pretty limited, but there's no guarantee it > would be forever. Can we make this more deterministic, and make sure we don't > call Shown() twice without calling Hidden(), etc? We're calling ToolbarActionViewDelegate::OnPopupShown()/Closed() to update the (un)pressed state of the action icon. We call them even when the popup doesn't really get opened/closed (e.g. tab switches) to update the icon. Can we add (Un)PressMenuButton() methods to ToolbarActionViewDelegate and ToolbarActionView so that we can call those instead (and call ToolbarActionViewDelegate::OnPopupShown()/Closed() only from MediaRouterAction::OnPopupShown()/Hidden()) ?
https://codereview.chromium.org/2155293002/diff/260001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2155293002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:240: delegate_->OnPopupShown(true); On 2016/08/10 18:32:37, takumif wrote: > On 2016/08/09 23:02:20, Devlin wrote: > > Calling OnPopupShown()/Hidden() so much like this makes me worried. Right > now, > > the logic contained in there is pretty limited, but there's no guarantee it > > would be forever. Can we make this more deterministic, and make sure we don't > > call Shown() twice without calling Hidden(), etc? > > We're calling ToolbarActionViewDelegate::OnPopupShown()/Closed() to update the > (un)pressed state of the action icon. We call them even when the popup doesn't > really get opened/closed (e.g. tab switches) to update the icon. Can we add > (Un)PressMenuButton() methods to ToolbarActionViewDelegate and ToolbarActionView > so that we can call those instead (and call > ToolbarActionViewDelegate::OnPopupShown()/Closed() only from > MediaRouterAction::OnPopupShown()/Hidden()) ? I think calling OnPopupShown()/Closed() when e.g. switching tabs is fine, because for the view delegate, that does signify the popup being shown or closed. My concern is more around that we can call it more frequently, e.g. in ToolbarActionsBarAnimationEnded() or in UpdatePopupState(). I'd rather we keep track of the popup's "shown" state and only call these methods when there's a change. Does that make sense?
https://codereview.chromium.org/2155293002/diff/260001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2155293002/diff/260001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:240: delegate_->OnPopupShown(true); On 2016/08/11 20:34:45, Devlin wrote: > On 2016/08/10 18:32:37, takumif wrote: > > On 2016/08/09 23:02:20, Devlin wrote: > > > Calling OnPopupShown()/Hidden() so much like this makes me worried. Right > > now, > > > the logic contained in there is pretty limited, but there's no guarantee it > > > would be forever. Can we make this more deterministic, and make sure we > don't > > > call Shown() twice without calling Hidden(), etc? > > > > We're calling ToolbarActionViewDelegate::OnPopupShown()/Closed() to update the > > (un)pressed state of the action icon. We call them even when the popup doesn't > > really get opened/closed (e.g. tab switches) to update the icon. Can we add > > (Un)PressMenuButton() methods to ToolbarActionViewDelegate and > ToolbarActionView > > so that we can call those instead (and call > > ToolbarActionViewDelegate::OnPopupShown()/Closed() only from > > MediaRouterAction::OnPopupShown()/Hidden()) ? > > I think calling OnPopupShown()/Closed() when e.g. switching tabs is fine, > because for the view delegate, that does signify the popup being shown or > closed. My concern is more around that we can call it more frequently, e.g. in > ToolbarActionsBarAnimationEnded() or in UpdatePopupState(). I'd rather we keep > track of the popup's "shown" state and only call these methods when there's a > change. Does that make sense? +1 rdcronin's recommendation, also discussed a bit offline with takumif :)
Patchset #13 (id:320001) has been deleted
Description was changed from ========== Show the Cast toolbar icon ephemerally when Cast is in use This CL changes the behavior of the Cast/Media Router toolbar component action icon. Currently, the only way to add the icon is by installing the Cast extension from the Web Store. After this change, the icon will be displayed temporarily whenever the Media Router dialog is open, or there is an active local media route. 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 also change the default position of the component actions on the toolbar from the leftmost to the rightmost (same behavior as extension actions). BUG=594577 ========== to ========== Show the Cast toolbar icon ephemerally when Cast is in use This CL changes the behavior of the Cast/Media Router toolbar component action icon. Currently, the only way to add the icon is by installing the Cast extension from the Web Store. After this change, the icon will be displayed temporarily whenever the Media Router dialog is open, or there is an active local media route. 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). BUG=594577 ==========
Description was changed from ========== Show the Cast toolbar icon ephemerally when Cast is in use This CL changes the behavior of the Cast/Media Router toolbar component action icon. Currently, the only way to add the icon is by installing the Cast extension from the Web Store. After this change, the icon will be displayed temporarily whenever the Media Router dialog is open, or there is an active local media route. 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). BUG=594577 ========== to ========== Show the Cast toolbar icon ephemerally when Cast is in use This CL changes the behavior of the Cast/Media Router toolbar component action icon. Currently, the only way to add the icon is by installing the Cast extension from the Web Store. After this change, the icon will be displayed temporarily whenever the Media Router dialog is open, or there is an active local media route. 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). BUG=594577 ==========
Patchset #13 (id:340001) has been deleted
Patchset #13 (id:360001) has been deleted
Patchset #13 (id:380001) has been deleted
I've added a boolean value that needs to be flipped every time OnPopupShown()/Closed() is called. I've also moved MaybeUpdateIcon() out of MRA::OnDialogHidden(), so that it needs to be called explicitly (e.g. it was confusing for MRA::UpdateDialogState() to sometimes result in the destruction of the action). Thanks for your time!
toolbar stuff lgtm, thanks for your patience on this! https://codereview.chromium.org/2155293002/diff/400001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2155293002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:224: // |controller| keeps track of |this| if |this| was created with the browser nit: indentation. https://codereview.chromium.org/2155293002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:263: DCHECK(delegate_); nit: This could even be UpdateState(), right? https://codereview.chromium.org/2155293002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:273: ->RemoveComponentAction(GetId()); nit: this could result in the deletion of this object. Typically, it's good to add a comment like: // Warning: |this| can be deleted here! so that folks know not to do anything else afterwards.
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, msw@chromium.org, apacible@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2155293002/#ps420001 (title: "Address Devlin's comments")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
takumif@chromium.org changed reviewers: + grt@chromium.org
grt@, could I get an lgtm for chrome/app/chrome_command_ids.h? Apparently "rslgtm" isn't recognized as an lgtm, so I can't commit yet. Thanks! https://codereview.chromium.org/2155293002/diff/400001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/media_router_action.cc (right): https://codereview.chromium.org/2155293002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:224: // |controller| keeps track of |this| if |this| was created with the browser On 2016/08/15 17:07:27, Devlin wrote: > nit: indentation. Done. https://codereview.chromium.org/2155293002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:263: DCHECK(delegate_); On 2016/08/15 17:07:27, Devlin wrote: > nit: This could even be UpdateState(), right? Done. https://codereview.chromium.org/2155293002/diff/400001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/media_router_action.cc:273: ->RemoveComponentAction(GetId()); On 2016/08/15 17:07:27, Devlin wrote: > nit: this could result in the deletion of this object. Typically, it's good to > add a comment like: > // Warning: |this| can be deleted here! > so that folks know not to do anything else afterwards. Done.
mfoltz@chromium.org changed reviewers: + anthonyvd@chromium.org, mfoltz@chromium.org, msw@chromium.org
Drive-by (relaying some F2F comments): - Make sure the corner case of disabling Media Router with about:flags is handled (ComponentMigrationHelper::OnFeatureDisabled) - Update the ComponentMigrationHelper unit tests Separately, we should discuss how much longer to keep the migration helper code after all users have been migrated from the Cast extension. It would be simpler to get rid of it, and just use the new context menu action/pref to manage the icon state.
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 takumif@chromium.org
On 2016/08/15 19:53:28, mfoltz_ooo_7-29_8-14 wrote: > Drive-by (relaying some F2F comments): > > - Make sure the corner case of disabling Media Router with about:flags is > handled (ComponentMigrationHelper::OnFeatureDisabled) > - Update the ComponentMigrationHelper unit tests > > Separately, we should discuss how much longer to keep the migration helper code > after all users have been migrated from the Cast extension. It would be simpler > to get rid of it, and just use the new context menu action/pref to manage the > icon state. I've addressed your two points regarding ComponentMigrationHelper. It'd be great if you could take a look. Thanks!
On 2016/08/15 at 22:44:40, takumif wrote: > On 2016/08/15 19:53:28, mfoltz_ooo_7-29_8-14 wrote: > > Drive-by (relaying some F2F comments): > > > > - Make sure the corner case of disabling Media Router with about:flags is > > handled (ComponentMigrationHelper::OnFeatureDisabled) > > - Update the ComponentMigrationHelper unit tests > > > > Separately, we should discuss how much longer to keep the migration helper code > > after all users have been migrated from the Cast extension. It would be simpler > > to get rid of it, and just use the new context menu action/pref to manage the > > icon state. > > I've addressed your two points regarding ComponentMigrationHelper. > It'd be great if you could take a look. Thanks! lgtm
takumif@chromium.org changed reviewers: - grt@chromium.org
Description was changed from ========== Show the Cast toolbar icon ephemerally when Cast is in use This CL changes the behavior of the Cast/Media Router toolbar component action icon. Currently, the only way to add the icon is by installing the Cast extension from the Web Store. After this change, the icon will be displayed temporarily whenever the Media Router dialog is open, or there is an active local media route. 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). BUG=594577 ========== to ========== Show the Cast toolbar icon ephemerally when Cast is in use This CL changes the behavior of the Cast/Media Router toolbar component action icon. Currently, the only way to add the icon is by installing the Cast extension from the Web Store. After this change, the icon will be displayed temporarily whenever the Media Router dialog is open, or there is an active local media route. 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). TBR=grt@chromium.org BUG=594577 ==========
takumif@chromium.org changed reviewers: + grt@chromium.org
The CQ bit was checked by takumif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, apacible@chromium.org, anthonyvd@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2155293002/#ps440001 (title: "Address Mark's comment, trybot failures, 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 ========== Show the Cast toolbar icon ephemerally when Cast is in use This CL changes the behavior of the Cast/Media Router toolbar component action icon. Currently, the only way to add the icon is by installing the Cast extension from the Web Store. After this change, the icon will be displayed temporarily whenever the Media Router dialog is open, or there is an active local media route. 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). TBR=grt@chromium.org BUG=594577 ========== to ========== Show the Cast toolbar icon ephemerally when Cast is in use This CL changes the behavior of the Cast/Media Router toolbar component action icon. Currently, the only way to add the icon is by installing the Cast extension from the Web Store. After this change, the icon will be displayed temporarily whenever the Media Router dialog is open, or there is an active local media route. 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). TBR=grt@chromium.org BUG=594577 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Show the Cast toolbar icon ephemerally when Cast is in use This CL changes the behavior of the Cast/Media Router toolbar component action icon. Currently, the only way to add the icon is by installing the Cast extension from the Web Store. After this change, the icon will be displayed temporarily whenever the Media Router dialog is open, or there is an active local media route. 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). TBR=grt@chromium.org BUG=594577 ========== to ========== Show the Cast toolbar icon ephemerally when Cast is in use This CL changes the behavior of the Cast/Media Router toolbar component action icon. Currently, the only way to add the icon is by installing the Cast extension from the Web Store. After this change, the icon will be displayed temporarily whenever the Media Router dialog is open, or there is an active local media route. 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). TBR=grt@chromium.org BUG=594577 Committed: https://crrev.com/de716ca8b6cffb4c7ae30990b3288b3499d28a4c Cr-Commit-Position: refs/heads/master@{#412280} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/de716ca8b6cffb4c7ae30990b3288b3499d28a4c Cr-Commit-Position: refs/heads/master@{#412280}
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:440001) has been created in https://codereview.chromium.org/2254543002/ by takumif@chromium.org. The reason for reverting is: This gives a compilation error for a Chrome-build-only unit test (MediaRouterContextualMenuUnitTest.ToggleCloudServicesItem). The unit test needs to be fixed..
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:440001) has been created in https://codereview.chromium.org/2260343002/ by takumif@chromium.org. The reason for reverting is: Reverting this CL since it is causing crashes and needs a redesign. Also reverting a related CL: https://codereview.chromium.org/2260873003/. |
