Action box menu

chrome/browser/extensions/extension_toolbar_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/extensions/extension_toolbar_model.h"
 
 #include "chrome/browser/extensions/extension_browser_event_router.h"
 #include "chrome/browser/extensions/extension_prefs.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #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"
 #include "content/public/browser/web_contents.h"
 
 using extensions::Extension;
 using extensions::ExtensionList;
 
 ExtensionToolbarModel::ExtensionToolbarModel(ExtensionService* service)
     : service_(service),
@@ skipping 25 matching lines @@
   observers_.RemoveObserver(observer);
 }
 
 void ExtensionToolbarModel::MoveBrowserAction(const Extension* extension,
                                               int index) {
   ExtensionList::iterator pos = std::find(begin(), end(), extension);
   if (pos == end()) {
     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));
 
   UpdatePrefs();
 }
 
 ExtensionToolbarModel::Action ExtensionToolbarModel::ExecuteBrowserAction(
     const Extension* extension,
     Browser* browser,
@@ skipping 29 matching lines @@
 void ExtensionToolbarModel::SetVisibleIconCount(int count) {
   visible_icon_count_ = count == static_cast<int>(size()) ? -1 : count;
   prefs_->SetInteger(prefs::kExtensionToolbarSize, visible_icon_count_);
 }
 
 void ExtensionToolbarModel::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   if (type == chrome::NOTIFICATION_EXTENSIONS_READY) {
-    InitializeExtensionList();
+    InitializeExtensionLists();
     return;
   }
 
   if (!service_->is_ready())
     return;
 
   const Extension* extension = NULL;
   if (type == chrome::NOTIFICATION_EXTENSION_UNLOADED) {
     extension = content::Details<extensions::UnloadedExtensionInfo>(
         details)->extension;
   } 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_ = "";
   last_extension_removed_index_ = -1;
 
   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));
 
   UpdatePrefs();
 }
 
 // Combine the currently enabled extensions that have browser actions (which
 // we get from the ExtensionService) with the ordering we get from the
 // pref service. For robustness we use a somewhat inefficient process:
 // 1. Create a vector of extensions sorted by their pref values. This vector may
 // 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);
   // The items that don't have a pref for their position.
   ExtensionList unsorted;
 
   // Create the lists.
   for (ExtensionSet::const_iterator it = service_->extensions()->begin();
        it != service_->extensions()->end(); ++it) {
     const Extension* extension = *it;
     if (!extension->browser_action())
       continue;
     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();
+  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);
-}
+  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);
 
-const Extension* ExtensionToolbarModel::GetExtensionByIndex(int index) const {
-  return toolitems_[index];
+  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) {
   int original_index = 0, i = 0;
   for (ExtensionList::iterator iter = begin(); iter != end();
        ++iter, ++original_index) {
     if (service_->IsIncognitoEnabled((*iter)->id())) {
       if (incognito_index == i)
         break;
       ++i;
     }
   }
   return original_index;
 }
 
 int ExtensionToolbarModel::OriginalIndexToIncognito(int original_index) {
   int incognito_index = 0, i = 0;
   for (ExtensionList::iterator iter = begin(); iter != end();
        ++iter, ++i) {
     if (original_index == i)
       break;
     if (service_->IsIncognitoEnabled((*iter)->id()))
       ++incognito_index;
   }
   return incognito_index;
 }