Implement the inline extensions/apps install UI for Cocoa.

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

Index: chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm
===================================================================
--- chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm	(revision 99418)
+++ chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm	(working copy)
@@ -5,6 +5,7 @@
 #import "chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.h"
 
 #include "base/mac/mac_util.h"
+#include "base/memory/scoped_nsobject.h"
 #include "base/string_util.h"
 #include "base/sys_string_conversions.h"
 #include "base/utf_string_conversions.h"
@@ -18,8 +19,17 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/l10n/l10n_util_mac.h"
 
+@interface ExtensionInstallDialogController ()
+- (bool)isInlineInstall;
+- (void)appendRatingStar:(const SkBitmap*)skiaImage;
+@end
+
 namespace {
 
+// Padding above the warnings separator, we must also subtract this when hiding
+// it.
+const CGFloat kWarningsSeparatorPadding = 14;
+
 // Maximum height we will adjust controls to when trying to accomodate their
 // contents.
 const CGFloat kMaxControlHeight = 400;
@@ -45,17 +55,25 @@
   [control setFrameOrigin:origin];
 }
 
+void AppendRatingStarsShim(const SkBitmap* skiaImage,
+                           ExtensionInstallDialogController* controller) {
+  [controller appendRatingStar:skiaImage];
 }
 
+}
+
 @implementation ExtensionInstallDialogController
 
 @synthesize iconView = iconView_;
 @synthesize titleField = titleField_;
 @synthesize subtitleField = subtitleField_;
 @synthesize warningsField = warningsField_;
-@synthesize warningsBox= warningsBox_;
 @synthesize cancelButton = cancelButton_;
 @synthesize okButton = okButton_;
+@synthesize warningsSeparator = warningsSeparator_;
+@synthesize ratingStars = ratingStars_;
+@synthesize ratingCountField = ratingCountField_;
+@synthesize userCountField = userCountField_;
 
 - (id)initWithParentWindow:(NSWindow*)window
                    profile:(Profile*)profile
@@ -63,12 +81,18 @@
                   delegate:(ExtensionInstallUI::Delegate*)delegate
                       icon:(SkBitmap*)icon
                     prompt:(const ExtensionInstallUI::Prompt&)prompt {
+  prompt_.reset(new ExtensionInstallUI::Prompt(prompt));

[Nico, 2011/09/05 22:59:17]

You shouldn't write member variables before "self = [super init]" ran. I suggest
using a local scoped_ptr here and then transferring ownership to the member in
the "if (self=init)" block. (You will have to inline the isInlineInstall call)

[Mihai Parparita -not on Chrome, 2011/09/06 00:54:11]

Moved initialization of prompt_ to be after the [super init] call. I didn't see
much benefit to the scoped_ptr (I just used the prompt param directly). I didn't
want it to be a pointer in the first place (it's a cheap type, and the other UI
implementations just have it as a member directly), but you can't have members
that don't have default constructors in Objective C++.

   NSString* nibpath = nil;
 
-  // We use a different XIB in the case of no permission warnings, that is a
-  // little bit more nicely laid out.
-  if (prompt.GetPermissionCount() == 0) {
+  // We use a different XIB in the case of inline installs or no permission
+  // warnings, that respectively show webstore ratings data and are a more
+  // nicely laid out.
+  if ([self isInlineInstall]) {
     nibpath = [base::mac::MainAppBundle()
+               pathForResource:@"ExtensionInstallPromptInline"
+                        ofType:@"nib"];
+  } else if (prompt.GetPermissionCount() == 0) {
+    nibpath = [base::mac::MainAppBundle()
                pathForResource:@"ExtensionInstallPromptNoWarnings"
                         ofType:@"nib"];
   } else {
@@ -82,32 +106,7 @@
     profile_ = profile;
     icon_ = *icon;
     delegate_ = delegate;
-
-    title_.reset([base::SysUTF16ToNSString(
-        prompt.GetHeading(extension->name())) retain]);
-    subtitle_.reset([base::SysUTF16ToNSString(
-        prompt.GetPermissionsHeader()) retain]);
-    button_.reset([base::SysUTF16ToNSString(
-        prompt.GetAcceptButtonLabel()) retain]);
-    NSString* cancel_button_label = prompt.HasAbortButtonLabel() ?
-        base::SysUTF16ToNSString(prompt.GetAbortButtonLabel()) :
-        l10n_util::GetNSString(IDS_CANCEL);
-    cancel_button_.reset([cancel_button_label retain]);
-
-    // We display the permission warnings as a simple text string, separated by
-    // newlines.
-    if (prompt.GetPermissionCount()) {
-      string16 joined_warnings;
-      for (size_t i = 0; i < prompt.GetPermissionCount(); ++i) {
-        if (i > 0)
-          joined_warnings += UTF8ToUTF16("\n\n");
-
-        joined_warnings += prompt.GetPermission(i);
-      }
-
-      warnings_.reset(
-          [base::SysUTF16ToNSString(joined_warnings) retain]);
-    }
+    extension_ = extension;
   }
   return self;
 }
@@ -120,6 +119,16 @@
         contextInfo:nil];
 }
 
+- (IBAction)storeLinkClicked:(id)sender {
+  GURL store_url(
+      extension_urls::GetWebstoreItemDetailURLPrefix() + extension_->id());
+  BrowserList::GetLastActiveWithProfile(profile_)->
+      OpenURL(store_url, GURL(), NEW_FOREGROUND_TAB, PageTransition::LINK);
+
+  delegate_->InstallUIAbort(true);

[Nico, 2011/09/05 22:59:17]

nit: Make it more clear what "true" means here, either by
"InstallUIAbort(/*open_rick_astley_tab=*/true)" or by "bool alsoRickRoll = true;
InstallUIAbort(alsoRickRoll)"

[Mihai Parparita -not on Chrome, 2011/09/06 00:54:11]

Done.

+  [NSApp endSheet:[self window]];

[Nico, 2011/09/05 22:59:17]

Window-modal sheets are not cool. Have you thought about using a tab-modal
dialog for this? (there are cross-platform classes for that)

[Mihai Parparita -not on Chrome, 2011/09/06 00:54:11]

Tab-modal dialogs sound like a good idea for a follow-up CL, added a TODO at
line 310. ConstrainedWindow* are the helper classes for this, yes?

[Nico, 2011/09/06 01:04:00]

Yes. Thanks.  (Maybe file a bug and add the bug # to the TODO)

[Mihai Parparita -not on Chrome, 2011/09/06 01:25:59]

Filed http://crbug.com/95455

+}
+
 - (IBAction)cancel:(id)sender {
   delegate_->InstallUIAbort(true);
   [NSApp endSheet:[self window]];
@@ -131,15 +140,33 @@
 }
 
 - (void)awakeFromNib {
-  [titleField_ setStringValue:title_.get()];
-  [subtitleField_ setStringValue:subtitle_.get()];
-  [okButton_ setTitle:button_.get()];
-  [cancelButton_ setTitle:cancel_button_.get()];
+  // Make sure we're the window's delegate as set in the nib.
+  DCHECK_EQ(self, static_cast<ExtensionInstallDialogController*>(
+                      [[self window] delegate]));
 
+  // Set control labels

[Nico, 2011/09/05 22:59:17]

nit: trailing period.

[Mihai Parparita -not on Chrome, 2011/09/06 00:54:11]

Done.

+  [titleField_ setStringValue:base::SysUTF16ToNSString(
+      prompt_->GetHeading(extension_->name()))];
+  [okButton_ setTitle:base::SysUTF16ToNSString(
+      prompt_->GetAcceptButtonLabel())];
+  [cancelButton_ setTitle:prompt_->HasAbortButtonLabel() ?
+      base::SysUTF16ToNSString(prompt_->GetAbortButtonLabel()) :
+      l10n_util::GetNSString(IDS_CANCEL)];

[Nico, 2011/09/05 22:59:17]

Do you need to resize the controls to make them adapt to their new texts, or do
all the xibs use the autosizing localizer stuff?

[Mihai Parparita -not on Chrome, 2011/09/06 00:54:11]

It looks like there's no autosizing going on (e.g. the button labels get clipped
in Russian in the current canary build too), so I've added some
GTMUILocalizerAndLayoutTweaker sizeToFitView calls.

[Nico, 2011/09/06 01:04:00]

This is sometimes doable all in the xib file without any code:
http://code.google.com/p/google-toolbox-for-mac/wiki/UILocalization
http://www.chromium.org/developers/design-documents/ui-localization/mac-notes

Please give these two documents a look, but if the current patch works fine as
is, you don't need to change it.

[Mihai Parparita -not on Chrome, 2011/09/06 01:25:59]

Yeah, I looked into that, but since I set the button titles in awakeFromNib, I
wasn't sure if that would happen before GTMWidthBasedTweaker would be run by the
nib.
Yeah, the code as is works for Russian.

+  if ([self isInlineInstall]) {
+    prompt_->AppendRatingStars(
+        reinterpret_cast<ExtensionInstallUI::Prompt::StarAppender>(
+            AppendRatingStarsShim),

[Nico, 2011/09/05 22:59:17]

You need this cast because AppendRatingStarsShim doesn't take void* as 2nd
argument, right? The "more correct" way is to let AppendRatingStarsShim() take a
void* and reinterpret_cast that to ExtensionInstallDialogController in the
function. (The only thing that reinterpret_cast<>() does portably is casting a
pointer to a different to a different type and then back to the original type.
Buy you can use static_cast for casts from and to void* as well.
http://stackoverflow.com/questions/573294/when-to-use-reinterpret-cast )

[Mihai Parparita -not on Chrome, 2011/09/06 00:54:11]

Done.

+        self);
+    [ratingCountField_ setStringValue:base::SysUTF16ToNSString(
+        prompt_->GetRatingCount())];
+    [userCountField_ setStringValue:base::SysUTF16ToNSString(
+        prompt_->GetUserCount())];
+  }
+
   NSImage* image = gfx::SkBitmapToNSImage(icon_);
   [iconView_ setImage:image];
 
-  // Reisze |titleField_| to fit title
+  // Resize |titleField_| to fit title
   CGFloat originalTitleWidth = [titleField_ frame].size.width;
   [titleField_ sizeToFit];
   CGFloat newTitleWidth = [titleField_ frame].size.width;
@@ -149,48 +176,72 @@
     [[self window] setFrame:frame display:NO];
   }
 
-  // Make sure we're the window's delegate as set in the nib.
-  DCHECK_EQ(self, static_cast<ExtensionInstallDialogController*>(
-                      [[self window] delegate]));
-
+  CGFloat totalOffset = 0.0;
   // If there are any warnings, then we have to do some special layout.
-  if ([warnings_.get() length] > 0) {
-    [warningsField_ setStringValue:warnings_.get()];
+  if (prompt_->GetPermissionCount() > 0) {
+    [subtitleField_ setStringValue:base::SysUTF16ToNSString(
+        prompt_->GetPermissionsHeader())];
 
+    // We display the permission warnings as a simple text string, separated by
+    // newlines.
+    string16 joined_warnings;

[Nico, 2011/09/05 22:59:17]

nit: variables objc methods have camelCaps names (even if they have a c++ type)

[Mihai Parparita -not on Chrome, 2011/09/06 00:54:11]

Leftover from past version of the file, fixed.

+    for (size_t i = 0; i < prompt_->GetPermissionCount(); ++i) {
+      if (i > 0)
+        joined_warnings += UTF8ToUTF16("\n");

[Nico, 2011/09/05 22:59:17]

nit: You could go from utf8 to nsstring directly (you probably want a
NSMutableString in that case)

[Mihai Parparita -not on Chrome, 2011/09/06 00:54:11]

Done.

+
+      joined_warnings += prompt_->GetPermission(i);
+    }
+    [warningsField_ setStringValue:base::SysUTF16ToNSString(joined_warnings)];
+
     // The dialog is laid out in the NIB exactly how we want it assuming that
     // each label fits on one line. However, for each label, we want to allow
     // wrapping onto multiple lines. So we accumulate an offset by measuring how
-    // big each label wants to be, and comparing it to how bit it actually is.
+    // big each label wants to be, and comparing it to how big it actually is.
     // Then we shift each label down and resize by the appropriate amount, then
     // finally resize the window.
-    CGFloat totalOffset = 0.0;
 
-    // Text fields.
-    totalOffset += AdjustControlHeightToFitContent(titleField_);
-    OffsetControlVertically(titleField_, -totalOffset);
+    // Additionally, in the store version of the dialog the icon extends past
+    // the one-line version of the permission field. Therefore when increasing
+    // the window size for multi-line permissions we don't have to add the full
+    //

[Nico, 2011/09/05 22:59:17]

you accidentally the rest of the comment

[Nico, 2011/09/06 01:04:00]

still missing

[Mihai Parparita -not on Chrome, 2011/09/06 01:25:59]

Done.

+    CGFloat warningsGrowthSlack = 0;
+    if ([warningsField_ frame].origin.y > [iconView_ frame].origin.y) {
+      warningsGrowthSlack =
+          [warningsField_ frame].origin.y - [iconView_ frame].origin.y;
+    }
 
     totalOffset += AdjustControlHeightToFitContent(subtitleField_);
     OffsetControlVertically(subtitleField_, -totalOffset);
 
-    CGFloat warningsOffset = AdjustControlHeightToFitContent(warningsField_);
-    OffsetControlVertically(warningsField_, -warningsOffset);
-    totalOffset += warningsOffset;
+    totalOffset += AdjustControlHeightToFitContent(warningsField_);
+    OffsetControlVertically(warningsField_, -totalOffset);
+    totalOffset = MAX(totalOffset - warningsGrowthSlack, 0);
+  } else if ([self isInlineInstall]) {
+    // Inline installs that don't have a permissions section need to hide
+    // controls related to that and shrink the window by the space they take
+    // up.
+    NSRect hiddenRect = NSUnionRect([warningsSeparator_ frame],
+                                    [subtitleField_ frame]);
+    hiddenRect = NSUnionRect(hiddenRect, [warningsField_ frame]);
+    [warningsSeparator_ setHidden:YES];
+    [subtitleField_ setHidden:YES];
+    [warningsField_ setHidden:YES];
+    totalOffset -= hiddenRect.size.height + kWarningsSeparatorPadding;
+  }
 
-    NSRect warningsBoxRect = [warningsBox_ frame];
-    warningsBoxRect.origin.y -= totalOffset;
-    warningsBoxRect.size.height += warningsOffset;
-    [warningsBox_ setFrame:warningsBoxRect];
-
-    // buttons are positioned automatically in the XIB.
-
-    // Finally, adjust the window size.
+  // If necessary, adjust the window size.
+  if (totalOffset) {
     NSRect currentRect = [[self window] frame];
     [[self window] setFrame:NSMakeRect(currentRect.origin.x,
                                        currentRect.origin.y - totalOffset,
                                        currentRect.size.width,
                                        currentRect.size.height + totalOffset)
-                    display:NO];
+                   display:NO];
   }
+
+  // Focus the cancel button, otherwise the store link gets the default focus
+  // for inline installs.
+  [[self window] makeFirstResponder:cancelButton_];

[Nico, 2011/09/05 22:59:17]

I think you can set the first responder in the nib somehow (maybe by making the
key equivalent the return key)

[Mihai Parparita -not on Chrome, 2011/09/06 00:54:11]

I selected the window in Interface Builder and then connected its
initialFirstResponder to the cancel button (this was only necessary in
ExtensionInstallPromptInline).

 }
 
 - (void)didEndSheet:(NSWindow*)sheet
@@ -203,6 +254,28 @@
   [self autorelease];
 }
 
+- (bool)isInlineInstall {
+  return prompt_->type() == ExtensionInstallUI::INLINE_INSTALL_PROMPT;
+}
+
+- (void)appendRatingStar:(const SkBitmap*)skiaImage {
+  NSImage* image = gfx::SkBitmapToNSImageWithColorSpace(
+      *skiaImage, base::mac::GetSystemColorSpace());
+  NSRect frame = NSMakeRect(0, 0, skiaImage->width(), skiaImage->height());
+  NSImageView* view = [[[NSImageView alloc] initWithFrame:frame] autorelease];

[Nico, 2011/09/05 22:59:17]

nit: I think we generally prefer scoped_nsobject over autorelease in chrome.

[Mihai Parparita -not on Chrome, 2011/09/06 00:54:11]

Done.

+  [view setImage:image];
+
+  // Add this star after all the other ones
+  CGFloat maxStarRight = 0;
+  if ([[ratingStars_ subviews] count]) {
+    maxStarRight = NSMaxX([[[ratingStars_ subviews] lastObject] frame]);
+  }
+  NSRect starBounds = NSMakeRect(maxStarRight, 0,
+                                 skiaImage->width(), skiaImage->height());
+  [view setFrame:starBounds];
+  [ratingStars_ addSubview:view];
+}
+
 @end  // ExtensionInstallDialogController
 
 void ShowExtensionInstallDialog(