Make ToolbarActionsModel ignore calls to AddComponentAction() before initialization

Make ToolbarActionsModel ignore calls to AddComponentAction() before initialization

When MediaRouterActionController calls ToolbarActionsModel::AddComponentAction() before the model is initialized, it leads to a crash. This change makes such calls return immediately.

To add the ignored actions at initialization, we should redesign ComponentToolbarActionsFactory and ComponentMigrationHelper to be responsible for that. Bug for that: crbug.com/660972

BUG=653100
Committed: https://crrev.com/7bf2d384cc5eed3055bc5df8a6688562f6b9e532
Cr-Commit-Position: refs/heads/master@{#429036}

[takumif, 2016-10-25 19:56:27]

Patchset #1 (id:1) has been deleted

[takumif, 2016-10-25 21:25:08]

Description was changed from

==========
.

BUG=
==========

to

==========
Make MediaRouterActionController check that actions are initialized

When MediaRouterActionController calls ToolbarActionsModel::AddComponentAction()
before the model is initialized, it causes a crash. This change adds an
ActionsInitialized() method to the ComponentActionsDelegate interface (through
which MRAController accesses the model) and makes MRAController check that the
model is initialized before adding or removing the media router action.

BUG=653100
==========

[takumif, 2016-10-25 21:25:08]

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

[takumif, 2016-10-25 21:33:18]

takumif@chromium.org changed reviewers:
+ msw@chromium.org

[takumif, 2016-10-25 21:35:13]

Description was changed from

==========
Make MediaRouterActionController check that actions are initialized

When MediaRouterActionController calls ToolbarActionsModel::AddComponentAction()
before the model is initialized, it causes a crash. This change adds an
ActionsInitialized() method to the ComponentActionsDelegate interface (through
which MRAController accesses the model) and makes MRAController check that the
model is initialized before adding or removing the media router action.

BUG=653100
==========

to

==========
Make MediaRouterActionController check that actions are initialized

When MediaRouterActionController calls
ToolbarActionsModel::Add/RemoveComponentAction() before the model is
initialized, it leads to a crash. This change adds an ActionsInitialized()
method to the ComponentActionsDelegate interface (through which MRAController
accesses the model) and makes MRAController check that the model is initialized
before adding or removing the media router action.

BUG=653100
==========

[takumif, 2016-10-25 21:43:24]

Please take a look, thank you!

[Devlin, 2016-10-25 22:12:13]

Can we add a unittest that exercises this behavior?  (Ideally, it should crash
without the patch, and work with the patch.)

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

https://codereview.chromium.org/2450633004/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_controller.cc:64: if
(!component_action_delegate_->ActionsInitialized())
I'd have a slight preference to put this check in
ToolbarActionsModel::AddComponentAction().

[takumif, 2016-10-26 01:22:00]

takumif@chromium.org changed reviewers:
- msw@chromium.org

[takumif, 2016-10-26 01:22:01]

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

https://codereview.chromium.org/2450633004/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_controller.cc:64: if
(!component_action_delegate_->ActionsInitialized())
On 2016/10/25 22:12:13, Devlin wrote:
> I'd have a slight preference to put this check in
> ToolbarActionsModel::AddComponentAction().

Yeah, that would be simpler.

I thought about adding the check to RemoveComponentAction() as well, but I think
it doesn't need it because if the model isn't initialized, HasAction() wouldn't
return true, in which case Remove shouldn't be called anyways.

[imcheng, 2016-10-26 01:32:03]

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:638: if
(!actions_initialized_)
Wait, so if we simply returned here, then wouldn't the fact that we needed to
add the MR icon to the toolbar get lost?

[Devlin, 2016-10-26 16:41:56]

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:638: if
(!actions_initialized_)
On 2016/10/26 01:32:03, imcheng wrote:
> Wait, so if we simply returned here, then wouldn't the fact that we needed to
> add the MR icon to the toolbar get lost?

We would have the same problem in Patch Set 1, if it exists here.

ToolbarActionsModel::OnReady checks with the ComponentActionsFactory for any
component actions.  Ideally, we should be able to add the component action iff
actions are initialized, and fetch all component actions once initialization
happens.

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right):

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:1468:
toolbar_model->AddComponentAction(component_action_id());
While this will trigger the crash, I think this would be more useful as a test
if it more closely replicated what was happening, where we were trying to
migrate MR over.

[takumif, 2016-10-26 18:42:06]

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:638: if
(!actions_initialized_)
On 2016/10/26 16:41:56, Devlin wrote:
> On 2016/10/26 01:32:03, imcheng wrote:
> > Wait, so if we simply returned here, then wouldn't the fact that we needed
to
> > add the MR icon to the toolbar get lost?
> 
> We would have the same problem in Patch Set 1, if it exists here.
> 
> ToolbarActionsModel::OnReady checks with the ComponentActionsFactory for any
> component actions.  Ideally, we should be able to add the component action iff
> actions are initialized, and fetch all component actions once initialization
> happens.

Right, the information does get lost. I think we do need to make MRAController
an observer for TAM, and call MRAC::MaybeAddOrRemoveAction() when TAM is
initialized. I'll add a new patch for that.

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right):

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:1468:
toolbar_model->AddComponentAction(component_action_id());
On 2016/10/26 16:41:56, Devlin wrote:
> While this will trigger the crash, I think this would be more useful as a test
> if it more closely replicated what was happening, where we were trying to
> migrate MR over.

I believe this is close to what is happening. MRAController is calling
TAM::AddComponentAction() to show the icon ephemerally, and that sometimes
happens before TAM is initialized. I think it isn't really related to migration.

[takumif, 2016-10-27 00:15:21]

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:638: if
(!actions_initialized_)
On 2016/10/26 18:42:05, takumif wrote:
> On 2016/10/26 16:41:56, Devlin wrote:
> > On 2016/10/26 01:32:03, imcheng wrote:
> > > Wait, so if we simply returned here, then wouldn't the fact that we needed
> to
> > > add the MR icon to the toolbar get lost?
> > 
> > We would have the same problem in Patch Set 1, if it exists here.
> > 
> > ToolbarActionsModel::OnReady checks with the ComponentActionsFactory for any
> > component actions.  Ideally, we should be able to add the component action
iff
> > actions are initialized, and fetch all component actions once initialization
> > happens.
> 
> Right, the information does get lost. I think we do need to make MRAController
> an observer for TAM, and call MRAC::MaybeAddOrRemoveAction() when TAM is
> initialized. I'll add a new patch for that.

As I've discussed with Derek offline, there's an issue with MRAC calling
AddComponentAction() in OnToolbarModelInitialized(). If MRAC is notified before
ToolbarActionsBars are, then AddComponentAction() might lead to
TAB::OnToolbarActionAdded() getting called with (from TAB's perspective) an
invalid index.

To ensure that TAM::AddComponentAction() is called after
TAB::OnToolbarModelInitialized(), we could add another observer method to
TAM::Observer that gets called after OnToolbarModelInitialized(), in which MRAC
calls AddComponentAction(). Another option is to make MRAC post a task to
TaskRunner in its OnToolbarModelInitialized().

Devlin, do you have a suggestion on what would be a good solution here?

[Devlin, 2016-10-28 15:17:15]

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:638: if
(!actions_initialized_)
On 2016/10/27 00:15:21, takumif wrote:
> On 2016/10/26 18:42:05, takumif wrote:
> > On 2016/10/26 16:41:56, Devlin wrote:
> > > On 2016/10/26 01:32:03, imcheng wrote:
> > > > Wait, so if we simply returned here, then wouldn't the fact that we
needed
> > to
> > > > add the MR icon to the toolbar get lost?
> > > 
> > > We would have the same problem in Patch Set 1, if it exists here.
> > > 
> > > ToolbarActionsModel::OnReady checks with the ComponentActionsFactory for
any
> > > component actions.  Ideally, we should be able to add the component action
> iff
> > > actions are initialized, and fetch all component actions once
initialization
> > > happens.
> > 
> > Right, the information does get lost. I think we do need to make
MRAController
> > an observer for TAM, and call MRAC::MaybeAddOrRemoveAction() when TAM is
> > initialized. I'll add a new patch for that.
> 
> As I've discussed with Derek offline, there's an issue with MRAC calling
> AddComponentAction() in OnToolbarModelInitialized(). If MRAC is notified
before
> ToolbarActionsBars are, then AddComponentAction() might lead to
> TAB::OnToolbarActionAdded() getting called with (from TAB's perspective) an
> invalid index.
> 
> To ensure that TAM::AddComponentAction() is called after
> TAB::OnToolbarModelInitialized(), we could add another observer method to
> TAM::Observer that gets called after OnToolbarModelInitialized(), in which
MRAC
> calls AddComponentAction(). Another option is to make MRAC post a task to
> TaskRunner in its OnToolbarModelInitialized().
> 
> Devlin, do you have a suggestion on what would be a good solution here?

It seems to me part of the problem is that there are a lot of pieces here
controlling when/if the action is shown - the ComponentToolbarActionsFactory,
the migrator, and the MediaRouterActionController.  It would be best if we can
simplify the flow from the ToolbarActionsModel's perspective to be something
like:

OnReady() {
  InitializeExtensionActions();
  InitializeComponentActions();
}

AddComponentAction() {
  if (!ready)
    return;  // Will add the action in OnReady().
}

And then in MRActionController:
AddComponentAction() {
  Factory->AddComponentAction(action);
  Model->AddCompnentAction(action);
}

We more-or-less already have most of this - the ToolbarActionsModel asks for the
component actions from the factory, which is how it should be.  If the factory
can be the source of truth for which actions should be shown, it means that we
have all bases covered (if the action is added before the model is ready, we
will add it once the model is ready by retrieving it from the factory; otherwise
we add it immediately) while also not having either class have too many
implementation details about the others (like needing to post a task since other
observers need to be notified).

Does that make sense?

[takumif, 2016-10-29 00:37:23]

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/2450633004/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:638: if
(!actions_initialized_)
On 2016/10/28 15:17:15, Devlin wrote:
> On 2016/10/27 00:15:21, takumif wrote:
> > On 2016/10/26 18:42:05, takumif wrote:
> > > On 2016/10/26 16:41:56, Devlin wrote:
> > > > On 2016/10/26 01:32:03, imcheng wrote:
> > > > > Wait, so if we simply returned here, then wouldn't the fact that we
> needed
> > > to
> > > > > add the MR icon to the toolbar get lost?
> > > > 
> > > > We would have the same problem in Patch Set 1, if it exists here.
> > > > 
> > > > ToolbarActionsModel::OnReady checks with the ComponentActionsFactory for
> any
> > > > component actions.  Ideally, we should be able to add the component
action
> > iff
> > > > actions are initialized, and fetch all component actions once
> initialization
> > > > happens.
> > > 
> > > Right, the information does get lost. I think we do need to make
> MRAController
> > > an observer for TAM, and call MRAC::MaybeAddOrRemoveAction() when TAM is
> > > initialized. I'll add a new patch for that.
> > 
> > As I've discussed with Derek offline, there's an issue with MRAC calling
> > AddComponentAction() in OnToolbarModelInitialized(). If MRAC is notified
> before
> > ToolbarActionsBars are, then AddComponentAction() might lead to
> > TAB::OnToolbarActionAdded() getting called with (from TAB's perspective) an
> > invalid index.
> > 
> > To ensure that TAM::AddComponentAction() is called after
> > TAB::OnToolbarModelInitialized(), we could add another observer method to
> > TAM::Observer that gets called after OnToolbarModelInitialized(), in which
> MRAC
> > calls AddComponentAction(). Another option is to make MRAC post a task to
> > TaskRunner in its OnToolbarModelInitialized().
> > 
> > Devlin, do you have a suggestion on what would be a good solution here?
> 
> It seems to me part of the problem is that there are a lot of pieces here
> controlling when/if the action is shown - the ComponentToolbarActionsFactory,
> the migrator, and the MediaRouterActionController.  It would be best if we can
> simplify the flow from the ToolbarActionsModel's perspective to be something
> like:
> 
> OnReady() {
>   InitializeExtensionActions();
>   InitializeComponentActions();
> }
> 
> AddComponentAction() {
>   if (!ready)
>     return;  // Will add the action in OnReady().
> }
> 
> And then in MRActionController:
> AddComponentAction() {
>   Factory->AddComponentAction(action);
>   Model->AddCompnentAction(action);
> }
> 
> We more-or-less already have most of this - the ToolbarActionsModel asks for
the
> component actions from the factory, which is how it should be.  If the factory
> can be the source of truth for which actions should be shown, it means that we
> have all bases covered (if the action is added before the model is ready, we
> will add it once the model is ready by retrieving it from the factory;
otherwise
> we add it immediately) while also not having either class have too many
> implementation details about the others (like needing to post a task since
other
> observers need to be notified).
> 
> Does that make sense?

Yes, it does make sense. I think it'd still be necessary for all of
ComponentToolbarActionsFactory, ComponentMigrationHelper, and MRActionController
to call ToolbarActionsModel::Add/RemoveComponentAction() though. 

Right now, the ComponentToolbarActionsFactory doesn't make any decisions for
whether to add an action. All decisions are made by the ComponentMigrationHelper
(plus the MRActionController).
ComponentToolbarActionsFactory::GetInitialComponentIds() just returns an empty
set. The initial set of component actions are added via
ComponentToolbarActionsFactory::HandleComponentMigrations(), which just calls
ComponentMigrationHelper::OnFeatureEnabled().

Right now the ComponentMigrationHelper takes care of adding actions whose
migration pref is set to true. Things are a bit confusing because we're using
the pref as the way to tell if an action should exist at initialization, and
we're setting it even if we're adding an action for reasons other than
migration.

I think it'd make sense for the ComponentToolbarActionsFactory to generate the
initial list of component actions by looking at the prefs and keeping track of
which actions have asked to be added before ToolbarActionsModel's
initialization. Perhaps the ToolbarActionsModel can notify
ComponentToolbarActionsFactory about it.

Currently ComponentToolbarActionsFactory is a singleton. For it to generate the
initial list of actions, it needs to be per-profile, and I think it makes sense
for the ToolbarActionsModel to own it. ToolbarActionsModel is the only class
that uses it. In that case we can make ComponentToolbarActionsFactory own
ComponentMigrationHelper; the ToolbarActionsModel doesn't use the helper
directly.

To summarize:

BEFORE
ComponentMigrationHelper:
 - Changes migration prefs when migrating
 - Adds/removes actions at both initialization and migration

AFTER
ComponentMigrationHelper:
 - Changes migration prefs when migrating
 - Adds/removes actions when migration happens after initialization
ComponentToolbarActionsFactory:
 - Generates the list of actions at initialization
ToolbarActionsModel:
 - Notifies ComponentToolbarActionsFactory when someone else (e.g. 
   MRActionController) attempts to add a component action before initialization

What do you think? I think this should happen in a different CL...

[Devlin, 2016-10-31 19:10:02]

On 2016/10/29 00:37:23, takumif wrote:
> To summarize:
> 
> BEFORE
> ComponentMigrationHelper:
>  - Changes migration prefs when migrating
>  - Adds/removes actions at both initialization and migration
> 
> AFTER
> ComponentMigrationHelper:
>  - Changes migration prefs when migrating
>  - Adds/removes actions when migration happens after initialization
> ComponentToolbarActionsFactory:
>  - Generates the list of actions at initialization
> ToolbarActionsModel:
>  - Notifies ComponentToolbarActionsFactory when someone else (e.g. 
>    MRActionController) attempts to add a component action before
initialization
> 
> What do you think? I think this should happen in a different CL...

In the interest of solving this crash, I'm fine with landing a smaller fix first
and then doing the rearchitecture separately.  For the smaller fix, do we care
that the component action gets lost in the case of trying to be added before the
model is ready?

For the bigger picture, I like the idea of having the ToolbarActionsModel own
the component action stuff.  I think I'd flesh it out a little more to say that
we make a ComponentToolbarActionsProvider, which has the initial list and owns
the ComponentMigrationHelper, and the migration helper supplies the actions to
that provider.  Then the ToolbarActionsModel *only* cares about querying the
provider.  I think that would make all this much easier, but it would be a
larger change, so let's hold off on that.

[takumif, 2016-10-31 21:14:22]

On 2016/10/31 19:10:02, Devlin wrote:
> On 2016/10/29 00:37:23, takumif wrote:
> > To summarize:
> > 
> > BEFORE
> > ComponentMigrationHelper:
> >  - Changes migration prefs when migrating
> >  - Adds/removes actions at both initialization and migration
> > 
> > AFTER
> > ComponentMigrationHelper:
> >  - Changes migration prefs when migrating
> >  - Adds/removes actions when migration happens after initialization
> > ComponentToolbarActionsFactory:
> >  - Generates the list of actions at initialization
> > ToolbarActionsModel:
> >  - Notifies ComponentToolbarActionsFactory when someone else (e.g. 
> >    MRActionController) attempts to add a component action before
> initialization
> > 
> > What do you think? I think this should happen in a different CL...
> 
> In the interest of solving this crash, I'm fine with landing a smaller fix
first
> and then doing the rearchitecture separately.  For the smaller fix, do we care
> that the component action gets lost in the case of trying to be added before
the
> model is ready?
> 
> For the bigger picture, I like the idea of having the ToolbarActionsModel own
> the component action stuff.  I think I'd flesh it out a little more to say
that
> we make a ComponentToolbarActionsProvider, which has the initial list and owns
> the ComponentMigrationHelper, and the migration helper supplies the actions to
> that provider.  Then the ToolbarActionsModel *only* cares about querying the
> provider.  I think that would make all this much easier, but it would be a
> larger change, so let's hold off on that.

For the smaller fix, I think it's fine for the component action to not get
added. When the ToolbarActionsModel is initializing, the user hasn't used the
Cast functionality yet, and the icon will be shown once they do.

[Devlin, 2016-10-31 21:17:59]

On 2016/10/31 21:14:22, takumif wrote:
> On 2016/10/31 19:10:02, Devlin wrote:
> > On 2016/10/29 00:37:23, takumif wrote:
> > > To summarize:
> > > 
> > > BEFORE
> > > ComponentMigrationHelper:
> > >  - Changes migration prefs when migrating
> > >  - Adds/removes actions at both initialization and migration
> > > 
> > > AFTER
> > > ComponentMigrationHelper:
> > >  - Changes migration prefs when migrating
> > >  - Adds/removes actions when migration happens after initialization
> > > ComponentToolbarActionsFactory:
> > >  - Generates the list of actions at initialization
> > > ToolbarActionsModel:
> > >  - Notifies ComponentToolbarActionsFactory when someone else (e.g. 
> > >    MRActionController) attempts to add a component action before
> > initialization
> > > 
> > > What do you think? I think this should happen in a different CL...
> > 
> > In the interest of solving this crash, I'm fine with landing a smaller fix
> first
> > and then doing the rearchitecture separately.  For the smaller fix, do we
care
> > that the component action gets lost in the case of trying to be added before
> the
> > model is ready?
> > 
> > For the bigger picture, I like the idea of having the ToolbarActionsModel
own
> > the component action stuff.  I think I'd flesh it out a little more to say
> that
> > we make a ComponentToolbarActionsProvider, which has the initial list and
owns
> > the ComponentMigrationHelper, and the migration helper supplies the actions
to
> > that provider.  Then the ToolbarActionsModel *only* cares about querying the
> > provider.  I think that would make all this much easier, but it would be a
> > larger change, so let's hold off on that.
> 
> For the smaller fix, I think it's fine for the component action to not get
> added. When the ToolbarActionsModel is initializing, the user hasn't used the
> Cast functionality yet, and the icon will be shown once they do.

okay, then this lgtm, but can you file a bug for the larger change and add a
TODO somewhere?

[takumif, 2016-10-31 22:02:01]

Patchset #3 (id:60001) has been deleted

[takumif, 2016-10-31 22:15:02]

Description was changed from

==========
Make MediaRouterActionController check that actions are initialized

When MediaRouterActionController calls
ToolbarActionsModel::Add/RemoveComponentAction() before the model is
initialized, it leads to a crash. This change adds an ActionsInitialized()
method to the ComponentActionsDelegate interface (through which MRAController
accesses the model) and makes MRAController check that the model is initialized
before adding or removing the media router action.

BUG=653100
==========

to

==========
Make ToolbarActionsModel ignore calls to AddComponentAction() before
initialization

When MediaRouterActionController calls ToolbarActionsModel::AddComponentAction()
before the model is initialized, it leads to a crash. This change makes such
calls return immediately.

To add the ignored actions at initialization, we should redesign
ComponentToolbarActionsFactory and ComponentMigrationHelper. Bug for that:
crbug.com/660972

BUG=653100
==========

[takumif, 2016-10-31 22:16:41]

Description was changed from

==========
Make ToolbarActionsModel ignore calls to AddComponentAction() before
initialization

When MediaRouterActionController calls ToolbarActionsModel::AddComponentAction()
before the model is initialized, it leads to a crash. This change makes such
calls return immediately.

To add the ignored actions at initialization, we should redesign
ComponentToolbarActionsFactory and ComponentMigrationHelper. Bug for that:
crbug.com/660972

BUG=653100
==========

to

==========
Make ToolbarActionsModel ignore calls to AddComponentAction() before
initialization

When MediaRouterActionController calls ToolbarActionsModel::AddComponentAction()
before the model is initialized, it leads to a crash. This change makes such
calls return immediately.

To add the ignored actions at initialization, we should redesign
ComponentToolbarActionsFactory and ComponentMigrationHelper to be responsible
for that. Bug for that: crbug.com/660972

BUG=653100
==========

[takumif, 2016-10-31 22:19:32]

On 2016/10/31 21:17:59, Devlin wrote:
> On 2016/10/31 21:14:22, takumif wrote:
> > On 2016/10/31 19:10:02, Devlin wrote:
> > > On 2016/10/29 00:37:23, takumif wrote:
> > > > To summarize:
> > > > 
> > > > BEFORE
> > > > ComponentMigrationHelper:
> > > >  - Changes migration prefs when migrating
> > > >  - Adds/removes actions at both initialization and migration
> > > > 
> > > > AFTER
> > > > ComponentMigrationHelper:
> > > >  - Changes migration prefs when migrating
> > > >  - Adds/removes actions when migration happens after initialization
> > > > ComponentToolbarActionsFactory:
> > > >  - Generates the list of actions at initialization
> > > > ToolbarActionsModel:
> > > >  - Notifies ComponentToolbarActionsFactory when someone else (e.g. 
> > > >    MRActionController) attempts to add a component action before
> > > initialization
> > > > 
> > > > What do you think? I think this should happen in a different CL...
> > > 
> > > In the interest of solving this crash, I'm fine with landing a smaller fix
> > first
> > > and then doing the rearchitecture separately.  For the smaller fix, do we
> care
> > > that the component action gets lost in the case of trying to be added
before
> > the
> > > model is ready?
> > > 
> > > For the bigger picture, I like the idea of having the ToolbarActionsModel
> own
> > > the component action stuff.  I think I'd flesh it out a little more to say
> > that
> > > we make a ComponentToolbarActionsProvider, which has the initial list and
> owns
> > > the ComponentMigrationHelper, and the migration helper supplies the
actions
> to
> > > that provider.  Then the ToolbarActionsModel *only* cares about querying
the
> > > provider.  I think that would make all this much easier, but it would be a
> > > larger change, so let's hold off on that.
> > 
> > For the smaller fix, I think it's fine for the component action to not get
> > added. When the ToolbarActionsModel is initializing, the user hasn't used
the
> > Cast functionality yet, and the icon will be shown once they do.
> 
> okay, then this lgtm, but can you file a bug for the larger change and add a
> TODO somewhere?

Yup, added!

[takumif, 2016-10-31 23:05:53]

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

[commit-bot: I haz the power, 2016-10-31 23:06:33]

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, 2016-11-01 00:06:44]

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

[commit-bot: I haz the power, 2016-11-01 00:06:45]

Dry run: This issue passed the CQ dry run.

[takumif, 2016-11-01 17:12:26]

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

[commit-bot: I haz the power, 2016-11-01 17:12:47]

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, 2016-11-01 17:16:52]

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

[commit-bot: I haz the power, 2016-11-01 17:16:53]

Dry run: This issue passed the CQ dry run.

[takumif, 2016-11-01 17:31:40]

The CQ bit was checked by takumif@chromium.org

[takumif, 2016-11-01 17:31:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2450633004/#ps80001
(title: "Add a TODO")

[commit-bot: I haz the power, 2016-11-01 17:32:26]

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, 2016-11-01 17:36:33]

Description was changed from

==========
Make ToolbarActionsModel ignore calls to AddComponentAction() before
initialization

When MediaRouterActionController calls ToolbarActionsModel::AddComponentAction()
before the model is initialized, it leads to a crash. This change makes such
calls return immediately.

To add the ignored actions at initialization, we should redesign
ComponentToolbarActionsFactory and ComponentMigrationHelper to be responsible
for that. Bug for that: crbug.com/660972

BUG=653100
==========

to

==========
Make ToolbarActionsModel ignore calls to AddComponentAction() before
initialization

When MediaRouterActionController calls ToolbarActionsModel::AddComponentAction()
before the model is initialized, it leads to a crash. This change makes such
calls return immediately.

To add the ignored actions at initialization, we should redesign
ComponentToolbarActionsFactory and ComponentMigrationHelper to be responsible
for that. Bug for that: crbug.com/660972

BUG=653100
==========

[commit-bot: I haz the power, 2016-11-01 17:36:35]

Committed patchset #3 (id:80001)

[imcheng, 2016-11-01 17:37:17]

lgtm

[commit-bot: I haz the power, 2016-11-01 17:53:10]

Description was changed from

==========
Make ToolbarActionsModel ignore calls to AddComponentAction() before
initialization

When MediaRouterActionController calls ToolbarActionsModel::AddComponentAction()
before the model is initialized, it leads to a crash. This change makes such
calls return immediately.

To add the ignored actions at initialization, we should redesign
ComponentToolbarActionsFactory and ComponentMigrationHelper to be responsible
for that. Bug for that: crbug.com/660972

BUG=653100
==========

to

==========
Make ToolbarActionsModel ignore calls to AddComponentAction() before
initialization

When MediaRouterActionController calls ToolbarActionsModel::AddComponentAction()
before the model is initialized, it leads to a crash. This change makes such
calls return immediately.

To add the ignored actions at initialization, we should redesign
ComponentToolbarActionsFactory and ComponentMigrationHelper to be responsible
for that. Bug for that: crbug.com/660972

BUG=653100

Committed: https://crrev.com/7bf2d384cc5eed3055bc5df8a6688562f6b9e532
Cr-Commit-Position: refs/heads/master@{#429036}
==========

[commit-bot: I haz the power, 2016-11-01 17:53:12]

Patchset 3 (id:??) landed as
https://crrev.com/7bf2d384cc5eed3055bc5df8a6688562f6b9e532
Cr-Commit-Position: refs/heads/master@{#429036}