Fix regression in showing bookmark star when an extension should override it

https://codereview.chromium.org/505373002 introduced a regression by
implicitly changing an Update() call to an UpdatePageActions() call on
extensions being loaded/unloaded. This is mostly correct, but we lost the
check to make sure an extension didn't override the bookmark star. Correct this.
Also add a regression test.

BUG=410372
TBR=sky@chromium.org (minor TestBrowserWindow change)

Review URL: https://codereview.chromium.org/540483002

Cr-Commit-Position: refs/heads/master@{#293737}

chrome/browser/extensions/location_bar_controller.cc

@@ -7,6 +7,11 @@
 #include "chrome/browser/extensions/active_script_controller.h"
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/location_bar/location_bar.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/common/feature_switch.h"
@@ -74,6 +79,25 @@ void LocationBarController::OnExtensionLoaded(
     ExtensionActionAPI::Get(browser_context)->
         NotifyPageActionsChanged(web_contents_);
   }
+
+  // We might also need to update the location bar if the extension can remove
+  // the bookmark star.
+  if (UIOverrides::RemovesBookmarkButton(extension)) {
+    Browser* browser = chrome::FindBrowserWithWebContents(web_contents_);
+    // In a perfect world, this can never be NULL. Unfortunately, since a
+    // LocationBarController is attached to most WebContents, we can't make that
+    // guarantee.
+    if (!browser)
+      return;
+    // window() can be NULL if this is called before CreateBrowserWindow()
+    // completes, and there won't be a location bar if the window has no toolbar
+    // (e.g., and app window).
+    LocationBar* location_bar =
+        browser->window() ? browser->window()->GetLocationBar() : NULL;
+    if (!location_bar)
+      return;
+    location_bar->UpdateBookmarkStarVisibility();
+  }
 }
 
 void LocationBarController::OnExtensionUnloaded(

chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h

@@ -67,6 +67,7 @@ class LocationBarViewMac : public LocationBar,
   virtual void UpdateManagePasswordsIconAndBubble() OVERRIDE;
   virtual void UpdatePageActions() OVERRIDE;
   virtual void InvalidatePageActions() OVERRIDE;
+  virtual void UpdateBookmarkStarVisibility() OVERRIDE;
   virtual bool ShowPageActionPopup(const extensions::Extension* extension,
                                    bool grant_active_tab) OVERRIDE;
   virtual void UpdateOpenPDFInReaderPrompt() OVERRIDE;
@@ -213,9 +214,6 @@ class LocationBarViewMac : public LocationBar,
   // Returns whether any updates were made.
   bool UpdateZoomDecoration();
 
-  // Ensures the star decoration is visible or hidden, as required.
-  void UpdateStarDecorationVisibility();
-
   // Updates the voice search decoration. Returns true if the visible state was
   // changed.
   bool UpdateMicSearchDecorationVisibility();

chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm

@@ -224,6 +224,10 @@ void LocationBarViewMac::InvalidatePageActions() {
   Layout();
 }
 
+void LocationBarViewMac::UpdateBookmarkStarVisibility() {
+  star_decoration_->SetVisible(IsStarEnabled());
+}
+
 bool LocationBarViewMac::ShowPageActionPopup(
     const extensions::Extension* extension, bool grant_active_tab) {
   for (ScopedVector<PageActionDecoration>::iterator iter =
@@ -313,7 +317,7 @@ bool LocationBarViewMac::GetBookmarkStarVisibility() {
 
 void LocationBarViewMac::SetEditable(bool editable) {
   [field_ setEditable:editable ? YES : NO];
-  UpdateStarDecorationVisibility();
+  UpdateBookmarkStarVisibility();
   UpdateZoomDecoration();
   UpdatePageActions();
   Layout();
@@ -328,7 +332,7 @@ void LocationBarViewMac::SetStarred(bool starred) {
     return;
 
   star_decoration_->SetStarred(starred);
-  UpdateStarDecorationVisibility();
+  UpdateBookmarkStarVisibility();
   OnDecorationsChanged();
 }
 
@@ -528,7 +532,7 @@ NSPoint LocationBarViewMac::GetPageActionBubblePoint(
 
 void LocationBarViewMac::Update(const WebContents* contents) {
   UpdateManagePasswordsIconAndBubble();
-  UpdateStarDecorationVisibility();
+  UpdateBookmarkStarVisibility();
   UpdateTranslateDecoration();
   UpdateZoomDecoration();
   RefreshPageActionDecorations();
@@ -658,7 +662,7 @@ void LocationBarViewMac::DeletePageActionDecorations() {
 }
 
 void LocationBarViewMac::OnEditBookmarksEnabledChanged() {
-  UpdateStarDecorationVisibility();
+  UpdateBookmarkStarVisibility();
   OnChanged();
 }
 
@@ -756,10 +760,6 @@ bool LocationBarViewMac::UpdateZoomDecoration() {
       ZoomController::FromWebContents(web_contents));
 }
 
-void LocationBarViewMac::UpdateStarDecorationVisibility() {
-  star_decoration_->SetVisible(IsStarEnabled());
-}
-
 bool LocationBarViewMac::UpdateMicSearchDecorationVisibility() {
   bool is_visible = !GetToolbarModel()->input_in_progress() &&
                     browser_->search_model()->voice_search_supported();

chrome/browser/ui/location_bar/location_bar.h

@@ -58,6 +58,9 @@ class LocationBar {
   // Updates the state of the page actions.
   virtual void UpdatePageActions() = 0;
 
+  // Updates the visibility of the bookmark star.
+  virtual void UpdateBookmarkStarVisibility() = 0;
+
   // Called when the page-action data needs to be refreshed, e.g. when an
   // extension is unloaded or crashes.
   virtual void InvalidatePageActions() = 0;

chrome/browser/ui/location_bar/location_bar_browsertest.cc

@@ -9,6 +9,7 @@
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_test_message_listener.h"
 #include "chrome/browser/extensions/test_extension_dir.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
@@ -17,7 +18,9 @@
 #include "chrome/browser/ui/location_bar/location_bar.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/extension_builder.h"
 #include "extensions/common/feature_switch.h"
+#include "extensions/common/value_builder.h"
 
 namespace {
 
@@ -39,14 +42,30 @@ const char kManifestSource[] =
 
 class LocationBarBrowserTest : public ExtensionBrowserTest {
  public:
+  LocationBarBrowserTest() {}
   virtual ~LocationBarBrowserTest() {}
 
  protected:
+  virtual void SetUpCommandLine(base::CommandLine* command_line) OVERRIDE;
+
   // Load an extension with a PageAction that sends a message when clicked.
   const extensions::Extension* LoadPageActionExtension(
       extensions::TestExtensionDir* dir);
+
+ private:
+  scoped_ptr<extensions::FeatureSwitch::ScopedOverride> enable_override_;
+
+  DISALLOW_COPY_AND_ASSIGN(LocationBarBrowserTest);
 };
 
+void LocationBarBrowserTest::SetUpCommandLine(base::CommandLine* command_line) {
+  // In order to let a vanilla extension override the bookmark star, we have to
+  // enable the switch.
+  enable_override_.reset(new extensions::FeatureSwitch::ScopedOverride(
+      extensions::FeatureSwitch::enable_override_bookmarks_ui(), true));
+  ExtensionBrowserTest::SetUpCommandLine(command_line);
+}
+
 const extensions::Extension* LocationBarBrowserTest::LoadPageActionExtension(
     extensions::TestExtensionDir* dir) {
   DCHECK(dir);
@@ -116,11 +135,46 @@ IN_PROC_BROWSER_TEST_F(LocationBarBrowserTest, MAYBE_PageActionUITest) {
   EXPECT_TRUE(clicked_listener.WaitUntilSatisfied());
 }
 
+// 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().SetBoolean("remove_button", true));
+  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.Pass())).Build();
+  extension_service()->AddExtension(extension.get());
+
+  // The star should now be hidden.
+  EXPECT_FALSE(location_bar->GetBookmarkStarVisibility());
+}
+
 class LocationBarBrowserTestWithRedesign : public LocationBarBrowserTest {
+ public:
+  LocationBarBrowserTestWithRedesign() {}
+  virtual ~LocationBarBrowserTestWithRedesign() {}
+
  private:
   virtual void SetUpCommandLine(base::CommandLine* command_line) OVERRIDE;
 
   scoped_ptr<extensions::FeatureSwitch::ScopedOverride> enable_redesign_;
+
+  DISALLOW_COPY_AND_ASSIGN(LocationBarBrowserTestWithRedesign);
 };
 
 void LocationBarBrowserTestWithRedesign::SetUpCommandLine(

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

@@ -981,13 +981,8 @@ void LocationBarView::Update(const WebContents* contents) {
       GetToolbarModel()->input_in_progress() ? NULL : GetWebContents();
   open_pdf_in_reader_view_->Update(web_contents_for_sub_views);
 
-  if (star_view_) {
-    star_view_->SetVisible(
-        browser_defaults::bookmarks_enabled && !is_popup_mode_ &&
-        !GetToolbarModel()->input_in_progress() &&
-        edit_bookmarks_enabled_.GetValue() &&
-        !IsBookmarkStarHiddenByExtension());
-  }
+  if (star_view_)
+    UpdateBookmarkStarVisibility();
 
   if (contents)
     omnibox_view_->OnTabChanged(contents);
@@ -1300,6 +1295,16 @@ void LocationBarView::InvalidatePageActions() {
   DeletePageActionViews();
 }
 
+void LocationBarView::UpdateBookmarkStarVisibility() {
+  if (star_view_) {
+    star_view_->SetVisible(
+        browser_defaults::bookmarks_enabled && !is_popup_mode_ &&
+        !GetToolbarModel()->input_in_progress() &&
+        edit_bookmarks_enabled_.GetValue() &&
+        !IsBookmarkStarHiddenByExtension());
+  }
+}
+
 bool LocationBarView::ShowPageActionPopup(
     const extensions::Extension* extension,
     bool grant_tab_permissions) {

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

@@ -349,6 +349,7 @@ class LocationBarView : public LocationBar,
   virtual void UpdateManagePasswordsIconAndBubble() OVERRIDE;
   virtual void UpdatePageActions() OVERRIDE;
   virtual void InvalidatePageActions() OVERRIDE;
+  virtual void UpdateBookmarkStarVisibility() OVERRIDE;
   virtual bool ShowPageActionPopup(const extensions::Extension* extension,
                                    bool grant_active_tab) OVERRIDE;
   virtual void UpdateOpenPDFInReaderPrompt() OVERRIDE;

chrome/test/base/test_browser_window.h

@@ -179,6 +179,7 @@ class TestBrowserWindow : public BrowserWindow {
     virtual void UpdateManagePasswordsIconAndBubble() OVERRIDE {}
     virtual void UpdatePageActions() OVERRIDE {}
     virtual void InvalidatePageActions() OVERRIDE {}
+    virtual void UpdateBookmarkStarVisibility() OVERRIDE {}
     virtual bool ShowPageActionPopup(const extensions::Extension* extension,
                                      bool grant_active_tab) OVERRIDE;
     virtual void UpdateOpenPDFInReaderPrompt() OVERRIDE {}