[Autofill] Add basic version of GridLayout

[Autofill] Add basic version of GridLayout

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

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

sail: PTAL - GridLayout, as required by
https://codereview.chromium.org/14704004/

[sail, 2013-05-02 16:49:20]

swapping with rsesek

[Robert Sesek, 2013-05-02 20:16:46]

Looks pretty good. How similar is the layout manager to the views version?

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/layou...
File chrome/browser/ui/cocoa/layout_view.h (right):

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/layou...
chrome/browser/ui/cocoa/layout_view.h:21: -
(void)setLayoutManager:(scoped_ptr<SimpleGridLayout>) layout;
nit: no space after )

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/layou...
File chrome/browser/ui/cocoa/layout_view.mm (right):

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/layou...
chrome/browser/ui/cocoa/layout_view.mm:11: -
(void)setLayoutManager:(scoped_ptr<SimpleGridLayout>) layout {
nit: no space after )

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
File chrome/browser/ui/cocoa/simple_grid_layout.h (right):

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
chrome/browser/ui/cocoa/simple_grid_layout.h:124: }
nit: blank line after

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
chrome/browser/ui/cocoa/simple_grid_layout.h:126: float ColumnLocation(int
column_index);
nit: blank line after

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
File chrome/browser/ui/cocoa/simple_grid_layout.mm (right):

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
chrome/browser/ui/cocoa/simple_grid_layout.mm:13: const int kAutoColumnIdStart =
1000000;  // Starting ID for autogeneration.
nit: no indention

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
chrome/browser/ui/cocoa/simple_grid_layout.mm:117: bool is_padding() { return
is_padding_; }
nit: blank line after

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
chrome/browser/ui/cocoa/simple_grid_layout.mm:129: Row(float resize_percent, int
fixed_height, ColumnSet* column_set);
nit: blank line after

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
chrome/browser/ui/cocoa/simple_grid_layout.mm:136: Row::Row(float
resize_percent, int fixed_height,  ColumnSet* column_set)
nit: extra space after ,

[groby-ooo-7-16, 2013-05-03 04:31:46]

It's completely based on it, but a reimplementation. (I didn't trust a
"copy/replace" approach :)

It has basic gridlayout functionality, but it misses

* Varying alignments for cell - they're all left-aligned
* better support for "preferred size". (Presumably won't work for all types of
views/cells anyways)
* column spans and row spans
* linked columns

I'd say it's about 30% of GridLayout. (I'd love to push it further and actually
make it cross-platform, but that met resistance ;)

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/layou...
File chrome/browser/ui/cocoa/layout_view.h (right):

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/layou...
chrome/browser/ui/cocoa/layout_view.h:21: -
(void)setLayoutManager:(scoped_ptr<SimpleGridLayout>) layout;
On 2013/05/02 20:16:46, rsesek wrote:
> nit: no space after )

Done.

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/layou...
File chrome/browser/ui/cocoa/layout_view.mm (right):

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/layou...
chrome/browser/ui/cocoa/layout_view.mm:11: -
(void)setLayoutManager:(scoped_ptr<SimpleGridLayout>) layout {
On 2013/05/02 20:16:46, rsesek wrote:
> nit: no space after )

Done.

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
File chrome/browser/ui/cocoa/simple_grid_layout.h (right):

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
chrome/browser/ui/cocoa/simple_grid_layout.h:124: }
On 2013/05/02 20:16:46, rsesek wrote:
> nit: blank line after

Done.

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
chrome/browser/ui/cocoa/simple_grid_layout.h:126: float ColumnLocation(int
column_index);
On 2013/05/02 20:16:46, rsesek wrote:
> nit: blank line after

Done.

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
File chrome/browser/ui/cocoa/simple_grid_layout.mm (right):

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
chrome/browser/ui/cocoa/simple_grid_layout.mm:13: const int kAutoColumnIdStart =
1000000;  // Starting ID for autogeneration.
On 2013/05/02 20:16:46, rsesek wrote:
> nit: no indention

Done.

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
chrome/browser/ui/cocoa/simple_grid_layout.mm:117: bool is_padding() { return
is_padding_; }
On 2013/05/02 20:16:46, rsesek wrote:
> nit: blank line after

Done.

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
chrome/browser/ui/cocoa/simple_grid_layout.mm:129: Row(float resize_percent, int
fixed_height, ColumnSet* column_set);
On 2013/05/02 20:16:46, rsesek wrote:
> nit: blank line after

Done.

https://codereview.chromium.org/14646021/diff/1/chrome/browser/ui/cocoa/simpl...
chrome/browser/ui/cocoa/simple_grid_layout.mm:136: Row::Row(float
resize_percent, int fixed_height,  ColumnSet* column_set)
On 2013/05/02 20:16:46, rsesek wrote:
> nit: extra space after ,

Done.

[Robert Sesek, 2013-05-07 17:14:49]

On 2013/05/03 04:31:46, groby wrote:
> It's completely based on it, but a reimplementation. (I didn't trust a
> "copy/replace" approach :)
> 
> It has basic gridlayout functionality, but it misses
> 
> * Varying alignments for cell - they're all left-aligned
> * better support for "preferred size". (Presumably won't work for all types of
> views/cells anyways)
> * column spans and row spans
> * linked columns
> 
> I'd say it's about 30% of GridLayout. (I'd love to push it further and
actually
> make it cross-platform, but that met resistance ;)

My resistance was to having a C++ class responsible for layout, which is still
the case here. I basically see two good options:

1. Just have a standalone Obj-C implementation that doesn't share code with the
views version.
2. Extract the LayoutManager class from views and modify it to take some minimal
interface instead of a views::View*. On top of this, build an Obj-C interface
for clients, that internally wraps the NSView*s with the C++ object used in the
cross-platform interface.

My resistance to (2) was that I think it's probably more work than it seems
(pull at one thing and what else comes up…).

[groby-ooo-7-16, 2013-05-07 17:38:45]

I still think (2) is a good approach, and based on what I've seen so far, it
seems somewhat doable. The view::Views interface is fairly minimal, and the only
item that will be "interesting" is the concept of preferredSize. I think it
wouldn't be unreasonable to demand that NSViews in a layout tree support that
selector.

That is one reason why I stuck with C++ - it leaves that road open.

But the short-term goal was to have a working version of layout that provides a
similar layout to the views version for the work on Autofill/requestAutocomple
and its dialog. The need to be somewhat identical was a bigger driver in
sticking close to the original.

And the need to be somewhat identical comes from the fact that
AutofillDialogView, the base class for this, gets info about the contained
fields in a format geared towards GridLayout's API.

[Robert Sesek, 2013-05-07 18:10:17]

On 2013/05/07 17:38:45, groby wrote:
> I still think (2) is a good approach, and based on what I've seen so far, it
> seems somewhat doable. The view::Views interface is fairly minimal, and the
only
> item that will be "interesting" is the concept of preferredSize. I think it
> wouldn't be unreasonable to demand that NSViews in a layout tree support that
> selector.
> 
> That is one reason why I stuck with C++ - it leaves that road open.
> 
> But the short-term goal was to have a working version of layout that provides
a
> similar layout to the views version for the work on Autofill/requestAutocomple
> and its dialog. The need to be somewhat identical was a bigger driver in
> sticking close to the original.
> 
> And the need to be somewhat identical comes from the fact that
> AutofillDialogView, the base class for this, gets info about the contained
> fields in a format geared towards GridLayout's API.

I agree (2) seems like the most ideal. I just don't want to send you to fight a
dependency hydra. But it does seem like it wouldn't be too bad. I think it's
worth a try, at least.

[groby-ooo-7-16, 2013-05-07 21:49:22]

So do I get to land this first, or does this mean I'll need to port it all right
now?

[Robert Sesek, 2013-05-07 22:27:52]

On 2013/05/07 21:49:22, groby wrote:
> So do I get to land this first, or does this mean I'll need to port it all
right
> now?

When would you be porting it?

[groby-ooo-7-16, 2013-05-07 22:31:27]

After the work on rAC is done. (That includes a "hopefully", yes)

I'd estimate a good week at least, and I can't spend that right now - rAC is
going to be at IO, and Mac is already behind. If that holds up this patch, I
guess I'll need to Frankenstein an ObjC solution for now.

[Robert Sesek, 2013-05-07 23:14:32]

On 2013/05/07 22:31:27, groby wrote:
> After the work on rAC is done. (That includes a "hopefully", yes)
> 
> I'd estimate a good week at least, and I can't spend that right now - rAC is
> going to be at IO, and Mac is already behind. If that holds up this patch, I
> guess I'll need to Frankenstein an ObjC solution for now.

If you need this as a stopgap then, can these files be moved to someplace less
general? I don't want to approve this code as a general solution to grid
layouts, but if it were in some subdir and the code tagged appropriately this is
probably OK as a temporary solution.

[groby-ooo-7-16, 2013-05-07 23:16:49]

Heh - it used to be in chrome/browser/ui/cocoa/autofill, and sail@ suggested
moving it out :)

If I put it back there, will that work for you?

[Robert Sesek, 2013-05-07 23:18:12]

On 2013/05/07 23:16:49, groby wrote:
> Heh - it used to be in chrome/browser/ui/cocoa/autofill, and sail@ suggested
> moving it out :)
> 
> If I put it back there, will that work for you?

Yes, I think so, if it's got a comment saying that something brighter and
shinier is coming in the future, with an associated bug to track the work.

[groby-ooo-7-16, 2013-05-13 21:57:40]

Here we go, all back to its humble origins :)

[Robert Sesek, 2013-05-13 22:15:08]

LGTM

https://codereview.chromium.org/14646021/diff/24001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/autofill/layout_view.mm (right):

https://codereview.chromium.org/14646021/diff/24001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/autofill/layout_view.mm:33: 
nit: extra blank line

[groby-ooo-7-16, 2013-05-13 22:40:11]

https://codereview.chromium.org/14646021/diff/24001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/autofill/layout_view.mm (right):

https://codereview.chromium.org/14646021/diff/24001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/autofill/layout_view.mm:33: 
On 2013/05/13 22:15:08, rsesek wrote:
> nit: extra blank line

Done.

[commit-bot: I haz the power, 2013-05-13 22:40:35]

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

[commit-bot: I haz the power, 2013-05-14 04:49:10]

Change committed as 199901