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 144b2b510f51723996e9e579d7d113c1b429930d..aff6f4c399ed2fa71f001b007892a98dbce81fd7 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));
@@ -122,7 +123,7 @@ void ExtensionToolbarModel::Observe(
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   if (type == chrome::NOTIFICATION_EXTENSIONS_READY) {
-    InitializeExtensionList();
+    InitializeExtensionLists();
     return;
   }
 
@@ -140,8 +141,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))
@@ -166,15 +167,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_ = "";
@@ -192,7 +193,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));
 
@@ -206,11 +207,99 @@ 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/18 01:37:25]

Nit: Please put at least your new functions in the same definition order as
declaration order.  (LGTM in advance if anything else in this file is out of
order and you want to TBR those changes separately ahead of time.)

[yefimt, 2012/07/18 23:18:13]

Done.

+  const std::vector<std::string> toolbar_order =

[Peter Kasting, 2012/07/18 01:37:25]

Nit: There's a lot of usage of vector<string> in the header and implementation,
you should probably typedef it somewhere.

[yefimt, 2012/07/18 23:18:13]

Done.

+      service_->extension_prefs()->GetToolbarOrder();
+  std::vector<std::string> action_box_order =
+      service_->extension_prefs()->GetActionBoxOrder();
+
+  // Create the lists.
+  for (ExtensionSet::const_iterator it = service_->extensions()->begin();

[Peter Kasting, 2012/07/18 01:37:25]

Tiny nit: Be consistent across the files in your change about whether you name
iterators |i|, |it|, or |iter|.  (Personally I prefer |i| but it doesn't really
matter much.)

[yefimt, 2012/07/18 23:18:13]

Done.

+       it != service_->extensions()->end(); ++it) {
+    const Extension* extension = *it;
+    if (!extension->browser_action())
+      continue;
+
+    std::vector<std::string>::const_iterator toolbar_pos =
+        std::find(toolbar_order.begin(), toolbar_order.end(), extension->id());
+    std::vector<std::string>::const_iterator action_box_pos =
+        std::find(action_box_order.begin(), action_box_order.end(),
+        extension->id());
+    if ((toolbar_pos == toolbar_order.end()) &&
+        (action_box_pos == action_box_order.end())) {

[Peter Kasting, 2012/07/18 01:37:25]

Nit: No {}

[yefimt, 2012/07/18 23:18:13]

Done.

+      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++) {
+    FOR_EACH_OBSERVER(Observer, observers_,
+                      BrowserActionAdded(action_box_menu_items_[i], i));
+    FOR_EACH_OBSERVER(Observer, observers_,
+                      BrowserActionAdded(toolbar_items_[i], i));
+  }
+}
+
+void ExtensionToolbarModel::AddToProperList(const Extension* extension,
+    const std::vector<std::string>& order,
+    ExtensionList* sorted,
+    ExtensionList* unsorted) {
+  std::vector<std::string>::const_iterator pos =
+      std::find(order.begin(), order.end(), extension->id());
+  if (pos != order.end()) {
+    int index = std::distance(order.begin(), pos);

[Peter Kasting, 2012/07/18 01:37:25]

Nit: Just use "pos - order.begin()"; inline into next statement

[yefimt, 2012/07/18 23:18:13]

Done.

+    (*sorted)[index] = extension;
+  } else {
+    unsorted->push_back(make_scoped_refptr(extension));
+  }
+}
+
+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) {

[Peter Kasting, 2012/07/18 01:37:25]

Nit: Why is this on a separate line?

[yefimt, 2012/07/18 23:18:13]

Done.

+    if (*iter != NULL)
+      result_list->push_back(*iter);
+  }
+  result_list->insert(result_list->end(), unsorted.begin(), unsorted.end());
+}
+
+void ExtensionToolbarModel::FillExtensionList(
+    const std::vector<std::string>& order, ExtensionList* result_list) {

[Peter Kasting, 2012/07/18 01:37:25]

Nit: One arg per line

[yefimt, 2012/07/18 23:18:13]

Done.

+  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)
+      return;
+    result_list->push_back(extension);
+  }
+}
+
+void ExtensionToolbarModel::PopulateForNonActionBoxMode() {
+  const std::vector<std::string> pref_order =
+      service_->extension_prefs()->GetToolbarOrder();
   // Items that have a pref for their position.
   ExtensionList sorted;
   sorted.resize(pref_order.size(), NULL);
@@ -226,35 +315,16 @@ 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_);
 
   // 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::UpdatePrefs() {
@@ -262,14 +332,20 @@ void ExtensionToolbarModel::UpdatePrefs() {
     return;
 
   std::vector<std::string> ids;
-  ids.reserve(toolitems_.size());
+  ids.reserve(toolbar_items_.size());
   for (ExtensionList::iterator iter = begin(); iter != end(); ++iter)
     ids.push_back((*iter)->id());
   service_->extension_prefs()->SetToolbarOrder(ids);
+
+  ids.clear();

[Peter Kasting, 2012/07/18 01:37:25]

Nit: Use a new temp instead of clearing the old one.

[yefimt, 2012/07/18 23:18:13]

Done.

+  ids.reserve(action_box_menu_items_.size());
+  for (size_t i = 0; i < action_box_menu_items_.size(); ++i)
+    ids.push_back(action_box_menu_items_[i]->id());
+  service_->extension_prefs()->SetActionBoxOrder(ids);
 }
 
 const Extension* ExtensionToolbarModel::GetExtensionByIndex(int index) const {
-  return toolitems_[index];
+  return toolbar_items_[index];
 }
 
 int ExtensionToolbarModel::IncognitoIndexToOriginal(int incognito_index) {
@@ -296,3 +372,8 @@ int ExtensionToolbarModel::OriginalIndexToIncognito(int original_index) {
   }
   return incognito_index;
 }
+
+const Extension* ExtensionToolbarModel::GetActionBoxExtensionByIndex(
+    int index) const {
+  return action_box_menu_items_[index];

[Peter Kasting, 2012/07/18 01:37:25]

Nit: Why not inline this as a unix_hacker()-style function like you did with the
size accessor?  (The same could be done with GetExtensionByIndex())

[yefimt, 2012/07/18 23:18:13]

Done.

+}