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}

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());

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_);

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) {

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);

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_;
 

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_);
 

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

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