[Autofill] Move functions from the AutofillPopupController to AutofillPopupLayoutModel

[Autofill] Move functions from the AutofillPopupController to AutofillPopupLayoutModel

The controller should only provide the data model and tell the view when to redraw.

This is functionality neutral, although quite a few tests are shuffled around.

For Mac, a delegate interface is introduced so that the Cocoa view can call into the view layout model.

BUG=575802
TEST=Existing

Committed: https://crrev.com/c338bb771c11d46e814176a1958ec26943d4f5c6
Cr-Commit-Position: refs/heads/master@{#370697}

[Mathieu, 2016-01-08 17:31:55]

Patchset #2 (id:20001) has been deleted

[Mathieu, 2016-01-08 19:02:13]

Patchset #1 (id:1) has been deleted

[Mathieu, 2016-01-08 19:02:25]

Patchset #2 (id:60001) has been deleted

[Mathieu, 2016-01-08 19:06:46]

Patchset #2 (id:80001) has been deleted

[Mathieu, 2016-01-08 19:51:15]

Patchset #3 (id:110001) has been deleted

[Mathieu, 2016-01-08 21:01:47]

Patchset #3 (id:130001) has been deleted

[Mathieu, 2016-01-08 21:02:13]

Patchset #3 (id:150001) has been deleted

[Mathieu, 2016-01-08 21:20:30]

Patchset #3 (id:170001) has been deleted

[Mathieu, 2016-01-08 21:25:00]

Description was changed from

==========
[Autofill] Move functions from the AutofillPopupController to AutofillPopupView

The view should be the one handling bounds and related calculations. The
controller should only provide the data model and tell the view when to redraw.

This is functionality neutral, although quite a few tests are shuffled around.

BUG=None
TEST=Existing
==========

to

==========
[Autofill] Move functions from the AutofillPopupController to AutofillPopupView

The AutofillPopupView (now a base class) should be the one handling bounds and
related calculations. The controller should only provide the data model and tell
the view when to redraw.

This is functionality neutral, although quite a few tests are shuffled around.

For Mac, a delegate interface is introduced so that the Cocoa view can call into
the base class.

BUG=575802
TEST=Existing
==========

[Mathieu, 2016-01-08 21:30:13]

mathp@chromium.org changed reviewers:
+ estade@chromium.org

[Mathieu, 2016-01-08 21:30:13]

Hi Evan, 

PTAL. I think it's a good step in the right direction. I've tested this on Linux
and Mac, and will run it on Android as well (both popup and keyboard accessory).

As I started moving things, some choices were made along the way (modifying
interfaces, test structures). Happy to discuss them. 

Note: I will not want to have this in M49 because it's functionality neutral and
I want it to bake, but PTAL now still.

[Mathieu, 2016-01-08 21:42:37]

Patchset #2 (id:100001) has been deleted

[Evan Stade, 2016-01-09 00:19:02]

https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.h (right):

https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_controller_impl.h:80: 
?

https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_controller_impl.h:86: #if
!defined(OS_ANDROID)
this will need to be !BUILDFLAG(ANDROID_JAVA_UI)

https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_view.h (right):

https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view.h:66: int GetDesiredPopupHeight()
const;
This design uses multiple inheritance, which is not permitted in Chrome.
(AutofillPopupViewViews extends both AutofillPopupView and
AutofillPopupBaseView, both of which contain implementation as opposed to being
pure interfaces.) You can move all this to a separate class and use composition
instead.

That said, the composition solution is still not great because it leaves us with
a super ad-hoc looking UI. Ideally, we'd reuse menus, so this would look more
like native menus (e.g. bookmark folder dropdown, context menu, etc.), and
luckily, there are already cross-platform menu models and platform-specific menu
view implementations. On my system, the spacing, the font colors, font sizing,
the highlight color, and other minor details like that are all different between
the autofill dropdown and a normal menu, and I don't think there's any good
reason for that, because it's all close enough that it seems like Autofill is
mimicking menus, poorly. It's a large part of why the Autofill popup looks janky
and old.

Granted, the existing menu code may not already do everything you want it to, so
it might need to be extended. I'm unsure how much work that would be. However, I
think the final result would be fewer lines of code and better visual alignment
with the rest of the platform.

[Mathieu, 2016-01-09 00:53:17]

Thanks

https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_view.h (right):

https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view.h:66: int GetDesiredPopupHeight()
const;
On 2016/01/09 00:19:02, Evan Stade wrote:
> This design uses multiple inheritance, which is not permitted in Chrome.
> (AutofillPopupViewViews extends both AutofillPopupView and
> AutofillPopupBaseView, both of which contain implementation as opposed to
being
> pure interfaces.) You can move all this to a separate class and use
composition
> instead.
> 
> That said, the composition solution is still not great because it leaves us
with
> a super ad-hoc looking UI. Ideally, we'd reuse menus, so this would look more
> like native menus (e.g. bookmark folder dropdown, context menu, etc.), and
> luckily, there are already cross-platform menu models and platform-specific
menu
> view implementations. On my system, the spacing, the font colors, font sizing,
> the highlight color, and other minor details like that are all different
between
> the autofill dropdown and a normal menu, and I don't think there's any good
> reason for that, because it's all close enough that it seems like Autofill is
> mimicking menus, poorly. It's a large part of why the Autofill popup looks
janky
> and old.
> 
> Granted, the existing menu code may not already do everything you want it to,
so
> it might need to be extended. I'm unsure how much work that would be. However,
I
> think the final result would be fewer lines of code and better visual
alignment
> with the rest of the platform.

In your mind, would AutofillPopupViewViews extend AutofillPopupBaseView and have
a member of class say AutofillPopupViewImpl, which would contain all bounds
related code which I've added here? 

RE menus: I found SimpleMenuModel which is used for rAc, amongst many others 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
it does look like an applicable thing indeed. Is it supported on all platforms
including Mac and Android? It looks like longer term work, and I'm not sure it
should block this change, since the goal is really to get views-related stuff
out of the PopupController and starting to make a better boundary. Let me know
what you think.

[Evan Stade, 2016-01-09 01:01:58]

On 2016/01/09 00:53:17, Mathieu Perreault wrote:
> Thanks
> 
>
https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
> File chrome/browser/ui/autofill/autofill_popup_view.h (right):
> 
>
https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
> chrome/browser/ui/autofill/autofill_popup_view.h:66: int
GetDesiredPopupHeight()
> const;
> On 2016/01/09 00:19:02, Evan Stade wrote:
> > This design uses multiple inheritance, which is not permitted in Chrome.
> > (AutofillPopupViewViews extends both AutofillPopupView and
> > AutofillPopupBaseView, both of which contain implementation as opposed to
> being
> > pure interfaces.) You can move all this to a separate class and use
> composition
> > instead.
> > 
> > That said, the composition solution is still not great because it leaves us
> with
> > a super ad-hoc looking UI. Ideally, we'd reuse menus, so this would look
more
> > like native menus (e.g. bookmark folder dropdown, context menu, etc.), and
> > luckily, there are already cross-platform menu models and platform-specific
> menu
> > view implementations. On my system, the spacing, the font colors, font
sizing,
> > the highlight color, and other minor details like that are all different
> between
> > the autofill dropdown and a normal menu, and I don't think there's any good
> > reason for that, because it's all close enough that it seems like Autofill
is
> > mimicking menus, poorly. It's a large part of why the Autofill popup looks
> janky
> > and old.
> > 
> > Granted, the existing menu code may not already do everything you want it
to,
> so
> > it might need to be extended. I'm unsure how much work that would be.
However,
> I
> > think the final result would be fewer lines of code and better visual
> alignment
> > with the rest of the platform.
> 
> In your mind, would AutofillPopupViewViews extend AutofillPopupBaseView and
have
> a member of class say AutofillPopupViewImpl, which would contain all bounds
> related code which I've added here? 

yes, although I wouldn't call the owned class that. Maybe PopupViewModel?
There's not really a great name for it because it shouldn't actually exist.

> 
> RE menus: I found SimpleMenuModel which is used for rAc, amongst many others 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
> it does look like an applicable thing indeed. Is it supported on all platforms
> including Mac and Android? It looks like longer term work, and I'm not sure it
> should block this change, since the goal is really to get views-related stuff
> out of the PopupController and starting to make a better boundary. Let me know
> what you think.

The thing is, I don't know if this change is a step in the right direction. It
may be an improvement over the current state of the world, but if the goal is to
reuse menu code, does this get us any closer? If not I think it may be more
prudent to spend our time (coding, reviewing) on the menu approach and not spend
whatever additional effort it would take to land this.

[Mathieu, 2016-01-09 01:14:40]

On 2016/01/09 01:01:58, Evan Stade wrote:
> On 2016/01/09 00:53:17, Mathieu Perreault wrote:
> > Thanks
> > 
> >
>
https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
> > File chrome/browser/ui/autofill/autofill_popup_view.h (right):
> > 
> >
>
https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
> > chrome/browser/ui/autofill/autofill_popup_view.h:66: int
> GetDesiredPopupHeight()
> > const;
> > On 2016/01/09 00:19:02, Evan Stade wrote:
> > > This design uses multiple inheritance, which is not permitted in Chrome.
> > > (AutofillPopupViewViews extends both AutofillPopupView and
> > > AutofillPopupBaseView, both of which contain implementation as opposed to
> > being
> > > pure interfaces.) You can move all this to a separate class and use
> > composition
> > > instead.
> > > 
> > > That said, the composition solution is still not great because it leaves
us
> > with
> > > a super ad-hoc looking UI. Ideally, we'd reuse menus, so this would look
> more
> > > like native menus (e.g. bookmark folder dropdown, context menu, etc.), and
> > > luckily, there are already cross-platform menu models and
platform-specific
> > menu
> > > view implementations. On my system, the spacing, the font colors, font
> sizing,
> > > the highlight color, and other minor details like that are all different
> > between
> > > the autofill dropdown and a normal menu, and I don't think there's any
good
> > > reason for that, because it's all close enough that it seems like Autofill
> is
> > > mimicking menus, poorly. It's a large part of why the Autofill popup looks
> > janky
> > > and old.
> > > 
> > > Granted, the existing menu code may not already do everything you want it
> to,
> > so
> > > it might need to be extended. I'm unsure how much work that would be.
> However,
> > I
> > > think the final result would be fewer lines of code and better visual
> > alignment
> > > with the rest of the platform.
> > 
> > In your mind, would AutofillPopupViewViews extend AutofillPopupBaseView and
> have
> > a member of class say AutofillPopupViewImpl, which would contain all bounds
> > related code which I've added here? 
> 
> yes, although I wouldn't call the owned class that. Maybe PopupViewModel?
> There's not really a great name for it because it shouldn't actually exist.
> 
> > 
> > RE menus: I found SimpleMenuModel which is used for rAc, amongst many others

> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
> > it does look like an applicable thing indeed. Is it supported on all
platforms
> > including Mac and Android? It looks like longer term work, and I'm not sure
it
> > should block this change, since the goal is really to get views-related
stuff
> > out of the PopupController and starting to make a better boundary. Let me
know
> > what you think.
> 
> The thing is, I don't know if this change is a step in the right direction. It
> may be an improvement over the current state of the world, but if the goal is
to
> reuse menu code, does this get us any closer? If not I think it may be more
> prudent to spend our time (coding, reviewing) on the menu approach and not
spend
> whatever additional effort it would take to land this.

I realize now that the SimpleMenuModel is obviously platform agnostic because
it's the Model, so it could replace a lot of code in the _controller_ (it deals
with accepting commands, adding or removing items), but it still looks like the
view implementation is still very platform specific (rAc's views implementation
uses a views::MenuRunner for example[1]). So this would then be a overhaul of
the controller _and_ the view code, which could potentially be done separately
(have the current view code use a MenuModel, and then switch the view to
MenuRunner). This CL goes in that direction by making a better view/model
separation so I don't think it's wasted work, really. 

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[Mathieu, 2016-01-09 01:36:51]

On 2016/01/09 01:14:40, Mathieu Perreault wrote:
> On 2016/01/09 01:01:58, Evan Stade wrote:
> > On 2016/01/09 00:53:17, Mathieu Perreault wrote:
> > > Thanks
> > > 
> > >
> >
>
https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
> > > File chrome/browser/ui/autofill/autofill_popup_view.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
> > > chrome/browser/ui/autofill/autofill_popup_view.h:66: int
> > GetDesiredPopupHeight()
> > > const;
> > > On 2016/01/09 00:19:02, Evan Stade wrote:
> > > > This design uses multiple inheritance, which is not permitted in Chrome.
> > > > (AutofillPopupViewViews extends both AutofillPopupView and
> > > > AutofillPopupBaseView, both of which contain implementation as opposed
to
> > > being
> > > > pure interfaces.) You can move all this to a separate class and use
> > > composition
> > > > instead.
> > > > 
> > > > That said, the composition solution is still not great because it leaves
> us
> > > with
> > > > a super ad-hoc looking UI. Ideally, we'd reuse menus, so this would look
> > more
> > > > like native menus (e.g. bookmark folder dropdown, context menu, etc.),
and
> > > > luckily, there are already cross-platform menu models and
> platform-specific
> > > menu
> > > > view implementations. On my system, the spacing, the font colors, font
> > sizing,
> > > > the highlight color, and other minor details like that are all different
> > > between
> > > > the autofill dropdown and a normal menu, and I don't think there's any
> good
> > > > reason for that, because it's all close enough that it seems like
Autofill
> > is
> > > > mimicking menus, poorly. It's a large part of why the Autofill popup
looks
> > > janky
> > > > and old.
> > > > 
> > > > Granted, the existing menu code may not already do everything you want
it
> > to,
> > > so
> > > > it might need to be extended. I'm unsure how much work that would be.
> > However,
> > > I
> > > > think the final result would be fewer lines of code and better visual
> > > alignment
> > > > with the rest of the platform.
> > > 
> > > In your mind, would AutofillPopupViewViews extend AutofillPopupBaseView
and
> > have
> > > a member of class say AutofillPopupViewImpl, which would contain all
bounds
> > > related code which I've added here? 
> > 
> > yes, although I wouldn't call the owned class that. Maybe PopupViewModel?
> > There's not really a great name for it because it shouldn't actually exist.
> > 
> > > 
> > > RE menus: I found SimpleMenuModel which is used for rAc, amongst many
others
> 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
> > > it does look like an applicable thing indeed. Is it supported on all
> platforms
> > > including Mac and Android? It looks like longer term work, and I'm not
sure
> it
> > > should block this change, since the goal is really to get views-related
> stuff
> > > out of the PopupController and starting to make a better boundary. Let me
> know
> > > what you think.
> > 
> > The thing is, I don't know if this change is a step in the right direction.
It
> > may be an improvement over the current state of the world, but if the goal
is
> to
> > reuse menu code, does this get us any closer? If not I think it may be more
> > prudent to spend our time (coding, reviewing) on the menu approach and not
> spend
> > whatever additional effort it would take to land this.
> 
> I realize now that the SimpleMenuModel is obviously platform agnostic because
> it's the Model, so it could replace a lot of code in the _controller_ (it
deals
> with accepting commands, adding or removing items), but it still looks like
the
> view implementation is still very platform specific (rAc's views
implementation
> uses a views::MenuRunner for example[1]). So this would then be a overhaul of
> the controller _and_ the view code, which could potentially be done separately
> (have the current view code use a MenuModel, and then switch the view to
> MenuRunner). This CL goes in that direction by making a better view/model
> separation so I don't think it's wasted work, really. 
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Note that what I moved here is the code that all views share in common (where
should the popup be situated on the page in popup_view_utils, what is the
width,height of the popup dictated by the padding inside the rows, etc). Even
with an overhaul of all the view implementations, this code will probably
survive in some form. It had no place in the controller (as you rightfully
pointed out and by doing so prompted this work). I don't think it should be
called PopupViewModel. Perhaps PopupViewBounds is probably closer to what this
code does.

[Mathieu, 2016-01-09 15:34:35]

On 2016/01/09 01:36:51, Mathieu Perreault wrote:
> On 2016/01/09 01:14:40, Mathieu Perreault wrote:
> > On 2016/01/09 01:01:58, Evan Stade wrote:
> > > On 2016/01/09 00:53:17, Mathieu Perreault wrote:
> > > > Thanks
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
> > > > File chrome/browser/ui/autofill/autofill_popup_view.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
> > > > chrome/browser/ui/autofill/autofill_popup_view.h:66: int
> > > GetDesiredPopupHeight()
> > > > const;
> > > > On 2016/01/09 00:19:02, Evan Stade wrote:
> > > > > This design uses multiple inheritance, which is not permitted in
Chrome.
> > > > > (AutofillPopupViewViews extends both AutofillPopupView and
> > > > > AutofillPopupBaseView, both of which contain implementation as opposed
> to
> > > > being
> > > > > pure interfaces.) You can move all this to a separate class and use
> > > > composition
> > > > > instead.
> > > > > 
> > > > > That said, the composition solution is still not great because it
leaves
> > us
> > > > with
> > > > > a super ad-hoc looking UI. Ideally, we'd reuse menus, so this would
look
> > > more
> > > > > like native menus (e.g. bookmark folder dropdown, context menu, etc.),
> and
> > > > > luckily, there are already cross-platform menu models and
> > platform-specific
> > > > menu
> > > > > view implementations. On my system, the spacing, the font colors, font
> > > sizing,
> > > > > the highlight color, and other minor details like that are all
different
> > > > between
> > > > > the autofill dropdown and a normal menu, and I don't think there's any
> > good
> > > > > reason for that, because it's all close enough that it seems like
> Autofill
> > > is
> > > > > mimicking menus, poorly. It's a large part of why the Autofill popup
> looks
> > > > janky
> > > > > and old.
> > > > > 
> > > > > Granted, the existing menu code may not already do everything you want
> it
> > > to,
> > > > so
> > > > > it might need to be extended. I'm unsure how much work that would be.
> > > However,
> > > > I
> > > > > think the final result would be fewer lines of code and better visual
> > > > alignment
> > > > > with the rest of the platform.
> > > > 
> > > > In your mind, would AutofillPopupViewViews extend AutofillPopupBaseView
> and
> > > have
> > > > a member of class say AutofillPopupViewImpl, which would contain all
> bounds
> > > > related code which I've added here? 
> > > 
> > > yes, although I wouldn't call the owned class that. Maybe PopupViewModel?
> > > There's not really a great name for it because it shouldn't actually
exist.
> > > 
> > > > 
> > > > RE menus: I found SimpleMenuModel which is used for rAc, amongst many
> others
> > 
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
> > > > it does look like an applicable thing indeed. Is it supported on all
> > platforms
> > > > including Mac and Android? It looks like longer term work, and I'm not
> sure
> > it
> > > > should block this change, since the goal is really to get views-related
> > stuff
> > > > out of the PopupController and starting to make a better boundary. Let
me
> > know
> > > > what you think.
> > > 
> > > The thing is, I don't know if this change is a step in the right
direction.
> It
> > > may be an improvement over the current state of the world, but if the goal
> is
> > to
> > > reuse menu code, does this get us any closer? If not I think it may be
more
> > > prudent to spend our time (coding, reviewing) on the menu approach and not
> > spend
> > > whatever additional effort it would take to land this.
> > 
> > I realize now that the SimpleMenuModel is obviously platform agnostic
because
> > it's the Model, so it could replace a lot of code in the _controller_ (it
> deals
> > with accepting commands, adding or removing items), but it still looks like
> the
> > view implementation is still very platform specific (rAc's views
> implementation
> > uses a views::MenuRunner for example[1]). So this would then be a overhaul
of
> > the controller _and_ the view code, which could potentially be done
separately
> > (have the current view code use a MenuModel, and then switch the view to
> > MenuRunner). This CL goes in that direction by making a better view/model
> > separation so I don't think it's wasted work, really. 
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> Note that what I moved here is the code that all views share in common (where
> should the popup be situated on the page in popup_view_utils, what is the
> width,height of the popup dictated by the padding inside the rows, etc). Even
> with an overhaul of all the view implementations, this code will probably
> survive in some form. It had no place in the controller (as you rightfully
> pointed out and by doing so prompted this work). I don't think it should be
> called PopupViewModel. Perhaps PopupViewBounds is probably closer to what this
> code does.

Actually thinking about this more, in their current implementation it makes
sense for all the different views (android, mac, views) to share a common base
class, AutofillPopupView. While it's unfortunate that multiple inheritance is
introduced (only in the case of AutofillPopupViewViews because password
generation needs AutofillPopupBaseView), it is "discouraged but permitted" in
the Chromium style guide. I think this change needs to be reviewed, and will put
us in a better situation regarding the controller/view separation.

[Mathieu, 2016-01-11 16:36:15]

Discussed with colleagues and removed the multiple inheritance.
AutofillPopupView is back to being a pure interface.

[Mathieu, 2016-01-11 16:51:43]

Patchset #6 (id:270001) has been deleted

[Mathieu, 2016-01-11 16:58:54]

Patchset #6 (id:290001) has been deleted

[Mathieu, 2016-01-11 17:27:11]

Description was changed from

==========
[Autofill] Move functions from the AutofillPopupController to AutofillPopupView

The AutofillPopupView (now a base class) should be the one handling bounds and
related calculations. The controller should only provide the data model and tell
the view when to redraw.

This is functionality neutral, although quite a few tests are shuffled around.

For Mac, a delegate interface is introduced so that the Cocoa view can call into
the base class.

BUG=575802
TEST=Existing
==========

to

==========
[Autofill] Move functions from the AutofillPopupController to AutofillPopupView

The AutofillPopupView (through a helper class) should be the one handling bounds
and related calculations. The controller should only provide the data model and
tell the view when to redraw.

This is functionality neutral, although quite a few tests are shuffled around.

For Mac, a delegate interface is introduced so that the Cocoa view can call into
the base class.

BUG=575802
TEST=Existing
==========

[Mathieu, 2016-01-11 18:33:11]

https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.h (right):

https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_controller_impl.h:80: 
On 2016/01/09 00:19:02, Evan Stade wrote:
> ?

Done.

https://codereview.chromium.org/1570783003/diff/210001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_controller_impl.h:86: #if
!defined(OS_ANDROID)
On 2016/01/09 00:19:02, Evan Stade wrote:
> this will need to be !BUILDFLAG(ANDROID_JAVA_UI)

I would rather punt this to a subsequent CL because if it ever needs to be
reverted, it is separate from the functionality neutral refactor.

[Mathieu, 2016-01-11 19:54:53]

Description was changed from

==========
[Autofill] Move functions from the AutofillPopupController to AutofillPopupView

The AutofillPopupView (through a helper class) should be the one handling bounds
and related calculations. The controller should only provide the data model and
tell the view when to redraw.

This is functionality neutral, although quite a few tests are shuffled around.

For Mac, a delegate interface is introduced so that the Cocoa view can call into
the base class.

BUG=575802
TEST=Existing
==========

to

==========
[Autofill] Move functions from the AutofillPopupController to AutofillPopupView

The AutofillPopupView (through a helper class) should be the one handling bounds
and related calculations. The controller should only provide the data model and
tell the view when to redraw.

This is functionality neutral, although quite a few tests are shuffled around.

For Mac, a delegate interface is introduced so that the Cocoa view can call into
the view helper.

BUG=575802
TEST=Existing
==========

[Mathieu, 2016-01-11 20:43:44]

Patchset #6 (id:310001) has been deleted

[Mathieu, 2016-01-11 20:46:33]

mathp@chromium.org changed reviewers:
+ isherman@chromium.org, tedchoc@chromium.org

[Mathieu, 2016-01-11 20:46:34]

+tedchoc@ for android
+isherman@ for ui/cocoa/autofill and general opinion

[Mathieu, 2016-01-11 21:02:19]

Patchset #5 (id:250001) has been deleted

[Ilya Sherman, 2016-01-11 21:06:45]

This seems ok to me in spirit, though I think the actual class names and
hierarchies are a little confusing.  I generally defer to Evan for UI-heavy
reviews, as he is far more experienced in this area than I am.

https://codereview.chromium.org/1570783003/diff/350001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_view_helper.h (right):

https://codereview.chromium.org/1570783003/diff/350001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view_helper.h:28: class
AutofillPopupViewHelper {
nit: "helper" is a pretty vague name.  How does the helper differ from the
delegate?  Perhaps there are class names that could clarify the difference?

https://codereview.chromium.org/1570783003/diff/350001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h (right):

https://codereview.chromium.org/1570783003/diff/350001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:42: public
AutofillPopupViewBridgeDelegate {
The bridge is its own delegate?  The naming of this class + interface confuses
me.

https://codereview.chromium.org/1570783003/diff/350001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:69:
scoped_ptr<AutofillPopupViewHelper> view_helper_;
nit: Can you omit the scoped_ptr wrapper?

[Evan Stade, 2016-01-11 23:09:38]

On 2016/01/09 01:14:40, Mathieu Perreault wrote:
> view implementation is still very platform specific (rAc's views
implementation
> uses a views::MenuRunner for example[1]).

That's like 4 lines of platform code, it's basically just a pass through to the
>>4 LOC in //ui/views/controls/menu, which is the tried and true (and consistent
looking) views code you'd be harnessing.

https://codereview.chromium.org/1570783003/diff/350001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_view_helper.cc (right):

https://codereview.chromium.org/1570783003/diff/350001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view_helper.cc:1: // Copyright (c)
2016 The Chromium Authors. All rights reserved.
when using menus, most or all of the code in this file would just disappear,
wouldn't it? I'm wondering why it's worth spending effort moving around code
that's going to be deleted. Even if it's in a better place now... the best place
is /dev/null ;)

If I understood you correctly, you said this code would survive in some form or
another, but I don't think these functions will exist:

GetDesiredPopup{Height,Width}
RowWidthWithoutText
GetAvailableWidthForRow
LineFromY
GetRowBounds

I think what you should do to make progress (and even convince me I'm wrong) is
to start working on the menu implementation and showing that it does actually
build on this change rather than taking off in a different direction. Heck,
there might be some reason why using menus just won't work or be worth it that
you'll uncover, and if that's the case it has bearing on this CL.

[Evan Stade, 2016-01-13 04:30:27]

thanks for looking into the menu idea. Indeed with the UI updates scheduled for
the near future, that seems like a square peg/round hole. It might still be nice
to attempt to reuse some of the constants in MenuConfig[1] which are defined
differently per platform in order to get a more natural appearance. You should
also strongly consider using NativeTheme for colors[2]. Both of these
suggestions are intended as follow ups to this CL.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/native_theme/na...

https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:275:
SetSelectedLine(view_->LineFromY(point.y()));
so in Model-View-Controller (or Model-View-Presenter, which is actually more
like what Chrome uses, although it uses the MVC nomenclature), the controller
owns or at least has access to the model. The model (here termed "helper")
shouldn't be owned by the view, rather the view should ask the controller (aka
delegate_ in this case) for the model. Then you wouldn't need all these new
thunks on AutofillPopupView.

https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_view.h (right):

https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view.h:35: static const size_t
kNamePadding = 15;
is there a reason to keep these constants here and not put them in the view
helper?

https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_view_helper.h (right):

https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view_helper.h:79:
autofill::view_utils::PopupViewUtils view_utils_;
utils is usually used as a name for namespaces, not classes. Why is this a
different class and not just part of this one?

[Mathieu, 2016-01-13 17:04:27]

Thanks, PTAL

https://codereview.chromium.org/1570783003/diff/350001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h (right):

https://codereview.chromium.org/1570783003/diff/350001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:42: public
AutofillPopupViewBridgeDelegate {
On 2016/01/11 21:06:45, Ilya Sherman wrote:
> The bridge is its own delegate?  The naming of this class + interface confuses
> me.

Yes, it's rather a AutofillPopupCocoaViewDelegate (it's a way for the cocoa view
to call into the bridge).

https://codereview.chromium.org/1570783003/diff/350001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:69:
scoped_ptr<AutofillPopupViewHelper> view_helper_;
On 2016/01/11 21:06:45, Ilya Sherman wrote:
> nit: Can you omit the scoped_ptr wrapper?

Done.

https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:275:
SetSelectedLine(view_->LineFromY(point.y()));
On 2016/01/13 04:30:27, Evan Stade wrote:
> so in Model-View-Controller (or Model-View-Presenter, which is actually more
> like what Chrome uses, although it uses the MVC nomenclature), the controller
> owns or at least has access to the model. The model (here termed "helper")
> shouldn't be owned by the view, rather the view should ask the controller (aka
> delegate_ in this case) for the model. Then you wouldn't need all these new
> thunks on AutofillPopupView.

I think we should move towards having the PopupController just holding on to the
Suggestions (what I call the model) and dealing with user actions (select,
remove, etc.).

The ViewHelper I've added here is holding the view's state: popup bounds, incl.
width, height, location of the view. As such I believe the view should own its
internal state and have the ViewHelper as a member.

I would move towards enforcing that AutofillPopupView is the interface through
which PopupController can tell it to redraw, or question which line corresponds
to point Y for example. AutofillPopupViewDelegate can be how the view can get
the suggestions (model) from the controller, or tell the controller something
happened on the view.

Further refactors would go towards the goal of view implementations having a
pointer to the PopupViewDelegate instead of the controller.

Accessors like popup_bounds shouldn't be on the controller either (to be
refactored as well).

So in essence, I think the goal of this refactor is to have the view own more of
its internal state, so I would suggest leaving the ViewHelper as a member of the
view.

https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_view.h (right):

https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view.h:35: static const size_t
kNamePadding = 15;
On 2016/01/13 04:30:27, Evan Stade wrote:
> is there a reason to keep these constants here and not put them in the view
> helper?

Moved to helper, removed kDeleteIcon* constants

https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_view_helper.h (right):

https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view_helper.h:79:
autofill::view_utils::PopupViewUtils view_utils_;
On 2016/01/13 04:30:27, Evan Stade wrote:
> utils is usually used as a name for namespaces, not classes. Why is this a
> different class and not just part of this one?

It is used in password_generation_controller_impl, which doesn't depend on
AutofillPopupViewHelper. They would probably also benefit from refactoring. I
will bring it up with them.

In the meantime I've renamed this to PopupViewCommon and brought it back to the
autofill namespace.

[Evan Stade, 2016-01-13 18:18:57]

https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:275:
SetSelectedLine(view_->LineFromY(point.y()));
> I would move towards enforcing that AutofillPopupView is the interface through
> which PopupController can tell it to redraw, or question which line
corresponds
> to point Y for example. AutofillPopupViewDelegate can be how the view can get
> the suggestions (model) from the controller, or tell the controller something
> happened on the view.

To me it's a code smell when you have to add a lot of pass-throughs. Right now
the call chain is

view -> controller (SetSelectedLineAtPoint)
controller -> view (LineFromY)
view -> view helper (LineFromY)

One way to fix this would be to change SetSelectedLineAtPoint to SetSelectedLine
(by index). If you can fix all the thunks this way, groovy.

[Mathieu, 2016-01-13 19:26:38]

On 2016/01/13 18:18:57, Evan Stade wrote:
>
https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
> File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):
> 
>
https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
> chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:275:
> SetSelectedLine(view_->LineFromY(point.y()));
> > I would move towards enforcing that AutofillPopupView is the interface
through
> > which PopupController can tell it to redraw, or question which line
> corresponds
> > to point Y for example. AutofillPopupViewDelegate can be how the view can
get
> > the suggestions (model) from the controller, or tell the controller
something
> > happened on the view.
> 
> To me it's a code smell when you have to add a lot of pass-throughs. Right now
> the call chain is
> 
> view -> controller (SetSelectedLineAtPoint)
> controller -> view (LineFromY)
> view -> view helper (LineFromY)
> 
> One way to fix this would be to change SetSelectedLineAtPoint to
SetSelectedLine
> (by index). If you can fix all the thunks this way, groovy.

Yes, agree about the code smell, the controller is still too involved in my
opinion. Regarding SetSelectedLine, I've started on the change (based on this
change, https://codereview.chromium.org/1588663002/), and since it's not
functionality neutral (and may need a similar refactor on the
password_generation_popup_controller_impl), I would rather punt to a subsequent
CL.

[Mathieu, 2016-01-13 19:28:04]

On 2016/01/13 19:26:38, Mathieu Perreault wrote:
> On 2016/01/13 18:18:57, Evan Stade wrote:
> >
>
https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
> > File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1570783003/diff/370001/chrome/browser/ui/auto...
> > chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:275:
> > SetSelectedLine(view_->LineFromY(point.y()));
> > > I would move towards enforcing that AutofillPopupView is the interface
> through
> > > which PopupController can tell it to redraw, or question which line
> > corresponds
> > > to point Y for example. AutofillPopupViewDelegate can be how the view can
> get
> > > the suggestions (model) from the controller, or tell the controller
> something
> > > happened on the view.
> > 
> > To me it's a code smell when you have to add a lot of pass-throughs. Right
now
> > the call chain is
> > 
> > view -> controller (SetSelectedLineAtPoint)
> > controller -> view (LineFromY)
> > view -> view helper (LineFromY)
> > 
> > One way to fix this would be to change SetSelectedLineAtPoint to
> SetSelectedLine
> > (by index). If you can fix all the thunks this way, groovy.
> 
> Yes, agree about the code smell, the controller is still too involved in my
> opinion. Regarding SetSelectedLine, I've started on the change (based on this
> change, https://codereview.chromium.org/1588663002/), and since it's not
> functionality neutral (and may need a similar refactor on the
> password_generation_popup_controller_impl), I would rather punt to a
subsequent
> CL.

^^ Same with other cases where the controller is involved. I've found that the
password generation stuff gets in the way, at least in the views code. I would
rather take them one by one and thoroughly test for regressions with the
password team.

[Evan Stade, 2016-01-14 03:01:58]

I said "If you can fix all the thunks this way, groovy." but you showed that it
would be hard to do so because of the entanglement with passwords. So now I fall
back on my initial suggestion of making the controller own the view helper (and
provide an accessor to it for the view, as needed). You have four (!) identical
thunks for LineFromY. That's not good.

[Mathieu, 2016-01-14 20:12:46]

Description was changed from

==========
[Autofill] Move functions from the AutofillPopupController to AutofillPopupView

The AutofillPopupView (through a helper class) should be the one handling bounds
and related calculations. The controller should only provide the data model and
tell the view when to redraw.

This is functionality neutral, although quite a few tests are shuffled around.

For Mac, a delegate interface is introduced so that the Cocoa view can call into
the view helper.

BUG=575802
TEST=Existing
==========

to

==========
[Autofill] Move functions from the AutofillPopupController to
AutofillPopupViewHelper

The controller should only provide the data model and tell the view when to
redraw.

This is functionality neutral, although quite a few tests are shuffled around.

For Mac, a delegate interface is introduced so that the Cocoa view can call into
the view helper.

BUG=575802
TEST=Existing
==========

[Mathieu, 2016-01-14 20:21:04]

Patchset #10 (id:430001) has been deleted

[Mathieu, 2016-01-14 20:33:55]

Patchset #10 (id:450001) has been deleted

[Mathieu, 2016-01-14 22:42:00]

Patchset #10 (id:470001) has been deleted

[Mathieu, 2016-01-14 23:09:35]

On 2016/01/14 03:01:58, Evan Stade (ooo till Tue 1.19) wrote:
> I said "If you can fix all the thunks this way, groovy." but you showed that
it
> would be hard to do so because of the entanglement with passwords. So now I
fall
> back on my initial suggestion of making the controller own the view helper
(and
> provide an accessor to it for the view, as needed). You have four (!)
identical
> thunks for LineFromY. That's not good.

Yes, I agree it's not ideal. As you mention in the other issue, the autofill and
password_generation should probably be decoupled to some extent at some point. 

So, I've put the helper as a member of the controller, with the explicit TODO
that it should go in the view one day, gated on some refactor of the
password_generation and autofill code. I will work with the password team to
improve the situation.

[Evan Stade, 2016-01-20 04:51:17]

much nicer!

https://codereview.chromium.org/1570783003/diff/490001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_controller.h (right):

https://codereview.chromium.org/1570783003/diff/490001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_controller.h:69: virtual
AutofillPopupViewHelper* view_helper() = 0;
can this be const and return const ref?

https://codereview.chromium.org/1570783003/diff/490001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_view_helper.h (right):

https://codereview.chromium.org/1570783003/diff/490001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view_helper.h:28: // TODO(mathp): This
should ideally be owned by the view, and not the popup
this comment is a bit long, I'd leave it at the generic:

TODO(mathp): investigate moving ownership of this class to the view.

https://codereview.chromium.org/1570783003/diff/490001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view_helper.h:33: class
AutofillPopupViewHelper {
can we call this something like LayoutModel instead of ViewHelper? There can be
multiple models in MVC. This one happens to handle layout.

https://codereview.chromium.org/1570783003/diff/490001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view_helper.h:70: int LineFromY(int
y);
could this be const?

https://codereview.chromium.org/1570783003/diff/490001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view_helper.h:76: gfx::Rect
GetRowBounds(size_t index);
could this be const?

[Mathieu, 2016-01-20 16:07:41]

Patchset #11 (id:510001) has been deleted

[Mathieu, 2016-01-20 16:40:12]

Patchset #11 (id:530001) has been deleted

[Mathieu, 2016-01-20 17:57:44]

Thanks, PTAL

https://codereview.chromium.org/1570783003/diff/490001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_controller.h (right):

https://codereview.chromium.org/1570783003/diff/490001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_controller.h:69: virtual
AutofillPopupViewHelper* view_helper() = 0;
On 2016/01/20 04:51:17, Evan Stade wrote:
> can this be const and return const ref?

Done.

https://codereview.chromium.org/1570783003/diff/490001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_view_helper.h (right):

https://codereview.chromium.org/1570783003/diff/490001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view_helper.h:28: // TODO(mathp): This
should ideally be owned by the view, and not the popup
On 2016/01/20 04:51:17, Evan Stade wrote:
> this comment is a bit long, I'd leave it at the generic:
> 
> TODO(mathp): investigate moving ownership of this class to the view.

Done.

https://codereview.chromium.org/1570783003/diff/490001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view_helper.h:33: class
AutofillPopupViewHelper {
On 2016/01/20 04:51:17, Evan Stade wrote:
> can we call this something like LayoutModel instead of ViewHelper? There can
be
> multiple models in MVC. This one happens to handle layout.

Sure I like the name

https://codereview.chromium.org/1570783003/diff/490001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view_helper.h:70: int LineFromY(int
y);
On 2016/01/20 04:51:17, Evan Stade wrote:
> could this be const?

Done.

https://codereview.chromium.org/1570783003/diff/490001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_view_helper.h:76: gfx::Rect
GetRowBounds(size_t index);
On 2016/01/20 04:51:17, Evan Stade wrote:
> could this be const?

Done.

[Evan Stade, 2016-01-20 19:15:49]

lgtm! you'll need more owners (for cocoa at least)

https://codereview.chromium.org/1570783003/diff/550001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right):

https://codereview.chromium.org/1570783003/diff/550001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_layout_model.h:35: static const size_t
kNamePadding = 15;
why size_t when the others are int? Normally we use int for padding.

[Mathieu, 2016-01-20 19:27:05]

Thanks!

Ilya: please review cocoa/
Ted: android please

https://codereview.chromium.org/1570783003/diff/550001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right):

https://codereview.chromium.org/1570783003/diff/550001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_layout_model.h:35: static const size_t
kNamePadding = 15;
On 2016/01/20 19:15:49, Evan Stade wrote:
> why size_t when the others are int? Normally we use int for padding.

Done.

[Ilya Sherman, 2016-01-20 23:25:18]

https://codereview.chromium.org/1570783003/diff/570001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h (right):

https://codereview.chromium.org/1570783003/diff/570001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:18:
autofill::AutofillPopupViewDelegate* popup_delegate_;  // weak
Why did you change this name?

https://codereview.chromium.org/1570783003/diff/570001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h (right):

https://codereview.chromium.org/1570783003/diff/570001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:44: //
AutofillPopupViewBridgeDelegate implementation.
nit: "AutofillPopupViewCocoaDelegate:"?

[Ilya Sherman, 2016-01-20 23:26:12]

By the way, please update the CL description to match its current state =)

[Mathieu, 2016-01-21 01:19:16]

Description was changed from

==========
[Autofill] Move functions from the AutofillPopupController to
AutofillPopupViewHelper

The controller should only provide the data model and tell the view when to
redraw.

This is functionality neutral, although quite a few tests are shuffled around.

For Mac, a delegate interface is introduced so that the Cocoa view can call into
the view helper.

BUG=575802
TEST=Existing
==========

to

==========
[Autofill] Move functions from the AutofillPopupController to
AutofillPopupLayoutModel

The controller should only provide the data model and tell the view when to
redraw.

This is functionality neutral, although quite a few tests are shuffled around.

For Mac, a delegate interface is introduced so that the Cocoa view can call into
the view layout model.

BUG=575802
TEST=Existing
==========

[Mathieu, 2016-01-21 01:25:08]

Thanks Ilya!

https://codereview.chromium.org/1570783003/diff/570001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h (right):

https://codereview.chromium.org/1570783003/diff/570001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:18:
autofill::AutofillPopupViewDelegate* popup_delegate_;  // weak
On 2016/01/20 23:25:18, Ilya Sherman wrote:
> Why did you change this name?

It conflicts with the |delegate_| in AutofillPopupViewCocoa which I thought was
slightly more deserving of the name (admittedly mostly for arbitrary reasons!).

https://codereview.chromium.org/1570783003/diff/570001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h (right):

https://codereview.chromium.org/1570783003/diff/570001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:44: //
AutofillPopupViewBridgeDelegate implementation.
On 2016/01/20 23:25:18, Ilya Sherman wrote:
> nit: "AutofillPopupViewCocoaDelegate:"?

Done.

[Ilya Sherman, 2016-01-21 01:27:03]

https://codereview.chromium.org/1570783003/diff/570001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h (right):

https://codereview.chromium.org/1570783003/diff/570001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:18:
autofill::AutofillPopupViewDelegate* popup_delegate_;  // weak
On 2016/01/21 01:25:08, Mathieu Perreault wrote:
> On 2016/01/20 23:25:18, Ilya Sherman wrote:
> > Why did you change this name?
> 
> It conflicts with the |delegate_| in AutofillPopupViewCocoa which I thought
was
> slightly more deserving of the name (admittedly mostly for arbitrary
reasons!). 

How does it conflict, exactly?  That variable is in a different class.  Is this
an Objective-C quirk?

[Mathieu, 2016-01-21 01:30:24]

On 2016/01/21 01:27:03, Ilya Sherman wrote:
>
https://codereview.chromium.org/1570783003/diff/570001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h
(right):
> 
>
https://codereview.chromium.org/1570783003/diff/570001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:18:
> autofill::AutofillPopupViewDelegate* popup_delegate_;  // weak
> On 2016/01/21 01:25:08, Mathieu Perreault wrote:
> > On 2016/01/20 23:25:18, Ilya Sherman wrote:
> > > Why did you change this name?
> > 
> > It conflicts with the |delegate_| in AutofillPopupViewCocoa which I thought
> was
> > slightly more deserving of the name (admittedly mostly for arbitrary
> reasons!). 
> 
> How does it conflict, exactly?  That variable is in a different class.  Is
this
> an Objective-C quirk?

AutofillPopupViewCocoa subclasses AutofilPopupBaseViewCocoa and the compiler is
evidently not at ease with two members being named the same.

[Ilya Sherman, 2016-01-21 01:32:04]

On 2016/01/21 01:30:24, Mathieu Perreault wrote:
> On 2016/01/21 01:27:03, Ilya Sherman wrote:
> >
>
https://codereview.chromium.org/1570783003/diff/570001/chrome/browser/ui/coco...
> > File chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h
> (right):
> > 
> >
>
https://codereview.chromium.org/1570783003/diff/570001/chrome/browser/ui/coco...
> > chrome/browser/ui/cocoa/autofill/autofill_popup_base_view_cocoa.h:18:
> > autofill::AutofillPopupViewDelegate* popup_delegate_;  // weak
> > On 2016/01/21 01:25:08, Mathieu Perreault wrote:
> > > On 2016/01/20 23:25:18, Ilya Sherman wrote:
> > > > Why did you change this name?
> > > 
> > > It conflicts with the |delegate_| in AutofillPopupViewCocoa which I
thought
> > was
> > > slightly more deserving of the name (admittedly mostly for arbitrary
> > reasons!). 
> > 
> > How does it conflict, exactly?  That variable is in a different class.  Is
> this
> > an Objective-C quirk?
> 
> AutofillPopupViewCocoa subclasses AutofilPopupBaseViewCocoa and the compiler
is
> evidently not at ease with two members being named the same.

Lame.  Okay, fair enough.  LGTM =)

[Miguel Garcia, 2016-01-21 13:59:57]

miguelg@chromium.org changed reviewers:
+ miguelg@chromium.org

[Miguel Garcia, 2016-01-21 13:59:57]

lgmt for  chrome/browser/ui/*

[Miguel Garcia, 2016-01-21 14:00:08]

lgtm

[Mathieu, 2016-01-21 14:15:36]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-01-21 14:15:37]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1570783003/#ps590001
(title: "addressed nit")

[commit-bot: I haz the power, 2016-01-21 14:16:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1570783003/590001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1570783003/590001

[Mathieu, 2016-01-21 14:31:58]

mathp@chromium.org changed reviewers:
- tedchoc@chromium.org

[commit-bot: I haz the power, 2016-01-21 15:24:25]

Description was changed from

==========
[Autofill] Move functions from the AutofillPopupController to
AutofillPopupLayoutModel

The controller should only provide the data model and tell the view when to
redraw.

This is functionality neutral, although quite a few tests are shuffled around.

For Mac, a delegate interface is introduced so that the Cocoa view can call into
the view layout model.

BUG=575802
TEST=Existing
==========

to

==========
[Autofill] Move functions from the AutofillPopupController to
AutofillPopupLayoutModel

The controller should only provide the data model and tell the view when to
redraw.

This is functionality neutral, although quite a few tests are shuffled around.

For Mac, a delegate interface is introduced so that the Cocoa view can call into
the view layout model.

BUG=575802
TEST=Existing
==========

[commit-bot: I haz the power, 2016-01-21 15:24:26]

Committed patchset #13 (id:590001)

[commit-bot: I haz the power, 2016-01-21 15:26:13]

Description was changed from

==========
[Autofill] Move functions from the AutofillPopupController to
AutofillPopupLayoutModel

The controller should only provide the data model and tell the view when to
redraw.

This is functionality neutral, although quite a few tests are shuffled around.

For Mac, a delegate interface is introduced so that the Cocoa view can call into
the view layout model.

BUG=575802
TEST=Existing
==========

to

==========
[Autofill] Move functions from the AutofillPopupController to
AutofillPopupLayoutModel

The controller should only provide the data model and tell the view when to
redraw.

This is functionality neutral, although quite a few tests are shuffled around.

For Mac, a delegate interface is introduced so that the Cocoa view can call into
the view layout model.

BUG=575802
TEST=Existing

Committed: https://crrev.com/c338bb771c11d46e814176a1958ec26943d4f5c6
Cr-Commit-Position: refs/heads/master@{#370697}
==========

[commit-bot: I haz the power, 2016-01-21 15:26:14]

Patchset 13 (id:??) landed as
https://crrev.com/c338bb771c11d46e814176a1958ec26943d4f5c6
Cr-Commit-Position: refs/heads/master@{#370697}