Remove extension-to-component migration mechanism

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}
Committed: https://chromium.googlesource.com/chromium/src/+/0cd92266b3b37f1218115a33e6fd08f693d7c421

[takumif, 2017-02-07 22:20:35]

Description was changed from

==========
Remove ComponentMigrationHelper

BUG=660972
==========

to

==========
Remove ComponentMigrationHelper

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.

BUG=660972
==========

[takumif, 2017-02-07 22:25:06]

Description was changed from

==========
Remove ComponentMigrationHelper

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.

BUG=660972
==========

to

==========
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.

BUG=660972
==========

[takumif, 2017-02-07 22:42:34]

Patchset #1 (id:1) has been deleted

[takumif, 2017-02-07 22:42:41]

Patchset #1 (id:20001) has been deleted

[takumif, 2017-02-07 22:49:09]

Description was changed from

==========
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.

BUG=660972
==========

to

==========
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 MediaRouterActionController will now be
responsible for it.

BUG=660972
==========

[takumif, 2017-02-07 22:49:52]

Description was changed from

==========
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 MediaRouterActionController will now be
responsible for it.

BUG=660972
==========

to

==========
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
==========

[takumif, 2017-02-07 22:51:43]

Patchset #1 (id:40001) has been deleted

[takumif, 2017-02-07 23:02:11]

takumif@chromium.org changed reviewers:
+ gab@chromium.org, imcheng@chromium.org, msw@chromium.org,
rdevlin.cronin@chromium.org

[takumif, 2017-02-07 23:02:12]

Please take a look at:

imcheng: *media_router*, overall
rdcronin: c/b/extensions/, *toolbar_actions_model*,
*component_toolbar_actions_factory*
msw: c/b/ui/toolbar/
gab: c/b/prefs/

Thank you!

[msw, 2017-02-08 02:14:02]

c/b/ui/toolbar lgtm with nits

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc
(right):

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:60:
router_.reset(new media_router::MockMediaRouter());
optional nit: base::MakeUnique here and twice more below

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc
(right):

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc:29: if
(extensions::FeatureSwitch::extension_action_redesign()->IsEnabled()) {
nit: remove curlies

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc:41: return
std::unique_ptr<ToolbarActionViewController>(
optional nit: base::MakeUnique

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:30: void
set_migrated_extension_id(const extensions::ExtensionId& extension_id) {
nit: remove this and |migrated_extension_id_| if they're unused

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:34: void
set_migrated_feature_enabled(bool enabled) {
nit: remove this and |migrated_feature_enabled_| if they're unused

[takumif, 2017-02-08 03:35:41]

Thanks for reviewing!

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc
(right):

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_controller_unittest.cc:60:
router_.reset(new media_router::MockMediaRouter());
On 2017/02/08 02:14:02, msw wrote:
> optional nit: base::MakeUnique here and twice more below

Done.

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc
(right):

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc:29: if
(extensions::FeatureSwitch::extension_action_redesign()->IsEnabled()) {
On 2017/02/08 02:14:02, msw wrote:
> nit: remove curlies

Done.

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc:41: return
std::unique_ptr<ToolbarActionViewController>(
On 2017/02/08 02:14:02, msw wrote:
> optional nit: base::MakeUnique

Done.

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:30: void
set_migrated_extension_id(const extensions::ExtensionId& extension_id) {
On 2017/02/08 02:14:02, msw wrote:
> nit: remove this and |migrated_extension_id_| if they're unused

Done.

https://codereview.chromium.org/2678083005/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:34: void
set_migrated_feature_enabled(bool enabled) {
On 2017/02/08 02:14:02, msw wrote:
> nit: remove this and |migrated_feature_enabled_| if they're unused

Done.

[Devlin, 2017-02-08 15:20:02]

Woo!  Love killing old code :)  A couple quick comments

https://codereview.chromium.org/2678083005/diff/80001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action_controller.cc (right):

https://codereview.chromium.org/2678083005/diff/80001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_controller.cc:119:
MediaRouterActionController::GetAlwaysShowActionPref(profile_);
nit: no need to prefix this, just GetAlwaysShowActionPref(profile_).

https://codereview.chromium.org/2678083005/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc
(right):

https://codereview.chromium.org/2678083005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:104: return
MediaRouterActionController::SetAlwaysShowActionPref(
no return from void function

https://codereview.chromium.org/2678083005/diff/80001/chrome/common/pref_name...
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/2678083005/diff/80001/chrome/common/pref_name...
chrome/common/pref_names.cc:1313: "toolbar_migrated_component_action_status";
We need to clean this pref up, meaning leave it in and remove it from users'
machines for awhile.  See MigrateObsoleteBrowserPrefs().

[takumif, 2017-02-08 17:38:15]

https://codereview.chromium.org/2678083005/diff/80001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action_controller.cc (right):

https://codereview.chromium.org/2678083005/diff/80001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_controller.cc:119:
MediaRouterActionController::GetAlwaysShowActionPref(profile_);
On 2017/02/08 15:20:01, Devlin (OOO feb 8 and feb 9) wrote:
> nit: no need to prefix this, just GetAlwaysShowActionPref(profile_).

Done.

https://codereview.chromium.org/2678083005/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc
(right):

https://codereview.chromium.org/2678083005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:104: return
MediaRouterActionController::SetAlwaysShowActionPref(
On 2017/02/08 15:20:01, Devlin (OOO feb 8 and feb 9) wrote:
> no return from void function

Oops, fixed.

https://codereview.chromium.org/2678083005/diff/80001/chrome/common/pref_name...
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/2678083005/diff/80001/chrome/common/pref_name...
chrome/common/pref_names.cc:1313: "toolbar_migrated_component_action_status";
On 2017/02/08 15:20:01, Devlin (OOO feb 8 and feb 9) wrote:
> We need to clean this pref up, meaning leave it in and remove it from users'
> machines for awhile.  See MigrateObsoleteBrowserPrefs().

Got it. I'm assuming it should go in MigrateObsoleteProfilePrefs(), since it's a
profile pref.

[gab, 2017-02-08 17:47:31]

https://codereview.chromium.org/2678083005/diff/80001/chrome/common/pref_name...
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/2678083005/diff/80001/chrome/common/pref_name...
chrome/common/pref_names.cc:1313: "toolbar_migrated_component_action_status";
On 2017/02/08 17:38:15, takumif wrote:
> On 2017/02/08 15:20:01, Devlin (OOO feb 8 and feb 9) wrote:
> > We need to clean this pref up, meaning leave it in and remove it from users'
> > machines for awhile.  See MigrateObsoleteBrowserPrefs().
> 
> Got it. I'm assuming it should go in MigrateObsoleteProfilePrefs(), since it's
a
> profile pref.

Correct. Still do remove the pref_names.h entry and inline it in
browser_prefs.cc's anonymous namespace as is done for other prefs there.

https://codereview.chromium.org/2678083005/diff/100001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2678083005/diff/100001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:742:
profile_prefs->ClearPref(kRestoreStartupURLsMigrationTime);
Remove year+ old migrations (there is a rolling cleanup policy here since no one
gets back to theirs a year later).

[takumif, 2017-02-08 18:12:14]

https://codereview.chromium.org/2678083005/diff/80001/chrome/common/pref_name...
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/2678083005/diff/80001/chrome/common/pref_name...
chrome/common/pref_names.cc:1313: "toolbar_migrated_component_action_status";
On 2017/02/08 17:47:30, gab wrote:
> On 2017/02/08 17:38:15, takumif wrote:
> > On 2017/02/08 15:20:01, Devlin (OOO feb 8 and feb 9) wrote:
> > > We need to clean this pref up, meaning leave it in and remove it from
users'
> > > machines for awhile.  See MigrateObsoleteBrowserPrefs().
> > 
> > Got it. I'm assuming it should go in MigrateObsoleteProfilePrefs(), since
it's
> a
> > profile pref.
> 
> Correct. Still do remove the pref_names.h entry and inline it in
> browser_prefs.cc's anonymous namespace as is done for other prefs there.

Done.

https://codereview.chromium.org/2678083005/diff/100001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2678083005/diff/100001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:742:
profile_prefs->ClearPref(kRestoreStartupURLsMigrationTime);
On 2017/02/08 17:47:30, gab wrote:
> Remove year+ old migrations (there is a rolling cleanup policy here since no
one
> gets back to theirs a year later).

Done.

[gab, 2017-02-08 19:47:00]

prefs lgtm

[imcheng, 2017-02-08 22:59:29]

lgtm

https://codereview.chromium.org/2678083005/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/component_action_delegate.h (right):

https://codereview.chromium.org/2678083005/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_action_delegate.h:10: // Object that knows
how to manage component actions in the toolbar model.
s/Object that knows how to manage/Manages

https://codereview.chromium.org/2678083005/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_action_delegate.h:13: // Adds or removes the
component action labeled by |action_id| from the
This needs a virtual destructor.

https://codereview.chromium.org/2678083005/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_action_delegate.h:14: // toolbar model.  The
caller will not add the same action twice.
s/will/must

https://codereview.chromium.org/2678083005/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action_controller.h (right):

https://codereview.chromium.org/2678083005/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_controller.h:27:
ComponentActionDelegate* component_action_delegate);
Can this be forward declared?

[takumif, 2017-02-09 00:05:56]

https://codereview.chromium.org/2678083005/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/component_action_delegate.h (right):

https://codereview.chromium.org/2678083005/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_action_delegate.h:10: // Object that knows
how to manage component actions in the toolbar model.
On 2017/02/08 22:59:29, imcheng wrote:
> s/Object that knows how to manage/Manages

Done.

https://codereview.chromium.org/2678083005/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_action_delegate.h:13: // Adds or removes the
component action labeled by |action_id| from the
On 2017/02/08 22:59:29, imcheng wrote:
> This needs a virtual destructor.

Done.

https://codereview.chromium.org/2678083005/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_action_delegate.h:14: // toolbar model.  The
caller will not add the same action twice.
On 2017/02/08 22:59:29, imcheng wrote:
> s/will/must

Done.

https://codereview.chromium.org/2678083005/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action_controller.h (right):

https://codereview.chromium.org/2678083005/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_controller.h:27:
ComponentActionDelegate* component_action_delegate);
On 2017/02/08 22:59:29, imcheng wrote:
> Can this be forward declared?

Done.

[Devlin, 2017-02-09 23:46:59]

lgtm

[takumif, 2017-02-09 23:49:34]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 23:50:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 01:04:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 01:04:40]

Dry run: This issue passed the CQ dry run.

[takumif, 2017-02-10 17:37:24]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 17:37:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 17:45:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 17:45:09]

Dry run: This issue passed the CQ dry run.

[takumif, 2017-02-10 17:45:39]

The CQ bit was checked by takumif@chromium.org

[takumif, 2017-02-10 17:45:39]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
imcheng@chromium.org, gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2678083005/#ps140001
(title: "Address Derek's comments")

[commit-bot: I haz the power, 2017-02-10 17:46:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 17:51:55]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486748739305810,
"parent_rev": "b5de4603585dba1e41900cbc5dc3fd4a65d200ff", "commit_rev":
"0cd92266b3b37f1218115a33e6fd08f693d7c421"}

[commit-bot: I haz the power, 2017-02-10 17:52:30]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/0cd92266b3b37f1218115a33e6fd...
==========

[commit-bot: I haz the power, 2017-02-10 17:52:31]

Committed patchset #5 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/0cd92266b3b37f1218115a33e6fd...