[Autofill] Add Details Section

[Autofill] Add Details Section

Provide UI for details of payment instrument.

R=sail@chromium.org
BUG=157274
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200338

[groby-ooo-7-16, 2013-04-30 23:11:18]

sail: PTAL

This one's a bit largish, mostly due to the grid_layout code that has been
ported. Couldn't split off a much smaller thing, sorry.

[sail, 2013-05-01 17:12:36]

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_details_container.h (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.h:13: class
AutofillDialogController;
no need to indent

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_details_container.mm (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:21:
sectionContainer.reset(
move into constructor

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:36: NSSize
detailSize;
need to initialize

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:38:
detailSize.width = NSWidth([[container view] frame]);
should this be a max or something?

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:44: self.view =
[[NSView alloc] init];
I don't think we should use dot notation (it's pretty inconsistent with the rest
of the code)

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:47:
for(AutofillSectionContainer* container in details_.get()) {
Alternatively, you could have single loop that iterates backwards.

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_details_container_unittest.mm
(right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container_unittest.mm:17:
virtual void SetUp() {
Could do this in the constructor too (like your other tests)

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:134: NSRect
contentRect = clientRect;
move this closer to where it's used?

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_layout_view.h (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_layout_view.h:15: @interface
AutofillLayoutView : NSView {
It might be nice to generalize this, something like
ui/base/cocoa/simple_layout_view_cocoa.h

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_layout_view.h:21: -
(void)setLayoutManager:(SimpleGridLayout*) layout;
since this takes ownership, pass a scoped_ptr instead?

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_layout_view.mm (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_layout_view.mm:15: -
(void)setFrame:(NSRect)frame {
should you override resizeSubviewsWithOldSize instead?

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_layout_view_unittest.mm (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_layout_view_unittest.mm:28:
SimpleGridLayout* layout_;  // weak, owned by view_.
how about adding an accessor in AutofillLayoutView and removing this?

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.h (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_section_container.h:26:
forSection:(autofill::DialogSection) section;
no space after )

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:53: self.view =
[self newSectionView:base::SysUTF16ToNSString(labelText)
no dot notation

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:57: -
(NSTextField*)newDetailSectionLabel:(NSString*)labelText {
My personal preference is to have things autoreleased or returned a
scoped_nsobject.

We have so little code like this that future maintainers are bound to get
tripped up.

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:82: NSRect frame
= NSMakeRect(
this would be easier to read as:
  NSRect frame = NSZeroRect;
  frame.size.width = ...
  frame.size.height = ..

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/simple_grid_layout.h (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/simple_grid_layout.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
I think it would be better to move this into ui/base/cocoa and send it out
separately. If not at least move it to chrome/browser/ui/cocoa/ and send it out
separately.

[groby-ooo-7-16, 2013-05-01 20:37:44]

Review comments addressed, GridLayout moved to separate change
(https://codereview.chromium.org/14646021/)

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_details_container.h (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.h:13: class
AutofillDialogController;
On 2013/05/01 17:12:36, sail wrote:
> no need to indent

Done.

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_details_container.mm (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:21:
sectionContainer.reset(
I'm confused - I call this several times, so I can't just move this code into
the constructor.

I'd also like to keep those calls in loadView, since they're actually
constructing sub-views.

On 2013/05/01 17:12:36, sail wrote:
> move into constructor

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:36: NSSize
detailSize;
Done. 

Only for my clarification: ObjC style guide says don't do that -
(http://www.corp.google.com/eng/doc/objcguide.xml#Initialization)

I assume does that only apply to objects, but not to structs? 

On 2013/05/01 17:12:36, sail wrote:
> need to initialize

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:38:
detailSize.width = NSWidth([[container view] frame]);
I was torn here - I only need the width of the first one, since they're all
guaranteed to be the same. A max gives the misleading impression they're not,
this looks like I missed max.

Changed to a much simpler approach in new CL.
On 2013/05/01 17:12:36, sail wrote:
> should this be a max or something?

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:44: self.view =
[[NSView alloc] init];
Done.

If we don't use dot notation, does it make sense to declare non-synthesized
properties as @property at all?

Also, FYI only, it's in sporadic use throughout our code - see
https://code.google.com/p/chromium/codesearch#search/&q=%22self.%22%20lang:%5...

On 2013/05/01 17:12:36, sail wrote:
> I don't think we should use dot notation (it's pretty inconsistent with the
rest
> of the code)

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:47:
for(AutofillSectionContainer* container in details_.get()) {
TIL: reverseObjectEnumerator - done.
On 2013/05/01 17:12:36, sail wrote:
> Alternatively, you could have single loop that iterates backwards.

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_details_container_unittest.mm
(right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container_unittest.mm:17:
virtual void SetUp() {
On 2013/05/01 17:12:36, sail wrote:
> Could do this in the constructor too (like your other tests)

Done.

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:134: NSRect
contentRect = clientRect;
On 2013/05/01 17:12:36, sail wrote:
> move this closer to where it's used?

Done.

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_layout_view.h (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_layout_view.h:15: @interface
AutofillLayoutView : NSView {
On 2013/05/01 17:12:36, sail wrote:
> It might be nice to generalize this, something like
> ui/base/cocoa/simple_layout_view_cocoa.h

Done. (in separate CL)

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_layout_view.h:21: -
(void)setLayoutManager:(SimpleGridLayout*) layout;
Done. (in separate CL)
On 2013/05/01 17:12:36, sail wrote:
> since this takes ownership, pass a scoped_ptr instead?

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_layout_view.mm (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_layout_view.mm:15: -
(void)setFrame:(NSRect)frame {
On 2013/05/01 17:12:36, sail wrote:
> should you override resizeSubviewsWithOldSize instead?

Done. (separate CL)

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_layout_view_unittest.mm (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_layout_view_unittest.mm:28:
SimpleGridLayout* layout_;  // weak, owned by view_.
Added accessor. Still keeping the pointer for convenience reason. (Separate CL)
On 2013/05/01 17:12:36, sail wrote:
> how about adding an accessor in AutofillLayoutView and removing this?

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.h (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_section_container.h:26:
forSection:(autofill::DialogSection) section;
On 2013/05/01 17:12:36, sail wrote:
> no space after )

Done.

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:53: self.view =
[self newSectionView:base::SysUTF16ToNSString(labelText)
On 2013/05/01 17:12:36, sail wrote:
> no dot notation

Done.

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:57: -
(NSTextField*)newDetailSectionLabel:(NSString*)labelText {
On 2013/05/01 17:12:36, sail wrote:
> My personal preference is to have things autoreleased or returned a
> scoped_nsobject.
> 
> We have so little code like this that future maintainers are bound to get
> tripped up.

Done.

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:82: NSRect frame
= NSMakeRect(
On 2013/05/01 17:12:36, sail wrote:
> this would be easier to read as:
>   NSRect frame = NSZeroRect;
>   frame.size.width = ...
>   frame.size.height = ..

Done.

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/simple_grid_layout.h (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/simple_grid_layout.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
I proposed this on chromium-dev - there was medium resistance to having
something close to Views' GridLayout widely available. (Mostly based on the fact
it's not ObjC-ish)

Picking chrome/browser/ui/cocoa as a compromise location.
On 2013/05/01 17:12:36, sail wrote:
> I think it would be better to move this into ui/base/cocoa and send it out
> separately. If not at least move it to chrome/browser/ui/cocoa/ and send it
out
> separately.

[sail, 2013-05-01 21:38:38]

Looks pretty good! Could you also include screenshots.

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_details_container.mm (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:21:
sectionContainer.reset(
On 2013/05/01 20:37:44, groby wrote:
> I'm confused - I call this several times, so I can't just move this code into
> the constructor.
> 
> I'd also like to keep those calls in loadView, since they're actually
> constructing sub-views.
> 
> On 2013/05/01 17:12:36, sail wrote:
> > move into constructor
> 

Oops, sorry. I meant combine declaration and initialization.

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:36: NSSize
detailSize;
On 2013/05/01 20:37:44, groby wrote:
> Done. 
> 
> Only for my clarification: ObjC style guide says don't do that -
> (http://www.corp.google.com/eng/doc/objcguide.xml#Initialization)
> 
> I assume does that only apply to objects, but not to structs? 
> 
> On 2013/05/01 17:12:36, sail wrote:
> > need to initialize
> 

That only applies to member variables.

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:57: -
(NSTextField*)newDetailSectionLabel:(NSString*)labelText {
On 2013/05/01 20:37:44, groby wrote:
> On 2013/05/01 17:12:36, sail wrote:
> > My personal preference is to have things autoreleased or returned a
> > scoped_nsobject.
> > 
> > We have so little code like this that future maintainers are bound to get
> > tripped up.
> 
> Done.

Didn't see this change in the latest patch.

[groby-ooo-7-16, 2013-05-01 22:36:41]

Fixed. Screenshot at https://screenshot.googleplex.com/ty229PBYv

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_details_container.mm (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_details_container.mm:21:
sectionContainer.reset(
On 2013/05/01 21:38:38, sail wrote:
> On 2013/05/01 20:37:44, groby wrote:
> > I'm confused - I call this several times, so I can't just move this code
into
> > the constructor.
> > 
> > I'd also like to keep those calls in loadView, since they're actually
> > constructing sub-views.
> > 
> > On 2013/05/01 17:12:36, sail wrote:
> > > move into constructor
> > 
> 
> Oops, sorry. I meant combine declaration and initialization.

Done.

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (right):

https://codereview.chromium.org/14704004/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:57: -
(NSTextField*)newDetailSectionLabel:(NSString*)labelText {
Ah, was talking about newSectionView - missed this one. 

Turned into its own class, per offline discussion.

On 2013/05/01 21:38:38, sail wrote:
> On 2013/05/01 20:37:44, groby wrote:
> > On 2013/05/01 17:12:36, sail wrote:
> > > My personal preference is to have things autoreleased or returned a
> > > scoped_nsobject.
> > > 
> > > We have so little code like this that future maintainers are bound to get
> > > tripped up.
> > 
> > Done.
> 
> Didn't see this change in the latest patch.

[groby-ooo-7-16, 2013-05-01 22:38:15]

BTW: Mocks for the final thing - which we are slooooowly approaching - are here:
https://folio.googleplex.com/chrome-ux/_mocks/030-wallet-chrome/27-desktop-wa...

[sail, 2013-05-01 22:58:24]

LGTM

The popup in the screenshot doesn't line up with the text fields. Feel free to
fix that in a separate CL (or ignore it if you're going to be implementing a
custom popup).

https://codereview.chromium.org/14704004/diff/13001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (right):

https://codereview.chromium.org/14704004/diff/13001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:57: -
(NSTextField*)newDetailSectionLabel:(NSString*)labelText {
Hm.. I don't think moving this into a separate class is a good idea.

My original nit was just about returning a object with a retain count of 1.

I think either it should go back to the way it used to be or return a
autoreleased object.

[groby-ooo-7-16, 2013-05-02 00:40:41]

Yes, the popups need presumably a new NSCell - the mocks call for a look similar
to ConstrainedWindowButtons. That'll come on the polish pass. I'll still have a
ton of basic functionality ahead. (I'm about 6 months behind the Views guys. :)

Changed the semantics for all creation functions use autorelease(), and renamed
functions to makeXXX - newXXX would have implied a retain count of 1.

[sail, 2013-05-02 00:48:52]

Second LGTM

[groby-ooo-7-16, 2013-05-03 05:06:17]

Tiny fix to a broken test while I wait for layout to land.

[commit-bot: I haz the power, 2013-05-14 23:34:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/14704004/53001

[commit-bot: I haz the power, 2013-05-15 00:21:09]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-15 01:18:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/14704004/68001

[commit-bot: I haz the power, 2013-05-15 04:01:30]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-15 18:01:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/14704004/68001

[commit-bot: I haz the power, 2013-05-15 20:27:03]

Change committed as 200338