Action box menu

chrome/browser/extensions/extension_toolbar_model.cc

Index: chrome/browser/extensions/extension_toolbar_model.cc
diff --git a/chrome/browser/extensions/extension_toolbar_model.cc b/chrome/browser/extensions/extension_toolbar_model.cc
index c105477096c6d50d6a2111bc50f77f8e9d03a477..df129655df0ba21df528e8f6c678845a68c23a2e 100644
--- a/chrome/browser/extensions/extension_toolbar_model.cc
+++ b/chrome/browser/extensions/extension_toolbar_model.cc
@@ -16,6 +16,7 @@
 #include "chrome/browser/ui/tab_contents/tab_contents.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/extension_switch_utils.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
@@ -61,23 +62,23 @@ void ExtensionToolbarModel::MoveBrowserAction(const Extension* extension,
     NOTREACHED();
     return;
   }
-  toolitems_.erase(pos);
+  toolbar_items_.erase(pos);
 
   int i = 0;
   bool inserted = false;
   for (ExtensionList::iterator iter = begin(); iter != end(); ++iter, ++i) {
     if (i == index) {
-      toolitems_.insert(iter, make_scoped_refptr(extension));
+      toolbar_items_.insert(iter, make_scoped_refptr(extension));
       inserted = true;
       break;
     }
   }
 
   if (!inserted) {
-    DCHECK_EQ(index, static_cast<int>(toolitems_.size()));
-    index = toolitems_.size();
+    DCHECK_EQ(index, static_cast<int>(toolbar_items_.size()));
+    index = toolbar_items_.size();
 
-    toolitems_.push_back(make_scoped_refptr(extension));
+    toolbar_items_.push_back(make_scoped_refptr(extension));
   }
 
   FOR_EACH_OBSERVER(Observer, observers_, BrowserActionMoved(extension, index));
@@ -127,7 +128,7 @@ void ExtensionToolbarModel::Observe(
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   if (type == chrome::NOTIFICATION_EXTENSIONS_READY) {
-    InitializeExtensionList();
+    InitializeExtensionLists();
     return;
   }
 
@@ -145,8 +146,8 @@ void ExtensionToolbarModel::Observe(
     // We don't want to add the same extension twice. It may have already been
     // added by EXTENSION_BROWSER_ACTION_VISIBILITY_CHANGED below, if the user
     // hides the browser action and then disables and enables the extension.
-    for (size_t i = 0; i < toolitems_.size(); i++) {
-      if (toolitems_[i].get() == extension)
+    for (size_t i = 0; i < toolbar_items_.size(); i++) {
+      if (toolbar_items_[i].get() == extension)
         return;  // Already exists.
     }
     if (service_->extension_prefs()->GetBrowserActionVisibility(extension))
@@ -171,15 +172,15 @@ void ExtensionToolbarModel::AddExtension(const Extension* extension) {
     return;
 
   if (extension->id() == last_extension_removed_ &&
-      last_extension_removed_index_ < toolitems_.size()) {
-    toolitems_.insert(begin() + last_extension_removed_index_,
+      last_extension_removed_index_ < toolbar_items_.size()) {
+    toolbar_items_.insert(begin() + last_extension_removed_index_,
                       make_scoped_refptr(extension));
     FOR_EACH_OBSERVER(Observer, observers_,
         BrowserActionAdded(extension, last_extension_removed_index_));
   } else {
-    toolitems_.push_back(make_scoped_refptr(extension));
+    toolbar_items_.push_back(make_scoped_refptr(extension));
     FOR_EACH_OBSERVER(Observer, observers_,
-                      BrowserActionAdded(extension, toolitems_.size() - 1));
+                      BrowserActionAdded(extension, toolbar_items_.size() - 1));
   }
 
   last_extension_removed_ = "";
@@ -197,7 +198,7 @@ void ExtensionToolbarModel::RemoveExtension(const Extension* extension) {
   last_extension_removed_ = extension->id();
   last_extension_removed_index_ = pos - begin();
 
-  toolitems_.erase(pos);
+  toolbar_items_.erase(pos);
   FOR_EACH_OBSERVER(Observer, observers_,
                     BrowserActionRemoved(extension));
 
@@ -211,11 +212,58 @@ void ExtensionToolbarModel::RemoveExtension(const Extension* extension) {
 // have holes.
 // 2. Create a vector of extensions that did not have a pref value.
 // 3. Remove holes from the sorted vector and append the unsorted vector.
-void ExtensionToolbarModel::InitializeExtensionList() {
+void ExtensionToolbarModel::InitializeExtensionLists() {
   DCHECK(service_->is_ready());
 
-  std::vector<std::string> pref_order = service_->extension_prefs()->
-      GetToolbarOrder();
+  if (extensions::switch_utils::IsActionBoxEnabled())
+    PopulateForActionBoxMode();
+  else
+    PopulateForNonActionBoxMode();
+
+  UpdatePrefs();
+
+  extensions_initialized_ = true;
+  FOR_EACH_OBSERVER(Observer, observers_, ModelLoaded());
+}
+
+void ExtensionToolbarModel::PopulateForActionBoxMode() {

[Peter Kasting, 2012/07/21 01:55:13]

This function never calls GetBrowserActionVisibility() anywhere, where
PopulateForNonActionBoxMode() does.  That seems questionable?  Is there a
problem here?

[yefimt, 2012/07/23 23:47:52]

No, Aaron advised that just using toolbar and action box orders should be
enough. We agreed that I'll clean up rest of the files in a separate CL. And
then after action box will be enabled by default, non action box functionality
will be removed

+  const VectorOfStrings toolbar_order =
+      service_->extension_prefs()->GetToolbarOrder();
+  VectorOfStrings action_box_order =
+      service_->extension_prefs()->GetActionBoxOrder();
+
+  // Create the lists.

[Peter Kasting, 2012/07/21 01:55:13]

Nit: This comment is misleading, because we don't create the lists until the
next block.

Instead, this should be something like "Add all browser actions not already in
the toolbar or action box to the action box."

But I question whether that's the right thing to do.  Shouldn't
GetActionBoxOrder() be responsible for doing something like this?

[yefimt, 2012/07/23 23:47:52]

Comment - done.
I dont think GetActionBoxOrder() should do it, but you are right that changes in
extensions are not handled when action box is enabled. Fixing it in Observe()

+  for (ExtensionSet::const_iterator iter = service_->extensions()->begin();
+       iter != service_->extensions()->end(); ++iter) {
+    const Extension* extension = *iter;
+    if (!extension->browser_action())
+      continue;
+
+    VectorOfStrings::const_iterator toolbar_pos =
+        std::find(toolbar_order.begin(), toolbar_order.end(), extension->id());
+    VectorOfStrings::const_iterator action_box_pos =
+        std::find(action_box_order.begin(), action_box_order.end(),
+        extension->id());

[Peter Kasting, 2012/07/21 01:55:13]

Nit: Indent even with first arg in line just above

[yefimt, 2012/07/23 23:47:52]

Done.

+    if ((toolbar_pos == toolbar_order.end()) &&
+        (action_box_pos == action_box_order.end()))
+      action_box_order.push_back(extension->id());
+  }
+
+  FillExtensionList(action_box_order, &action_box_menu_items_);
+  FillExtensionList(toolbar_order, &toolbar_items_);
+
+  // Inform observers.
+  for (size_t i = 0; i < toolbar_items_.size(); i++) {

[Peter Kasting, 2012/07/21 01:55:13]

This loop seems wrong for a couple of reasons.

(1) I don't think |action_box_menu_items_| is guaranteed to be the same length
as |toolbar_items_| (especially after the loop above!), so it doesn't seem safe
to blindly index into one based on the size of the other.  This should probably
be two loops.
(2) Are the items in these two lists guaranteed to be fully disjoint?  If not,
we'll notify about some of them twice.  Seems bad.

[yefimt, 2012/07/23 23:47:52]

Yes an extension guarantied to be in the one list only

+    FOR_EACH_OBSERVER(Observer, observers_,
+                      BrowserActionAdded(action_box_menu_items_[i], i));

[Peter Kasting, 2012/07/21 01:55:13]

Maybe not in scope for this change, but it seems like maybe BrowserActionAdded()
should pass along whether an extension is in the action box or the toolbar, or
there should be a separate observer function for these two cases?  I guess I
don't know what the observers ultimately do with this, but it seems like if it's
important that we pass along the order number to the observer, then this would
be important too...

[yefimt, 2012/07/23 23:47:52]

I see your point. I'll keep it in mind when get to the rest of functionality

+    FOR_EACH_OBSERVER(Observer, observers_,
+                      BrowserActionAdded(toolbar_items_[i], i));
+  }
+}
+
+void ExtensionToolbarModel::PopulateForNonActionBoxMode() {
+  const VectorOfStrings pref_order =
+      service_->extension_prefs()->GetToolbarOrder();
   // Items that have a pref for their position.
   ExtensionList sorted;
   sorted.resize(pref_order.size(), NULL);
@@ -231,50 +279,71 @@ void ExtensionToolbarModel::InitializeExtensionList() {
     if (!service_->extension_prefs()->GetBrowserActionVisibility(extension))
       continue;
 
-    std::vector<std::string>::iterator pos =
-        std::find(pref_order.begin(), pref_order.end(), extension->id());
-    if (pos != pref_order.end()) {
-      int index = std::distance(pref_order.begin(), pos);
-      sorted[index] = extension;
-    } else {
-      unsorted.push_back(make_scoped_refptr(extension));
-    }
+    AddToProperList(extension, pref_order, &sorted, &unsorted);
   }
 
-  // Merge the lists.
-  toolitems_.reserve(sorted.size() + unsorted.size());
-  for (ExtensionList::iterator iter = sorted.begin();
-       iter != sorted.end(); ++iter) {
-    if (*iter != NULL)
-      toolitems_.push_back(*iter);
-  }
-  toolitems_.insert(toolitems_.end(), unsorted.begin(), unsorted.end());
+  MergeLists(sorted, unsorted, &toolbar_items_);

[Peter Kasting, 2012/07/21 01:55:13]

Nit: This function is only called once, and is short and simple enough that I
probably wouldn't factor it out into a helper.

[yefimt, 2012/07/23 23:47:52]

Done.

[yefimt, 2012/07/23 23:47:52]

Done.

 
   // Inform observers.
-  for (size_t i = 0; i < toolitems_.size(); i++) {
+  for (size_t i = 0; i < toolbar_items_.size(); i++) {
     FOR_EACH_OBSERVER(Observer, observers_,
-                      BrowserActionAdded(toolitems_[i], i));
+                      BrowserActionAdded(toolbar_items_[i], i));
   }
+}
 
-  UpdatePrefs();
+void ExtensionToolbarModel::AddToProperList(const Extension* extension,
+    const VectorOfStrings& order,
+    ExtensionList* sorted,
+    ExtensionList* unsorted) {

[Peter Kasting, 2012/07/21 01:55:13]

Nit: I suggest:

  DCHECK_EQ(order.size(), sorted->size());

...atop this function to make it clear that the offset access below is safe.

[yefimt, 2012/07/23 23:47:52]

Done.

+  VectorOfStrings::const_iterator pos =
+      std::find(order.begin(), order.end(), extension->id());
+  if (pos != order.end()) {

[Peter Kasting, 2012/07/21 01:55:13]

Nit: No {} needed

[yefimt, 2012/07/23 23:47:52]

Done.

+    (*sorted)[pos - order.begin()] = extension;
+  } else {
+    unsorted->push_back(make_scoped_refptr(extension));
+  }
+}
 
-  extensions_initialized_ = true;
-  FOR_EACH_OBSERVER(Observer, observers_, ModelLoaded());
+void ExtensionToolbarModel::MergeLists(const ExtensionList& sorted,
+                                       const ExtensionList& unsorted,
+                                       ExtensionList* result_list) {
+  result_list->reserve(sorted.size() + unsorted.size());
+  for (ExtensionList::const_iterator iter = sorted.begin();
+       iter != sorted.end(); ++iter) {
+    if (*iter != NULL)

[Peter Kasting, 2012/07/21 01:55:13]

Is this check actually necessary?  It seems like it could only succeed if
GetToolbarOrder() returns a list that has elements in it that don't actually
appear in ExtensionService::extensions().  That seems like it shouldn't happen.

If this isn't necessary, then this whole function can be eliminated, and the
caller can simply append |unsorted| to the end of |sorted| and then notify.

(I know the origin of this is from the original code; if it's difficult to
determine the answer to this you can do it as a followup, but file a bug for it
if so.)

[yefimt, 2012/07/23 23:47:52]

I think you are right, I cannot see how it could be NULL.

[Aaron Boodman, 2012/07/23 23:57:51]

This can happen due to sync. The order is synced across machines, but sometimes
a machine won't necessarily have the extensions loaded that are specified here.

[yefimt, 2012/07/24 00:14:49]

OK, restored original code

[Peter Kasting, 2012/07/24 04:27:29]

Thanks, that's good to know!  Yefim, can you make sure to add comments about
this (in the various spots I was concerned about) so the next person who reads
the various places in this file I've asked about will have the benefit of
Aaron's knowledge here?

+      result_list->push_back(*iter);

[Peter Kasting, 2012/07/21 01:55:13]

This seems to assume |result_list| was empty on entry.  I suggest DCHECKing
that.

[yefimt, 2012/07/23 23:47:52]

Done.

+  }
+  result_list->insert(result_list->end(), unsorted.begin(), unsorted.end());
+}
+
+void ExtensionToolbarModel::FillExtensionList(const VectorOfStrings& order,
+                                              ExtensionList* result_list) {
+  result_list->clear();
+  result_list->reserve(order.size());
+  for (size_t i = 0; i < order.size(); ++i) {
+    const extensions::Extension* extension =
+        service_->GetExtensionById(order[i], false);
+    if (!extension)

[Peter Kasting, 2012/07/21 01:55:13]

Can this happen?  This seems like another case that should only happen if the
order lists have entries that aren't valid extensions.

Either this should be eliminated, be a DCHECK(extension), or if this case really
can happen, then the code should probably just skip the item and not return
entirely:

    if (extension)
      result_list->push_back(extension);

[yefimt, 2012/07/23 23:47:52]

I think it could happen. Extensions order coming from preferences, it could be
edited or corrupted.

+      return;
+    result_list->push_back(extension);
+  }
 }
 
 void ExtensionToolbarModel::UpdatePrefs() {
   if (!service_->extension_prefs())
     return;
 
-  std::vector<std::string> ids;
-  ids.reserve(toolitems_.size());
+  VectorOfStrings toolbar_ids;
+  toolbar_ids.reserve(toolbar_items_.size());
   for (ExtensionList::iterator iter = begin(); iter != end(); ++iter)

[Peter Kasting, 2012/07/21 01:55:13]

Nit: I think this can be a const_iterator (and below, if you choose to change
that one, see comment there)

[yefimt, 2012/07/23 23:47:52]

Done.

-    ids.push_back((*iter)->id());
-  service_->extension_prefs()->SetToolbarOrder(ids);
-}
-
-const Extension* ExtensionToolbarModel::GetExtensionByIndex(int index) const {
-  return toolitems_[index];
+    toolbar_ids.push_back((*iter)->id());
+  service_->extension_prefs()->SetToolbarOrder(toolbar_ids);
+
+  VectorOfStrings action_box_ids;
+  action_box_ids.reserve(action_box_menu_items_.size());
+  for (size_t i = 0; i < action_box_menu_items_.size(); ++i)

[Peter Kasting, 2012/07/21 01:55:13]

Nit: Is there a reason you use an iterator above and an index here?  I think
it'd be better to be consistent (I'd use an iterator for both).

[yefimt, 2012/07/23 23:47:52]

Done.

+    action_box_ids.push_back(action_box_menu_items_[i]->id());
+  service_->extension_prefs()->SetActionBoxOrder(action_box_ids);
 }
 
 int ExtensionToolbarModel::IncognitoIndexToOriginal(int incognito_index) {