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();
   }
   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)
+    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))
       AddExtension(extension);
   } else if (type == chrome::NOTIFICATION_EXTENSION_UNLOADED) {
     RemoveExtension(extension);
   } else if (
         type ==
         chrome::NOTIFICATION_EXTENSION_BROWSER_ACTION_VISIBILITY_CHANGED) {
     if (service_->extension_prefs()->GetBrowserActionVisibility(extension))
       AddExtension(extension);
     else
       RemoveExtension(extension);
   } else {
     NOTREACHED() << "Received unexpected notification";
   }
 }
 
 void ExtensionToolbarModel::AddExtension(const Extension* extension) {
   // 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_,
+      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_ = "";
   last_extension_removed_index_ = -1;
 
   UpdatePrefs();
 }
 
 void ExtensionToolbarModel::RemoveExtension(const Extension* extension) {
   ExtensionList::iterator pos = std::find(begin(), end(), extension);
   if (pos == end()) {
     return;
   }
 
   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));
 
   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() {

[Peter Kasting, 2012/07/21 01:55:13]

This function never calls GetBrowserActionVisibility() anywhere, where
PopulateForNonActionBoxMode() does.  That seems questionable?  Is there a
problem here?

[yefimt, 2012/07/23 23:47:52]

No, Aaron advised that just using toolbar and action box orders should be
enough. We agreed that I'll clean up rest of the files in a separate CL. And
then after action box will be enabled by default, non action box functionality
will be removed

+  const VectorOfStrings toolbar_order =
+      service_->extension_prefs()->GetToolbarOrder();
+  VectorOfStrings action_box_order =
+      service_->extension_prefs()->GetActionBoxOrder();
+
+  // Create the lists.

[Peter Kasting, 2012/07/21 01:55:13]

Nit: This comment is misleading, because we don't create the lists until the
next block.

Instead, this should be something like "Add all browser actions not already in
the toolbar or action box to the action box."

But I question whether that's the right thing to do.  Shouldn't
GetActionBoxOrder() be responsible for doing something like this?

[yefimt, 2012/07/23 23:47:52]

Comment - done.
I dont think GetActionBoxOrder() should do it, but you are right that changes in
extensions are not handled when action box is enabled. Fixing it in Observe()

+  for (ExtensionSet::const_iterator iter = service_->extensions()->begin();
+       iter != service_->extensions()->end(); ++iter) {
+    const Extension* extension = *iter;
+    if (!extension->browser_action())
+      continue;
+
+    VectorOfStrings::const_iterator toolbar_pos =
+        std::find(toolbar_order.begin(), toolbar_order.end(), extension->id());
+    VectorOfStrings::const_iterator action_box_pos =
+        std::find(action_box_order.begin(), action_box_order.end(),
+        extension->id());

[Peter Kasting, 2012/07/21 01:55:13]

Nit: Indent even with first arg in line just above

[yefimt, 2012/07/23 23:47:52]

Done.

+    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.
+  for (size_t i = 0; i < toolbar_items_.size(); i++) {

[Peter Kasting, 2012/07/21 01:55:13]

This loop seems wrong for a couple of reasons.

(1) I don't think |action_box_menu_items_| is guaranteed to be the same length
as |toolbar_items_| (especially after the loop above!), so it doesn't seem safe
to blindly index into one based on the size of the other.  This should probably
be two loops.
(2) Are the items in these two lists guaranteed to be fully disjoint?  If not,
we'll notify about some of them twice.  Seems bad.

[yefimt, 2012/07/23 23:47:52]

Yes an extension guarantied to be in the one list only

+    FOR_EACH_OBSERVER(Observer, observers_,
+                      BrowserActionAdded(action_box_menu_items_[i], i));

[Peter Kasting, 2012/07/21 01:55:13]

Maybe not in scope for this change, but it seems like maybe BrowserActionAdded()
should pass along whether an extension is in the action box or the toolbar, or
there should be a separate observer function for these two cases?  I guess I
don't know what the observers ultimately do with this, but it seems like if it's
important that we pass along the order number to the observer, then this would
be important too...

[yefimt, 2012/07/23 23:47:52]

I see your point. I'll keep it in mind when get to the rest of functionality

+    FOR_EACH_OBSERVER(Observer, observers_,
+                      BrowserActionAdded(toolbar_items_[i], i));
+  }
+}
+
+void ExtensionToolbarModel::PopulateForNonActionBoxMode() {
+  const VectorOfStrings 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())

[Peter Kasting, 2012/07/21 01:55:13]

Nit: Can combine with the next condition by adding "||"

[yefimt, 2012/07/23 23:47:52]

I thought it is more readable this way and second one eventually will go away
completely.

       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();
+  MergeLists(sorted, unsorted, &toolbar_items_);

[Peter Kasting, 2012/07/21 01:55:13]

Nit: This function is only called once, and is short and simple enough that I
probably wouldn't factor it out into a helper.

[yefimt, 2012/07/23 23:47:52]

Done.

[yefimt, 2012/07/23 23:47:52]

Done.

+
+  // Inform observers.
+  for (size_t i = 0; i < toolbar_items_.size(); i++) {
+    FOR_EACH_OBSERVER(Observer, observers_,
+                      BrowserActionAdded(toolbar_items_[i], i));
+  }
+}
+
+void ExtensionToolbarModel::AddToProperList(const Extension* extension,
+    const VectorOfStrings& order,
+    ExtensionList* sorted,
+    ExtensionList* unsorted) {

[Peter Kasting, 2012/07/21 01:55:13]

Nit: I suggest:

  DCHECK_EQ(order.size(), sorted->size());

...atop this function to make it clear that the offset access below is safe.

[yefimt, 2012/07/23 23:47:52]

Done.

+  VectorOfStrings::const_iterator pos =
+      std::find(order.begin(), order.end(), extension->id());
+  if (pos != order.end()) {

[Peter Kasting, 2012/07/21 01:55:13]

Nit: No {} needed

[yefimt, 2012/07/23 23:47:52]

Done.

+    (*sorted)[pos - order.begin()] = 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) {
     if (*iter != NULL)

[Peter Kasting, 2012/07/21 01:55:13]

Is this check actually necessary?  It seems like it could only succeed if
GetToolbarOrder() returns a list that has elements in it that don't actually
appear in ExtensionService::extensions().  That seems like it shouldn't happen.

If this isn't necessary, then this whole function can be eliminated, and the
caller can simply append |unsorted| to the end of |sorted| and then notify.

(I know the origin of this is from the original code; if it's difficult to
determine the answer to this you can do it as a followup, but file a bug for it
if so.)

[yefimt, 2012/07/23 23:47:52]

I think you are right, I cannot see how it could be NULL.

[Aaron Boodman, 2012/07/23 23:57:51]

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.

[yefimt, 2012/07/24 00:14:49]

OK, restored original code

[Peter Kasting, 2012/07/24 04:27:29]

Thanks, that's good to know!  Yefim, can you make sure to add comments about
this (in the various spots I was concerned about) so the next person who reads
the various places in this file I've asked about will have the benefit of
Aaron's knowledge here?

-      toolitems_.push_back(*iter);
+      result_list->push_back(*iter);

[Peter Kasting, 2012/07/21 01:55:13]

This seems to assume |result_list| was empty on entry.  I suggest DCHECKing
that.

[yefimt, 2012/07/23 23:47:52]

Done.

   }
-  toolitems_.insert(toolitems_.end(), unsorted.begin(), unsorted.end());
+  result_list->insert(result_list->end(), unsorted.begin(), unsorted.end());
+}
 
-  // Inform observers.
-  for (size_t i = 0; i < toolitems_.size(); i++) {
-    FOR_EACH_OBSERVER(Observer, observers_,
-                      BrowserActionAdded(toolitems_[i], i));
+void ExtensionToolbarModel::FillExtensionList(const VectorOfStrings& 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 =
+        service_->GetExtensionById(order[i], false);
+    if (!extension)

[Peter Kasting, 2012/07/21 01:55:13]

Can this happen?  This seems like another case that should only happen if the
order lists have entries that aren't valid extensions.

Either this should be eliminated, be a DCHECK(extension), or if this case really
can happen, then the code should probably just skip the item and not return
entirely:

    if (extension)
      result_list->push_back(extension);

[yefimt, 2012/07/23 23:47:52]

I think it could happen. Extensions order coming from preferences, it could be
edited or corrupted.

+      return;
+    result_list->push_back(extension);
   }
-
-  UpdatePrefs();
-
-  extensions_initialized_ = true;
-  FOR_EACH_OBSERVER(Observer, observers_, ModelLoaded());
 }
 
 void ExtensionToolbarModel::UpdatePrefs() {
   if (!service_->extension_prefs())
     return;
 
-  std::vector<std::string> ids;
-  ids.reserve(toolitems_.size());
+  VectorOfStrings toolbar_ids;
+  toolbar_ids.reserve(toolbar_items_.size());
   for (ExtensionList::iterator iter = begin(); iter != end(); ++iter)

[Peter Kasting, 2012/07/21 01:55:13]

Nit: I think this can be a const_iterator (and below, if you choose to change
that one, see comment there)

[yefimt, 2012/07/23 23:47:52]

Done.

-    ids.push_back((*iter)->id());
-  service_->extension_prefs()->SetToolbarOrder(ids);
-}
+    toolbar_ids.push_back((*iter)->id());
+  service_->extension_prefs()->SetToolbarOrder(toolbar_ids);
 
-const Extension* ExtensionToolbarModel::GetExtensionByIndex(int index) const {
-  return toolitems_[index];
+  VectorOfStrings action_box_ids;
+  action_box_ids.reserve(action_box_menu_items_.size());
+  for (size_t i = 0; i < action_box_menu_items_.size(); ++i)

[Peter Kasting, 2012/07/21 01:55:13]

Nit: Is there a reason you use an iterator above and an index here?  I think
it'd be better to be consistent (I'd use an iterator for both).

[yefimt, 2012/07/23 23:47:52]

Done.

+    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;
 }