Bookmark pop-up doesn't open if Ctrl+D is set as keyboard shortcut for added extensions.

Presently when we selects bookmark icon then it checks weather Ctrl+D has been set as overridden command for any extension if yes, then it executes extension popup for that extension.

Changes done so that when bookmarking a page is done via bookmark icon by mouse click then bookmark overridden commands should not be considered as overriding happened for keyboard shortcuts.
As user have overridden shortcut key for bookmark to launch extension pop up, not the bookmarking via selection of bookmark icon by mouse event.

TBR=sky@chromium.org

BUG=426791

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

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

8 files changed, 61 insertions, 45 deletions

diff --git a/chrome/browser/browser_commands_unittest.cc b/chrome/browser/browser_commands_unittest.cc
index eb110ad..b92d173 100644
--- a/chrome/browser/browser_commands_unittest.cc
+++ b/chrome/browser/browser_commands_unittest.cc
@@ -149,7 +149,7 @@ TEST_F(BrowserCommandsTest, BookmarkCurrentPage) {
   browser()->OpenURL(OpenURLParams(
       url1, Referrer(), CURRENT_TAB, ui::PAGE_TRANSITION_TYPED, false));
 
-  chrome::BookmarkCurrentPage(browser());
+  chrome::BookmarkCurrentPageAllowingExtensionOverrides(browser());
 
   // It should now be bookmarked in the bookmark model.
   EXPECT_EQ(profile(), browser()->profile());
diff --git a/chrome/browser/ui/browser_command_controller.cc b/chrome/browser/ui/browser_command_controller.cc
index a49e338..6b8cb11 100644
--- a/chrome/browser/ui/browser_command_controller.cc
+++ b/chrome/browser/ui/browser_command_controller.cc
@@ -528,7 +528,7 @@ void BrowserCommandController::ExecuteCommandWithDisposition(
       SavePage(browser_);
       break;
     case IDC_BOOKMARK_PAGE:
-      BookmarkCurrentPage(browser_);
+      BookmarkCurrentPageAllowingExtensionOverrides(browser_);
       break;
     case IDC_BOOKMARK_ALL_TABS:
       BookmarkAllTabs(browser_);
diff --git a/chrome/browser/ui/browser_commands.cc b/chrome/browser/ui/browser_commands.cc
index d35e866..6e54930 100644
--- a/chrome/browser/ui/browser_commands.cc
+++ b/chrome/browser/ui/browser_commands.cc
@@ -167,38 +167,6 @@ bool GetBookmarkOverrideCommand(
 }
 #endif
 
-void BookmarkCurrentPageInternal(Browser* browser) {
-  content::RecordAction(UserMetricsAction("Star"));
-
-  BookmarkModel* model =
-      BookmarkModelFactory::GetForProfile(browser->profile());
-  if (!model || !model->loaded())
-    return;  // Ignore requests until bookmarks are loaded.
-
-  GURL url;
-  base::string16 title;
-  WebContents* web_contents =
-      browser->tab_strip_model()->GetActiveWebContents();
-  GetURLAndTitleToBookmark(web_contents, &url, &title);
-  bool is_bookmarked_by_any = model->IsBookmarked(url);
-  if (!is_bookmarked_by_any &&
-      web_contents->GetBrowserContext()->IsOffTheRecord()) {
-    // If we're incognito the favicon may not have been saved. Save it now
-    // so that bookmarks have an icon for the page.
-    FaviconTabHelper::FromWebContents(web_contents)->SaveFavicon();
-  }
-  bool was_bookmarked_by_user = bookmarks::IsBookmarkedByUser(model, url);
-  bookmarks::AddIfNotBookmarked(model, url, title);
-  bool is_bookmarked_by_user = bookmarks::IsBookmarkedByUser(model, url);
-  // Make sure the model actually added a bookmark before showing the star. A
-  // bookmark isn't created if the url is invalid.
-  if (browser->window()->IsActive() && is_bookmarked_by_user) {
-    // Only show the bubble if the window is active, otherwise we may get into
-    // weird situations where the bubble is deleted as soon as it is shown.
-    browser->window()->ShowBookmarkBubble(url, was_bookmarked_by_user);
-  }
-}
-
 // Based on |disposition|, creates a new tab as necessary, and returns the
 // appropriate tab to navigate.  If that tab is the current tab, reverts the
 // location bar contents, since all browser-UI-triggered navigations should
@@ -744,7 +712,39 @@ void Exit() {
   chrome::AttemptUserExit();
 }
 
-void BookmarkCurrentPage(Browser* browser) {
+void BookmarkCurrentPageIgnoringExtensionOverrides(Browser* browser) {
+  content::RecordAction(UserMetricsAction("Star"));
+
+  BookmarkModel* model =
+      BookmarkModelFactory::GetForProfile(browser->profile());
+  if (!model || !model->loaded())
+    return;  // Ignore requests until bookmarks are loaded.
+
+  GURL url;
+  base::string16 title;
+  WebContents* web_contents =
+      browser->tab_strip_model()->GetActiveWebContents();
+  GetURLAndTitleToBookmark(web_contents, &url, &title);
+  bool is_bookmarked_by_any = model->IsBookmarked(url);
+  if (!is_bookmarked_by_any &&
+      web_contents->GetBrowserContext()->IsOffTheRecord()) {
+    // If we're incognito the favicon may not have been saved. Save it now
+    // so that bookmarks have an icon for the page.
+    FaviconTabHelper::FromWebContents(web_contents)->SaveFavicon();
+  }
+  bool was_bookmarked_by_user = bookmarks::IsBookmarkedByUser(model, url);
+  bookmarks::AddIfNotBookmarked(model, url, title);
+  bool is_bookmarked_by_user = bookmarks::IsBookmarkedByUser(model, url);
+  // Make sure the model actually added a bookmark before showing the star. A
+  // bookmark isn't created if the url is invalid.
+  if (browser->window()->IsActive() && is_bookmarked_by_user) {
+    // Only show the bubble if the window is active, otherwise we may get into
+    // weird situations where the bubble is deleted as soon as it is shown.
+    browser->window()->ShowBookmarkBubble(url, was_bookmarked_by_user);
+  }
+}
+
+void BookmarkCurrentPageAllowingExtensionOverrides(Browser* browser) {
   DCHECK(!chrome::ShouldRemoveBookmarkThisPageUI(browser->profile()));
 
 #if defined(ENABLE_EXTENSIONS)
@@ -770,8 +770,7 @@ void BookmarkCurrentPage(Browser* browser) {
     return;
   }
 #endif
-
-  BookmarkCurrentPageInternal(browser);
+  BookmarkCurrentPageIgnoringExtensionOverrides(browser);
 }
 
 bool CanBookmarkCurrentPage(const Browser* browser) {
diff --git a/chrome/browser/ui/browser_commands.h b/chrome/browser/ui/browser_commands.h
index b677e7d..d6df19d 100644
--- a/chrome/browser/ui/browser_commands.h
+++ b/chrome/browser/ui/browser_commands.h
@@ -94,7 +94,8 @@ content::WebContents* DuplicateTabAt(Browser* browser, int index);
 bool CanDuplicateTabAt(Browser* browser, int index);
 void ConvertPopupToTabbedBrowser(Browser* browser);
 void Exit();
-void BookmarkCurrentPage(Browser* browser);
+void BookmarkCurrentPageIgnoringExtensionOverrides(Browser* browser);
+void BookmarkCurrentPageAllowingExtensionOverrides(Browser* browser);
 bool CanBookmarkCurrentPage(const Browser* browser);
 void BookmarkAllTabs(Browser* browser);
 bool CanBookmarkAllTabs(const Browser* browser);
diff --git a/chrome/browser/ui/views/location_bar/bubble_icon_view.h b/chrome/browser/ui/views/location_bar/bubble_icon_view.h
index 76428243..504cf87 100644
--- a/chrome/browser/ui/views/location_bar/bubble_icon_view.h
+++ b/chrome/browser/ui/views/location_bar/bubble_icon_view.h
@@ -18,7 +18,7 @@ class BubbleIconView : public views::ImageView {
     EXECUTE_SOURCE_GESTURE,
   };
 
-  explicit BubbleIconView(CommandUpdater* command_updater, int command_id);
+  BubbleIconView(CommandUpdater* command_updater, int command_id);
   ~BubbleIconView() override;
 
   // Returns true if a related bubble is showing.
@@ -38,10 +38,11 @@ class BubbleIconView : public views::ImageView {
   // ui::EventHandler:
   void OnGestureEvent(ui::GestureEvent* event) override;
 
- private:
+ protected:
   // Calls OnExecuting and runs |command_id_| with a valid |command_updater_|.
-  void ExecuteCommand(ExecuteSource source);
+  virtual void ExecuteCommand(ExecuteSource source);
 
+ private:
   // The CommandUpdater for the Browser object that owns the location bar.
   CommandUpdater* command_updater_;
 
diff --git a/chrome/browser/ui/views/location_bar/location_bar_view.cc b/chrome/browser/ui/views/location_bar/location_bar_view.cc
index 1679bda..0bbc23b 100644
--- a/chrome/browser/ui/views/location_bar/location_bar_view.cc
+++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc
@@ -334,7 +334,7 @@ void LocationBarView::Init() {
   translate_icon_view_->SetVisible(false);
   AddChildView(translate_icon_view_);
 
-  star_view_ = new StarView(command_updater());
+  star_view_ = new StarView(command_updater(), browser_);
   star_view_->SetVisible(false);
   AddChildView(star_view_);
 
diff --git a/chrome/browser/ui/views/location_bar/star_view.cc b/chrome/browser/ui/views/location_bar/star_view.cc
index b5627a0..368119d 100644
--- a/chrome/browser/ui/views/location_bar/star_view.cc
+++ b/chrome/browser/ui/views/location_bar/star_view.cc
@@ -8,6 +8,8 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/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/grit/generated_resources.h"
@@ -15,8 +17,8 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 
-StarView::StarView(CommandUpdater* command_updater)
-    : BubbleIconView(command_updater, IDC_BOOKMARK_PAGE) {
+StarView::StarView(CommandUpdater* command_updater, Browser* browser)
+    : BubbleIconView(command_updater, IDC_BOOKMARK_PAGE), browser_(browser) {
   set_id(VIEW_ID_STAR_BUTTON);
   SetToggled(false);
 }
@@ -52,3 +54,12 @@ void StarView::OnExecuting(
                             entry_point,
                             BOOKMARK_ENTRY_POINT_LIMIT);
 }
+
+void StarView::ExecuteCommand(ExecuteSource source) {
+  if (browser_) {
+    OnExecuting(source);
+    chrome::BookmarkCurrentPageIgnoringExtensionOverrides(browser_);
+  } else {
+    BubbleIconView::ExecuteCommand(source);
+  }
+}
diff --git a/chrome/browser/ui/views/location_bar/star_view.h b/chrome/browser/ui/views/location_bar/star_view.h
index 58f4f2e..1b0e63c 100644
--- a/chrome/browser/ui/views/location_bar/star_view.h
+++ b/chrome/browser/ui/views/location_bar/star_view.h
@@ -7,12 +7,13 @@
 
 #include "chrome/browser/ui/views/location_bar/bubble_icon_view.h"
 
+class Browser;
 class CommandUpdater;
 
 // The star icon to show a bookmark bubble.
 class StarView : public BubbleIconView {
  public:
-  explicit StarView(CommandUpdater* command_updater);
+  StarView(CommandUpdater* command_updater, Browser* browser);
   ~StarView() override;
 
   // Toggles the star on or off.
@@ -22,8 +23,11 @@ class StarView : public BubbleIconView {
   // BubbleIconView:
   bool IsBubbleShowing() const override;
   void OnExecuting(BubbleIconView::ExecuteSource execute_source) override;
+  void ExecuteCommand(ExecuteSource source) override;
 
  private:
+  Browser* browser_;
+
   DISALLOW_COPY_AND_ASSIGN(StarView);
 };