Implement the inline extensions/apps install UI for Cocoa.

Implement the inline extensions/apps install UI for Cocoa.

This required a new .xib (ExtensionInstallPromptInline) that shows store data
(star rating, number of reviews, number of users). Current .xibs
(ExtensionInstallPrompt and ExtensionInstallPromptNoWarnings) were rearranged
slightly to have the icon on the right and use a bulleted list for the list
of permissions (instead of a box) per Cole's mock.

This is the Cocoa side of r99407 (views) and r99205 (GTK).

BUG=93380
TEST=no
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99695

[Mihai Parparita -not on Chrome, 2011-09-05 22:39:12]

+Alcor in case this isn't turning out the way he expected.

Screenshots:
- Inline install, one permission: http://cl.ly/3X2F450J0X2e2u0v3o0D
- Store install, one permission: http://cl.ly/341Y0P3e0Y350T1i3r25
- Inline install, no permissions: http://cl.ly/451y1k443R0S2a0w0W3b
- Store install, no permissions: http://cl.ly/2J223S0w1X021r1C1i3y
- Inline install, many permissions: http://cl.ly/081u2j1T1v3O1k442B0z
- Store install, many permissions: http://cl.ly/201z3X2x3L3g0b210F2A

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

Looks pretty good already!

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.h
(right):

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.h:48: //
For unit test use only
nit: We usually put things that are only for unit tests into a "TestingAPI"
category (
http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=Test...
)

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm
(right):

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:84:
prompt_.reset(new ExtensionInstallUI::Prompt(prompt));
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)

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:128:
delegate_->InstallUIAbort(true);
nit: Make it more clear what "true" means here, either by
"InstallUIAbort(/*open_rick_astley_tab=*/true)" or by "bool alsoRickRoll = true;
InstallUIAbort(alsoRickRoll)"

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:129:
[NSApp endSheet:[self window]];
Window-modal sheets are not cool. Have you thought about using a tab-modal
dialog for this? (there are cross-platform classes for that)

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:147:
// Set control labels
nit: trailing period.

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:154:
l10n_util::GetNSString(IDS_CANCEL)];
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?

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:158:
AppendRatingStarsShim),
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 )

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:187:
string16 joined_warnings;
nit: variables objc methods have camelCaps names (even if they have a c++ type)

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:190:
joined_warnings += UTF8ToUTF16("\n");
nit: You could go from utf8 to nsstring directly (you probably want a
NSMutableString in that case)

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:206:
//
you accidentally the rest of the comment

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:244:
[[self window] makeFirstResponder:cancelButton_];
I think you can set the first responder in the nib somehow (maybe by making the
key equivalent the return key)

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:265:
NSImageView* view = [[[NSImageView alloc] initWithFrame:frame] autorelease];
nit: I think we generally prefer scoped_nsobject over autorelease in chrome.

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
File
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller_unittest.mm
(right):

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller_unittest.mm:294:
new MockExtensionInstallUIDelegate);
Can't you stack-allocate this?

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller_unittest.mm:340:
// just invisible
trailing .

[Nico, 2011-09-05 23:03:38]

Forgot to ask: Do you have 2x art for the star images etc? Have you checked this
looks ok in highres mode? (Launch QuartzDebug, enabled highdpi modes, log out,
log back in, switch to a highres resolution in sysprefs->displays. Requires a
lion box.)

Since highres isn't available by default, it's not high priority, but something
that needs to be done eventually.

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

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.h
(right):

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.h:48: //
For unit test use only
On 2011/09/05 22:59:17, Nico wrote:
> nit: We usually put things that are only for unit tests into a "TestingAPI"
> category (
>
http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=Test...
> )

As discussed in #chromium, this isn't doable here since you can't use categories
with synthesized properties.

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm
(right):

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:84:
prompt_.reset(new ExtensionInstallUI::Prompt(prompt));
On 2011/09/05 22:59:17, Nico wrote:
> 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)

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++.

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:128:
delegate_->InstallUIAbort(true);
On 2011/09/05 22:59:17, Nico wrote:
> nit: Make it more clear what "true" means here, either by
> "InstallUIAbort(/*open_rick_astley_tab=*/true)" or by "bool alsoRickRoll =
true;
> InstallUIAbort(alsoRickRoll)"

Done.

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:129:
[NSApp endSheet:[self window]];
On 2011/09/05 22:59:17, Nico wrote:
> Window-modal sheets are not cool. Have you thought about using a tab-modal
> dialog for this? (there are cross-platform classes for that)

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?

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:147:
// Set control labels
On 2011/09/05 22:59:17, Nico wrote:
> nit: trailing period.

Done.

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:154:
l10n_util::GetNSString(IDS_CANCEL)];
On 2011/09/05 22:59:17, Nico wrote:
> 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?

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.

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:158:
AppendRatingStarsShim),
On 2011/09/05 22:59:17, Nico wrote:
> 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 )

Done.

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:187:
string16 joined_warnings;
On 2011/09/05 22:59:17, Nico wrote:
> nit: variables objc methods have camelCaps names (even if they have a c++
type)

Leftover from past version of the file, fixed.

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:190:
joined_warnings += UTF8ToUTF16("\n");
On 2011/09/05 22:59:17, Nico wrote:
> nit: You could go from utf8 to nsstring directly (you probably want a
> NSMutableString in that case)

Done.

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:244:
[[self window] makeFirstResponder:cancelButton_];
On 2011/09/05 22:59:17, Nico wrote:
> I think you can set the first responder in the nib somehow (maybe by making
the
> key equivalent the return key)

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

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:265:
NSImageView* view = [[[NSImageView alloc] initWithFrame:frame] autorelease];
On 2011/09/05 22:59:17, Nico wrote:
> nit: I think we generally prefer scoped_nsobject over autorelease in chrome.

Done.

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
File
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller_unittest.mm
(right):

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller_unittest.mm:294:
new MockExtensionInstallUIDelegate);
On 2011/09/05 22:59:17, Nico wrote:
> Can't you stack-allocate this?

Was just following the pattern in the rest of the file, but yeah, you can.
Switched all the tests.

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller_unittest.mm:340:
// just invisible
On 2011/09/05 22:59:17, Nico wrote:
> trailing .

Done.

[Mihai Parparita -not on Chrome, 2011-09-06 00:55:12]

Don't have 2x art (Cole?) or access to a Lion box at the moment. Is there a
TODO I should add?

Mihai

On Mon, Sep 5, 2011 at 4:03 PM, <thakis@chromium.org> wrote:

> Forgot to ask: Do you have 2x art for the star images etc? Have you checked
> this
> looks ok in highres mode? (Launch QuartzDebug, enabled highdpi modes, log
> out,
> log back in, switch to a highres resolution in sysprefs->displays. Requires
> a
> lion box.)
>
> Since highres isn't available by default, it's not high priority, but
> something
> that needs to be done eventually.
>
>
>
http://codereview.chromium.**org/7835039/<http://codereview.chromium.org/7835...
>

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

LGTM with missing comment fixed.

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm
(right):

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:129:
[NSApp endSheet:[self window]];
On 2011/09/06 00:54:11, Mihai Parparita wrote:
> On 2011/09/05 22:59:17, Nico wrote:
> > Window-modal sheets are not cool. Have you thought about using a tab-modal
> > dialog for this? (there are cross-platform classes for that)
> 
> 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?

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

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:154:
l10n_util::GetNSString(IDS_CANCEL)];
On 2011/09/06 00:54:11, Mihai Parparita wrote:
> On 2011/09/05 22:59:17, Nico wrote:
> > 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?
> 
> 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.

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.

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:206:
//
On 2011/09/05 22:59:17, Nico wrote:
> you accidentally the rest of the comment

still missing

http://codereview.chromium.org/7835039/diff/15/chrome/browser/ui/cocoa/extens...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.h
(right):

http://codereview.chromium.org/7835039/diff/15/chrome/browser/ui/cocoa/extens...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.h:17:
#import "third_party/GTM/AppKit/GTMUILocalizerAndLayoutTweaker.h"
Looks unused? Move to mm

http://codereview.chromium.org/7835039/diff/15/chrome/browser/ui/cocoa/extens...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm
(right):

http://codereview.chromium.org/7835039/diff/15/chrome/browser/ui/cocoa/extens...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:157:
prompt_->AppendRatingStars(AppendRatingStarsShim, self);
The AppendRatingStarsShim() return value appears unused; consider changing the
typedef for the function to just void and changing all 3 platforms accordingly.

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

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm
(right):

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:129:
[NSApp endSheet:[self window]];
On 2011/09/06 01:04:00, Nico wrote:
> Yes. Thanks.  (Maybe file a bug and add the bug # to the TODO)

Filed http://crbug.com/95455

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:154:
l10n_util::GetNSString(IDS_CANCEL)];
On 2011/09/06 01:04:00, Nico wrote:
> This is sometimes doable all in the xib file without any code:

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.

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

Yeah, the code as is works for Russian.

http://codereview.chromium.org/7835039/diff/1/chrome/browser/ui/cocoa/extensi...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:206:
//
On 2011/09/06 01:04:00, Nico wrote:
> On 2011/09/05 22:59:17, Nico wrote:
> > you accidentally the rest of the comment
> 
> still missing

Done.

http://codereview.chromium.org/7835039/diff/15/chrome/browser/ui/cocoa/extens...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.h
(right):

http://codereview.chromium.org/7835039/diff/15/chrome/browser/ui/cocoa/extens...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.h:17:
#import "third_party/GTM/AppKit/GTMUILocalizerAndLayoutTweaker.h"
On 2011/09/06 01:04:00, Nico wrote:
> Looks unused? Move to mm

Done.

http://codereview.chromium.org/7835039/diff/15/chrome/browser/ui/cocoa/extens...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm
(right):

http://codereview.chromium.org/7835039/diff/15/chrome/browser/ui/cocoa/extens...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:157:
prompt_->AppendRatingStars(AppendRatingStarsShim, self);
On 2011/09/06 01:04:00, Nico wrote:
> The AppendRatingStarsShim() return value appears unused; consider changing the
> typedef for the function to just void and changing all 3 platforms
accordingly.

Done. Also removed the reinterpret_cast for the other platforms.

[Nico, 2011-09-06 01:30:17]

LGTM!

[alcor, 2011-09-06 17:30:43]

Looks good. Can we do it on white, though?

On Mon, Sep 5, 2011 at 6:30 PM, <thakis@chromium.org> wrote:

> LGTM!
>
>
http://codereview.chromium.**org/7835039/<http://codereview.chromium.org/7835...
>

[Mihai Parparita -not on Chrome, 2011-09-06 17:42:37]

Which part? The whole dialog or just the list of permissions?

Mihai

On Tue, Sep 6, 2011 at 10:29 AM, Nicholas Jitkoff <alcor@google.com> wrote:

> Looks good. Can we do it on white, though?
>
> On Mon, Sep 5, 2011 at 6:30 PM, <thakis@chromium.org> wrote:
>
>> LGTM!
>>
>>
http://codereview.chromium.**org/7835039/<http://codereview.chromium.org/7835...
>>
>
>

[alcor, 2011-09-06 17:48:58]

The whole dialog

On Tue, Sep 6, 2011 at 10:42 AM, Mihai Parparita <mihaip@chromium.org>wrote:

> Which part? The whole dialog or just the list of permissions?
>
> Mihai
>
>
> On Tue, Sep 6, 2011 at 10:29 AM, Nicholas Jitkoff <alcor@google.com>wrote:
>
>> Looks good. Can we do it on white, though?
>>
>> On Mon, Sep 5, 2011 at 6:30 PM, <thakis@chromium.org> wrote:
>>
>>> LGTM!
>>>
>>>
http://codereview.chromium.**org/7835039/<http://codereview.chromium.org/7835...
>>>
>>
>>
>

[Mihai Parparita -not on Chrome, 2011-09-06 17:56:20]

Does that also mean that it shouldn't be a sheet anymore (the current
background isn't actually gray, it's the regular sheet transparency)?

Mihai

On Tue, Sep 6, 2011 at 10:48 AM, Nicholas Jitkoff <alcor@google.com> wrote:

> The whole dialog
>
> On Tue, Sep 6, 2011 at 10:42 AM, Mihai Parparita <mihaip@chromium.org>wrote:
>
>> Which part? The whole dialog or just the list of permissions?
>>
>> Mihai
>>
>>
>> On Tue, Sep 6, 2011 at 10:29 AM, Nicholas Jitkoff <alcor@google.com>wrote:
>>
>>> Looks good. Can we do it on white, though?
>>>
>>> On Mon, Sep 5, 2011 at 6:30 PM, <thakis@chromium.org> wrote:
>>>
>>>> LGTM!
>>>>
>>>>
http://codereview.chromium.**org/7835039/<http://codereview.chromium.org/7835...
>>>>
>>>
>>>
>>
>

[alcor, 2011-09-06 18:03:23]

It should still be a sheet, but white. A lot of the web store assets (like
stars) look poor on gray.

On Tue, Sep 6, 2011 at 10:56 AM, Mihai Parparita <mihaip@chromium.org>wrote:

> Does that also mean that it shouldn't be a sheet anymore (the current
> background isn't actually gray, it's the regular sheet transparency)?
>
> Mihai
>
> On Tue, Sep 6, 2011 at 10:48 AM, Nicholas Jitkoff <alcor@google.com>wrote:
>
>> The whole dialog
>>
>> On Tue, Sep 6, 2011 at 10:42 AM, Mihai Parparita <mihaip@chromium.org>wrote:
>>
>>> Which part? The whole dialog or just the list of permissions?
>>>
>>> Mihai
>>>
>>>
>>> On Tue, Sep 6, 2011 at 10:29 AM, Nicholas Jitkoff <alcor@google.com>wrote:
>>>
>>>> Looks good. Can we do it on white, though?
>>>>
>>>> On Mon, Sep 5, 2011 at 6:30 PM, <thakis@chromium.org> wrote:
>>>>
>>>>> LGTM!
>>>>>
>>>>>
http://codereview.chromium.**org/7835039/<http://codereview.chromium.org/7835...
>>>>>
>>>>
>>>>
>>>
>>
>