Make ToolbarActionsModel ignore calls to AddComponentAction() before initialization

chrome/browser/ui/toolbar/toolbar_actions_model.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/toolbar/toolbar_actions_model.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/location.h"
@@ skipping 617 matching lines @@
   return base::ContainsValue(toolbar_items_, item);
 }
 
 bool ToolbarActionsModel::HasComponentAction(
     const std::string& action_id) const {
   DCHECK(use_redesign_);
   return HasItem(ToolbarItem(action_id, COMPONENT_ACTION));
 }
 
 void ToolbarActionsModel::AddComponentAction(const std::string& action_id) {
+  if (!actions_initialized_)

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

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]

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.

[takumif, 2016/10/26 18:42:05]

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.

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

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]

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:22]

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

+    return;
+
   DCHECK(use_redesign_);
   ToolbarItem component_item(action_id, COMPONENT_ACTION);
   DCHECK(!HasItem(component_item));
   AddItem(component_item);
 }
 
 void ToolbarActionsModel::RemoveComponentAction(const std::string& action_id) {
   DCHECK(use_redesign_);
   ToolbarItem component_item(action_id, COMPONENT_ACTION);
   DCHECK(HasItem(component_item));
@@ skipping 197 matching lines @@
         prefs_->GetInteger(extensions::pref_names::kToolbarSize);
     if (saved_icon_count != visible_icon_count_)
       SetVisibleIconCount(saved_icon_count);
   }
 }
 
 const extensions::Extension* ToolbarActionsModel::GetExtensionById(
     const std::string& id) const {
   return extension_registry_->enabled_extensions().GetByID(id);
 }