Password manager: Implement password generation UI for Mac.

Password manager: Implement password generation UI for Mac.

BUG=114092
R=estade@chromium.org, isherman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270648

[Patrick Dubroy, 2014-05-06 15:43:51]

Hi Ilya (and gcasto FYI),

PTAL for early feedback. This CL is not *nearly* complete, but I'd like to get
some quick feedback about whether I'm heading in the right direction. In
particular, what do you think about the idea of introducing a new class called
AutofillPopupBaseViewCocoa, that is the superclass of both the autofill popup
and the password generation popup? This is how Garrett implemented it in Views.

https://codereview.chromium.org/267183002/diff/1/chrome/browser/ui/cocoa/auto...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right):

https://codereview.chromium.org/267183002/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:55:
@implementation AutofillPopupBaseViewCocoa
I will move this into a separate file -- it's just temporarily in this file for
ease of cut and pasting as I figure out the boundaries between this class and
the subclasses.

[Patrick Dubroy, 2014-05-06 15:44:53]

(Let's try this again)

Hi Ilya (and gcasto FYI),

PTAL for early feedback. This CL is not *nearly* complete, but I'd like to get
some quick feedback about whether I'm heading in the right direction. In
particular, what do you think about the idea of introducing a new class called
AutofillPopupBaseViewCocoa, that is the superclass of both the autofill popup
and the password generation popup? This is how Garrett implemented it in Views.

https://codereview.chromium.org/267183002/diff/1/chrome/browser/ui/cocoa/auto...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right):

https://codereview.chromium.org/267183002/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:55:
@implementation AutofillPopupBaseViewCocoa
I will move this into a separate file -- it's just temporarily in this file for
ease of cut and pasting as I figure out the boundaries between this class and
the subclasses.

[Garrett Casto, 2014-05-07 00:22:33]

FWIW, organization seems reasonable to me.

[Ilya Sherman, 2014-05-07 00:34:43]

Organization seems reasonable to me too.  In fact, I'd encourage even one more
shared base class :)

https://codereview.chromium.org/267183002/diff/1/chrome/browser/ui/cocoa/auto...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm
(right):

https://codereview.chromium.org/267183002/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:69: }
There's a whole bunch of duplicated code in this file.  Are you planning to
share most of it?

[Patrick Dubroy, 2014-05-07 16:48:36]

https://codereview.chromium.org/267183002/diff/1/chrome/browser/ui/cocoa/auto...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm
(right):

https://codereview.chromium.org/267183002/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:69: }
On 2014/05/07 00:34:44, Ilya Sherman wrote:
> There's a whole bunch of duplicated code in this file.  Are you planning to
> share most of it?

I've taken a crack at it in the latest patchset. Basically, I've pushed all the
shared functionality into the common view class. I decided against creating a
shared superclass for the bridges, because it feels like there is already enough
inheritance going on amongst all these classes that I didn't want to make it
more confusing. What do you think?

I think the contents of showPopup and hidePopup don't *really* belong in the
view, but if I were to put them in another class, I'd be tempted to put it in a
class that I could use through composition, e.g. AutofillPopupControllerCocoa or
something. Any thoughts?

[Ilya Sherman, 2014-05-08 00:34:32]

https://codereview.chromium.org/267183002/diff/1/chrome/browser/ui/cocoa/auto...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm
(right):

https://codereview.chromium.org/267183002/diff/1/chrome/browser/ui/cocoa/auto...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:69: }
On 2014/05/07 16:48:36, Patrick Dubroy wrote:
> On 2014/05/07 00:34:44, Ilya Sherman wrote:
> > There's a whole bunch of duplicated code in this file.  Are you planning to
> > share most of it?
> 
> I've taken a crack at it in the latest patchset. Basically, I've pushed all
the
> shared functionality into the common view class. I decided against creating a
> shared superclass for the bridges, because it feels like there is already
enough
> inheritance going on amongst all these classes that I didn't want to make it
> more confusing. What do you think?
> 
> I think the contents of showPopup and hidePopup don't *really* belong in the
> view, but if I were to put them in another class, I'd be tempted to put it in
a
> class that I could use through composition, e.g. AutofillPopupControllerCocoa
or
> something. Any thoughts?

I'm fine with either composition or inheritance.  Your current approach seems
reasonable.  Thanks!

[Patrick Dubroy, 2014-05-08 16:38:16]

PTAL. The implementation is now working.

[Patrick Dubroy, 2014-05-08 16:38:51]

+estade for changes in password_generation_popup_view_views.cc

[Evan Stade, 2014-05-08 17:50:28]

https://codereview.chromium.org/267183002/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_view.h (right):

https://codereview.chromium.org/267183002/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_view.h:40: static SkColor
GetDividerColor();
why are these functions instead of just constants?

[Patrick Dubroy, 2014-05-08 20:37:29]

https://codereview.chromium.org/267183002/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_view.h (right):

https://codereview.chromium.org/267183002/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_view.h:40: static SkColor
GetDividerColor();
On 2014/05/08 17:50:29, Evan Stade wrote:
> why are these functions instead of just constants?

Because they require static initializers, and AFAIK we are supposed to avoid
those in Chromium
(https://groups.google.com/a/chromium.org/d/msg/chromium-dev/p6h3HC8Wro4/o37Lb...).

[Evan Stade, 2014-05-08 20:52:41]

https://codereview.chromium.org/267183002/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_view.h (right):

https://codereview.chromium.org/267183002/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_view.h:40: static SkColor
GetDividerColor();
On 2014/05/08 20:37:30, Patrick Dubroy wrote:
> On 2014/05/08 17:50:29, Evan Stade wrote:
> > why are these functions instead of just constants?
> 
> Because they require static initializers, and AFAIK we are supposed to avoid
> those in Chromium
>
(https://groups.google.com/a/chromium.org/d/msg/chromium-dev/p6h3HC8Wro4/o37Lb...).

SkColor is POD, so static initializers are OK.

[Patrick Dubroy, 2014-05-08 21:11:47]

On 2014/05/08 20:52:41, Evan Stade wrote:
>
https://codereview.chromium.org/267183002/diff/40001/chrome/browser/ui/autofi...
> File chrome/browser/ui/autofill/password_generation_popup_view.h (right):
> 
>
https://codereview.chromium.org/267183002/diff/40001/chrome/browser/ui/autofi...
> chrome/browser/ui/autofill/password_generation_popup_view.h:40: static SkColor
> GetDividerColor();
> On 2014/05/08 20:37:30, Patrick Dubroy wrote:
> > On 2014/05/08 17:50:29, Evan Stade wrote:
> > > why are these functions instead of just constants?
> > 
> > Because they require static initializers, and AFAIK we are supposed to avoid
> > those in Chromium
> >
>
(https://groups.google.com/a/chromium.org/d/msg/chromium-dev/p6h3HC8Wro4/o37Lb...).
> 
> SkColor is POD, so static initializers are OK.

Looks like a function call:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...

Or am I missing something?

[Evan Stade, 2014-05-08 22:18:39]

On 2014/05/08 21:11:47, Patrick Dubroy wrote:
> On 2014/05/08 20:52:41, Evan Stade wrote:
> >
>
https://codereview.chromium.org/267183002/diff/40001/chrome/browser/ui/autofi...
> > File chrome/browser/ui/autofill/password_generation_popup_view.h (right):
> > 
> >
>
https://codereview.chromium.org/267183002/diff/40001/chrome/browser/ui/autofi...
> > chrome/browser/ui/autofill/password_generation_popup_view.h:40: static
SkColor
> > GetDividerColor();
> > On 2014/05/08 20:37:30, Patrick Dubroy wrote:
> > > On 2014/05/08 17:50:29, Evan Stade wrote:
> > > > why are these functions instead of just constants?
> > > 
> > > Because they require static initializers, and AFAIK we are supposed to
avoid
> > > those in Chromium
> > >
> >
>
(https://groups.google.com/a/chromium.org/d/msg/chromium-dev/p6h3HC8Wro4/o37Lb...).
> > 
> > SkColor is POD, so static initializers are OK.
> 
> Looks like a function call:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...
> 
> Or am I missing something?

looks like a macro to me (in release mode). But the problem with static init of
non-PODs is that the construction/destruction order is unpredictable. PODs don't
have constructors or destructors so they're ok. See
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/color_con...
for prior art.

[Patrick Dubroy, 2014-05-09 12:30:42]

On 2014/05/08 22:18:39, Evan Stade wrote:
> On 2014/05/08 21:11:47, Patrick Dubroy wrote:
> > On 2014/05/08 20:52:41, Evan Stade wrote:
> > >
> >
>
https://codereview.chromium.org/267183002/diff/40001/chrome/browser/ui/autofi...
> > > File chrome/browser/ui/autofill/password_generation_popup_view.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/267183002/diff/40001/chrome/browser/ui/autofi...
> > > chrome/browser/ui/autofill/password_generation_popup_view.h:40: static
> SkColor
> > > GetDividerColor();
> > > On 2014/05/08 20:37:30, Patrick Dubroy wrote:
> > > > On 2014/05/08 17:50:29, Evan Stade wrote:
> > > > > why are these functions instead of just constants?
> > > > 
> > > > Because they require static initializers, and AFAIK we are supposed to
> avoid
> > > > those in Chromium
> > > >
> > >
> >
>
(https://groups.google.com/a/chromium.org/d/msg/chromium-dev/p6h3HC8Wro4/o37Lb...).
> > > 
> > > SkColor is POD, so static initializers are OK.
> > 
> > Looks like a function call:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...
> > 
> > Or am I missing something?
> 
> looks like a macro to me (in release mode). But the problem with static init
of
> non-PODs is that the construction/destruction order is unpredictable. PODs
don't
> have constructors or destructors so they're ok. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/color_con...
> for prior art.

Ok, I changed it back to a constant.

[Patrick Dubroy, 2014-05-09 12:44:26]

https://codereview.chromium.org/267183002/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_view.h (right):

https://codereview.chromium.org/267183002/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_view.h:40: static SkColor
GetDividerColor();
On 2014/05/08 20:52:41, Evan Stade wrote:
> On 2014/05/08 20:37:30, Patrick Dubroy wrote:
> > On 2014/05/08 17:50:29, Evan Stade wrote:
> > > why are these functions instead of just constants?
> > 
> > Because they require static initializers, and AFAIK we are supposed to avoid
> > those in Chromium
> >
>
(https://groups.google.com/a/chromium.org/d/msg/chromium-dev/p6h3HC8Wro4/o37Lb...).
> 
> SkColor is POD, so static initializers are OK.

Done.

[Patrick Dubroy, 2014-05-09 13:07:07]

Ilya, Evan, PTAL. I'd appreciate if you can review this ASAP, so I have a chance
to address comments and hopefully make the M36 branch. Thanks!

[Evan Stade, 2014-05-09 20:45:58]

!cocoa lgtm

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_view.h (right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_view.h:38: static const
SkColor kExplanatoryTextBackgroundColor;
nit: these should go near the other const at the top

[Ilya Sherman, 2014-05-09 21:51:28]

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_view.cc (right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_view.cc:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
Ultra nit: No need for "(c)"

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h (right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
Ultra nit: No need for "(c)".  Please change this throughout the CL.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:13: class
AutofillPopupController;
nit: Not needed?

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:15: }  //
namespace autofill
nit: No need for the trailing "// namespace" comment when forward-declaring.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:28: -
(NSColor*)subtextColor;
nit: Alphabetize?

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:32: -
(void)controllerDestroyed;
The base view has no concept of a controller.  Perhaps this should be called
delegateDestroyed?

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:35:
(autofill::AutofillPopupViewDelegate*)delegate frame:(NSRect)frame;
nit: I'd write this as

- (id)initWithDelegate:(autofill::AutofillPopupViewDelegate*)delegate
                 frame:(NSRect)frame;

(so, shorten the method name, and align on colons)

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:37: // Draw
the popup's background and border.
nit: De-indent.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.mm (right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.mm:76: -
(void)drawBackgroundAndBorderInRect:(NSRect)dirtyRect {
nit: Doesn't look like dirtyRect is used.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h (right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:28:
AutofillPopupViewCocoa* view, AutofillPopupController* controller);
nit: No longer needs to be explicit.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:28:
AutofillPopupViewCocoa* view, AutofillPopupController* controller);
nit: Please add docs.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:28:
AutofillPopupViewCocoa* view, AutofillPopupController* controller);
nit: One param per line.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:43:
AutofillPopupViewCocoa* view_;  // Weak reference, owned by |window_|.
|window_| no longer refers to any member of this class.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:43:
AutofillPopupViewCocoa* view_;  // Weak reference, owned by |window_|.
Is the |view_| guaranteed to outlive the bridge?  If so, please document that. 
Otherwise, how does this class know whether it is safe to access the variable?

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm (right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:18:
AutofillPopupViewCocoa* view, AutofillPopupController* controller)
nit: Please write one param per line if they don't all fit on a single line.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:19: :
view_(view), controller_(controller) {
nit: Likewise, please split this into two lines.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:26: [view_
hidePopup];
There used to be a |delete this| call here.  Who is responsible for freeing the
memory now?

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:46:
frame:NSZeroRect] autorelease];
Autorelease doesn't seem safe here.  What prevents the view from being destroyed
before the popup is shown?

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h
(right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
nit: 2014.  Please fix throughout.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h:13:
#import
"chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h"
nit: Can this #import be tucked into the implementation file?

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h:15:
@class AutofillPopupViewCocoa;
nit: Did you mean to forward-declare the password generation class?

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h:24: //
which actually implements the view.
nit: I saw that you updated the corresponding comment on the other bridge class.
 Please make the comments for the two classes match.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h:33: //
AutofillPopupView implementation.
nit: "// PasswordGenerationPopupView:" (at least, I assume that's where these
methods are coming from)

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h:40:
PasswordGenerationPopupViewCocoa* view_;  // Weak, owned by |window_|.
Same question here about lifetime and member variable access.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm
(right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:38: }
nit: Can this be combined with the method above it?  (That is, do you need both
methods?)

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:45:
frame:NSZeroRect] autorelease];
Same comment about memory management.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h
(right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
nit: 2014

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:10:
#import "ui/base/cocoa/base_view.h"
nit: This is probably not needed -- right?

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:12:
#import "chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.h"
nit: Should this be base_view?

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:13:
#import
"chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h"
Am I misreading this, or is this header attempting to import itself?

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:24: }
nit: Please leave a blank line after this one.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm
(right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
nit: 2014

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:27:
@end
What's this for?

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:36:
return [self initWithController:NULL frame:frame];
nit: If the method should not be reachable, just return nil.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:52:
controller_ = NULL;
Should this call [super controllerDestroyed] as well?

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:113:
bounds:helpBounds
Hmm, shouldn't this not include the pixel used for the divider?

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:158: }
These methods don't seem to be declared anywhere.  Can they just be free
functions defined at the top of the class, rather than class methods?

https://codereview.chromium.org/267183002/diff/130001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/267183002/diff/130001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:419:
'browser/ui/autofill/password_generation_popup_view.h',
nit: The previous order was correct: This file is sorted alphabetically.

[Patrick Dubroy, 2014-05-12 14:13:38]

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_view.cc (right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_view.cc:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> Ultra nit: No need for "(c)"

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h (right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> Ultra nit: No need for "(c)".  Please change this throughout the CL.

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:13: class
AutofillPopupController;
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: Not needed?

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:15: }  //
namespace autofill
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: No need for the trailing "// namespace" comment when forward-declaring.

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:28: -
(NSColor*)subtextColor;
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: Alphabetize?

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:32: -
(void)controllerDestroyed;
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> The base view has no concept of a controller.  Perhaps this should be called
> delegateDestroyed?

Actually, I don't think we need this at all.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:35:
(autofill::AutofillPopupViewDelegate*)delegate frame:(NSRect)frame;
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: I'd write this as
> 
> - (id)initWithDelegate:(autofill::AutofillPopupViewDelegate*)delegate
>                  frame:(NSRect)frame;
> 
> (so, shorten the method name, and align on colons)

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:37: // Draw
the popup's background and border.
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: De-indent.

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.mm (right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.mm:76: -
(void)drawBackgroundAndBorderInRect:(NSRect)dirtyRect {
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: Doesn't look like dirtyRect is used.

I just removed the parameter altogether, since the background and border cover
the whole bounds, I don't think we need to pass the dirty rect.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h (right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:28:
AutofillPopupViewCocoa* view, AutofillPopupController* controller);
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: No longer needs to be explicit.

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:28:
AutofillPopupViewCocoa* view, AutofillPopupController* controller);
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: Please add docs.

I can't think of any useful documentation to add here.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:28:
AutofillPopupViewCocoa* view, AutofillPopupController* controller);
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: One param per line.

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:43:
AutofillPopupViewCocoa* view_;  // Weak reference, owned by |window_|.
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> |window_| no longer refers to any member of this class.

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:43:
AutofillPopupViewCocoa* view_;  // Weak reference, owned by |window_|.
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> |window_| no longer refers to any member of this class.

Fixed.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm (right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:18:
AutofillPopupViewCocoa* view, AutofillPopupController* controller)
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: Please write one param per line if they don't all fit on a single line.

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:19: :
view_(view), controller_(controller) {
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: Likewise, please split this into two lines.

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:26: [view_
hidePopup];
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> There used to be a |delete this| call here.  Who is responsible for freeing
the
> memory now?

Right, I screwed this up when I was refactoring. I'd intended to push the
ownership out to the caller of AutofillPopupView::Create(), but for now I've
reverted to the way it was before.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:46:
frame:NSZeroRect] autorelease];
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> Autorelease doesn't seem safe here.  What prevents the view from being
destroyed
> before the popup is shown?

Hmmm, good point. I've moved this code into the constructor, and used a
scoped_nsobject. I think that makes the ownership more clear, because
effectively, the bridge *does* own the view.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h
(right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: 2014.  Please fix throughout.

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h:13:
#import
"chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h"
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: Can this #import be tucked into the implementation file?

Yep, done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h:15:
@class AutofillPopupViewCocoa;
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: Did you mean to forward-declare the password generation class?

Yeah, done. Though, oddly it seems it's not necessary (?)

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h:24: //
which actually implements the view.
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: I saw that you updated the corresponding comment on the other bridge
class.
>  Please make the comments for the two classes match.

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h:33: //
AutofillPopupView implementation.
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: "// PasswordGenerationPopupView:" (at least, I assume that's where these
> methods are coming from)

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h:40:
PasswordGenerationPopupViewCocoa* view_;  // Weak, owned by |window_|.
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> Same question here about lifetime and member variable access.

Fixed.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm
(right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:38: }
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: Can this be combined with the method above it?  (That is, do you need
both
> methods?)

I don't think they should be combined. They are re-implemented from the
PasswordGenerationPopupView interface, and they have different semantics, it
just happens that I'm using the same implementation for both.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:45:
frame:NSZeroRect] autorelease];
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> Same comment about memory management.

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h
(right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: 2014

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:10:
#import "ui/base/cocoa/base_view.h"
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: This is probably not needed -- right?

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:12:
#import "chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.h"
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: Should this be base_view?

Yep, fixed.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:13:
#import
"chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h"
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> Am I misreading this, or is this header attempting to import itself?

Yes, not sure what happened there! Fixed.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.h:24: }
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm
(right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: 2014

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:27:
@end
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> What's this for?

Not sure. Removed.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:36:
return [self initWithController:NULL frame:frame];
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: If the method should not be reachable, just return nil.

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:52:
controller_ = NULL;
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> Should this call [super controllerDestroyed] as well?

Actually, I don't think we need this at all. It's being sent right before the
view is destroyed (see here: http://goo.gl/vS6dsk).

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:113:
bounds:helpBounds
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> Hmm, shouldn't this not include the pixel used for the divider?

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:158: }
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> These methods don't seem to be declared anywhere.  Can they just be free
> functions defined at the top of the class, rather than class methods?

Done.

https://codereview.chromium.org/267183002/diff/130001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/267183002/diff/130001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:419:
'browser/ui/autofill/password_generation_popup_view.h',
On 2014/05/09 21:51:28, Ilya Sherman wrote:
> nit: The previous order was correct: This file is sorted alphabetically.

Done.

[Ilya Sherman, 2014-05-12 21:46:10]

Thanks!  Just one outstanding issue around memory management:

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h
(right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.h:15:
@class AutofillPopupViewCocoa;
On 2014/05/12 14:13:39, Patrick Dubroy wrote:
> On 2014/05/09 21:51:28, Ilya Sherman wrote:
> > nit: Did you mean to forward-declare the password generation class?
> 
> Yeah, done. Though, oddly it seems it's not necessary (?)

I think it's because you were importing the header previously.

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm
(right):

https://codereview.chromium.org/267183002/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa.mm:52:
controller_ = NULL;
On 2014/05/12 14:13:39, Patrick Dubroy wrote:
> On 2014/05/09 21:51:28, Ilya Sherman wrote:
> > Should this call [super controllerDestroyed] as well?
> 
> Actually, I don't think we need this at all. It's being sent right before the
> view is destroyed (see here: http://goo.gl/vS6dsk).

The view is destroyed asynchronously, though -- the NSWindow holds a reference
to it.  I remember this being important (and hairy, I admit) when I originally
implemented it.  I'd prefer to leave things as they are for now, and then
possibly clean up the memory model in a later CL.

https://codereview.chromium.org/267183002/diff/150001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.mm (right):

https://codereview.chromium.org/267183002/diff/150001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.mm:90: return;
This code is currently unreachable.  I think that's a problem -- it's possible
for the delegate to be destroyed prior to |self| being destroyed, which means
that there would be a use-after-free.  That's why there was a
-controllerDestroyed method to begin win.

https://codereview.chromium.org/267183002/diff/150001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm
(right):

https://codereview.chromium.org/267183002/diff/150001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:21:
initWithController:controller frame:NSZeroRect]);
nit: Please write this as one arg per line.

[Patrick Dubroy, 2014-05-13 12:18:04]

https://codereview.chromium.org/267183002/diff/150001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.mm (right):

https://codereview.chromium.org/267183002/diff/150001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.mm:90: return;
On 2014/05/12 21:46:10, Ilya Sherman wrote:
> This code is currently unreachable.  I think that's a problem -- it's possible
> for the delegate to be destroyed prior to |self| being destroyed, which means
> that there would be a use-after-free.  That's why there was a
> -controllerDestroyed method to begin win.

Ok, I've added back controllerDestroyed.

https://codereview.chromium.org/267183002/diff/150001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm
(right):

https://codereview.chromium.org/267183002/diff/150001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:21:
initWithController:controller frame:NSZeroRect]);
On 2014/05/12 21:46:10, Ilya Sherman wrote:
> nit: Please write this as one arg per line.

Done.

[Ilya Sherman, 2014-05-13 18:32:54]

LGTM, thanks!

[Patrick Dubroy, 2014-05-14 09:55:49]

The CQ bit was checked by dubroy@chromium.org

[commit-bot: I haz the power, 2014-05-14 09:56:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dubroy@chromium.org/267183002/220001

[commit-bot: I haz the power, 2014-05-14 11:50:08]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-14 11:53:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-14 11:53:19]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[Patrick Dubroy, 2014-05-14 14:22:03]

The CQ bit was checked by dubroy@chromium.org

[commit-bot: I haz the power, 2014-05-14 14:22:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dubroy@chromium.org/267183002/220001

[commit-bot: I haz the power, 2014-05-14 15:14:34]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-14 15:32:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-14 15:32:14]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Patrick Dubroy, 2014-05-14 18:53:03]

The CQ bit was checked by dubroy@chromium.org

[commit-bot: I haz the power, 2014-05-14 18:54:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dubroy@chromium.org/267183002/220001

[commit-bot: I haz the power, 2014-05-14 19:11:27]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-14 19:25:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-14 19:25:28]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Patrick Dubroy, 2014-05-15 09:10:47]

Committed patchset #14 manually as r270648 (presubmit successful).