[Extensions UI] Share more code in ExtensionInstalledBubble

Move implemention for getting the how-to-use description to
the platform-agnostic class.

BUG=458301
TBR=sky@chromium.org (OWNERS for ui/extensions; Finnur reviewed)

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

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

chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.h

@@ -7,8 +7,8 @@
 
 #import <Cocoa/Cocoa.h>
 
+#include "base/mac/scoped_nsobject.h"
 #include "base/memory/scoped_ptr.h"
-#import "chrome/browser/ui/cocoa/browser_window_controller.h"
 #import "chrome/browser/ui/cocoa/base_bubble_controller.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 
@@ -17,7 +17,6 @@ class ExtensionInstalledBubble;
 class ExtensionInstalledBubbleBridge;
 @class HyperlinkTextView;
 @class HoverCloseButton;
-@class InfoBubbleView;
 
 namespace extensions {
 class BundleInstaller;

chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm

@@ -5,16 +5,12 @@
 #import "chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.h"
 
 #include "base/i18n/rtl.h"
-#include "base/mac/bundle_locations.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
-#include "chrome/browser/chrome_notification_types.h"
-#include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/bundle_installer.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
-#include "chrome/browser/signin/signin_promo.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_window.h"
@@ -33,16 +29,13 @@
 #include "chrome/browser/ui/extensions/extension_installed_bubble.h"
 #include "chrome/browser/ui/singleton_tabs.h"
 #include "chrome/browser/ui/sync/sync_promo_ui.h"
-#include "chrome/common/extensions/api/commands/commands_handler.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "chrome/common/extensions/api/omnibox/omnibox_handler.h"
 #include "chrome/common/extensions/sync_helper.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_registrar.h"
-#include "content/public/browser/notification_source.h"
+#include "components/signin/core/browser/signin_metrics.h"
 #include "extensions/browser/install/extension_install_ui.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/feature_switch.h"
@@ -54,7 +47,6 @@
 using content::BrowserThread;
 using extensions::BundleInstaller;
 using extensions::Extension;
-using extensions::UnloadedExtensionInfo;
 
 class ExtensionInstalledBubbleBridge
     : public ExtensionInstalledBubble::Delegate {
@@ -328,65 +320,6 @@ bool ExtensionInstalledBubbleBridge::MaybeShowNow() {
   return window;
 }
 
-- (bool)hasActivePageAction:(extensions::Command*)command {
-  extensions::CommandService* command_service =
-      extensions::CommandService::Get(browser_->profile());
-  if (type_ == extension_installed_bubble::kPageAction) {
-    if (extensions::CommandsInfo::GetPageActionCommand([self extension]) &&
-        command_service->GetPageActionCommand(
-            [self extension]->id(),
-            extensions::CommandService::ACTIVE,
-            command,
-            NULL)) {
-      return true;
-    }
-  }
-
-  return false;
-}
-
-- (bool)hasActiveBrowserAction:(extensions::Command*)command {
-  extensions::CommandService* command_service =
-      extensions::CommandService::Get(browser_->profile());
-  if (type_ == extension_installed_bubble::kBrowserAction) {
-    if (extensions::CommandsInfo::GetBrowserActionCommand([self extension]) &&
-        command_service->GetBrowserActionCommand(
-            [self extension]->id(),
-            extensions::CommandService::ACTIVE,
-            command,
-            NULL)) {
-      return true;
-    }
-  }
-
-  return false;
-}
-
-- (NSString*)installMessageForCurrentExtensionAction {
-  if (type_ == extension_installed_bubble::kPageAction) {
-    extensions::Command page_action_command;
-    if ([self hasActivePageAction:&page_action_command]) {
-      return l10n_util::GetNSStringF(
-          IDS_EXTENSION_INSTALLED_PAGE_ACTION_INFO_WITH_SHORTCUT,
-          page_action_command.accelerator().GetShortcutText());
-    } else {
-      return l10n_util::GetNSString(
-          IDS_EXTENSION_INSTALLED_PAGE_ACTION_INFO);
-    }
-  } else {
-    CHECK_EQ(extension_installed_bubble::kBrowserAction, type_);
-    extensions::Command browser_action_command;
-    if ([self hasActiveBrowserAction:&browser_action_command]) {
-      return l10n_util::GetNSStringF(
-          IDS_EXTENSION_INSTALLED_BROWSER_ACTION_INFO_WITH_SHORTCUT,
-          browser_action_command.accelerator().GetShortcutText());
-    } else {
-      return l10n_util::GetNSString(
-          IDS_EXTENSION_INSTALLED_BROWSER_ACTION_INFO);
-    }
-  }
-}
-
 // Calculate the height of each install message, resizing messages in their
 // frames to fit window width.  Return the new window height, based on the
 // total of all message heights.
@@ -459,8 +392,8 @@ bool ExtensionInstalledBubbleBridge::MaybeShowNow() {
   // If type is browser/page action, include a special message about them.
   if (type_ == extension_installed_bubble::kBrowserAction ||
       type_ == extension_installed_bubble::kPageAction) {
-    [howToUse_ setStringValue:[self
-        installMessageForCurrentExtensionAction]];
+    [howToUse_ setStringValue:base::SysUTF16ToNSString(
+         installedBubble_->GetHowToUseDescription())];
     [howToUse_ setHidden:NO];
     [[howToUse_ cell]
         setFont:[NSFont systemFontOfSize:[NSFont smallSystemFontSize]]];
@@ -472,10 +405,8 @@ bool ExtensionInstalledBubbleBridge::MaybeShowNow() {
 
   // If type is omnibox keyword, include a special message about the keyword.
   if (type_ == extension_installed_bubble::kOmniboxKeyword) {
-    [howToUse_ setStringValue:l10n_util::GetNSStringF(
-        IDS_EXTENSION_INSTALLED_OMNIBOX_KEYWORD_INFO,
-        base::UTF8ToUTF16(extensions::OmniboxInfo::GetKeyword(
-            [self extension])))];
+    [howToUse_ setStringValue:base::SysUTF16ToNSString(
+         installedBubble_->GetHowToUseDescription())];
     [howToUse_ setHidden:NO];
     [[howToUse_ cell]
         setFont:[NSFont systemFontOfSize:[NSFont smallSystemFontSize]]];
@@ -510,9 +441,7 @@ bool ExtensionInstalledBubbleBridge::MaybeShowNow() {
     newWindowHeight += sync_promo_height;
   }
 
-  extensions::Command command;
-  if ([self hasActivePageAction:&command] ||
-      [self hasActiveBrowserAction:&command]) {
+  if (installedBubble_->has_command_keybinding()) {
     [manageShortcutLink_ setHidden:NO];
     [[manageShortcutLink_ cell]
         setFont:[NSFont systemFontOfSize:[NSFont smallSystemFontSize]]];
@@ -627,7 +556,6 @@ bool ExtensionInstalledBubbleBridge::MaybeShowNow() {
     [promo_.get() setFrame:frame];
   }
 
-  extensions::Command command;
   if (![manageShortcutLink_ isHidden]) {
     NSRect manageShortcutFrame = [manageShortcutLink_ frame];
     manageShortcutFrame.origin.y = NSMinY(frame) - (

chrome/browser/ui/extensions/extension_installed_bubble.cc

@@ -8,18 +8,22 @@
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "chrome/common/extensions/api/omnibox/omnibox_handler.h"
+#include "chrome/common/extensions/command.h"
+#include "chrome/grit/generated_resources.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
+#include "ui/base/l10n/l10n_util.h"
 
-using content::Details;
 using extensions::Extension;
 
 namespace {
@@ -29,6 +33,28 @@ const int kAnimationWaitMs = 50;
 // How often we retry when waiting for browser action animation to end.
 const int kAnimationWaitRetries = 10;
 
+// Returns the keybinding for an extension command, or a null if none exists.
+scoped_ptr<extensions::Command> GetCommand(
+    const std::string& extension_id,
+    Profile* profile,
+    ExtensionInstalledBubble::BubbleType type) {
+  scoped_ptr<extensions::Command> result;
+  extensions::Command command;
+  extensions::CommandService* command_service =
+      extensions::CommandService::Get(profile);
+  bool has_command = false;
+  if (type == ExtensionInstalledBubble::BROWSER_ACTION) {
+    has_command = command_service->GetBrowserActionCommand(
+        extension_id, extensions::CommandService::ACTIVE, &command, nullptr);
+  } else if (type == ExtensionInstalledBubble::PAGE_ACTION) {
+    has_command = command_service->GetPageActionCommand(
+        extension_id, extensions::CommandService::ACTIVE, &command, nullptr);
+  }
+  if (has_command)
+    result.reset(new extensions::Command(command));
+  return result.Pass();
+}
+
 } // namespace
 
 ExtensionInstalledBubble::ExtensionInstalledBubble(Delegate* delegate,
@@ -71,6 +97,36 @@ void ExtensionInstalledBubble::IgnoreBrowserClosing() {
                     content::Source<Browser>(browser_));
 }
 
+base::string16 ExtensionInstalledBubble::GetHowToUseDescription() const {
+  int message_id = 0;
+  base::string16 extra;
+  if (action_command_)
+    extra = action_command_->accelerator().GetShortcutText();
+
+  switch (type_) {
+    case BROWSER_ACTION:
+      message_id = extra.empty() ? IDS_EXTENSION_INSTALLED_BROWSER_ACTION_INFO :
+          IDS_EXTENSION_INSTALLED_BROWSER_ACTION_INFO_WITH_SHORTCUT;
+      break;
+    case PAGE_ACTION:
+      message_id = extra.empty() ? IDS_EXTENSION_INSTALLED_PAGE_ACTION_INFO :
+          IDS_EXTENSION_INSTALLED_PAGE_ACTION_INFO_WITH_SHORTCUT;
+      break;
+    case OMNIBOX_KEYWORD:
+      extra =
+          base::UTF8ToUTF16(extensions::OmniboxInfo::GetKeyword(extension_));
+      message_id = IDS_EXTENSION_INSTALLED_OMNIBOX_KEYWORD_INFO;
+      break;
+    case GENERIC:
+      break;
+  }
+
+  if (message_id == 0)
+    return base::string16();
+  return extra.empty() ? l10n_util::GetStringUTF16(message_id) :
+      l10n_util::GetStringFUTF16(message_id, extra);
+}
+
 void ExtensionInstalledBubble::ShowInternal() {
   if (delegate_->MaybeShowNow())
     return;
@@ -87,6 +143,9 @@ void ExtensionInstalledBubble::OnExtensionLoaded(
     content::BrowserContext* browser_context,
     const extensions::Extension* extension) {
   if (extension == extension_) {
+    // Parse the extension command, if one exists.
+    action_command_ = GetCommand(extension_->id(), browser_->profile(), type_);
+
     animation_wait_retries_ = 0;
     // PostTask to ourself to allow all EXTENSION_LOADED Observers to run.
     base::MessageLoopForUI::current()->PostTask(

chrome/browser/ui/extensions/extension_installed_bubble.h

@@ -15,6 +15,7 @@
 class Browser;
 
 namespace extensions {
+class Command;
 class Extension;
 class ExtensionRegistry;
 }
@@ -64,10 +65,14 @@ class ExtensionInstalledBubble : public content::NotificationObserver,
   const Browser* browser() const { return browser_; }
   const SkBitmap& icon() const { return icon_; }
   BubbleType type() const { return type_; }
+  bool has_command_keybinding() const { return action_command_; }
 
   // Stop listening to NOTIFICATION_BROWSER_CLOSING.
   void IgnoreBrowserClosing();
 
+  // Returns the string describing how to use the new extension.
+  base::string16 GetHowToUseDescription() const;
+
  private:
   // Delegates showing the view to our |view_|. Called internally via PostTask.
   void ShowInternal();
@@ -95,6 +100,9 @@ class ExtensionInstalledBubble : public content::NotificationObserver,
   BubbleType type_;
   content::NotificationRegistrar registrar_;
 
+  // The command to execute the extension action, if one exists.
+  scoped_ptr<extensions::Command> action_command_;
+
   // Listen to extension load, unloaded notifications.
   ScopedObserver<extensions::ExtensionRegistry,
                  extensions::ExtensionRegistryObserver>

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

@@ -9,8 +9,6 @@
 
 #include "base/i18n/rtl.h"
 #include "base/strings/utf_string_conversions.h"
-#include "chrome/browser/chrome_notification_types.h"
-#include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/profiles/profile.h"
@@ -28,7 +26,6 @@
 #include "chrome/browser/ui/views/toolbar/toolbar_action_view.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "chrome/browser/ui/views/toolbar/wrench_toolbar_button.h"
-#include "chrome/common/extensions/api/omnibox/omnibox_handler.h"
 #include "chrome/common/extensions/sync_helper.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/chromium_strings.h"
@@ -89,10 +86,8 @@ class InstalledBubbleContent : public views::View,
                                public views::ButtonListener,
                                public views::LinkListener {
  public:
-  InstalledBubbleContent(Browser* browser,
-                         const Extension* extension,
-                         ExtensionInstalledBubble::BubbleType type,
-                         const SkBitmap* icon);
+  InstalledBubbleContent(const ExtensionInstalledBubble& bubble,
+                         Browser* browser);
 
   // Overridden from views::ButtonListener.
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
@@ -109,9 +104,6 @@ class InstalledBubbleContent : public views::View,
     SIGN_IN_PROMO   = 1 << 3,
   };
 
-  bool GetKeybinding(extensions::Command* command);
-  base::string16 GetHowToUseDescription(const base::string16& key);
-
   // Layout the signin promo at coordinates |offset_x| and |offset_y|. Returns
   // the height (in pixels) of the promo UI.
   int LayoutSigninPromo(int offset_x, int offset_y);
@@ -124,9 +116,6 @@ class InstalledBubbleContent : public views::View,
   // The browser we're associated with.
   Browser* browser_;
 
-  // The id of the extension just installed.
-  const std::string extension_id_;
-
   // The string that contains the link text at the beginning of the sign-in
   // promo text.
   base::string16 signin_promo_link_text_;
@@ -138,9 +127,6 @@ class InstalledBubbleContent : public views::View,
   // whited out so the link can be drawn in its place.
   ScopedVector<gfx::RenderText> sign_in_promo_lines_;
 
-  // The type of the bubble to show (Browser Action, Omnibox keyword, etc).
-  ExtensionInstalledBubble::BubbleType type_;
-
   // A bitmask containing the various flavors of bubble sections to show.
   int flavors_;
 
@@ -159,13 +145,9 @@ class InstalledBubbleContent : public views::View,
 };
 
 InstalledBubbleContent::InstalledBubbleContent(
-    Browser* browser,
-    const Extension* extension,
-    ExtensionInstalledBubble::BubbleType type,
-    const SkBitmap* icon)
+    const ExtensionInstalledBubble& bubble,
+    Browser* browser)
     : browser_(browser),
-      extension_id_(extension->id()),
-      type_(type),
       flavors_(NONE),
       height_of_signin_promo_(0u),
       how_to_use_(NULL),
@@ -189,23 +171,18 @@ InstalledBubbleContent::InstalledBubbleContent(
   //     or a link to configure the keybinding shortcut (if one exists).
   // Extra info can include a promo for signing into sync.
 
-  // First figure out the keybinding situation.
-  extensions::Command command;
-  bool has_keybinding = GetKeybinding(&command);
-  base::string16 key;  // Keyboard shortcut or keyword to display in bubble.
-
+  const Extension* extension = bubble.extension();
   if (extensions::sync_helper::IsSyncableExtension(extension) &&
       SyncPromoUI::ShouldShowSyncPromo(browser->profile()))
     flavors_ |= SIGN_IN_PROMO;
 
   // Determine the bubble flavor we want, based on the extension type.
-  switch (type_) {
+  switch (bubble.type()) {
     case ExtensionInstalledBubble::BROWSER_ACTION:
-    case ExtensionInstalledBubble::PAGE_ACTION: {
+    case ExtensionInstalledBubble::PAGE_ACTION:
       flavors_ |= HOW_TO_USE;
-      if (has_keybinding) {
+      if (bubble.has_command_keybinding()) {
         flavors_ |= SHOW_KEYBINDING;
-        key = command.accelerator().GetShortcutText();
       } else {
         // The How-To-Use text makes the bubble seem a little crowded when the
         // extension has a keybinding, so the How-To-Manage text is not shown
@@ -213,36 +190,32 @@ InstalledBubbleContent::InstalledBubbleContent(
         flavors_ |= HOW_TO_MANAGE;
       }
       break;
-    }
-    case ExtensionInstalledBubble::OMNIBOX_KEYWORD: {
+    case ExtensionInstalledBubble::OMNIBOX_KEYWORD:
       flavors_ |= HOW_TO_USE | HOW_TO_MANAGE;
-      key = base::UTF8ToUTF16(extensions::OmniboxInfo::GetKeyword(extension));
       break;
-    }
-    case ExtensionInstalledBubble::GENERIC: {
+    case ExtensionInstalledBubble::GENERIC:
       break;
-    }
-    default: {
+    default:
       // When adding a new bubble type, the flavor needs to be set.
       static_assert(ExtensionInstalledBubble::GENERIC == 3,
           "kBubbleType enum has changed, this switch statement must "
           "be updateed");
       break;
-    }
   }
 
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   const gfx::FontList& font_list =
       rb.GetFontList(ui::ResourceBundle::BaseFont);
 
+  const SkBitmap& icon = bubble.icon();
   // Add the icon (for all flavors).
   // Scale down to 43x43, but allow smaller icons (don't scale up).
-  gfx::Size size(icon->width(), icon->height());
+  gfx::Size size(icon.width(), icon.height());
   if (size.width() > kIconSize || size.height() > kIconSize)
     size = gfx::Size(kIconSize, kIconSize);
   icon_ = new views::ImageView();
   icon_->SetImageSize(size);
-  icon_->SetImage(gfx::ImageSkia::CreateFrom1xBitmap(*icon));
+  icon_->SetImage(gfx::ImageSkia::CreateFrom1xBitmap(icon));
   AddChildView(icon_);
 
   // Add the heading (for all flavors).
@@ -256,7 +229,7 @@ InstalledBubbleContent::InstalledBubbleContent(
   AddChildView(heading_);
 
   if (flavors_ & HOW_TO_USE) {
-    how_to_use_ = new views::Label(GetHowToUseDescription(key));
+    how_to_use_ = new views::Label(bubble.GetHowToUseDescription());
     how_to_use_->SetFontList(font_list);
     how_to_use_->SetMultiLine(true);
     how_to_use_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
@@ -326,58 +299,6 @@ void InstalledBubbleContent::LinkClicked(views::Link* source, int event_flags) {
   chrome::Navigate(&params);
 }
 
-bool InstalledBubbleContent::GetKeybinding(extensions::Command* command) {
-  extensions::CommandService* command_service =
-      extensions::CommandService::Get(browser_->profile());
-  if (type_ == ExtensionInstalledBubble::BROWSER_ACTION) {
-    return command_service->GetBrowserActionCommand(
-        extension_id_,
-        extensions::CommandService::ACTIVE,
-        command,
-        NULL);
-  } else if (type_ == ExtensionInstalledBubble::PAGE_ACTION) {
-    return command_service->GetPageActionCommand(
-        extension_id_,
-        extensions::CommandService::ACTIVE,
-        command,
-        NULL);
-  } else {
-    return false;
-  }
-}
-
-base::string16 InstalledBubbleContent::GetHowToUseDescription(
-    const base::string16& key) {
-  switch (type_) {
-    case ExtensionInstalledBubble::BROWSER_ACTION:
-      if (!key.empty()) {
-        return l10n_util::GetStringFUTF16(
-            IDS_EXTENSION_INSTALLED_BROWSER_ACTION_INFO_WITH_SHORTCUT, key);
-      } else {
-        return l10n_util::GetStringUTF16(
-            IDS_EXTENSION_INSTALLED_BROWSER_ACTION_INFO);
-      }
-      break;
-    case ExtensionInstalledBubble::PAGE_ACTION:
-      if (!key.empty()) {
-        return l10n_util::GetStringFUTF16(
-            IDS_EXTENSION_INSTALLED_PAGE_ACTION_INFO_WITH_SHORTCUT, key);
-      } else {
-        return l10n_util::GetStringUTF16(
-            IDS_EXTENSION_INSTALLED_PAGE_ACTION_INFO);
-      }
-      break;
-    case ExtensionInstalledBubble::OMNIBOX_KEYWORD:
-      return l10n_util::GetStringFUTF16(
-          IDS_EXTENSION_INSTALLED_OMNIBOX_KEYWORD_INFO, key);
-      break;
-    default:
-      NOTREACHED();
-      break;
-  }
-  return base::string16();
-}
-
 int InstalledBubbleContent::LayoutSigninPromo(int offset_x, int offset_y) {
   sign_in_promo_lines_.clear();
   int height = 0;
@@ -589,9 +510,7 @@ bool ExtensionInstalledBubbleView::MaybeShowNow() {
             views::BubbleBorder::TOP_LEFT :
             views::BubbleBorder::TOP_RIGHT);
   SetLayoutManager(new views::FillLayout());
-  AddChildView(new InstalledBubbleContent(
-      bubble_.browser(), bubble_.extension(), bubble_.type(),
-      &bubble_.icon()));
+  AddChildView(new InstalledBubbleContent(bubble_, bubble_.browser()));
 
   views::BubbleDelegateView::CreateBubble(this)->Show();