Stop showing page actions in the location bar with redesign enabled

chrome/browser/extensions/extension_action_test_util.cc

+// Copyright 2014 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_action_test_util.h"
+
+#include "chrome/browser/extensions/extension_action.h"
+#include "chrome/browser/extensions/extension_action_manager.h"
+#include "chrome/browser/extensions/extension_toolbar_model.h"
+#include "chrome/browser/extensions/location_bar_controller.h"
+#include "chrome/browser/extensions/tab_helper.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sessions/session_tab_helper.h"
+#include "content/public/browser/web_contents.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/feature_switch.h"
+
+namespace extensions {
+namespace extension_action_test_util {
+
+namespace {
+
+size_t GetPageActionCount(content::WebContents* web_contents,
+                          bool only_count_visible) {
+  DCHECK(web_contents);
+  std::vector<ExtensionAction*> page_actions;
+  // Page actions are either stored in the location bar (and provided by the
+  // LocationBarController), or in the main toolbar (and provided by the
+  // ExtensionToolbarModel), depending on whether or not the extension action
+  // redesign is enabled.
+  if (!FeatureSwitch::extension_action_redesign()->IsEnabled()) {
+    page_actions =
+        TabHelper::FromWebContents(web_contents)->
+            location_bar_controller()->GetCurrentActions();
+  } else {
+    ExtensionToolbarModel* toolbar_model =
+        ExtensionToolbarModel::Get(
+            Profile::FromBrowserContext(web_contents->GetBrowserContext()));
+    const ExtensionList& toolbar_extensions = toolbar_model->toolbar_items();
+    ExtensionActionManager* action_manager =
+        ExtensionActionManager::Get(web_contents->GetBrowserContext());
+    for (ExtensionList::const_iterator iter = toolbar_extensions.begin();
+         iter != toolbar_extensions.end(); ++iter) {
+      ExtensionAction* extension_action = action_manager->GetPageAction(**iter);
+      if (extension_action)
+        page_actions.push_back(extension_action);

[Finnur, 2014/08/28 11:35:34]

Isn't it more efficient (and clearer) to check visibility here (if
|only_count_visible| is true) and return page_actions.size() afterwards?

And then move all code below us in this function into the if clause (to line
35).

[Devlin, 2014/08/28 18:08:53]

I was torn (and originally had it that way).  The downside is that then we have
to check visibility in two places, rather than having the same approach for when
the switch is on/off.

I'm not too worried about performance (only used in testing), but if you think
it's less readable this way, definitely worth changing.
Done.

[Finnur, 2014/08/29 12:10:41]

Yeah, I think this is a little bit better.

+    }
+  }
+
+  size_t count = page_actions.size();
+
+  // Trim any invisible page actions, if necessary.
+  if (only_count_visible) {
+    int tab_id = SessionTabHelper::IdForTab(web_contents);
+    for (std::vector<ExtensionAction*>::iterator iter = page_actions.begin();
+         iter != page_actions.end();) {
+      if (!(*iter)->GetIsVisible(tab_id))
+        --count;
+    }
+  }
+
+  return count;
+}
+
+}  // namespace
+
+size_t GetVisiblePageActionCount(content::WebContents* web_contents) {
+  return GetPageActionCount(web_contents, true);
+}
+
+size_t GetTotalPageActionCount(content::WebContents* web_contents) {
+  return GetPageActionCount(web_contents, false);
+}
+
+}  // namespace extension_action_test_util
+}  // namespace extensions