New issue
Advanced search Search tips

Issue 660972 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

Redesign ComponentToolbarActionsFactory and ComponentMigrationHelper

Project Member Reported by taku...@chromium.org, Oct 31 2016

Issue description

This CL [1] makes ToolbarActionsModel ignore AddComponentAction() calls before initialization. Such calls are sometimes made by MediaRouterActionController, which results in the media router toolbar action not being shown when it's supposed to.

To be able to show the ignored actions at ToolbarActionsModel's initialization, we should redesign ComponentToolbarActionsFactory and ComponentMigrationHelper (more discussion in the CL's comments).

[1] https://codereview.chromium.org/2450633004/
 

Comment 1 by mfo...@chromium.org, Oct 31 2016

Note:

We are pretty much force migrating everyone onto Media Router for M56, removing the Cast extension soon thereafter, and then updating documentation for users to use the built in context menu instead of going through the extension install flow.

Unless there are other anticipated users of the component migration helper, I would be fine with deleting it.

Devlin, do you know of any other actions that might use the ComponentMigrationHelper in the future? Will it be okay to remove it?
Remove it! :)  Unless there is something that can use it immediately or there is ongoing work, we shouldn't keep unused code around - it's too easy to think something might use it, have it go another way, and forget about it.  Plus, it's git, so it's always there if we need it again. :)
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 10 2017

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

commit 0cd92266b3b37f1218115a33e6fd08f693d7c421
Author: takumif <takumif@chromium.org>
Date: Fri Feb 10 17:51:51 2017

Remove extension-to-component migration mechanism

Now that we've retired the old Cast extension from CWS, the migration mechanism
from the old extension to Media Router is no longer necessary. So this CL
removes ComponentMigrationHelper and the migration mechanism around it.

ComponentActionDelegate, an interface implemented by ToolbarActionsModel, used
to belong to ComponentMigrationHelper. It'll now be in its own file under
c/b/ui/toolbar.

ComponentMigrationHelper used to be responsible for determining whether to pin
the Cast/Media Router toolbar icon, and now MediaRouterActionController will be
responsible for it.

BUG= 660972 

Review-Url: https://codereview.chromium.org/2678083005
Cr-Commit-Position: refs/heads/master@{#449651}

[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/extensions/BUILD.gn
[delete] https://crrev.com/b5de4603585dba1e41900cbc5dc3fd4a65d200ff/chrome/browser/extensions/component_migration_helper.cc
[delete] https://crrev.com/b5de4603585dba1e41900cbc5dc3fd4a65d200ff/chrome/browser/extensions/component_migration_helper.h
[delete] https://crrev.com/b5de4603585dba1e41900cbc5dc3fd4a65d200ff/chrome/browser/extensions/component_migration_helper_unittest.cc
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/prefs/browser_prefs.cc
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/BUILD.gn
[add] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/toolbar/component_action_delegate.h
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/toolbar/component_toolbar_actions_factory.h
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/toolbar/media_router_action_controller.cc
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/toolbar/media_router_action_controller.h
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/toolbar/media_router_contextual_menu.cc
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/toolbar/media_router_contextual_menu.h
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/toolbar/mock_media_router_action_controller.cc
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/toolbar/toolbar_actions_model.cc
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/toolbar/toolbar_actions_model.h
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/common/pref_names.cc
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/common/pref_names.h
[modify] https://crrev.com/0cd92266b3b37f1218115a33e6fd08f693d7c421/chrome/test/BUILD.gn

Status: Started (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Feb 24 2017

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

commit e6007d0b43c4a3ee34598349aa3246086f451feb
Author: takumif <takumif@chromium.org>
Date: Fri Feb 24 23:27:06 2017

Make ToolbarActionsModel own ComponentToolbarActionsFactory

This CL makes ToolbarActionsModel own ComponentToolbarActionsFactory, which used
to be a stateless singleton.

This change also makes ComponentToolbarActionsFactory keep track of component
actions that call ToolbarActionsModel::AddComponentAction() before
ToolbarActionsModel is initialized. Those actions are added to the set obtained
via ComponentToolbarActionsFactory::GetInitialComponentIds(), and are added to
the toolbar in ToolbarActionsModel::Populate().

MockComponentToolbarActionsFactory and tests are modified to reflect the changes
to ComponentToolbarActionsFactory.

BUG= 660972 

Review-Url: https://codereview.chromium.org/2613713005
Cr-Commit-Position: refs/heads/master@{#452992}

[modify] https://crrev.com/e6007d0b43c4a3ee34598349aa3246086f451feb/chrome/browser/policy/policy_browsertest.cc
[modify] https://crrev.com/e6007d0b43c4a3ee34598349aa3246086f451feb/chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc
[modify] https://crrev.com/e6007d0b43c4a3ee34598349aa3246086f451feb/chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc
[modify] https://crrev.com/e6007d0b43c4a3ee34598349aa3246086f451feb/chrome/browser/ui/toolbar/component_toolbar_actions_factory.h
[modify] https://crrev.com/e6007d0b43c4a3ee34598349aa3246086f451feb/chrome/browser/ui/toolbar/component_toolbar_actions_factory_unittest.cc
[modify] https://crrev.com/e6007d0b43c4a3ee34598349aa3246086f451feb/chrome/browser/ui/toolbar/media_router_contextual_menu.cc
[modify] https://crrev.com/e6007d0b43c4a3ee34598349aa3246086f451feb/chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc
[modify] https://crrev.com/e6007d0b43c4a3ee34598349aa3246086f451feb/chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h
[modify] https://crrev.com/e6007d0b43c4a3ee34598349aa3246086f451feb/chrome/browser/ui/toolbar/toolbar_actions_model.cc
[modify] https://crrev.com/e6007d0b43c4a3ee34598349aa3246086f451feb/chrome/browser/ui/toolbar/toolbar_actions_model.h
[modify] https://crrev.com/e6007d0b43c4a3ee34598349aa3246086f451feb/chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment