Port the Extension Install Bubble changes to Mac

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

 // Copyright (c) 2012 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.
 
 #import "chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.h"
 
 #include "base/i18n/rtl.h"
 #include "base/mac/bundle_locations.h"
 #include "base/mac/mac_util.h"
 #include "base/sys_string_conversions.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/extensions/api/commands/command_service.h"
+#include "chrome/browser/extensions/api/commands/command_service_factory.h"
 #include "chrome/browser/extensions/bundle_installer.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/cocoa/browser_window_cocoa.h"
 #include "chrome/browser/ui/cocoa/browser_window_controller.h"
 #include "chrome/browser/ui/cocoa/extensions/browser_actions_controller.h"
 #include "chrome/browser/ui/cocoa/hover_close_button.h"
 #include "chrome/browser/ui/cocoa/info_bubble_view.h"
 #include "chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h"
 #include "chrome/browser/ui/cocoa/toolbar/toolbar_controller.h"
@@ skipping 81 matching lines @@
     icon_.reset([gfx::SkBitmapToNSImage(icon) retain]);
     pageActionRemoved_ = NO;
 
     if (bundle_) {
       type_ = extension_installed_bubble::kBundle;
     } else if (!extension->omnibox_keyword().empty()) {
       type_ = extension_installed_bubble::kOmniboxKeyword;
     } else if (extension->browser_action()) {
       type_ = extension_installed_bubble::kBrowserAction;
     } else if (extension->page_action() &&
-               !extension->page_action()->default_icon_path().empty()) {
+               (!extension->page_action()->default_icon_path().empty() ||
+                extension->page_action_command())) {
       type_ = extension_installed_bubble::kPageAction;
     } else {
       NOTREACHED();  // kGeneric installs handled in extension_install_prompt.
     }

[Nico, 2012/08/23 15:42:12]

Same here: Is this if() the same in all UI implementations?

 
     if (type_ == extension_installed_bubble::kBundle) {
       [self showWindow:self];
     } else {
       // Start showing window only after extension has fully loaded.
       extensionObserver_.reset(new ExtensionLoadedNotificationObserver(
           self, browser->profile()));
     }
   }
   return self;
@@ skipping 160 matching lines @@
     base::i18n::AdjustStringForLocaleDirection(&extension_name);
     [extensionInstalledMsg_ setStringValue:l10n_util::GetNSStringF(
         IDS_EXTENSION_INSTALLED_HEADING, extension_name)];
     [GTMUILocalizerAndLayoutTweaker
       sizeToFitFixedWidthTextField:extensionInstalledMsg_];
     newWindowHeight += [extensionInstalledMsg_ frame].size.height +
         extension_installed_bubble::kInnerVerticalMargin;
   }
 
   // If type is page action, include a special message about page actions.
-  if (type_ == extension_installed_bubble::kPageAction) {
+  if (type_ == extension_installed_bubble::kBrowserAction ||
+      type_ == extension_installed_bubble::kPageAction) {
+    extensions::CommandService* command_service =
+        extensions::CommandServiceFactory::GetForProfile(browser_->profile());
+    if (type_ == extension_installed_bubble::kPageAction) {
+      extensions::Command page_action_command;
+      if (extension_->page_action_command() &&
+          command_service->GetPageActionCommand(
+              extension_->id(),
+              extensions::CommandService::ACTIVE_ONLY,
+              &page_action_command,
+              NULL)) {
+        [extraInfoMsg_ setStringValue:l10n_util::GetNSStringF(
+             IDS_EXTENSION_INSTALLED_PAGE_ACTION_INFO_WITH_SHORTCUT,
+             page_action_command.accelerator().GetShortcutText())];
+      }
+    } else {
+      extensions::Command browser_action_command;
+      if (extension_->browser_action_command() &&
+          command_service->GetBrowserActionCommand(
+              extension_->id(),
+              extensions::CommandService::ACTIVE_ONLY,
+              &browser_action_command,
+              NULL)) {
+        [extraInfoMsg_ setStringValue:l10n_util::GetNSStringF(
+            IDS_EXTENSION_INSTALLED_BROWSER_ACTION_INFO_WITH_SHORTCUT,
+            browser_action_command.accelerator().GetShortcutText())];
+      } else {
+        [extraInfoMsg_ setStringValue:l10n_util::GetNSString(
+            IDS_EXTENSION_INSTALLED_BROWSER_ACTION_INFO)];
+      }
+    }
     [extraInfoMsg_ setHidden:NO];
     [[extraInfoMsg_ cell]
         setFont:[NSFont systemFontOfSize:[NSFont smallSystemFontSize]]];
     [GTMUILocalizerAndLayoutTweaker
         sizeToFitFixedWidthTextField:extraInfoMsg_];
     newWindowHeight += [extraInfoMsg_ frame].size.height +
         extension_installed_bubble::kInnerVerticalMargin;
   }

[Nico, 2012/08/23 15:42:12]

This block is now long and nested. Please extract parts of it into new functions
(e.g. the whole block, or maybe you find a way to share some code between the
similar looking two branches. But I'm asking mostly to keep
)calculateWindowHeight readable, not because I'm worried about code reuse)

[Finnur, 2012/08/24 12:55:26]

Done.

On 2012/08/23 15:42:12, Nico wrote:

 
   // If type is omnibox keyword, include a special message about the keyword.
   if (type_ == extension_installed_bubble::kOmniboxKeyword) {
     [extraInfoMsg_ setStringValue:l10n_util::GetNSStringF(
         IDS_EXTENSION_INSTALLED_OMNIBOX_KEYWORD_INFO,
         UTF8ToUTF16(extension_->omnibox_keyword()))];
     [extraInfoMsg_ setHidden:NO];
     [[extraInfoMsg_ cell]
         setFont:[NSFont systemFontOfSize:[NSFont smallSystemFontSize]]];
     [GTMUILocalizerAndLayoutTweaker
@@ skipping 96 matching lines @@
     return;
   }
 
   NSRect extensionMessageFrame1 = [extensionInstalledMsg_ frame];
   NSRect extensionMessageFrame2 = [extensionInstalledInfoMsg_ frame];
 
   extensionMessageFrame1.origin.y = newWindowHeight - (
       extensionMessageFrame1.size.height +
       extension_installed_bubble::kOuterVerticalMargin);
   [extensionInstalledMsg_ setFrame:extensionMessageFrame1];
-  if (type_ == extension_installed_bubble::kPageAction ||
+  if (type_ == extension_installed_bubble::kBrowserAction ||
+      type_ == extension_installed_bubble::kPageAction ||
       type_ == extension_installed_bubble::kOmniboxKeyword) {

[Nico, 2012/08/23 15:42:12]

This again looks like something that should be shared across ports.

     // The extra message is only shown when appropriate.
     NSRect extraMessageFrame = [extraInfoMsg_ frame];
     extraMessageFrame.origin.y = extensionMessageFrame1.origin.y - (
         extraMessageFrame.size.height +
         extension_installed_bubble::kInnerVerticalMargin);
     [extraInfoMsg_ setFrame:extraMessageFrame];
     extensionMessageFrame2.origin.y = extraMessageFrame.origin.y - (
         extensionMessageFrame2.size.height +
         extension_installed_bubble::kInnerVerticalMargin);
   } else {
@@ skipping 15 matching lines @@
 
 - (NSRect)getExtensionInstalledInfoMsgFrame {
   return [extensionInstalledInfoMsg_ frame];
 }
 
 - (void)extensionUnloaded:(id)sender {
   extension_ = NULL;
 }
 
 @end