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..c00af589e346ec632d6fbf67350ce9211b8c19d5 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;
   }
 
@@ -141,45 +142,57 @@ void ExtensionToolbarModel::Observe(
   } else {
     extension = content::Details<const Extension>(details).ptr();
   }
+  bool found_in_toolbar = FindInExtensionList(extension, toolbar_items_);

[msw, 2012/07/24 23:41:55]

nit: Instead of using two bools here, use a single extensions::ExtensionList*
|container| or |list| or similar that's NULL or points to the right container;
that should simplify the code below (and the code here if you implement my
suggestion in the header about returning the containing list).

[yefimt, 2012/07/25 21:09:21]

I believe it is much more readable as it is. Using a pointer   will make portion
where extension is removed from one of the list more difficult to parse.

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

I disagree, it'd be: RemoveExtension(extension, container);
Which is shorter and seems clearer than the current patch:

160 if (found_in_toolbar)
161   RemoveExtension(extension, &toolbar_items_);
162 if (found_in_action_box_menu)
163   RemoveExtension(extension, &action_box_menu_items_);

It's not a big deal though, I suppose.

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

Done.

+  bool found_in_action_box_menu =
+      FindInExtensionList(extension, action_box_menu_items_);
   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;  // Already exists.

[msw, 2012/07/24 23:41:55]

nit: remoove inline comment, it's explained well above.

[yefimt, 2012/07/25 21:09:21]

Done.

+    if (extensions::switch_utils::IsActionBoxEnabled())
+      AddExtension(extension, &action_box_menu_items_);
+    else
+      if (service_->extension_prefs()->GetBrowserActionVisibility(extension))

[msw, 2012/07/24 23:41:55]

nit: fit on line above as "else if"

[yefimt, 2012/07/25 21:09:21]

Done.

+        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 ==
         chrome::NOTIFICATION_EXTENSION_BROWSER_ACTION_VISIBILITY_CHANGED) {
-    if (service_->extension_prefs()->GetBrowserActionVisibility(extension))
-      AddExtension(extension);
-    else
-      RemoveExtension(extension);
+    if (extensions::switch_utils::IsActionBoxEnabled()) {
+      // TODO: Implement this when implementing drag & drop for action box menu.

[msw, 2012/07/24 23:41:55]

nit: use TODO(ldap): format.

[yefimt, 2012/07/25 21:09:21]

Done.

+    } else {
+      if (service_->extension_prefs()->GetBrowserActionVisibility(extension))
+        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* result_list) {

[msw, 2012/07/24 23:41:55]

nit: Name |list| instead of |result_list| and similar below. What's result-y
about it?

[yefimt, 2012/07/25 21:09:21]

Done.

   // 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_ < result_list->size()) {
+    result_list->insert(result_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));
+    result_list->push_back(make_scoped_refptr(extension));
     FOR_EACH_OBSERVER(Observer, observers_,
-                      BrowserActionAdded(extension, toolitems_.size() - 1));
+                      BrowserActionAdded(extension, result_list->size() - 1));
   }
 
   last_extension_removed_ = "";
@@ -188,16 +201,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* result_list) {
+  ExtensionList::iterator pos =
+      std::find(result_list->begin(), result_list->end(), extension);
   if (pos == end()) {
     return;
   }
 
   last_extension_removed_ = extension->id();
-  last_extension_removed_index_ = pos - begin();
+  last_extension_removed_index_ = pos - result_list->begin();
 
-  toolitems_.erase(pos);
+  result_list->erase(pos);
   FOR_EACH_OBSERVER(Observer, observers_,
                     BrowserActionRemoved(extension));
 
@@ -211,11 +226,59 @@ 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.

[msw, 2012/07/24 23:41:55]

nit: explain why the prefs list might miss some extensions.

[yefimt, 2012/07/25 21:09:21]

I dont think I can, it could be hundreds of reasons. For any of them we need to
add these extensions to the list or user would never see these extensions.
(Simplest reason preferences file was edited or corrupted).

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

Fair enough, consider at least adding "The prefs list may omit some extensions."
or similar.

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

Done.

+  for (ExtensionSet::const_iterator iter = service_->extensions()->begin();
+       iter != service_->extensions()->end(); ++iter) {
+    const Extension* extension = *iter;
+    if (!extension->browser_action())
+      continue;
+
+    extensions::ExtensionPrefs::ExtensionIdSet::const_iterator toolbar_pos =

[msw, 2012/07/24 23:41:55]

nit: continue early if it's in the first list, don't search the second list;
alternatively, inline the conditionals to achieve the same.

[yefimt, 2012/07/25 21:09:21]

Done.

+        std::find(toolbar_order.begin(), toolbar_order.end(), extension->id());
+    extensions::ExtensionPrefs::ExtensionIdSet::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()))
+      action_box_order.push_back(extension->id());
+  }
+
+  FillExtensionList(action_box_order, &action_box_menu_items_);
+  FillExtensionList(toolbar_order, &toolbar_items_);
+
+  // Inform observers.

[msw, 2012/07/24 23:41:55]

nit: Can this be built-in by calling AddExtension from FillExtensionList?

[yefimt, 2012/07/25 21:09:21]

Done.

+  for (size_t i = 0; i < action_box_menu_items_.size(); i++)
+    FOR_EACH_OBSERVER(Observer, observers_,
+                      BrowserActionAdded(action_box_menu_items_[i], i));
+  for (size_t i = 0; i < toolbar_items_.size(); i++)
+    FOR_EACH_OBSERVER(Observer, observers_,
+                      BrowserActionAdded(toolbar_items_[i], i));
+}
+
+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 +294,80 @@ 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) {
+  toolbar_items_.clear();
+  toolbar_items_.reserve(sorted.size() + unsorted.size());
+  for (ExtensionList::const_iterator iter = sorted.begin();

[msw, 2012/07/24 23:41:55]

nit: use toolbar_items_.insert for |sorted| (like unsorted), instead of looping.

[yefimt, 2012/07/25 21:09:21]

Some could be NULL and need to be excluded

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

Good point! Sorry.

+       iter != sorted.end(); ++iter) {
     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();
+void ExtensionToolbarModel::AddToProperList(const Extension* extension,

[msw, 2012/07/24 23:41:55]

nit: I think if args don't fit aligned to inside the paren, they are all
supposed to be indented b 4 spaces (move |extension| to the next line).

[msw, 2012/07/24 23:41:55]

nit: make this a file-local function in an anon namespace.

[yefimt, 2012/07/25 21:09:21]

I removed it at all, it was used in one place only

+    const extensions::ExtensionPrefs::ExtensionIdSet& order,
+    ExtensionList* sorted,
+    ExtensionList* unsorted) {
+  DCHECK_EQ(order.size(), sorted->size());
+  extensions::ExtensionPrefs::ExtensionIdSet::const_iterator pos =
+      std::find(order.begin(), order.end(), extension->id());
+  if (pos != order.end())
+    (*sorted)[pos - order.begin()] = extension;
+  else
+    unsorted->push_back(make_scoped_refptr(extension));
+}
 
-  extensions_initialized_ = true;
-  FOR_EACH_OBSERVER(Observer, observers_, ModelLoaded());
+void ExtensionToolbarModel::FillExtensionList(
+    const extensions::ExtensionPrefs::ExtensionIdSet& 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 =

[msw, 2012/07/24 23:41:55]

nit: declare a single non-const Extension* outside the loop and re-use here.

[yefimt, 2012/07/25 21:09:21]

GetExtensionById() returns const Extension*, so it wont work unless i do
const_cast

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

Fair enough!

+        service_->GetExtensionById(order[i], false);
+    if (!extension)

[msw, 2012/07/24 23:41:55]

nit: invert NULL check and just perform the single statement on success

[yefimt, 2012/07/25 21:09:21]

Done.

+      continue;
+    result_list->push_back(extension);

[msw, 2012/07/24 23:41:55]

nit: Call AddExtension here instead, and remove the observer notification code
from PopulateForActionBoxMode

[yefimt, 2012/07/25 21:09:21]

Done.

+  }
+}
+
+bool ExtensionToolbarModel::FindInExtensionList(

[msw, 2012/07/24 23:41:55]

nit: make this a file-local function in an anon namespace.

[yefimt, 2012/07/25 21:09:21]

Done.

+    const Extension* extension,
+    const extensions::ExtensionList& extension_list) {
+  for (size_t i = 0; i < extension_list.size(); i++) {
+    if (extension_list[i].get() == extension)
+      return true;
+  }
+  return false;
 }
 
+
 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) {