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..4b60419074976dc281c1bacb08026602ba973267 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"
@@ -24,6 +25,21 @@
 using extensions::Extension;
 using extensions::ExtensionList;
 
+namespace {
+
+// Returns index of |extension| in |extension_list|
+// or -1 if not found.

[msw, 2012/07/25 23:02:03]

nit: comment fits on one line.

[yefimt, 2012/07/31 00:10:11]

Done.

+int FindInExtensionList(const Extension* extension,

[msw, 2012/07/25 23:02:03]

Why return an index if the two callers just convert that to a bool?

[Peter Kasting, 2012/07/26 20:37:17]

Yeah, this is weird.

I think I agree with Mike that the best way to do this is instead to have a
member function that looks through both |toolbar_items_| and
|action_box_menu_items_| and returns the ExtensionList* which contains the
extension in question, or NULL if neither.  The only problem with this would be
if an extension could be in both lists at once, but that's not supposed to
happen, is it?

[yefimt, 2012/07/31 00:10:11]

Done.

[yefimt, 2012/07/31 00:10:11]

Done.

+                        const extensions::ExtensionList& extension_list) {
+  for (size_t i = 0; i < extension_list.size(); i++) {
+    if (extension_list[i].get() == extension)
+      return i;
+  }
+  return -1;
+}
+
+}  // namespace
+
 ExtensionToolbarModel::ExtensionToolbarModel(ExtensionService* service)
     : service_(service),
       prefs_(service->profile()->GetPrefs()),
@@ -61,23 +77,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 +143,7 @@ void ExtensionToolbarModel::Observe(
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   if (type == chrome::NOTIFICATION_EXTENSIONS_READY) {
-    InitializeExtensionList();
+    InitializeExtensionLists();
     return;
   }
 
@@ -141,45 +157,57 @@ void ExtensionToolbarModel::Observe(
   } else {
     extension = content::Details<const Extension>(details).ptr();
   }
+  bool found_in_toolbar = FindInExtensionList(extension, toolbar_items_) >= 0;
+  bool found_in_action_box_menu =
+      FindInExtensionList(extension, action_box_menu_items_) >= 0;
   if (type == chrome::NOTIFICATION_EXTENSION_LOADED) {
     // 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)
-        return;  // Already exists.
-    }
-    if (service_->extension_prefs()->GetBrowserActionVisibility(extension))
-      AddExtension(extension);
+    if (found_in_toolbar || found_in_action_box_menu)
+      return;
+    if (extensions::switch_utils::IsActionBoxEnabled())
+      AddExtension(extension, &action_box_menu_items_);
+    else if (service_->extension_prefs()->GetBrowserActionVisibility(extension))
+      AddExtension(extension, &toolbar_items_);
   } else if (type == chrome::NOTIFICATION_EXTENSION_UNLOADED) {
-    RemoveExtension(extension);
+    if (found_in_toolbar)
+      RemoveExtension(extension, &toolbar_items_);
+    if (found_in_action_box_menu)
+      RemoveExtension(extension, &action_box_menu_items_);
   } else if (
         type ==

[Peter Kasting, 2012/07/26 20:37:17]

Nit: move this up onto prior line

[yefimt, 2012/07/31 00:10:11]

Done.

         chrome::NOTIFICATION_EXTENSION_BROWSER_ACTION_VISIBILITY_CHANGED) {

[Peter Kasting, 2012/07/26 20:37:17]

Nit: Indent 4, not 6

[yefimt, 2012/07/31 00:10:11]

Done.

-    if (service_->extension_prefs()->GetBrowserActionVisibility(extension))
-      AddExtension(extension);
-    else
-      RemoveExtension(extension);
+    if (extensions::switch_utils::IsActionBoxEnabled()) {
+      // TODO(yefim): Implement this when implementing drag & drop
+      // for action box menu.
+    } else {
+      if (service_->extension_prefs()->GetBrowserActionVisibility(extension))

[Peter Kasting, 2012/07/26 20:37:17]

Nit: Combine with "} else {" above so you have "if ... else if ... else"

[yefimt, 2012/07/31 00:10:11]

I think current formatting is better because if statement fits to a single line.

+        AddExtension(extension, &toolbar_items_);
+      else
+        RemoveExtension(extension, &toolbar_items_);
+    }
   } else {
     NOTREACHED() << "Received unexpected notification";
   }
 }
 
-void ExtensionToolbarModel::AddExtension(const Extension* extension) {
+void ExtensionToolbarModel::AddExtension(const Extension* extension,
+                                         ExtensionList* list) {
   // We only care about extensions with browser actions.
   if (!extension->browser_action())
     return;
 
   if (extension->id() == last_extension_removed_ &&
-      last_extension_removed_index_ < toolitems_.size()) {
-    toolitems_.insert(begin() + last_extension_removed_index_,
-                      make_scoped_refptr(extension));
+      last_extension_removed_index_ < list->size()) {
+    list->insert(list->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));
+    list->push_back(make_scoped_refptr(extension));
     FOR_EACH_OBSERVER(Observer, observers_,
-                      BrowserActionAdded(extension, toolitems_.size() - 1));
+                      BrowserActionAdded(extension, list->size() - 1));
   }
 
   last_extension_removed_ = "";
@@ -188,16 +216,18 @@ void ExtensionToolbarModel::AddExtension(const Extension* extension) {
   UpdatePrefs();
 }
 
-void ExtensionToolbarModel::RemoveExtension(const Extension* extension) {
-  ExtensionList::iterator pos = std::find(begin(), end(), extension);
+void ExtensionToolbarModel::RemoveExtension(const Extension* extension,
+                                            ExtensionList* list) {
+  ExtensionList::iterator pos =
+      std::find(list->begin(), list->end(), extension);
   if (pos == end()) {

[Peter Kasting, 2012/07/26 20:37:17]

Nit: No {}

[yefimt, 2012/07/31 00:10:11]

Done.

     return;
   }
 
   last_extension_removed_ = extension->id();
-  last_extension_removed_index_ = pos - begin();
+  last_extension_removed_index_ = pos - list->begin();
 
-  toolitems_.erase(pos);
+  list->erase(pos);
   FOR_EACH_OBSERVER(Observer, observers_,
                     BrowserActionRemoved(extension));
 
@@ -211,11 +241,55 @@ 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() {
+  const extensions::ExtensionPrefs::ExtensionIdSet toolbar_order =
+      service_->extension_prefs()->GetToolbarOrder();
+  extensions::ExtensionPrefs::ExtensionIdSet action_box_order =
+      service_->extension_prefs()->GetActionBoxOrder();
+
+  // Add all browser actions not already in the toolbar or action box
+  // to the action box.
+  for (ExtensionSet::const_iterator iter = service_->extensions()->begin();
+       iter != service_->extensions()->end(); ++iter) {
+    const Extension* extension = *iter;
+    if (!extension->browser_action())

[Peter Kasting, 2012/07/26 20:37:17]

Nit: Much shorter:

  if (extension->browser_action() &&
      (std::find(toolbar_order.begin(), toolbar_order.end(), extension->id()) ==
          toolbar_order.end()) &&
      (std::find(action_box_order.begin(), action_box_order.end(),
                 extension->id()) != action_box_order.end()))
    action_box_order.push_back(extension->id());

[yefimt, 2012/07/31 00:10:11]

Wow, it is shorter, but would it be shorter to collapse all white space and end
of lines. Shorter is not always more readable. Sorry.
I think current formatting is way more readable and consistent (last one is to
Mike's comment to reverse condition in the last if statement).
There are three conditions we use to filter extensions out. They are separately
stated and easily parsable.

[Peter Kasting, 2012/07/31 02:30:42]

Can you at least get rid of your temporaries?  They each require a full line of
text just to redeclare the iterator type.

I don't really agree that separating them from each other -- or using continue
for a common case -- is necessarily the most readable way to do things, but this
is so trivial there's no point arguing over it.

[yefimt, 2012/07/31 17:19:35]

Done.

+      continue;
+
+    extensions::ExtensionPrefs::ExtensionIdSet::const_iterator toolbar_pos =
+        std::find(toolbar_order.begin(), toolbar_order.end(), extension->id());
+    if (toolbar_pos != toolbar_order.end())
+      continue;
+
+    extensions::ExtensionPrefs::ExtensionIdSet::const_iterator action_box_pos =
+        std::find(action_box_order.begin(), action_box_order.end(),
+                  extension->id());
+    if (action_box_pos != action_box_order.end())
+      continue;
+
+    action_box_order.push_back(extension->id());
+  }
+
+  FillExtensionList(action_box_order, &action_box_menu_items_);
+  FillExtensionList(toolbar_order, &toolbar_items_);
+}
+
+void ExtensionToolbarModel::PopulateForNonActionBoxMode() {
+  const extensions::ExtensionPrefs::ExtensionIdSet pref_order =
+      service_->extension_prefs()->GetToolbarOrder();
   // Items that have a pref for their position.
   ExtensionList sorted;
   sorted.resize(pref_order.size(), NULL);
@@ -231,50 +305,63 @@ void ExtensionToolbarModel::InitializeExtensionList() {
     if (!service_->extension_prefs()->GetBrowserActionVisibility(extension))
       continue;
 
-    std::vector<std::string>::iterator pos =
+    extensions::ExtensionPrefs::ExtensionIdSet::const_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 {
+    if (pos != pref_order.end())
+      sorted[pos - pref_order.begin()] = extension;
+    else
       unsorted.push_back(make_scoped_refptr(extension));
-    }
   }
 
   // Merge the lists.
-  toolitems_.reserve(sorted.size() + unsorted.size());
-  for (ExtensionList::iterator iter = sorted.begin();
-       iter != sorted.end(); ++iter) {
+  toolbar_items_.clear();
+  toolbar_items_.reserve(sorted.size() + unsorted.size());
+  for (ExtensionList::const_iterator iter = sorted.begin();
+       iter != sorted.end(); ++iter) {
+    // 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.

[Peter Kasting, 2012/07/26 20:37:17]

Nit: Even clearer:

"It's possible for the extension order to contain items that aren't actually
loaded on this machine.  For example, when extension sync is on, we sync the
extension order as-is but double-check with the user before syncing
NPAPI-containing extensions, so if one of those is not actually synced, we'll
get a NULL in the list.  This sort of case can also happen if some error
prevents an extension from loading."

[yefimt, 2012/07/31 00:10:11]

Done.

     if (*iter != NULL)
-      toolitems_.push_back(*iter);
+      toolbar_items_.push_back(*iter);
   }
-  toolitems_.insert(toolitems_.end(), unsorted.begin(), unsorted.end());
+  toolbar_items_.insert(toolbar_items_.end(), unsorted.begin(),
+                        unsorted.end());
 
   // 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();
-
-  extensions_initialized_ = true;
-  FOR_EACH_OBSERVER(Observer, observers_, ModelLoaded());
+void ExtensionToolbarModel::FillExtensionList(
+    const extensions::ExtensionPrefs::ExtensionIdSet& order,
+    ExtensionList* list) {
+  list->clear();
+  list->reserve(order.size());
+  for (size_t i = 0; i < order.size(); ++i) {
+    const extensions::Extension* extension =
+        service_->GetExtensionById(order[i], false);
+    if (extension)
+      AddExtension(extension, list);
+  }
 }
 
 void ExtensionToolbarModel::UpdatePrefs() {
   if (!service_->extension_prefs())
     return;
 
-  std::vector<std::string> ids;
-  ids.reserve(toolitems_.size());
-  for (ExtensionList::iterator iter = begin(); iter != end(); ++iter)
-    ids.push_back((*iter)->id());
-  service_->extension_prefs()->SetToolbarOrder(ids);
-}
-
-const Extension* ExtensionToolbarModel::GetExtensionByIndex(int index) const {
-  return toolitems_[index];
+  extensions::ExtensionPrefs::ExtensionIdSet toolbar_ids;
+  toolbar_ids.reserve(toolbar_items_.size());
+  for (size_t i = 0; i < toolbar_items_.size(); ++i)
+    toolbar_ids.push_back(toolbar_items_[i]->id());
+  service_->extension_prefs()->SetToolbarOrder(toolbar_ids);
+
+  extensions::ExtensionPrefs::ExtensionIdSet action_box_ids;
+  action_box_ids.reserve(action_box_menu_items_.size());
+  for (size_t i = 0; i < action_box_menu_items_.size(); ++i)
+    action_box_ids.push_back(action_box_menu_items_[i]->id());
+  service_->extension_prefs()->SetActionBoxOrder(action_box_ids);
 }
 
 int ExtensionToolbarModel::IncognitoIndexToOriginal(int incognito_index) {