Refactor the bookmark star icon to match all other PageActionIconViews

This is a cleanup refactor, the bookmark star is the last PageActionIconView
that isn't contained by OmniboxPageActionIconContainerView.

This CL updates its implementation to conform with all the other PAIVs
and remove specialised LocationBar methods now that it fits
generic interface.

Bug: 788051
Change-Id: I5f382b2eddc767e896f7853c3cc27595dacd3f57
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1808940
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697481}

chrome/browser/ui/BUILD.gn

@@ -961,7 +961,6 @@ jumbo_split_static_library("ui") {
       "intent_picker_tab_helper.h",
       "layout_constants.cc",
       "layout_constants.h",
-      "location_bar/location_bar.cc",
       "location_bar/location_bar.h",
       "managed_ui.cc",
       "managed_ui.h",

chrome/browser/ui/location_bar/location_bar.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/ui/location_bar/location_bar.h"
-
-#include <memory>
-
-#include "base/scoped_observer.h"
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/common/extensions/manifest_handlers/ui_overrides_handler.h"
-#include "content/public/browser/web_contents.h"
-#include "extensions/browser/extension_registry.h"
-#include "extensions/browser/extension_registry_observer.h"
-#include "extensions/common/extension_set.h"
-#include "extensions/common/feature_switch.h"
-#include "extensions/common/permissions/permissions_data.h"
-
-class LocationBar::ExtensionLoadObserver
-    : public extensions::ExtensionRegistryObserver {
- public:
-  ExtensionLoadObserver(LocationBar* location_bar,
-                        extensions::ExtensionRegistry* registry)
-      : location_bar_(location_bar), scoped_observer_(this) {
-    scoped_observer_.Add(registry);
-  }
-
-  ~ExtensionLoadObserver() override = default;
-
-  void OnExtensionLoaded(content::BrowserContext* browser_context,
-                         const extensions::Extension* extension) override {
-    if (extensions::UIOverrides::RemovesBookmarkButton(extension))
-      location_bar_->UpdateBookmarkStarVisibility();
-  }
-
-  void OnExtensionUnloaded(
-      content::BrowserContext* browser_context,
-      const extensions::Extension* extension,
-      extensions::UnloadedExtensionReason reason) override {
-    if (extensions::UIOverrides::RemovesBookmarkButton(extension))
-      location_bar_->UpdateBookmarkStarVisibility();
-  }
-
- private:
-  // The location bar that owns this object (thus always safe to use).
-  LocationBar* const location_bar_;
-
-  ScopedObserver<extensions::ExtensionRegistry,
-                 extensions::ExtensionRegistryObserver>
-      scoped_observer_;
-
-  DISALLOW_COPY_AND_ASSIGN(ExtensionLoadObserver);
-};
-
-LocationBar::LocationBar(Profile* profile) : profile_(profile) {
-  if (profile_) {  // profile_ can be null in tests.
-    extension_load_observer_ = std::make_unique<ExtensionLoadObserver>(
-        this, extensions::ExtensionRegistry::Get(profile_));
-  }
-}
-
-LocationBar::~LocationBar() {
-}
-
-bool LocationBar::IsBookmarkStarHiddenByExtension() const {
-  const extensions::ExtensionSet& extension_set =
-      extensions::ExtensionRegistry::Get(profile_)->enabled_extensions();
-  for (extensions::ExtensionSet::const_iterator i = extension_set.begin();
-       i != extension_set.end(); ++i) {
-    if (extensions::UIOverrides::RemovesBookmarkButton(i->get()) &&
-        ((*i)->permissions_data()->HasAPIPermission(
-             extensions::APIPermission::kBookmarkManagerPrivate) ||
-         extensions::FeatureSwitch::enable_override_bookmarks_ui()
-             ->IsEnabled()))
-      return true;
-  }
-
-  return false;
-}

chrome/browser/ui/location_bar/location_bar.h

@@ -15,7 +15,6 @@
 
 class LocationBarTesting;
 class OmniboxView;
-class Profile;
 
 namespace content {
 class WebContents;
@@ -28,8 +27,6 @@ class WebContents;
 // location bar to be mocked for testing.
 class LocationBar {
  public:
-  explicit LocationBar(Profile* profile);
-
   // The details necessary to open the user's desired omnibox match.
   virtual GURL GetDestinationURL() const = 0;
   virtual WindowOpenDisposition GetWindowOpenDisposition() const = 0;
@@ -61,9 +58,6 @@ class LocationBar {
   // Updates the state of the images showing the content settings status.
   virtual void UpdateContentSettingsIcons() = 0;
 
-  // Updates the visibility of the bookmark star.
-  virtual void UpdateBookmarkStarVisibility() = 0;
-
   // Saves the state of the location bar to the specified WebContents, so that
   // it can be restored later. (Done when switching tabs).
   virtual void SaveStateToContents(content::WebContents* contents) = 0;
@@ -77,29 +71,12 @@ class LocationBar {
   // Returns a pointer to the testing interface.
   virtual LocationBarTesting* GetLocationBarForTesting() = 0;
 
-  Profile* profile() const { return profile_; }
-
  protected:
-  virtual ~LocationBar();
-
-  // Checks if any extension has requested that the bookmark star be hidden.
-  bool IsBookmarkStarHiddenByExtension() const;
-
- private:
-  class ExtensionLoadObserver;
-
-  Profile* profile_;
-
-  std::unique_ptr<ExtensionLoadObserver> extension_load_observer_;
-
-  DISALLOW_COPY_AND_ASSIGN(LocationBar);
+  virtual ~LocationBar() = default;
 };
 
 class LocationBarTesting {
  public:
-  // Returns whether or not the bookmark star decoration is visible.
-  virtual bool GetBookmarkStarVisibility() = 0;
-
   // Invokes the content setting image at |index|, displaying the bubble.
   // Returns false if there is none.
   virtual bool TestContentSettingImagePressed(size_t index) = 0;
@@ -108,7 +85,7 @@ class LocationBarTesting {
   virtual bool IsContentSettingBubbleShowing(size_t index) = 0;
 
  protected:
-  virtual ~LocationBarTesting() {}
+  virtual ~LocationBarTesting() = default;
 };
 
 #endif  // CHROME_BROWSER_UI_LOCATION_BAR_LOCATION_BAR_H_

chrome/browser/ui/location_bar/location_bar_browsertest.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/ui/location_bar/location_bar.h"
-
-#include <memory>
-
-#include "base/macros.h"
-#include "chrome/browser/extensions/extension_browsertest.h"
-#include "chrome/browser/extensions/extension_service.h"
-#include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/browser_window.h"
-#include "extensions/common/extension.h"
-#include "extensions/common/extension_builder.h"
-#include "extensions/common/feature_switch.h"
-#include "extensions/common/value_builder.h"
-
-class LocationBarBrowserTest : public extensions::ExtensionBrowserTest {
- public:
-  LocationBarBrowserTest() {}
-  ~LocationBarBrowserTest() override {}
-
- protected:
-  void SetUpCommandLine(base::CommandLine* command_line) override;
-
- private:
-  std::unique_ptr<extensions::FeatureSwitch::ScopedOverride> enable_override_;
-
-  DISALLOW_COPY_AND_ASSIGN(LocationBarBrowserTest);
-};
-
-void LocationBarBrowserTest::SetUpCommandLine(base::CommandLine* command_line) {
-  extensions::ExtensionBrowserTest::SetUpCommandLine(command_line);
-
-  // In order to let a vanilla extension override the bookmark star, we have to
-  // enable the switch.
-  enable_override_ =
-      std::make_unique<extensions::FeatureSwitch::ScopedOverride>(
-          extensions::FeatureSwitch::enable_override_bookmarks_ui(), true);
-}
-
-// Test that installing an extension that overrides the bookmark star
-// successfully hides the star.
-IN_PROC_BROWSER_TEST_F(LocationBarBrowserTest,
-                       ExtensionCanOverrideBookmarkStar) {
-  LocationBarTesting* location_bar =
-      browser()->window()->GetLocationBar()->GetLocationBarForTesting();
-  // By default, we should show the star.
-  EXPECT_TRUE(location_bar->GetBookmarkStarVisibility());
-
-  // Create and install an extension that overrides the bookmark star.
-  extensions::DictionaryBuilder chrome_ui_overrides;
-  chrome_ui_overrides.Set(
-      "bookmarks_ui",
-      extensions::DictionaryBuilder().Set("remove_button", true).Build());
-  scoped_refptr<const extensions::Extension> extension =
-      extensions::ExtensionBuilder()
-          .SetManifest(
-              extensions::DictionaryBuilder()
-                  .Set("name", "overrides star")
-                  .Set("manifest_version", 2)
-                  .Set("version", "0.1")
-                  .Set("description", "override the star")
-                  .Set("chrome_ui_overrides", chrome_ui_overrides.Build())
-                  .Build())
-          .Build();
-  extension_service()->AddExtension(extension.get());
-
-  // The star should now be hidden.
-  EXPECT_FALSE(location_bar->GetBookmarkStarVisibility());
-}

chrome/browser/ui/page_action/page_action_icon_type.h

@@ -20,6 +20,7 @@ enum class PageActionIconType {
   kNativeFileSystemAccess,
   kClickToCall,
   kCookieControls,
+  kBookmarkStar,
 };
 
 #endif  // CHROME_BROWSER_UI_PAGE_ACTION_PAGE_ACTION_ICON_TYPE_H_

chrome/browser/ui/views/extensions/bookmark_override_browsertest.cc

@@ -49,7 +49,9 @@ IN_PROC_BROWSER_TEST_F(BookmarkOverrideTest, DISABLED_NonOverrideStarClick) {
   // Check that the BookmarkBubbleView is shown when clicking on the star.
   BrowserView* browser_view = reinterpret_cast<BrowserView*>(
       browser()->window());
-  views::View* star_view = browser_view->toolbar()->location_bar()->star_view();
+  views::View* star_view =
+      browser_view->toolbar_button_provider()->GetPageActionIconView(
+          PageActionIconType::kBookmarkStar);
 
   ui::MouseEvent pressed_event(
       ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(), ui::EventTimeForNow(),

chrome/browser/ui/views/frame/browser_view.cc

@@ -803,7 +803,11 @@ void BrowserView::UpdateLoadingAnimations(bool should_animate) {
 }
 
 void BrowserView::SetStarredState(bool is_starred) {
-  GetLocationBarView()->SetStarToggled(is_starred);
+  PageActionIconView* star_icon =
+      toolbar_button_provider_->GetPageActionIconView(
+          PageActionIconType::kBookmarkStar);
+  if (star_icon)
+    star_icon->SetActive(is_starred);
 }
 
 void BrowserView::SetTranslateIconToggled(bool is_lit) {

chrome/browser/ui/views/location_bar/location_bar_view.h

@@ -26,7 +26,6 @@
 #include "chrome/browser/ui/views/location_bar/location_icon_view.h"
 #include "chrome/browser/ui/views/omnibox/omnibox_view_views.h"
 #include "chrome/browser/ui/views/page_action/page_action_icon_view.h"
-#include "components/prefs/pref_member.h"
 #include "components/security_state/core/security_state.h"
 #include "ui/base/material_design/material_design_controller_observer.h"
 #include "ui/gfx/animation/slide_animation.h"
@@ -48,7 +47,6 @@ class OmniboxPopupView;
 class OmniboxPageActionIconContainerView;
 class Profile;
 class SelectedKeywordView;
-class StarView;
 
 namespace views {
 class ImageButton;
@@ -131,12 +129,6 @@ class LocationBarView : public LocationBar,
   // Returns the delegate.
   Delegate* delegate() const { return delegate_; }
 
-  // Toggles the star on or off.
-  void SetStarToggled(bool on);
-
-  // The star. It may not be visible.  It will be null when |browser_| is null.
-  StarView* star_view() { return star_view_; }
-
   OmniboxPageActionIconContainerView*
   omnibox_page_action_icon_container_view() {
     return omnibox_page_action_icon_container_view_;
@@ -219,6 +211,7 @@ class LocationBarView : public LocationBar,
   void OnOmniboxHovered(bool is_hovering);
 
   Browser* browser() { return browser_; }
+  Profile* profile() { return profile_; }
 
   // LocationIconView::Delegate
   bool IsEditingOrEmpty() const override;
@@ -290,13 +283,11 @@ class LocationBarView : public LocationBar,
   void AcceptInput(base::TimeTicks match_selection_timestamp) override;
   void FocusSearch() override;
   void UpdateContentSettingsIcons() override;
-  void UpdateBookmarkStarVisibility() override;
   void SaveStateToContents(content::WebContents* contents) override;
   const OmniboxView* GetOmniboxView() const override;
   LocationBarTesting* GetLocationBarForTesting() override;
 
   // LocationBarTesting:
-  bool GetBookmarkStarVisibility() override;
   bool TestContentSettingImagePressed(size_t index) override;
   bool IsContentSettingBubbleShowing(size_t index) override;
 
@@ -349,6 +340,9 @@ class LocationBarView : public LocationBar,
   // window, so this may be NULL.
   Browser* const browser_;
 
+  // May be nullptr in tests.
+  Profile* const profile_;
+
   OmniboxViewViews* omnibox_view_ = nullptr;
 
   // Our delegate.
@@ -383,9 +377,6 @@ class LocationBarView : public LocationBar,
   OmniboxPageActionIconContainerView* omnibox_page_action_icon_container_view_ =
       nullptr;
 
-  // The star for bookmarking.  It will be null when |browser_| is null.
-  StarView* star_view_ = nullptr;
-
   // An [x] that appears in touch mode (when the OSK is visible) and allows the
   // user to clear all text.
   views::ImageButton* clear_all_button_ = nullptr;
@@ -397,13 +388,6 @@ class LocationBarView : public LocationBar,
   // bar is read-only.
   const bool is_popup_mode_;
 
-  // Tracks this preference to determine whether bookmark editing is allowed.
-  BooleanPrefMember edit_bookmarks_enabled_;
-
-  // A list of all page action icons that haven't yet migrated into the
-  // PageActionIconContainerView (https://crbug.com/788051), ordered by focus.
-  std::vector<PageActionIconView*> page_action_icons_;
-
   // The focus ring, if one is in use.
   std::unique_ptr<views::FocusRing> focus_ring_;
 

chrome/browser/ui/views/location_bar/star_view.cc

@@ -7,18 +7,27 @@
 #include <string>
 
 #include "base/metrics/histogram_macros.h"
+#include "base/scoped_observer.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/defaults.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/bookmarks/bookmark_stats.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h"
 #include "chrome/browser/ui/views/feature_promos/feature_promo_bubble_view.h"
+#include "chrome/common/extensions/manifest_handlers/ui_overrides_handler.h"
 #include "chrome/grit/generated_resources.h"
+#include "components/bookmarks/common/bookmark_pref_names.h"
 #include "components/omnibox/browser/vector_icons.h"
 #include "components/strings/grit/components_strings.h"
 #include "components/variations/variations_associated_data.h"
+#include "content/public/browser/web_contents.h"
+#include "extensions/common/extension_set.h"
+#include "extensions/common/feature_switch.h"
+#include "extensions/common/permissions/permissions_data.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/paint_vector_icon.h"
@@ -47,16 +56,19 @@ StarView::StarView(CommandUpdater* command_updater,
                    PageActionIconView::Delegate* delegate)
     : PageActionIconView(command_updater, IDC_BOOKMARK_THIS_TAB, delegate),
       browser_(browser) {
+  DCHECK(browser_);
+  extension_observer_.Add(
+      extensions::ExtensionRegistry::Get(browser_->profile()));
+  edit_bookmarks_enabled_.Init(
+      bookmarks::prefs::kEditBookmarksEnabled, browser_->profile()->GetPrefs(),
+      base::BindRepeating(&StarView::EditBookmarksPrefUpdated,
+                          base::Unretained(this)));
   SetID(VIEW_ID_STAR_BUTTON);
-  SetToggled(false);
+  SetActive(false);
 }
 
 StarView::~StarView() {}
 
-void StarView::SetToggled(bool on) {
-  PageActionIconView::SetActiveInternal(on);
-}
-
 void StarView::ShowPromo() {
   FeaturePromoBubbleView* bookmark_promo_bubble =
       FeaturePromoBubbleView::CreateOwned(
@@ -66,15 +78,18 @@ void StarView::ShowPromo() {
   if (!bookmark_promo_observer_.IsObserving(
           bookmark_promo_bubble->GetWidget())) {
     bookmark_promo_observer_.Add(bookmark_promo_bubble->GetWidget());
-    SetActiveInternal(false);
+    SetActive(false);
     UpdateIconImage();
   }
 }
 
 bool StarView::Update() {
-  // Updates are handled by |LocationBarView::UpdateBookmarkStarVisibility()|.
-  // TODO(https://crbug.com/788051): Make updates handled here.
-  return false;
+  bool was_visible = GetVisible();
+  SetVisible(browser_defaults::bookmarks_enabled &&
+             !delegate()->IsLocationBarUserInputInProgress() &&
+             edit_bookmarks_enabled_.GetValue() &&
+             !IsBookmarkStarHiddenByExtension());
+  return was_visible != GetVisible();
 }
 
 void StarView::OnExecuting(PageActionIconView::ExecuteSource execute_source) {
@@ -95,12 +110,8 @@ void StarView::OnExecuting(PageActionIconView::ExecuteSource execute_source) {
 }
 
 void StarView::ExecuteCommand(ExecuteSource source) {
-  if (browser_) {
-    OnExecuting(source);
-    chrome::BookmarkCurrentTabIgnoringExtensionOverrides(browser_);
-  } else {
-    PageActionIconView::ExecuteCommand(source);
-  }
+  OnExecuting(source);
+  chrome::BookmarkCurrentTabIgnoringExtensionOverrides(browser_);
 }
 
 views::BubbleDialogDelegateView* StarView::GetBubble() const {
@@ -126,7 +137,44 @@ SkColor StarView::GetInkDropBaseColor() const {
 void StarView::OnWidgetDestroying(views::Widget* widget) {
   if (bookmark_promo_observer_.IsObserving(widget)) {
     bookmark_promo_observer_.Remove(widget);
-    SetActiveInternal(false);
+    SetActive(false);
     UpdateIconImage();
   }
 }
+
+void StarView::OnExtensionLoaded(content::BrowserContext* browser_context,
+                                 const extensions::Extension* extension) {
+  if (extensions::UIOverrides::RemovesBookmarkButton(extension))
+    Update();
+}
+
+void StarView::OnExtensionUnloaded(content::BrowserContext* browser_context,
+                                   const extensions::Extension* extension,
+                                   extensions::UnloadedExtensionReason reason) {
+  if (extensions::UIOverrides::RemovesBookmarkButton(extension))
+    Update();
+}
+
+void StarView::EditBookmarksPrefUpdated() {
+  Update();
+}
+
+bool StarView::IsBookmarkStarHiddenByExtension() const {
+  const extensions::ExtensionSet& extension_set =
+      extensions::ExtensionRegistry::Get(browser_->profile())
+          ->enabled_extensions();
+  for (const scoped_refptr<const extensions::Extension> extension :
+       extension_set) {
+    if (!extensions::UIOverrides::RemovesBookmarkButton(extension.get()))
+      continue;
+    if (extension->permissions_data()->HasAPIPermission(
+            extensions::APIPermission::kBookmarkManagerPrivate)) {
+      return true;
+    }
+    if (extensions::FeatureSwitch::enable_override_bookmarks_ui()
+            ->IsEnabled()) {
+      return true;
+    }
+  }
+  return false;
+}

chrome/browser/ui/views/location_bar/star_view.h

@@ -8,6 +8,9 @@
 #include "base/macros.h"
 #include "base/scoped_observer.h"
 #include "chrome/browser/ui/views/page_action/page_action_icon_view.h"
+#include "components/prefs/pref_member.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_registry_observer.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_observer.h"
 
@@ -15,16 +18,15 @@ class Browser;
 class CommandUpdater;
 
 // The star icon to show a bookmark bubble.
-class StarView : public PageActionIconView, public views::WidgetObserver {
+class StarView : public PageActionIconView,
+                 public views::WidgetObserver,
+                 public extensions::ExtensionRegistryObserver {
  public:
   StarView(CommandUpdater* command_updater,
            Browser* browser,
            PageActionIconView::Delegate* delegate);
   ~StarView() override;
 
-  // Toggles the star on or off.
-  void SetToggled(bool on);
-
   // Shows the BookmarkPromoBubbleView when the BookmarkTracker calls for it.
   void ShowPromo();
 
@@ -41,14 +43,31 @@ class StarView : public PageActionIconView, public views::WidgetObserver {
   // views::WidgetObserver:
   void OnWidgetDestroying(views::Widget* widget) override;
 
+  // extensions::ExtensionRegistryObserver:
+  void OnExtensionLoaded(content::BrowserContext* browser_context,
+                         const extensions::Extension* extension) override;
+  void OnExtensionUnloaded(content::BrowserContext* browser_context,
+                           const extensions::Extension* extension,
+                           extensions::UnloadedExtensionReason reason) override;
+
  private:
+  void EditBookmarksPrefUpdated();
+  bool IsBookmarkStarHiddenByExtension() const;
+
   Browser* const browser_;
 
+  BooleanPrefMember edit_bookmarks_enabled_;
+
   // Observes the BookmarkPromoBubbleView's widget. Used to tell whether the
   // promo is open and gets called back when it closes.
   ScopedObserver<views::Widget, views::WidgetObserver> bookmark_promo_observer_{
       this};
 
+  // Observes Extensions for changes to their |extensions::UIOverrides|.
+  ScopedObserver<extensions::ExtensionRegistry,
+                 extensions::ExtensionRegistryObserver>
+      extension_observer_{this};
+
   DISALLOW_COPY_AND_ASSIGN(StarView);
 };
 

chrome/browser/ui/views/location_bar/star_view_browsertest.cc → chrome/browser/ui/views/location_bar/star_view_interactive_uitest.cc

@@ -10,18 +10,22 @@
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/views/page_action/page_action_icon_view.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "chrome/common/pref_names.h"
-#include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/interactive_test_utils.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/bookmarks/browser/bookmark_utils.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
+#include "extensions/common/extension_builder.h"
+#include "extensions/common/feature_switch.h"
 #include "ui/base/ui_base_switches.h"
 #include "ui/events/event_utils.h"
 #include "ui/views/animation/test/ink_drop_host_view_test_api.h"
@@ -33,21 +37,32 @@
 
 namespace {
 
-class StarViewTest : public InProcessBrowserTest {
+class StarViewTest : public extensions::ExtensionBrowserTest {
  public:
-  StarViewTest() = default;
+  StarViewTest()
+      // In order to let a vanilla extension override the bookmark star, we have
+      // to enable the switch.
+      : enable_override_(
+            extensions::FeatureSwitch::enable_override_bookmarks_ui(),
+            true) {}
   ~StarViewTest() override = default;
 
+  PageActionIconView* GetStarIcon() {
+    return BrowserView::GetBrowserViewForBrowser(browser())
+        ->toolbar_button_provider()
+        ->GetPageActionIconView(PageActionIconType::kBookmarkStar);
+  }
+
  private:
+  extensions::FeatureSwitch::ScopedOverride enable_override_;
+
   DISALLOW_COPY_AND_ASSIGN(StarViewTest);
 };
 
 // Verify that clicking the bookmark star a second time hides the bookmark
 // bubble.
 IN_PROC_BROWSER_TEST_F(StarViewTest, HideOnSecondClick) {
-  BrowserView* browser_view =
-      reinterpret_cast<BrowserView*>(browser()->window());
-  views::View* star_view = browser_view->toolbar()->location_bar()->star_view();
+  views::View* star_icon = GetStarIcon();
 
   ui::MouseEvent pressed_event(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
                                ui::EventTimeForNow(), ui::EF_LEFT_MOUSE_BUTTON,
@@ -58,37 +73,63 @@ IN_PROC_BROWSER_TEST_F(StarViewTest, HideOnSecondClick) {
 
   // Verify that clicking once shows the bookmark bubble.
   EXPECT_FALSE(BookmarkBubbleView::bookmark_bubble());
-  star_view->OnMousePressed(pressed_event);
+  star_icon->OnMousePressed(pressed_event);
   EXPECT_FALSE(BookmarkBubbleView::bookmark_bubble());
-  star_view->OnMouseReleased(released_event);
+  star_icon->OnMouseReleased(released_event);
   EXPECT_TRUE(BookmarkBubbleView::bookmark_bubble());
 
   // Verify that clicking again doesn't reshow it.
-  star_view->OnMousePressed(pressed_event);
+  star_icon->OnMousePressed(pressed_event);
   // Hide the bubble manually. In the browser this would normally happen during
   // the event processing.
   BookmarkBubbleView::Hide();
   base::RunLoop().RunUntilIdle();
   EXPECT_FALSE(BookmarkBubbleView::bookmark_bubble());
-  star_view->OnMouseReleased(released_event);
+  star_icon->OnMouseReleased(released_event);
   EXPECT_FALSE(BookmarkBubbleView::bookmark_bubble());
 }
 
 IN_PROC_BROWSER_TEST_F(StarViewTest, InkDropHighlighted) {
-  BrowserView* browser_view =
-      reinterpret_cast<BrowserView*>(browser()->window());
-  StarView* star_view = browser_view->toolbar()->location_bar()->star_view();
-  views::test::InkDropHostViewTestApi ink_drop_test_api(star_view);
+  PageActionIconView* star_icon = GetStarIcon();
+  views::test::InkDropHostViewTestApi ink_drop_test_api(star_icon);
 
   if (ink_drop_test_api.HasInkDrop()) {
     GURL url("http://test.com");
     bookmarks::BookmarkModel* model =
         BookmarkModelFactory::GetForBrowserContext(browser()->profile());
     bookmarks::AddIfNotBookmarked(model, url, /*title=*/base::string16());
-    browser_view->ShowBookmarkBubble(url, false);
+    browser()->window()->ShowBookmarkBubble(url, false);
     EXPECT_EQ(ink_drop_test_api.GetInkDrop()->GetTargetInkDropState(),
               views::InkDropState::ACTIVATED);
   }
 }
 
+// Test that installing an extension that overrides the bookmark star
+// successfully hides the star.
+IN_PROC_BROWSER_TEST_F(StarViewTest, ExtensionCanOverrideBookmarkStar) {
+  // By default, we should show the star.
+  EXPECT_TRUE(GetStarIcon()->GetVisible());
+
+  // Create and install an extension that overrides the bookmark star.
+  extensions::DictionaryBuilder chrome_ui_overrides;
+  chrome_ui_overrides.Set(
+      "bookmarks_ui",
+      extensions::DictionaryBuilder().Set("remove_button", true).Build());
+  scoped_refptr<const extensions::Extension> extension =
+      extensions::ExtensionBuilder()
+          .SetManifest(
+              extensions::DictionaryBuilder()
+                  .Set("name", "overrides star")
+                  .Set("manifest_version", 2)
+                  .Set("version", "0.1")
+                  .Set("description", "override the star")
+                  .Set("chrome_ui_overrides", chrome_ui_overrides.Build())
+                  .Build())
+          .Build();
+  extension_service()->AddExtension(extension.get());
+
+  // The star should now be hidden.
+  EXPECT_FALSE(GetStarIcon()->GetVisible());
+}
+
 }  // namespace

chrome/browser/ui/views/omnibox/rounded_omnibox_results_frame.cc

@@ -122,7 +122,7 @@ gfx::Insets GetContentInsets() {
 
 RoundedOmniboxResultsFrame::RoundedOmniboxResultsFrame(
     views::View* contents,
-    const LocationBarView* location_bar)
+    LocationBarView* location_bar)
     : contents_(contents) {
   // Host the contents in its own View to simplify layout and clipping.
   contents_host_ = new views::View();

chrome/browser/ui/views/omnibox/rounded_omnibox_results_frame.h

@@ -17,7 +17,7 @@ class LocationBarView;
 class RoundedOmniboxResultsFrame : public views::View {
  public:
   RoundedOmniboxResultsFrame(views::View* contents,
-                             const LocationBarView* location_bar);
+                             LocationBarView* location_bar);
   ~RoundedOmniboxResultsFrame() override;
 
   // Hook to customize Widget initialization.

chrome/browser/ui/views/page_action/omnibox_page_action_icon_container_view.cc

@@ -13,6 +13,7 @@
 #include "chrome/browser/ui/views/location_bar/cookie_controls_icon_view.h"
 #include "chrome/browser/ui/views/location_bar/find_bar_icon.h"
 #include "chrome/browser/ui/views/location_bar/intent_picker_view.h"
+#include "chrome/browser/ui/views/location_bar/star_view.h"
 #include "chrome/browser/ui/views/location_bar/zoom_bubble_view.h"
 #include "chrome/browser/ui/views/native_file_system/native_file_system_access_icon_view.h"
 #include "chrome/browser/ui/views/page_action/pwa_install_view.h"
@@ -131,6 +132,11 @@ OmniboxPageActionIconContainerView::OmniboxPageActionIconContainerView(
             params.command_updater, params.page_action_icon_delegate);
         page_action_icons_.push_back(save_card_icon_view_);
         break;
+      case PageActionIconType::kBookmarkStar:
+        star_view_ = new StarView(params.command_updater, params.browser,
+                                  params.page_action_icon_delegate);
+        page_action_icons_.push_back(star_view_);
+        break;
     }
   }
 
@@ -152,9 +158,6 @@ OmniboxPageActionIconContainerView::~OmniboxPageActionIconContainerView() {}
 
 PageActionIconView* OmniboxPageActionIconContainerView::GetIconView(
     PageActionIconType type) {
-  // TODO(https://crbug.com/788051): Update page action icons here as update
-  // methods are migrated out of LocationBar to the PageActionIconContainer
-  // interface.
   switch (type) {
     case PageActionIconType::kFind:
       return find_bar_icon_;
@@ -184,6 +187,8 @@ PageActionIconView* OmniboxPageActionIconContainerView::GetIconView(
       return local_card_migration_icon_view_;
     case PageActionIconType::kSaveCard:
       return save_card_icon_view_;
+    case PageActionIconType::kBookmarkStar:
+      return star_view_;
   }
   return nullptr;
 }

chrome/browser/ui/views/page_action/omnibox_page_action_icon_container_view.h

@@ -25,6 +25,7 @@ class NativeFileSystemAccessIconView;
 class PwaInstallView;
 class ReaderModeIconView;
 class SharingIconView;
+class StarView;
 class TranslateIconView;
 class ZoomView;
 
@@ -104,6 +105,7 @@ class OmniboxPageActionIconContainerView
   autofill::LocalCardMigrationIconView* local_card_migration_icon_view_ =
       nullptr;
   autofill::SaveCardIconView* save_card_icon_view_ = nullptr;
+  StarView* star_view_ = nullptr;
 
   std::vector<PageActionIconView*> page_action_icons_;
 

chrome/browser/ui/views/page_action/page_action_icon_view.cc

@@ -194,6 +194,13 @@ void PageActionIconView::SetIconColor(SkColor icon_color) {
   UpdateIconImage();
 }
 
+void PageActionIconView::SetActive(bool active) {
+  if (active_ == active)
+    return;
+  active_ = active;
+  UpdateIconImage();
+}
+
 void PageActionIconView::UpdateIconImage() {
   const ui::NativeTheme* theme = GetNativeTheme();
   SkColor icon_color = active_
@@ -204,13 +211,6 @@ void PageActionIconView::UpdateIconImage() {
                                           icon_color, GetVectorIconBadge()));
 }
 
-void PageActionIconView::SetActiveInternal(bool active) {
-  if (active_ == active)
-    return;
-  active_ = active;
-  UpdateIconImage();
-}
-
 content::WebContents* PageActionIconView::GetWebContents() const {
   return delegate_->GetWebContentsForPageActionIconView();
 }

chrome/browser/ui/views/page_action/page_action_icon_view.h

@@ -62,6 +62,10 @@ class PageActionIconView : public IconLabelBubbleView {
 
   void set_icon_size(int size) { icon_size_ = size; }
 
+  // Sets the active state of the icon. An active icon will be displayed in a
+  // "call to action" color.
+  void SetActive(bool active);
+
   // Updates the visibility of the icon based on the associated model state,
   // returns whether any change occurred.
   virtual bool Update() = 0;
@@ -134,10 +138,6 @@ class PageActionIconView : public IconLabelBubbleView {
   // Updates the icon image after some state has changed.
   virtual void UpdateIconImage();
 
-  // Sets the active state of the icon. An active icon will be displayed in a
-  // "call to action" color.
-  void SetActiveInternal(bool active);
-
   // Returns the associated web contents from the delegate.
   content::WebContents* GetWebContents() const;
 

chrome/browser/ui/views/reader_mode/reader_mode_icon_view.cc

@@ -36,7 +36,7 @@ bool ReaderModeIconView::Update() {
   // TODO(gilmanmh): Display icon for only those pages that are likely to
   // render well in Reader Mode.
   SetVisible(true);
-  SetActiveInternal(IsDistilledPage(contents->GetLastCommittedURL()));
+  SetActive(IsDistilledPage(contents->GetLastCommittedURL()));
 
   // Notify the icon when navigation to and from a distilled page occurs so that
   // it can hide the inkdrop.

chrome/browser/ui/views/toolbar/toolbar_view.cc

@@ -433,15 +433,16 @@ void ToolbarView::ShowBookmarkBubble(
     bool already_bookmarked,
     bookmarks::BookmarkBubbleObserver* observer) {
   views::View* const anchor_view = location_bar();
-  PageActionIconView* const star_view = location_bar()->star_view();
+  PageActionIconView* const bookmark_star_icon =
+      GetPageActionIconView(PageActionIconType::kBookmarkStar);
 
   std::unique_ptr<BubbleSyncPromoDelegate> delegate;
 #if !defined(OS_CHROMEOS)
   // ChromeOS does not show the signin promo.
   delegate.reset(new BookmarkBubbleSignInDelegate(browser_));
 #endif
-  BookmarkBubbleView::ShowBubble(anchor_view, star_view, gfx::Rect(), nullptr,
-                                 observer, std::move(delegate),
+  BookmarkBubbleView::ShowBubble(anchor_view, bookmark_star_icon, gfx::Rect(),
+                                 nullptr, observer, std::move(delegate),
                                  browser_->profile(), url, already_bookmarked);
 }
 

chrome/test/BUILD.gn

@@ -1205,7 +1205,6 @@ if (!is_android) {
       "../browser/ui/find_bar/find_bar_platform_helper_mac_browsertest.mm",
       "../browser/ui/hats/hats_service_browsertest.cc",
       "../browser/ui/javascript_dialogs/javascript_dialog_browsertest.cc",
-      "../browser/ui/location_bar/location_bar_browsertest.cc",
       "../browser/ui/login/login_handler_browsertest.cc",
       "../browser/ui/native_window_tracker_browsertest.cc",
       "../browser/ui/passwords/manage_passwords_test.cc",
@@ -5595,7 +5594,7 @@ if (!is_android) {
         "../browser/ui/views/keyboard_access_browsertest.cc",
         "../browser/ui/views/location_bar/location_icon_view_interactive_uitest.cc",
         "../browser/ui/views/location_bar/selected_keyword_view_interactive_uitest.cc",
-        "../browser/ui/views/location_bar/star_view_browsertest.cc",
+        "../browser/ui/views/location_bar/star_view_interactive_uitest.cc",
         "../browser/ui/views/menu_controller_interactive_uitest.cc",
         "../browser/ui/views/menu_interactive_uitest.cc",
         "../browser/ui/views/menu_item_view_interactive_uitest.cc",

chrome/test/base/test_browser_window.h

@@ -195,8 +195,8 @@ class TestBrowserWindow : public BrowserWindow {
  private:
   class TestLocationBar : public LocationBar {
    public:
-    TestLocationBar() : LocationBar(NULL) {}
-    ~TestLocationBar() override {}
+    TestLocationBar() = default;
+    ~TestLocationBar() override = default;
 
     // LocationBar:
     GURL GetDestinationURL() const override;
@@ -208,7 +208,6 @@ class TestBrowserWindow : public BrowserWindow {
     void FocusLocation(bool select_all) override {}
     void FocusSearch() override {}
     void UpdateContentSettingsIcons() override {}
-    void UpdateBookmarkStarVisibility() override {}
     void SaveStateToContents(content::WebContents* contents) override {}
     void Revert() override {}
     const OmniboxView* GetOmniboxView() const override;