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;
 
+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()),
       extensions_initialized_(false) {
   DCHECK(service_);
 
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED,
                  content::Source<Profile>(service_->profile()));
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,
                  content::Source<Profile>(service_->profile()));
@@ skipping 17 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_) >= 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_ = "";
   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* 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));
 
   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.
+  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);
   // 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 =
+    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();
+  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);
-}
+  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;
 }