o Cleans up canonical extension_install_ui.cc to avoid #ifdefs when feasible....

o Cleans up canonical extension_install_ui.cc to avoid #ifdefs when feasible.
o Adds Cocoa implementation of the extension installation prompt.
o Added new cross-platform implementations of the extension install error prompt.
o Got rid of unused extension install strings from early implementations.
o Added a string to display as the header of the error dialog, since it was inline English.

Patch by Andrew Bonventre <andybons@gmail.com>;

BUG=19654
TEST=Install an extension on any platform, observe consistent visual behavior and messaging.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30091

[Andrew Bonventre, 2009-10-23 22:09:34]

Take a look. First attempt :).

[Andrew Bonventre, 2009-10-23 22:12:05]

http://codereview.chromium.org/333015/diff/1/3
File chrome/browser/extensions/extension_install_ui.cc (right):

http://codereview.chromium.org/333015/diff/1/3#newcode13
Line 13: #include "base/utf_string_conversions.h"
Will remove this in next upload...

[Mark Mentovai, 2009-10-23 22:52:22]

http://codereview.chromium.org/333015/diff/1/8
File chrome/browser/cocoa/extension_install_prompt.mm (right):

http://codereview.chromium.org/333015/diff/1/8#newcode5
Line 5: #include <string>
Style: C++ library headers after C system headers.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

http://codereview.chromium.org/333015/diff/1/8#newcode22
Line 22: Profile* profile, Delegate* delegate, Extension* extension, SkBitmap*
icon,
You're just going to dereference the icon with *icon when you pass it to
SkBitmapToNSImage.  Maybe you want to take it as a const SkBitmap& instead.

Actually, scratch that, it looks like the other platforms have good reason to
take this as a pointer.

http://codereview.chromium.org/333015/diff/1/8#newcode24
Line 24: NSAlert *alert = [[[NSAlert alloc] init] autorelease];
Here's another OCD:

You're free to choose whether you want the * on the type name or the variable
name, but you should stay consistent.  You do "NSAlert *alert" here, but a
couple lines above, it was "Profile* profile".

Some Chromium contributors try to bully you into preferring one style over the
other, but they're wrong.  Just be consistent with the code around you.  Since
this is your own file, pick what you like.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Pointe...

http://codereview.chromium.org/333015/diff/1/8#newcode48
Line 48: [alert runModal];
Check to make sure that this does what you want without any buttons.

http://codereview.chromium.org/333015/diff/1/3
File chrome/browser/extensions/extension_install_ui.cc (right):

http://codereview.chromium.org/333015/diff/1/3#newcode117
Line 117: ShowExtensionInstallPrompt(profile_, delegate, extension,
install_icon,
Much cleaner.

http://codereview.chromium.org/333015/diff/1/3#newcode160
Line 160: GetNewInfoBarDelegate(new_theme, tab_contents);
Looks like it'd all fit on one line.

http://codereview.chromium.org/333015/diff/1/2
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/333015/diff/1/2#newcode37
Line 37: // NOTE: The implementation of these functions are platform-specific.
grammar nit: implementationS

http://codereview.chromium.org/333015/diff/1/5
File chrome/browser/gtk/extension_install_prompt_gtk.cc (right):

http://codereview.chromium.org/333015/diff/1/5#newcode128
Line 128: GTK_MESSAGE_ERROR, GTK_BUTTONS_OK, "%s", error.c_str());
I don't know much about GTK, but since you set a title on this dialog in
Windows, I checked to see whether that would be possible with GTK.

Looks like it is, if you call gtk_window_set_title(GTK_WINDOW(dialog), "your
title").  At least, we do that in some other code.

It's probably worth getting it to match Windows if GTK dialogs, unlike Mac
NSAlerts, are expected to have titles.

[Aaron Boodman, 2009-10-24 03:00:34]

lgtm

http://codereview.chromium.org/333015/diff/1/2
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/333015/diff/1/2#newcode66
Line 66: void ShowThemeInfoBar(Extension* new_theme);
These two methods could use a short comment.

[Aaron Boodman, 2009-10-24 03:01:07]

Actually, for UI stuff, I'm going to start requesting screen caps. Starting now
:)

[Mark Mentovai, 2009-10-24 14:57:03]

http://codereview.chromium.org/333015/diff/1/8
File chrome/browser/cocoa/extension_install_prompt.mm (right):

http://codereview.chromium.org/333015/diff/1/8#newcode48
Line 48: [alert runModal];
This should give you an OK button, but we shouldn't use it.  Here's why:

If we take the standard OK button on an NSAlert, the string will come from the
OS.  The OS strings have only been translated into a handful of languages. 
Chrome has been translated into an assload of languages.

If someone runs Chrome in a language that's part of the (assload - handful)
group, their UI will be localized, but they'll fall back to English (or
something else) for this.

For that reason, we never rely on the OS strings.  We should add:

[alert addButtonWithTitle:l10n_util::GetNSString(IDS_OK)];

I'm assuming that OK is the button title we want here.

[Andrew Bonventre, 2009-10-25 05:45:15]

Screenshots:
Old: http://andybons.com/chrome/old_extension_install.png
New: http://andybons.com/chrome/new_extension_install.png

Is there a canonical method for associating arbitrary files to CLs?

http://codereview.chromium.org/333015/diff/1/8
File chrome/browser/cocoa/extension_install_prompt.mm (right):

http://codereview.chromium.org/333015/diff/1/8#newcode5
Line 5: #include <string>
On 2009/10/23 22:52:22, Mark Mentovai wrote:
> Style: C++ library headers after C system headers.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

Done.

http://codereview.chromium.org/333015/diff/1/8#newcode24
Line 24: NSAlert *alert = [[[NSAlert alloc] init] autorelease];
On 2009/10/23 22:52:22, Mark Mentovai wrote:
> Here's another OCD:
> 
> You're free to choose whether you want the * on the type name or the variable
> name, but you should stay consistent.  You do "NSAlert *alert" here, but a
> couple lines above, it was "Profile* profile".
> 
> Some Chromium contributors try to bully you into preferring one style over the
> other, but they're wrong.  Just be consistent with the code around you.  Since
> this is your own file, pick what you like.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Pointe...

Done.

http://codereview.chromium.org/333015/diff/1/8#newcode48
Line 48: [alert runModal];
On 2009/10/24 14:57:03, Mark Mentovai wrote:
> This should give you an OK button, but we shouldn't use it.  Here's why:
> 
> If we take the standard OK button on an NSAlert, the string will come from the
> OS.  The OS strings have only been translated into a handful of languages. 
> Chrome has been translated into an assload of languages.
> 
> If someone runs Chrome in a language that's part of the (assload - handful)
> group, their UI will be localized, but they'll fall back to English (or
> something else) for this.
> 
> For that reason, we never rely on the OS strings.  We should add:
> 
> [alert addButtonWithTitle:l10n_util::GetNSString(IDS_OK)];
> 
> I'm assuming that OK is the button title we want here.

Done.

http://codereview.chromium.org/333015/diff/1/3
File chrome/browser/extensions/extension_install_ui.cc (right):

http://codereview.chromium.org/333015/diff/1/3#newcode13
Line 13: #include "base/utf_string_conversions.h"
On 2009/10/23 22:12:05, Andrew Bonventre wrote:
> Will remove this in next upload...

Actually, functions are used here that are defined in this header.

http://codereview.chromium.org/333015/diff/1/3#newcode160
Line 160: GetNewInfoBarDelegate(new_theme, tab_contents);
On 2009/10/23 22:52:22, Mark Mentovai wrote:
> Looks like it'd all fit on one line.

Nope, but made it more consistent with style below.

http://codereview.chromium.org/333015/diff/1/2
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/333015/diff/1/2#newcode37
Line 37: // NOTE: The implementation of these functions are platform-specific.
On 2009/10/23 22:52:22, Mark Mentovai wrote:
> grammar nit: implementationS

Done.

http://codereview.chromium.org/333015/diff/1/2#newcode66
Line 66: void ShowThemeInfoBar(Extension* new_theme);
On 2009/10/24 03:00:35, Aaron Boodman wrote:
> These two methods could use a short comment.

Done.

http://codereview.chromium.org/333015/diff/1/5
File chrome/browser/gtk/extension_install_prompt_gtk.cc (right):

http://codereview.chromium.org/333015/diff/1/5#newcode128
Line 128: GTK_MESSAGE_ERROR, GTK_BUTTONS_OK, "%s", error.c_str());
On 2009/10/23 22:52:22, Mark Mentovai wrote:
> I don't know much about GTK, but since you set a title on this dialog in
> Windows, I checked to see whether that would be possible with GTK.
> 
> Looks like it is, if you call gtk_window_set_title(GTK_WINDOW(dialog), "your
> title").  At least, we do that in some other code.
> 
> It's probably worth getting it to match Windows if GTK dialogs, unlike Mac
> NSAlerts, are expected to have titles.

Done.

[Mark Mentovai, 2009-10-25 19:34:50]

LGTM

http://codereview.chromium.org/333015/diff/5001/5002
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/333015/diff/5001/5002#newcode69
Line 69: // Returns the delegate to control the browser's info bar. This is
within its
Blank line before this so that it doesn't look like it runs together with the
above.

http://codereview.chromium.org/333015/diff/5001/5002#newcode72
Line 72: TabContents* tab_contents);
And blank line below to keep it from the data members for the same reason.

[Andrew Bonventre, 2009-10-26 16:02:41]

http://codereview.chromium.org/333015/diff/5001/5002
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/333015/diff/5001/5002#newcode69
Line 69: // Returns the delegate to control the browser's info bar. This is
within its
On 2009/10/25 19:34:50, Mark Mentovai wrote:
> Blank line before this so that it doesn't look like it runs together with the
> above.

Done.

http://codereview.chromium.org/333015/diff/5001/5002#newcode72
Line 72: TabContents* tab_contents);
On 2009/10/25 19:34:50, Mark Mentovai wrote:
> And blank line below to keep it from the data members for the same reason.

Done.

[Avi (use Gerrit), 2009-10-26 16:17:19]

Looks good.

http://codereview.chromium.org/333015/diff/1005/1006
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/333015/diff/1005/1006#newcode68
Line 68: 
no blank line between comment and declaration

http://codereview.chromium.org/333015/diff/1005/1008
File chrome/browser/views/extensions/extension_install_prompt.cc (right):

http://codereview.chromium.org/333015/diff/1005/1008#newcode164
Line 164: #if defined(OS_WIN)
Can we reuse the "InstallDialogContent" scheme as used above?

[Andrew Bonventre, 2009-10-26 19:15:27]

http://codereview.chromium.org/333015/diff/1005/1006
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/333015/diff/1005/1006#newcode68
Line 68: 
On 2009/10/26 16:17:19, Avi wrote:
> no blank line between comment and declaration

Done.

[Mark Mentovai, 2009-10-26 21:13:30]

r30091 committed