Broke out layout metric information from ViewsDelegate to LayoutProvider

Broke out layout metric information from ViewsDelegate to ViewsLayoutDelegate

BUG=687349
TBR=msw@chromium.org

Review-Url: https://codereview.chromium.org/2758323002
Cr-Commit-Position: refs/heads/master@{#464448}
Committed: https://chromium.googlesource.com/chromium/src/+/4e8cac4101f13be9f86b455f06ec7b46fc707e93

[kylix_rd, 2017-03-20 17:17:55]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 17:18:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 18:44:08]

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

[commit-bot: I haz the power, 2017-03-20 18:44:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kylix_rd, 2017-03-21 18:34:01]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 18:35:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 19:44:44]

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

[commit-bot: I haz the power, 2017-03-21 19:44:45]

Dry run: This issue passed the CQ dry run.

[kylix_rd, 2017-03-22 18:33:14]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 18:34:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 19:10:20]

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

[commit-bot: I haz the power, 2017-03-22 19:10:21]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[kylix_rd, 2017-03-23 13:29:57]

Description was changed from

==========
Broke out layout metric information from ViewsDelegate to ViewsLayoutDelegate

BUG=687349
==========

to

==========
Broke out layout metric information from ViewsDelegate to ViewsLayoutDelegate

BUG=687349
==========

[kylix_rd, 2017-03-23 13:29:57]

kylixrd@chromium.org changed reviewers:
+ pkasting@chromium.org

[kylix_rd, 2017-03-23 13:39:35]

While this CL isn't complete, I'm pushing it anyway so I can start to get
feedback before I go too deep down the rabbit hole. There are already some
issues related to the timing of the initialization of the new
ViewsLayoutDelegate (and its descendants).

Things to do:

o Fix the instantiation issues (I have a plan)
o Move the functionality from the current LayoutDelegate over to
ChromeViewsLayoutDelegate.
o Clean up all the uses of LayoutDelegate in favor of the
ChromeViewsLayoutDelegate.

Keep in mind this is a work in progress, so there will be some cruft and some
incomplete areas. Also, some of the try-bots are failing due to the
aforementioned instantiation issues.

I'm not too crazy about how the enumeration is being "extended"... Declaring the
enumerations without the assignments and then using a translation array in the
implementation of the overrides would look cleaner.

[kylix_rd, 2017-03-23 16:51:52]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 16:52:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kylix_rd, 2017-03-23 16:52:50]

Patchset #1 (id:1) has been deleted

[kylix_rd, 2017-03-23 16:53:02]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-03-23 17:09:19]

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

[commit-bot: I haz the power, 2017-03-23 17:09:21]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[kylix_rd, 2017-03-23 20:55:54]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 20:56:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 21:22:08]

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

[commit-bot: I haz the power, 2017-03-23 21:22:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2017-03-24 00:16:38]

On 2017/03/23 13:39:35, kylix_rd wrote:
> I'm not too crazy about how the enumeration is being "extended"... Declaring
the
> enumerations without the assignments and then using a translation array in the
> implementation of the overrides would look cleaner.

I strongly suggest doing what Trent did with the Typography stuff, which itself
was modeled on the content/ enums that need extension like this.

Basically, use an enum rather than an enum class, and have the functions that
take these take ints.  If there's any risk of argument confusion (e.g. Foo(int
enum_x, int enum_y) is prone to swapping X and Y), then have X have a range of
theoretically-allowed values, define Y to start above that range, and DCHECK in
the code that the args are in the expected ranges.

We hashed around this for a while, and various alternate ideas.  There are clear
downsides to the lack of compile-time typechecking for this, but in the end they
were just outweighed by the readability/maintenance gain on not using enum
classes or the like.  And +1 to using the same pattern everywhere we're trying
to solve the same problem, even if that pattern is imperfect.

[Peter Kasting, 2017-03-24 00:19:08]

On 2017/03/24 00:16:38, Peter Kasting wrote:
> Basically, use an enum rather than an enum class, and have the functions that
> take these take ints.

(Maybe I should have clarified, but the followon here is that then, you can do
something like:

ViewsLayoutDelegate {
  enum X {
    A,
    B,
    LAST_X
  }
};

ChromeViewsLayoutDelegate {
  enum X {
    C = ViewsLayoutDelegate::LAST_X,
    D
  }
};

See [chrome_]typography.y for more.)

[Peter Kasting, 2017-03-24 00:45:22]

Didn't review most things, as the majority of this looks mechanical.  Just
looked at some of the core classes.

https://codereview.chromium.org/2758323002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate.cc:60: // ChromeViewsDelegate*
views_delegate = nullptr;
Remove entirely

https://codereview.chromium.org/2758323002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate.h:98: new
ChromeViewsLayoutDelegate());
Nit: = base::MakeUnique<ChromeViewsLayoutDelegate>()?

https://codereview.chromium.org/2758323002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:35: : new
ChromeViewsLayoutDelegate());
Seems like it'd be better to do something like:

  return static_cast<ChromeViewsLayoutDelegate*>(ViewsLayoutDelegate::Get());

This lets the actual object creation be handled in one place
(ViewsLayoutDelegate).  It assumes that that will actually create a
ChromeViewsLayoutDelegate, which seems like a good assumption for caller code
that is trying to use a ChromeViewsLayoutDelegate.

(In a world with RTTI we could do something like dynamic_cast this and then
DCHECK that it's non-null, but I don't think the sanity-checking is worth
building a non-RTTI equivalent piece of functionality.)

https://codereview.chromium.org/2758323002/diff/60001/ui/views/layout/views_l...
File ui/views/layout/views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/60001/ui/views/layout/views_l...
ui/views/layout/views_layout_delegate.cc:26:
ViewsDelegate::GetInstance()->set_views_layout_delegate(delegate);
If ViewsDelegate doesn't own this (see below comment), you don't need any of
this.  If it does, it seems like ViewsDelegate should create this object and
store it on itself (using a factory function to do so so it can create the right
type?), rather than having this class' constructor try to write itself into the
ViewsDelegate.

https://codereview.chromium.org/2758323002/diff/60001/ui/views/layout/views_l...
ui/views/layout/views_layout_delegate.cc:35: return layout_delegate_;
Either this object is owned by the ViewsDelegate, or it's not.

The easier method is that it's not.  Remove the scoped_ptr on the ViewsDelegate
and the getter/setter functions there, and implement this the same way
ViewsDelegate works today.

Otherwise, eliminate layout_delegate_, let ViewsDelegate manage that
exclusively, and implement this method (if it exists at all) as calling through
to the ViewsDelegate getter to return the authoritative value.

https://codereview.chromium.org/2758323002/diff/60001/ui/views/views_delegate.cc
File ui/views/views_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/60001/ui/views/views_delegate...
ui/views/views_delegate.cc:37: layout_delegate_.swap(layout_delegate);
Nit: Inline these function bodies in the header since these functions are named
unix_hacker()-style (or change the names, but it looks like inlining is
appropriate)

https://codereview.chromium.org/2758323002/diff/60001/ui/views/views_delegate.h
File ui/views/views_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/60001/ui/views/views_delegate...
ui/views/views_delegate.h:119: std::unique_ptr<ViewsLayoutDelegate>&
layout_delegate);
Seems like this should be passed by value instead of non-const ref.  See
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
.

[kylix_rd, 2017-03-24 13:48:17]

On 2017/03/24 00:16:38, Peter Kasting wrote:
> On 2017/03/23 13:39:35, kylix_rd wrote:
> > I'm not too crazy about how the enumeration is being "extended"... Declaring
> the
> > enumerations without the assignments and then using a translation array in
the
> > implementation of the overrides would look cleaner.
> 
> I strongly suggest doing what Trent did with the Typography stuff, which
itself
> was modeled on the content/ enums that need extension like this.
> 
> Basically, use an enum rather than an enum class, and have the functions that
> take these take ints.  If there's any risk of argument confusion (e.g. Foo(int
> enum_x, int enum_y) is prone to swapping X and Y), then have X have a range of
> theoretically-allowed values, define Y to start above that range, and DCHECK
in
> the code that the args are in the expected ranges.
> 
> We hashed around this for a while, and various alternate ideas.  There are
clear
> downsides to the lack of compile-time typechecking for this, but in the end
they
> were just outweighed by the readability/maintenance gain on not using enum
> classes or the like.  And +1 to using the same pattern everywhere we're trying
> to solve the same problem, even if that pattern is imperfect.

Yes, I think the syntax is ugly, however, I'm not sure we should be tossing type
safety for a prettier syntax.

[kylix_rd, 2017-03-24 14:07:16]

https://codereview.chromium.org/2758323002/diff/60001/ui/views/layout/views_l...
File ui/views/layout/views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/60001/ui/views/layout/views_l...
ui/views/layout/views_layout_delegate.cc:35: return layout_delegate_;
On 2017/03/24 00:45:21, Peter Kasting wrote:
> Either this object is owned by the ViewsDelegate, or it's not.
> 
> The easier method is that it's not.  Remove the scoped_ptr on the
ViewsDelegate
> and the getter/setter functions there, and implement this the same way
> ViewsDelegate works today.
> 
> Otherwise, eliminate layout_delegate_, let ViewsDelegate manage that
> exclusively, and implement this method (if it exists at all) as calling
through
> to the ViewsDelegate getter to return the authoritative value.

There is a bit of a chicken/egg problem here that isn't immediately obvious; On
some platforms (such as ChromeOS) a different ViewsDelegate descendant is used,
and the existing LayoutDelegate is created on-demand (lazy initialized). Now
that the LayoutDelegate features are being pushed down to the views layer, that
lazy initialization may not be appropriate.

If the code in the lower-level views layer, which only knows about the
ViewsLayoutDelegate, executes before code in the higher-level browser layer,
then an instance of ViewsLayoutDelegate will be constructed. Then, when the code
in the higher level layers touches the ChromeViewsLayoutDelegate, it will try
and instantiate an instance of that type. It won't supersede the
ViewsLayoutDelegate instance and the low-level views won't have the
harmony-enabled delegate.

I've located where the ChromeViewsDelegate is being explicitly constructed and
can certainly do the same for the ChromeViewsLayoutDelegate. However, for
ChromeOS, a different ViewsDelegate descendant is being constructed at the same
level as ui/views (ui/views/mus/aura_init.cc). I've not located an appropriate
location to construct a ChromeViewsLayoutDelegate instance up in the
chrome/browser/ui/views layer.

[kylix_rd, 2017-03-24 15:22:17]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 15:23:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kylix_rd, 2017-03-24 16:31:14]

On 2017/03/24 14:07:16, kylix_rd wrote:
>
https://codereview.chromium.org/2758323002/diff/60001/ui/views/layout/views_l...
> File ui/views/layout/views_layout_delegate.cc (right):
> 
>
https://codereview.chromium.org/2758323002/diff/60001/ui/views/layout/views_l...
> ui/views/layout/views_layout_delegate.cc:35: return layout_delegate_;
> On 2017/03/24 00:45:21, Peter Kasting wrote:
> > Either this object is owned by the ViewsDelegate, or it's not.
> > 
> > The easier method is that it's not.  Remove the scoped_ptr on the
> ViewsDelegate
> > and the getter/setter functions there, and implement this the same way
> > ViewsDelegate works today.
> > 
> > Otherwise, eliminate layout_delegate_, let ViewsDelegate manage that
> > exclusively, and implement this method (if it exists at all) as calling
> through
> > to the ViewsDelegate getter to return the authoritative value.
> 
> There is a bit of a chicken/egg problem here that isn't immediately obvious;
On
> some platforms (such as ChromeOS) a different ViewsDelegate descendant is
used,
> and the existing LayoutDelegate is created on-demand (lazy initialized). Now
> that the LayoutDelegate features are being pushed down to the views layer,
that
> lazy initialization may not be appropriate.
> 
> If the code in the lower-level views layer, which only knows about the
> ViewsLayoutDelegate, executes before code in the higher-level browser layer,
> then an instance of ViewsLayoutDelegate will be constructed. Then, when the
code
> in the higher level layers touches the ChromeViewsLayoutDelegate, it will try
> and instantiate an instance of that type. It won't supersede the
> ViewsLayoutDelegate instance and the low-level views won't have the
> harmony-enabled delegate.
> 
> I've located where the ChromeViewsDelegate is being explicitly constructed and
> can certainly do the same for the ChromeViewsLayoutDelegate. However, for
> ChromeOS, a different ViewsDelegate descendant is being constructed at the
same
> level as ui/views (ui/views/mus/aura_init.cc). I've not located an appropriate
> location to construct a ChromeViewsLayoutDelegate instance up in the
> chrome/browser/ui/views layer.

I think I have found the location to initialize the ChromeViewsLayoutDelegate
instance. There are still some issues related to some of the unit tests on
chromeos, but I should be able to get that one figured out soon.

[kylix_rd, 2017-03-24 16:40:15]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-03-24 16:54:33]

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

[commit-bot: I haz the power, 2017-03-24 16:54:34]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kylix_rd, 2017-03-24 17:01:56]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 17:02:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 17:42:04]

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

[commit-bot: I haz the power, 2017-03-24 17:42:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kylix_rd, 2017-03-24 18:18:56]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 18:20:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kylix_rd, 2017-03-24 18:31:38]

Patchset #4 (id:120001) has been deleted

[kylix_rd, 2017-03-24 18:32:25]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 18:34:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 18:50:52]

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

[commit-bot: I haz the power, 2017-03-24 18:50:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kylix_rd, 2017-03-24 18:54:28]

Patchset #4 (id:140001) has been deleted

[kylix_rd, 2017-03-24 18:55:49]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 18:56:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 19:41:07]

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

[commit-bot: I haz the power, 2017-03-24 19:41:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2017-03-24 19:58:43]

On 2017/03/24 13:48:17, kylix_rd wrote:
> On 2017/03/24 00:16:38, Peter Kasting wrote:
> > On 2017/03/23 13:39:35, kylix_rd wrote:
> > > I'm not too crazy about how the enumeration is being "extended"...
Declaring
> > the
> > > enumerations without the assignments and then using a translation array in
> the
> > > implementation of the overrides would look cleaner.
> > 
> > I strongly suggest doing what Trent did with the Typography stuff, which
> itself
> > was modeled on the content/ enums that need extension like this.
> > 
> > Basically, use an enum rather than an enum class, and have the functions
that
> > take these take ints.  If there's any risk of argument confusion (e.g.
Foo(int
> > enum_x, int enum_y) is prone to swapping X and Y), then have X have a range
of
> > theoretically-allowed values, define Y to start above that range, and DCHECK
> in
> > the code that the args are in the expected ranges.
> > 
> > We hashed around this for a while, and various alternate ideas.  There are
> clear
> > downsides to the lack of compile-time typechecking for this, but in the end
> they
> > were just outweighed by the readability/maintenance gain on not using enum
> > classes or the like.  And +1 to using the same pattern everywhere we're
trying
> > to solve the same problem, even if that pattern is imperfect.
> 
> Yes, I think the syntax is ugly, however, I'm not sure we should be tossing
type
> safety for a prettier syntax.

Right.  I hear you, and that's the same main concern Scott/Trent/I had when
discussing this previously.

One question would be whether there are factors about this case that are
different from the typography case (or the notification enums in content/),
which make the tradeoffs different.

If there aren't any, then basically I would say, you can try to convince us we
made the wrong call before and we should change the other instances of this, or
you can go with the solution we ended up on after talking about this.  The only
route I wouldn't go is to do one thing there and another thing here without
having agreement that there are really different tradeoffs in play.

[kylix_rd, 2017-03-24 20:17:44]

Patchset #4 (id:160001) has been deleted

[kylix_rd, 2017-03-24 20:19:30]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 20:20:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 21:39:30]

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

[commit-bot: I haz the power, 2017-03-24 21:39:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2017-03-24 22:51:14]

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc (right):

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc:40:
views_delegate_.set_layout_delegate(nullptr);
Why is this line needed?  Same comment elsewhere this pattern appears.

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:31: return
layout_delegate_;
It still seems like this could just be

  return static_cast<ChromeViewsLayoutDelegate*>(ViewsLayoutDelegate::Get());

The distinction is that if someone tried to call this method after having
created a ViewsLayoutDelegate (instead of a ChromeViewsLayoutDelegate), your
implementation would return null, while mine would trigger UB.  It doesn't seem
like that's worth tracking |layout_delegate_|, since code trying to call this
should have guaranteed it created the right kind of delegate already?

https://codereview.chromium.org/2758323002/diff/180001/ui/views/views_delegat...
File ui/views/views_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/180001/ui/views/views_delegat...
ui/views/views_delegate.cc:32: touch_selection_menu_runner_.reset(new
TouchSelectionMenuRunnerViews());
Nit: Prefer =MakeUnique to reset(new

(That said, I just realized you were moving this code rather than rewriting, so
whatever)

[kylix_rd, 2017-03-27 18:18:28]

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc (right):

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc:40:
views_delegate_.set_layout_delegate(nullptr);
On 2017/03/24 22:51:13, Peter Kasting wrote:
> Why is this line needed?  Same comment elsewhere this pattern appears.

Since these delegates are singletons and they set/clear the one global variable,
constructing a new instance while there may be one already constructed, (ie. the
default ViewsLayoutDelegate constructed by the views_delegate_ instance) would
cause the global to be set back to when the old delegate is destroyed. This
would lose the global delegate.

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:31: return
layout_delegate_;
On 2017/03/24 22:51:13, Peter Kasting wrote:
> It still seems like this could just be
> 
>   return static_cast<ChromeViewsLayoutDelegate*>(ViewsLayoutDelegate::Get());
> 
> The distinction is that if someone tried to call this method after having
> created a ViewsLayoutDelegate (instead of a ChromeViewsLayoutDelegate), your
> implementation would return null, while mine would trigger UB.  It doesn't
seem
> like that's worth tracking |layout_delegate_|, since code trying to call this
> should have guaranteed it created the right kind of delegate already?

I was also trying to avoid an unsafe downcast.

[Peter Kasting, 2017-03-27 20:06:48]

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc (right):

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc:40:
views_delegate_.set_layout_delegate(nullptr);
On 2017/03/27 18:18:27, kylix_rd wrote:
> On 2017/03/24 22:51:13, Peter Kasting wrote:
> > Why is this line needed?  Same comment elsewhere this pattern appears.
> 
> Since these delegates are singletons and they set/clear the one global
variable,
> constructing a new instance while there may be one already constructed, (ie.
the
> default ViewsLayoutDelegate constructed by the views_delegate_ instance) would
> cause the global to be set back to when the old delegate is destroyed. This
> would lose the global delegate.

This sounds like the API is easy to subtly use wrong.

It seems like either we should remove setters, and if you want to set a new
delegate, you do it by constructing one, so the code here would just construct a
ChromeViewsLayoutDelegate and that would auto-set the global; or, we should not
have constructing these delegates auto-set any globals, and everywhere that
makes one should do that manually if they want to update the global.  The first
route would be harder to misuse.

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:31: return
layout_delegate_;
On 2017/03/27 18:18:27, kylix_rd wrote:
> On 2017/03/24 22:51:13, Peter Kasting wrote:
> > It still seems like this could just be
> > 
> >   return
static_cast<ChromeViewsLayoutDelegate*>(ViewsLayoutDelegate::Get());
> > 
> > The distinction is that if someone tried to call this method after having
> > created a ViewsLayoutDelegate (instead of a ChromeViewsLayoutDelegate), your
> > implementation would return null, while mine would trigger UB.  It doesn't
> seem
> > like that's worth tracking |layout_delegate_|, since code trying to call
this
> > should have guaranteed it created the right kind of delegate already?
> 
> I was also trying to avoid an unsafe downcast.

Right, that's what I was saying I didn't think was worth the extra tracking
complexity, since we shouldn't get into that scenario, and if we do we don't
handle it today anyway.

[kylix_rd, 2017-03-28 17:03:22]

Patchset #5 (id:200001) has been deleted

[kylix_rd, 2017-03-28 17:55:41]

Patchset #5 (id:220001) has been deleted

[kylix_rd, 2017-03-28 19:07:45]

Patchset #5 (id:240001) has been deleted

[kylix_rd, 2017-03-29 17:54:13]

Patchset #6 (id:280001) has been deleted

[kylix_rd, 2017-03-29 18:12:21]

Patchset #6 (id:300001) has been deleted

[kylix_rd, 2017-03-29 18:56:04]

Here's the latest. It's now passing the ChromeOS try-bot. I've also gone ahead
and implemented the "last-constructed-wins" idea for the ViewsLayouDelegate and
descendant. Rather than a weak_ptr, it just keeps the same global reference.

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc (right):

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc:40:
views_delegate_.set_layout_delegate(nullptr);
On 2017/03/27 20:06:48, Peter Kasting wrote:
> On 2017/03/27 18:18:27, kylix_rd wrote:
> > On 2017/03/24 22:51:13, Peter Kasting wrote:
> > > Why is this line needed?  Same comment elsewhere this pattern appears.
> > 
> > Since these delegates are singletons and they set/clear the one global
> variable,
> > constructing a new instance while there may be one already constructed, (ie.
> the
> > default ViewsLayoutDelegate constructed by the views_delegate_ instance)
would
> > cause the global to be set back to when the old delegate is destroyed. This
> > would lose the global delegate.
> 
> This sounds like the API is easy to subtly use wrong.

Short of imposing more conditions on its use, I'm not sure of a better solution.
This is really only a problem with unit tests. In this instance, the
TestViewsDelegate only references ui/views, so constructing a corresponding
ViewsLayoutDelegate is the only one visible. Some tests don't need a
ChromeViewsLayoutDelegate to be present since c/b/ui/views aren't exercised or
referenced. Other tests do directly or indirectly reference views from
c/b/ui/views, which may require the presence of a ChromeViewsLayoutDelgate
instance.
 
> It seems like either we should remove setters, and if you want to set a new
> delegate, you do it by constructing one, so the code here would just construct
a
> ChromeViewsLayoutDelegate and that would auto-set the global; or, we should
not
> have constructing these delegates auto-set any globals, and everywhere that
> makes one should do that manually if they want to update the global.  The
first
> route would be harder to misuse.

One approach may be to implement a "last constructed wins" model. Change the
constructor(s) to blindly set the global and declare the global(s) as a
std::weak_ptr<>. In all the cases I've seen so far there is always at least a
std::unique_ptr<> reference or it is constructed in-place within another object
instance that is explicitly life-time managed. There would still need to be a
setter in this case, but there would no longer be any interleaved setting of the
global.

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:31: return
layout_delegate_;
On 2017/03/27 20:06:48, Peter Kasting wrote:
> On 2017/03/27 18:18:27, kylix_rd wrote:
> > On 2017/03/24 22:51:13, Peter Kasting wrote:
> > > It still seems like this could just be
> > > 
> > >   return
> static_cast<ChromeViewsLayoutDelegate*>(ViewsLayoutDelegate::Get());
> > > 
> > > The distinction is that if someone tried to call this method after having
> > > created a ViewsLayoutDelegate (instead of a ChromeViewsLayoutDelegate),
your
> > > implementation would return null, while mine would trigger UB.  It doesn't
> > seem
> > > like that's worth tracking |layout_delegate_|, since code trying to call
> this
> > > should have guaranteed it created the right kind of delegate already?
> > 
> > I was also trying to avoid an unsafe downcast.
> 
> Right, that's what I was saying I didn't think was worth the extra tracking
> complexity, since we shouldn't get into that scenario, and if we do we don't
> handle it today anyway.

Once I've verified that any calls to ChromeViewsLayoutDelegate::Get() will never
return null, then I'll feel more comfortable with removing this global and
coalescing it with the one from ViewsLayoutGlobal::Get().

[kylix_rd, 2017-03-29 21:06:25]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 21:07:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 21:44:45]

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

[commit-bot: I haz the power, 2017-03-29 21:44:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2017-03-30 00:49:04]

Didn't have time to review everything.

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/options/vpn_config_view.cc:508:
ChromeViewsLayoutDelegate* delegate = ChromeViewsLayoutDelegate::Get();
Nit: For this and other files in this directory, use the base (views) version of
the delegate and constants, then remove chrome_views_layout_delegate.h from the
DEPS for this directory.

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
File chrome/browser/ui/views/arc_app_dialog_view.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
chrome/browser/ui/views/arc_app_dialog_view.cc:141: // TODO: Harmony - review
the use of constants below
Nit: I wouldn't add this TODO for now, as we haven't even figured out if this
class is on the list of things we'll be touching.

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view_unittest.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view_unittest.cc:17: #include
"chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h"
Nit: Additions no longer needed?

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc:40:
views_delegate_.set_layout_delegate(nullptr);
So we can remove set(nullptr) calls like this now, right?

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:36: return
ui::MaterialDesignController::IsSecondaryUiMaterial()
It seems like this conditional shouldn't be here in this CL if you aren't
planning to remove LayoutDelegate until a subsequent CL.

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h:17: BUBBLE_DIALOG
= static_cast<int>(views::InsetsMetric::BUBBLE_DIALOG),
I still am not comfortable moving forward with this way of doing the enum unless
sky and tapted think it's a good idea and want to either change the other places
or note why the requirements here differ.

https://codereview.chromium.org/2758323002/diff/320001/ui/views/views_delegat...
File ui/views/views_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/ui/views/views_delegat...
ui/views/views_delegate.cc:25: : views_tsc_factory_(new
ViewsTouchEditingControllerFactory) {
Nit: |editing_controller_factory_| might be a better name.

(I know you're just moving this code.)

https://codereview.chromium.org/2758323002/diff/320001/ui/views/views_delegat...
ui/views/views_delegate.cc:32: touch_selection_menu_runner_.reset(new
TouchSelectionMenuRunnerViews());
Nit: Prefer =MakeUnique to reset(new

[kylix_rd, 2017-03-30 18:06:21]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 18:06:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 18:11:51]

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

[commit-bot: I haz the power, 2017-03-30 18:11:52]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kylix_rd, 2017-03-30 18:22:15]

Patchset #8 (id:360001) has been deleted

[kylix_rd, 2017-03-30 19:28:00]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 19:29:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kylix_rd, 2017-03-30 19:34:08]

Getting closer. More to come.

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/options/vpn_config_view.cc:508:
ChromeViewsLayoutDelegate* delegate = ChromeViewsLayoutDelegate::Get();
On 2017/03/30 00:49:04, Peter Kasting wrote:
> Nit: For this and other files in this directory, use the base (views) version
of
> the delegate and constants, then remove chrome_views_layout_delegate.h from
the
> DEPS for this directory.

Duh... I kind of went on auto-pilot and didn't look close enough that the
ViewsLayoutDelegate was sufficient here. Thanks.

Done.

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
File chrome/browser/ui/views/arc_app_dialog_view.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
chrome/browser/ui/views/arc_app_dialog_view.cc:141: // TODO: Harmony - review
the use of constants below
On 2017/03/30 00:49:04, Peter Kasting wrote:
> Nit: I wouldn't add this TODO for now, as we haven't even figured out if this
> class is on the list of things we'll be touching.

Done.

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view_unittest.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view_unittest.cc:17: #include
"chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h"
On 2017/03/30 00:49:04, Peter Kasting wrote:
> Nit: Additions no longer needed?

Done.

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc:40:
views_delegate_.set_layout_delegate(nullptr);
On 2017/03/30 00:49:04, Peter Kasting wrote:
> So we can remove set(nullptr) calls like this now, right?

Yep. Done.

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:36: return
ui::MaterialDesignController::IsSecondaryUiMaterial()
On 2017/03/30 00:49:04, Peter Kasting wrote:
> It seems like this conditional shouldn't be here in this CL if you aren't
> planning to remove LayoutDelegate until a subsequent CL.

The plan is to eliminate the LayoutDelegate and introduce a
HarmonyViewsLayoutDelegate once all the references to LayoutDelegate have been
refactored out.

If you'd prefer, I can push that work to another follow-on CL in order to reduce
the size of this CL.

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h:17: BUBBLE_DIALOG
= static_cast<int>(views::InsetsMetric::BUBBLE_DIALOG),
On 2017/03/30 00:49:04, Peter Kasting wrote:
> I still am not comfortable moving forward with this way of doing the enum
unless
> sky and tapted think it's a good idea and want to either change the other
places
> or note why the requirements here differ.

I'm still mulling this over as well. I'm leaving it in place until I get the
rest of the CL handled before I tackle this issue.

https://codereview.chromium.org/2758323002/diff/320001/ui/views/views_delegat...
File ui/views/views_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/ui/views/views_delegat...
ui/views/views_delegate.cc:25: : views_tsc_factory_(new
ViewsTouchEditingControllerFactory) {
On 2017/03/30 00:49:04, Peter Kasting wrote:
> Nit: |editing_controller_factory_| might be a better name.
> 
> (I know you're just moving this code.)

Done.

https://codereview.chromium.org/2758323002/diff/320001/ui/views/views_delegat...
ui/views/views_delegate.cc:32: touch_selection_menu_runner_.reset(new
TouchSelectionMenuRunnerViews());
On 2017/03/30 00:49:04, Peter Kasting wrote:
> Nit: Prefer =MakeUnique to reset(new

Done.

[Peter Kasting, 2017-03-30 19:40:02]

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/options/vpn_config_view.cc:508:
ChromeViewsLayoutDelegate* delegate = ChromeViewsLayoutDelegate::Get();
On 2017/03/30 19:34:07, kylix_rd wrote:
> On 2017/03/30 00:49:04, Peter Kasting wrote:
> > Nit: For this and other files in this directory, use the base (views)
version
> of
> > the delegate and constants, then remove chrome_views_layout_delegate.h from
> the
> > DEPS for this directory.
> 
> Duh... I kind of went on auto-pilot and didn't look close enough that the
> ViewsLayoutDelegate was sufficient here. Thanks.

Do you think it makes sense to suggest as a rule of thumb "use the core class
everywhere you don't actually need the Chrome-specific side"?  That might make
future componentization easier (if any occurs).

If this makes sense, it might be a minor argument for the "simpler enum model"
approach, because there will be no enum overlap -- so whether code really needs
to depend on Chrome-side values will become more obvious.

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:36: return
ui::MaterialDesignController::IsSecondaryUiMaterial()
On 2017/03/30 19:34:07, kylix_rd wrote:
> On 2017/03/30 00:49:04, Peter Kasting wrote:
> > It seems like this conditional shouldn't be here in this CL if you aren't
> > planning to remove LayoutDelegate until a subsequent CL.
> 
> The plan is to eliminate the LayoutDelegate and introduce a
> HarmonyViewsLayoutDelegate once all the references to LayoutDelegate have been
> refactored out.

I'm OK with doing that in this CL if you want.  I wasn't sure whether that was
your plan, hence the conditional nature of my comment :)

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h:17: BUBBLE_DIALOG
= static_cast<int>(views::InsetsMetric::BUBBLE_DIALOG),
On 2017/03/30 19:34:07, kylix_rd wrote:
> On 2017/03/30 00:49:04, Peter Kasting wrote:
> > I still am not comfortable moving forward with this way of doing the enum
> unless
> > sky and tapted think it's a good idea and want to either change the other
> places
> > or note why the requirements here differ.
> 
> I'm still mulling this over as well. I'm leaving it in place until I get the
> rest of the CL handled before I tackle this issue.

SGTM.

[commit-bot: I haz the power, 2017-03-30 19:49:21]

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

[commit-bot: I haz the power, 2017-03-30 19:49:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kylix_rd, 2017-03-30 20:02:44]

Patchset #8 (id:380001) has been deleted

[kylix_rd, 2017-03-30 20:03:11]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 20:04:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 20:37:16]

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

[commit-bot: I haz the power, 2017-03-30 20:37:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kylix_rd, 2017-03-30 21:17:44]

Patchset #8 (id:400001) has been deleted

[kylix_rd, 2017-03-30 21:17:59]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 21:19:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 22:22:56]

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

[commit-bot: I haz the power, 2017-03-30 22:22:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kylix_rd, 2017-03-30 22:32:53]

Patchset #8 (id:420001) has been deleted

[kylix_rd, 2017-03-30 22:55:57]

Patchset #8 (id:440001) has been deleted

[kylix_rd, 2017-03-30 22:56:04]

Patchset #8 (id:440001) has been deleted

[kylix_rd, 2017-03-30 23:52:19]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 23:53:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 01:57:31]

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

[commit-bot: I haz the power, 2017-03-31 01:57:32]

Dry run: This issue passed the CQ dry run.

[kylix_rd, 2017-03-31 21:10:24]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 21:11:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kylix_rd, 2017-03-31 21:12:40]

I used a hybrid approach to the enumerations in this pass. I just want to see
how it plays. If it doesn't, I'll rework it.

I've also removed the LayoutDelegate and HarmonyLayoutDelegate classes in favor
of the new versions.

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/options/vpn_config_view.cc:508:
ChromeViewsLayoutDelegate* delegate = ChromeViewsLayoutDelegate::Get();
On 2017/03/30 19:40:02, Peter Kasting wrote:
> On 2017/03/30 19:34:07, kylix_rd wrote:
> > On 2017/03/30 00:49:04, Peter Kasting wrote:
> > > Nit: For this and other files in this directory, use the base (views)
> version
> > of
> > > the delegate and constants, then remove chrome_views_layout_delegate.h
from
> > the
> > > DEPS for this directory.
> > 
> > Duh... I kind of went on auto-pilot and didn't look close enough that the
> > ViewsLayoutDelegate was sufficient here. Thanks.
> 
> Do you think it makes sense to suggest as a rule of thumb "use the core class
> everywhere you don't actually need the Chrome-specific side"?  That might make
> future componentization easier (if any occurs).

Yes, that makes sense. We just need to ensure that only those things that are
truly "core" use the core class. We don't want folks pushing things into the
core which rightly belong in the Chrome-specific side. It should be easy to
determine, but I've found that it pays to be very specific. It also doesn't make
sense to mix core and Chrome stuff in the same file. In general, things in
c/b/ui/views should use the ChromeViewsLayoutDelegate, and other stuff should
favor ViewsLayoutDelegate when it makes sense.
 
> If this makes sense, it might be a minor argument for the "simpler enum model"
> approach, because there will be no enum overlap -- so whether code really
needs
> to depend on Chrome-side values will become more obvious.

Hmmm... that is a reasonable argument for the simpler enum approach. I've not
zeroed in on another, more palatable approach yet but I'm determined.

[commit-bot: I haz the power, 2017-03-31 21:19:38]

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

[commit-bot: I haz the power, 2017-03-31 21:19:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2017-03-31 22:14:29]

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/2758323002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/options/vpn_config_view.cc:508:
ChromeViewsLayoutDelegate* delegate = ChromeViewsLayoutDelegate::Get();
On 2017/03/31 21:12:40, kylix_rd wrote:
> On 2017/03/30 19:40:02, Peter Kasting wrote:
> > On 2017/03/30 19:34:07, kylix_rd wrote:
> > > On 2017/03/30 00:49:04, Peter Kasting wrote:
> > > > Nit: For this and other files in this directory, use the base (views)
> > version
> > > of
> > > > the delegate and constants, then remove chrome_views_layout_delegate.h
> from
> > > the
> > > > DEPS for this directory.
> > > 
> > > Duh... I kind of went on auto-pilot and didn't look close enough that the
> > > ViewsLayoutDelegate was sufficient here. Thanks.
> > 
> > Do you think it makes sense to suggest as a rule of thumb "use the core
class
> > everywhere you don't actually need the Chrome-specific side"?  That might
make
> > future componentization easier (if any occurs).
> 
> Yes, that makes sense. We just need to ensure that only those things that are
> truly "core" use the core class. We don't want folks pushing things into the
> core which rightly belong in the Chrome-specific side. It should be easy to
> determine, but I've found that it pays to be very specific. It also doesn't
make
> sense to mix core and Chrome stuff in the same file. In general, things in
> c/b/ui/views should use the ChromeViewsLayoutDelegate, and other stuff should
> favor ViewsLayoutDelegate when it makes sense.

I think we're probably on the same page in general, but I would be more open to
people using different types in the same file if some of the uses can get away
with only using the core class.  But there's little difference to me either way
on that one since the pros and cons of the two routes are so minor.

I definitely would not want people to end up pushing things down into ui/views
that could have stayed in chrome/ just because they didn't realize they could
switch delegate types.  I don't know how significant that risk is (my guess is
not very).

[kylix_rd, 2017-04-03 12:57:30]

Patchset #9 (id:470001) has been deleted

[kylix_rd, 2017-04-03 12:57:41]

Patchset #9 (id:470001) has been deleted

[kylix_rd, 2017-04-03 12:57:58]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 12:58:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 14:20:16]

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

[commit-bot: I haz the power, 2017-04-03 14:20:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kylix_rd, 2017-04-03 17:47:30]

kylixrd@chromium.org changed reviewers:
+ benwells@chromium.org, grt@chromium.org, oshima@chromium.org, sky@chromium.org

[kylix_rd, 2017-04-03 17:48:03]

kylixrd@chromium.org changed reviewers:
+ gab@chromium.org
- grt@chromium.org

[kylix_rd, 2017-04-03 17:56:13]

Here's the breakdown:

oshima@:

ash/*
chrome/browser/chromeos/*

gab@:

chrome/browser/ui/views/first_run/*

benwells@:

chrome/browser/ui/views/extensions/*

sky@, pkasting@:

ui/views/*

[gab, 2017-04-03 18:22:05]

first_run/ rs-lgtm (for trivial naming side-effects you can TBR)

[Peter Kasting, 2017-04-03 21:53:20]

On 2017/04/03 17:56:13, kylix_rd wrote:
> Here's the breakdown:
> 
> oshima@:
> 
> ash/*
> chrome/browser/chromeos/*
> 
> gab@:
> 
> chrome/browser/ui/views/first_run/*
> 
> benwells@:
> 
> chrome/browser/ui/views/extensions/*
> 
> sky@, pkasting@:
> 
> ui/views/*

Did you mean to have me look at c/b/ui/views/*.not_covered_already?

[oshima, 2017-04-03 22:35:12]

ash,chromeos lgtm

[benwells, 2017-04-04 00:53:58]

c/b/ui/views/extensions lgtm

[Peter Kasting, 2017-04-04 02:08:57]

Heck with it, I just reviewed everything.

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:31: return
layout_delegate_;
On 2017/03/29 18:56:04, kylix_rd wrote:
> On 2017/03/27 20:06:48, Peter Kasting wrote:
> > On 2017/03/27 18:18:27, kylix_rd wrote:
> > > On 2017/03/24 22:51:13, Peter Kasting wrote:
> > > > It still seems like this could just be
> > > > 
> > > >   return
> > static_cast<ChromeViewsLayoutDelegate*>(ViewsLayoutDelegate::Get());
> > > > 
> > > > The distinction is that if someone tried to call this method after
having
> > > > created a ViewsLayoutDelegate (instead of a ChromeViewsLayoutDelegate),
> your
> > > > implementation would return null, while mine would trigger UB.  It
doesn't
> > > seem
> > > > like that's worth tracking |layout_delegate_|, since code trying to call
> > this
> > > > should have guaranteed it created the right kind of delegate already?
> > > 
> > > I was also trying to avoid an unsafe downcast.
> > 
> > Right, that's what I was saying I didn't think was worth the extra tracking
> > complexity, since we shouldn't get into that scenario, and if we do we don't
> > handle it today anyway.
> 
> Once I've verified that any calls to ChromeViewsLayoutDelegate::Get() will
never
> return null, then I'll feel more comfortable with removing this global and
> coalescing it with the one from ViewsLayoutGlobal::Get().

Was this something you were going to do before landing this patch?  If not,
should it have a TODO somewhere?

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/chromeo...
File chrome/browser/chromeos/options/DEPS (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/chromeo...
chrome/browser/chromeos/options/DEPS:2: # TODO(xdai): Remove this rule after
bugs 687340 and 687349 are fixed.
Nit: Remove reference to bug 687349

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_ash_unittest.cc
(right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_ash_unittest.cc:34:
base::MakeUnique<ChromeViewsLayoutDelegate>());
I'm wondering how long this set could theoretically persist.  Maybe TearDown()
should restore the previous layout delegate (would take a bit of API munging to
do, basically set_layout_delegate() would have to return the old unique_ptr). 
This would guarantee that future tests in the same process couldn't be affected.

We could make some helper NeedsChromeLayoutDelegate object that tests like this
can include as a member.  Its default constructor/destructor would take care of
setting and resetting the layout delegate.  Then this could be used in all the
cases where you currently call set_layout_delegate().

This might even allow making set_layout_delegate() private and friending that
class, though I don't know if that's worth it.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view_unittest.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view_unittest.cc:21: #include
"ui/views/test/test_views_delegate.h"
Nit: This #include doesn't seem necessary?

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:59: // ChromeViewsDelegate*
views_delegate = nullptr;
Remove entirely

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:66: #if !defined(OS_WIN)
Nit: Can now probably kill this #ifdef by initializing the remaining Win-only
bits in the class declaration

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.h:98: new
ChromeViewsLayoutDelegate());
This block should not have been added, I think.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/cookie_info_view.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/cookie_info_view.cc:152:
layout_delegate->GetInsetsMetric(ChromeInsetsMetric::DIALOG_BUTTON));
This is a behavior change (bottom inset will become nonzero).

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/global_error_bubble_view_unittest.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/global_error_bubble_view_unittest.cc:93:
views_delegate_.set_layout_delegate(nullptr);
Not necessary

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:15: 
Nit: Remove blank line

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:16:
ChromeViewsLayoutDelegate* layout_delegate_ = nullptr;
Nit: I'd name this |g_layout_delegate| since it's more like a global than a
member

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:49:
static_cast<views::InsetsMetric>(metric));
Nit: A blank line below this top group (in each of these two functions) might
make it more obvious where the break is between the "forwarding part" of the
switch and the part that handles constants specific to the chrome side.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:51: return
gfx::Insets(views::kPanelHorizMargin,
Nit: Shouldn't this be kPanelVertMargin?

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:52:
views::kButtonHEdgeMarginNew);
Nit: Seems like these should use GetDistanceMetric()

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:80: return
views::kMinimumButtonWidth;
I think after this patch this constant is only present in this file.  If so,
this CL or a followup one should go ahead and move this and any similarly-unique
constants into this function/file.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h:15: enum class
ChromeInsetsMetric {
Looks like you stuck with the enum class route.  Did you want to come to
agreement on that here or do a separate email thread about it?

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h:16: // The
margins that should be applied around a bubble dialog.
Nit: Even if we stick with the enum route, don't copy the comments on these from
the base class -- just have a commentless top section that "mirrors over" the
base values, then save comments for the new cases.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h:27: // The insets
applied around a content panel
Nit: Trailing period.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/harmony_views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_views_layout_delegate.cc:34: return
kVisibleMargin - kHarmonyLayoutUnit / 4;
Nit: Should probably refer to the VECTOR_IMAGE_BUTTON insets

I was kicking around whether having a few more distance metrics (some of the
ones that got removed) would let the base class define the insets in terms of
GetDistanceMetric() calls.  Then this class wouldn't need to override
GetInsetsMetric() at all because it would Just Do The Right Thing.  I'm not sure
whether this is worth it, but if we went this route, then the kHarmonyLayoutUnit
/ 4 bit would become a distance metric and you could refer to that here instead
of reading the route off the insets.

Incidentally, if these distance metrics were only to be used by the
implementation of GetInsetsMetric()/GetDistanceMetric(), and you used non-class
enums for the actual metrics, then you could easily define a set of "protected
identifiers" in the base class, and they'd be usable by subclasses but not
external callers.  (I'm sorry if I sound like a broken record on the enum issue,
it merely occurs to me that it would facilitate this.)

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_views_layout_delegate.cc:36: case
ChromeDistanceMetric::RELATED_BUTTON_HORIZONTAL:
Nit: Might make sense to collapse together cases that return the same value. 
Not only is this shorter, once Harmony is the only style it will make it clearer
which metrics are just conceptual relics and should be merged into others.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/task_manager_view.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/task_manager_view.cc:335: // TODO: Harmony - review the
use of the constants below
Nit: I probably wouldn't add this comment (there are a lot of other similar
places we could add such a comment and hopefully Patti is already on top of all
these)

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc
(right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc:51:
views_delegate()->set_layout_delegate(nullptr);
Should no longer be necessary.

https://codereview.chromium.org/2758323002/diff/490001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.h (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.h:154: views::TestViewsDelegate*
views_delegate();
Nit: This should be defined here or else named CamelCase.

Consider naming this test_views_delegate() instead, as the code which calls it
does so to call methods available in TestViewsDelegate that aren't in
ViewsDelegate (or it could just use ViewsDelegate::Get()), and it was confusing
me trying to read such code as I reviewed this CL (I was thinking, "how are you
calling ViewsDelegate::set_layout_delegate(), that doesn't exist").

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
File ui/views/layout/views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
ui/views/layout/views_layout_delegate.cc:16: 
Nit: Remove blank line

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
ui/views/layout/views_layout_delegate.cc:17: static ViewsLayoutDelegate*
layout_delegate_ = nullptr;
Nit: I'd name this |g_layout_delegate| since it's more like a global than a
member

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
File ui/views/layout/views_layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
ui/views/layout/views_layout_delegate.h:47: class VIEWS_EXPORT
ViewsLayoutDelegate {
Can we just call this LayoutDelegate, and call the subclasses
ChromeLayoutDelegate and HarmonyLayoutDelegate?

I don't see what having "Views" in all three class names buys us, especially
when users of this class already preface with views::.

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
ui/views/layout/views_layout_delegate.h:58: // enumeration element.
Nit: "Returns the distance for the given metric"?  Similarly above "Returns the
insets for the given metric"?

Basically, I don't like saying "between elements" here since not all distances
are between elements, and I don't like either of the comment saying they return
an "XXX metric" since that's the name of the identifying enum rather than the
actual thing-being-returned-itself.

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
ui/views/layout/views_layout_delegate.h:66: DefaultTypographyProvider
typography_provider_;
Seems like this should be a static local in GetTypographyProvider() to avoid
subclasses inheriting it.

https://codereview.chromium.org/2758323002/diff/490001/ui/views/test/test_vie...
File ui/views/test/test_views_delegate_aura.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/ui/views/test/test_vie...
ui/views/test/test_views_delegate_aura.cc:22: layout_delegate_(new
ViewsLayoutDelegate()) {}
Nit: Prefer calling MakeUnique to using raw new

https://codereview.chromium.org/2758323002/diff/490001/ui/views/test/test_vie...
File ui/views/test/test_views_delegate_mac.mm (right):

https://codereview.chromium.org/2758323002/diff/490001/ui/views/test/test_vie...
ui/views/test/test_views_delegate_mac.mm:16: layout_delegate_(new
ViewsLayoutDelegate()) {}
Nit: MakeUnique

https://codereview.chromium.org/2758323002/diff/490001/ui/views/test/views_te...
File ui/views/test/views_test_base.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/ui/views/test/views_te...
ui/views/test/views_test_base.cc:14: #include
"ui/views/layout/views_layout_delegate.h"
Nit: This #include doesn't seem necessary?

[kylix_rd, 2017-04-04 20:28:24]

Addressed most of the feedback, including the rename of ViewsLayoutDelegate,
ChromeViewsLayoutDelegate and HarmonyViewsLayoutDelegate along with the
corresponding source files.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/chromeo...
File chrome/browser/chromeos/options/DEPS (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/chromeo...
chrome/browser/chromeos/options/DEPS:2: # TODO(xdai): Remove this rule after
bugs 687340 and 687349 are fixed.
On 2017/04/04 02:08:51, Peter Kasting (busy 4-4-17) wrote:
> Nit: Remove reference to bug 687349

Done.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_ash_unittest.cc
(right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_ash_unittest.cc:34:
base::MakeUnique<ChromeViewsLayoutDelegate>());
On 2017/04/04 02:08:51, Peter Kasting wrote:
> I'm wondering how long this set could theoretically persist.  Maybe TearDown()
> should restore the previous layout delegate (would take a bit of API munging
to
> do, basically set_layout_delegate() would have to return the old unique_ptr). 
> This would guarantee that future tests in the same process couldn't be
affected.

I don't think that is necessary since ash_test_helper_'s lifetime is tied to the
test class. AIUI, SetUp()/TearDown() are called around each individual test. Of
course new tests on this test class will share the same layout delegate. Is that
what you meant?
 
> We could make some helper NeedsChromeLayoutDelegate object that tests like
this
> can include as a member.  Its default constructor/destructor would take care
of
> setting and resetting the layout delegate.  Then this could be used in all the
> cases where you currently call set_layout_delegate().

Hmm... that's an interesting idea. I'll need to think about ensuring the
overridden layout vs the default delegates are set in the correct order. In this
instance, AshTestBase sets the layout delegate in SetUp(), which is clearly
after the class itself is constructed.

> This might even allow making set_layout_delegate() private and friending that
> class, though I don't know if that's worth it.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view_unittest.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view_unittest.cc:21: #include
"ui/views/test/test_views_delegate.h"
On 2017/04/04 02:08:51, Peter Kasting wrote:
> Nit: This #include doesn't seem necessary?

Done.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:59: // ChromeViewsDelegate*
views_delegate = nullptr;
On 2017/04/04 02:08:51, Peter Kasting wrote:
> Remove entirely

Done.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:66: #if !defined(OS_WIN)
On 2017/04/04 02:08:51, Peter Kasting wrote:
> Nit: Can now probably kill this #ifdef by initializing the remaining Win-only
> bits in the class declaration

Not sure how to initialize weak_factory_ within the class declaration.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/cookie_info_view.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/cookie_info_view.cc:152:
layout_delegate->GetInsetsMetric(ChromeInsetsMetric::DIALOG_BUTTON));
On 2017/04/04 02:08:52, Peter Kasting wrote:
> This is a behavior change (bottom inset will become nonzero).

I changed this for a reason... I just cannot remember exactly why at the moment.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/global_error_bubble_view_unittest.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/global_error_bubble_view_unittest.cc:93:
views_delegate_.set_layout_delegate(nullptr);
On 2017/04/04 02:08:52, Peter Kasting wrote:
> Not necessary

Done.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:51: return
gfx::Insets(views::kPanelHorizMargin,
On 2017/04/04 02:08:53, Peter Kasting wrote:
> Nit: Shouldn't this be kPanelVertMargin?

Maybe... however, this is copied from layout_utils::CreatePanelLayout(), which
directly constructed a gfx::Insets() with PANEL_CONTENT_MARGIN. In the original
LayoutDelegate, that enum value returned views::kPanelHorizMargin from
GetMetric().

I added PANEL_LAYOUT as a new insets metric and simply used the original values
for the non-Harmony mode. Maybe PANEL_CONTENT_MARGIN was wrong?

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:80: return
views::kMinimumButtonWidth;
On 2017/04/04 02:08:53, Peter Kasting wrote:
> I think after this patch this constant is only present in this file.  If so,
> this CL or a followup one should go ahead and move this and any
similarly-unique
> constants into this function/file.

Eventually, all the views::kXXXX constants will be made private and moved to the
appropriate xxx_layout_delegate.cc file(s). This can be done at that point.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h:15: enum class
ChromeInsetsMetric {
On 2017/04/04 02:08:54, Peter Kasting wrote:
> Looks like you stuck with the enum class route.  Did you want to come to
> agreement on that here or do a separate email thread about it?

Either way. I prefer to use an enum class since it is properly scoped and
doesn't "infect" the outer namespace with extra symbols. While this is a slight
departure from the other technique, it is the same in spirit. It is also
type-safe with compile-time checking.

There isn't the likelihood of another level of inheritance (aside from the
harmony specialization), so this enum can be considered "sealed".

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h:16: // The
margins that should be applied around a bubble dialog.
On 2017/04/04 02:08:53, Peter Kasting wrote:
> Nit: Even if we stick with the enum route, don't copy the comments on these
from
> the base class -- just have a commentless top section that "mirrors over" the
> base values, then save comments for the new cases.

Done.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h:27: // The insets
applied around a content panel
On 2017/04/04 02:08:54, Peter Kasting wrote:
> Nit: Trailing period.

Done.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/harmony_views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_views_layout_delegate.cc:36: case
ChromeDistanceMetric::RELATED_BUTTON_HORIZONTAL:
On 2017/04/04 02:08:54, Peter Kasting wrote:
> Nit: Might make sense to collapse together cases that return the same value. 
> Not only is this shorter, once Harmony is the only style it will make it
clearer
> which metrics are just conceptual relics and should be merged into others.

I thought doing about that. I was just concerned that some of these metrics are
still in a state of flux while the Harmony specs are mapped to real-world
metrics. Once they're nailed down, we can coalesce the like-valued items. Are
you confident that there will be no more changes?

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/task_manager_view.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/task_manager_view.cc:335: // TODO: Harmony - review the
use of the constants below
On 2017/04/04 02:08:54, Peter Kasting wrote:
> Nit: I probably wouldn't add this comment (there are a lot of other similar
> places we could add such a comment and hopefully Patti is already on top of
all
> these)

Done.

https://codereview.chromium.org/2758323002/diff/490001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.h (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.h:154: views::TestViewsDelegate*
views_delegate();
On 2017/04/04 02:08:55, Peter Kasting wrote:
> Nit: This should be defined here or else named CamelCase.

Renamed to test_views_delegate().

> Consider naming this test_views_delegate() instead, as the code which calls it
> does so to call methods available in TestViewsDelegate that aren't in
> ViewsDelegate (or it could just use ViewsDelegate::Get()), and it was
confusing
> me trying to read such code as I reviewed this CL (I was thinking, "how are
you
> calling ViewsDelegate::set_layout_delegate(), that doesn't exist").

Good point.

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
File ui/views/layout/views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
ui/views/layout/views_layout_delegate.cc:16: 
On 2017/04/04 02:08:55, Peter Kasting wrote:
> Nit: Remove blank line

Done.

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
ui/views/layout/views_layout_delegate.cc:17: static ViewsLayoutDelegate*
layout_delegate_ = nullptr;
On 2017/04/04 02:08:55, Peter Kasting wrote:
> Nit: I'd name this |g_layout_delegate| since it's more like a global than a
> member

Done.

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
File ui/views/layout/views_layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
ui/views/layout/views_layout_delegate.h:47: class VIEWS_EXPORT
ViewsLayoutDelegate {
On 2017/04/04 02:08:56, Peter Kasting wrote:
> Can we just call this LayoutDelegate, and call the subclasses
> ChromeLayoutDelegate and HarmonyLayoutDelegate?
> 
> I don't see what having "Views" in all three class names buys us, especially
> when users of this class already preface with views::.

Now that the original LayoutDelegate and HarmonyLayoutDelegate classes are gone,
this makes sense.

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
ui/views/layout/views_layout_delegate.h:58: // enumeration element.
On 2017/04/04 02:08:56, Peter Kasting wrote:
> Nit: "Returns the distance for the given metric"?  Similarly above "Returns
the
> insets for the given metric"?

Done.
 
> Basically, I don't like saying "between elements" here since not all distances
> are between elements, and I don't like either of the comment saying they
return
> an "XXX metric" since that's the name of the identifying enum rather than the
> actual thing-being-returned-itself.

What about "edge(s)"? That is, essentially, what the distances are between. For
width/height, it is between the top/bottom or left/right edges. Between
controls, it would be the top edge of one to the bottom edge of another.

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
ui/views/layout/views_layout_delegate.h:66: DefaultTypographyProvider
typography_provider_;
On 2017/04/04 02:08:56, Peter Kasting wrote:
> Seems like this should be a static local in GetTypographyProvider() to avoid
> subclasses inheriting it.

OK. I just moved the implementation from ViewsDelegate.

https://codereview.chromium.org/2758323002/diff/490001/ui/views/test/test_vie...
File ui/views/test/test_views_delegate_aura.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/ui/views/test/test_vie...
ui/views/test/test_views_delegate_aura.cc:22: layout_delegate_(new
ViewsLayoutDelegate()) {}
On 2017/04/04 02:08:56, Peter Kasting wrote:
> Nit: Prefer calling MakeUnique to using raw new

Done.

https://codereview.chromium.org/2758323002/diff/490001/ui/views/test/test_vie...
File ui/views/test/test_views_delegate_mac.mm (right):

https://codereview.chromium.org/2758323002/diff/490001/ui/views/test/test_vie...
ui/views/test/test_views_delegate_mac.mm:16: layout_delegate_(new
ViewsLayoutDelegate()) {}
On 2017/04/04 02:08:56, Peter Kasting wrote:
> Nit: MakeUnique

Done.

https://codereview.chromium.org/2758323002/diff/490001/ui/views/test/views_te...
File ui/views/test/views_test_base.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/ui/views/test/views_te...
ui/views/test/views_test_base.cc:14: #include
"ui/views/layout/views_layout_delegate.h"
On 2017/04/04 02:08:57, Peter Kasting wrote:
> Nit: This #include doesn't seem necessary?

Done.

[kylix_rd, 2017-04-04 20:50:03]

Removed DEPS file and fixed some non-Windows platform builds.

https://codereview.chromium.org/2758323002/diff/510001/chrome/browser/chromeo...
File chrome/browser/chromeos/options/DEPS (right):

https://codereview.chromium.org/2758323002/diff/510001/chrome/browser/chromeo...
chrome/browser/chromeos/options/DEPS:5: 
I have a patch that will remove this file entirely. When I merged, the
layout_utils.* files were removed and the functionality merged into the
GridLayout class from ui/views/layout.

[kylix_rd, 2017-04-04 21:10:14]

Patchset #11 (id:530001) has been deleted

[kylix_rd, 2017-04-05 13:21:33]

Patchset #11 (id:550001) has been deleted

[kylix_rd, 2017-04-05 15:01:55]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 15:03:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 16:04:02]

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

[commit-bot: I haz the power, 2017-04-05 16:04:03]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[kylix_rd, 2017-04-05 18:17:02]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 18:17:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 18:40:57]

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

[commit-bot: I haz the power, 2017-04-05 18:40:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Peter Kasting, 2017-04-05 19:30:10]

Have not re-reviewed

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:66: #if !defined(OS_WIN)
On 2017/04/04 20:28:22, kylix_rd wrote:
> On 2017/04/04 02:08:51, Peter Kasting wrote:
> > Nit: Can now probably kill this #ifdef by initializing the remaining
Win-only
> > bits in the class declaration
> 
> Not sure how to initialize weak_factory_ within the class declaration.

= base::WeakPtrFactory<ChromeViewsDelegate>(this);

?

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.cc:51: return
gfx::Insets(views::kPanelHorizMargin,
On 2017/04/04 20:28:23, kylix_rd wrote:
> On 2017/04/04 02:08:53, Peter Kasting wrote:
> > Nit: Shouldn't this be kPanelVertMargin?
> 
> Maybe... however, this is copied from layout_utils::CreatePanelLayout(), which
> directly constructed a gfx::Insets() with PANEL_CONTENT_MARGIN. In the
original
> LayoutDelegate, that enum value returned views::kPanelHorizMargin from
> GetMetric().
> 
> I added PANEL_LAYOUT as a new insets metric and simply used the original
values
> for the non-Harmony mode. Maybe PANEL_CONTENT_MARGIN was wrong?

kPanelVertMargin and kPanelHorizMargin had the same value, so the goal of
PANEL_CONTENT_MARGIN was to provide a single value to collapse the two together,
to eliminate the separate concept.

So this suggestion (to use kPanelVertMargin) would have no functional effect,
but would be more semantically correct if you're using the underlying separate
constants instead of the "new" joint metric.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h:15: enum class
ChromeInsetsMetric {
On 2017/04/04 20:28:23, kylix_rd wrote:
> On 2017/04/04 02:08:54, Peter Kasting wrote:
> > Looks like you stuck with the enum class route.  Did you want to come to
> > agreement on that here or do a separate email thread about it?
> 
> Either way. I prefer to use an enum class since it is properly scoped and
> doesn't "infect" the outer namespace with extra symbols. While this is a
slight
> departure from the other technique, it is the same in spirit. It is also
> type-safe with compile-time checking.
> 
> There isn't the likelihood of another level of inheritance (aside from the
> harmony specialization), so this enum can be considered "sealed".

Let's do it as a separate thread for clarity.  Why don't you send something to
sky/tapted/me and say I asked that we all get on the same page regarding what to
do both here and elsewhere.

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/harmony_views_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_views_layout_delegate.cc:36: case
ChromeDistanceMetric::RELATED_BUTTON_HORIZONTAL:
On 2017/04/04 20:28:23, kylix_rd wrote:
> On 2017/04/04 02:08:54, Peter Kasting wrote:
> > Nit: Might make sense to collapse together cases that return the same value.

> > Not only is this shorter, once Harmony is the only style it will make it
> clearer
> > which metrics are just conceptual relics and should be merged into others.
> 
> I thought doing about that. I was just concerned that some of these metrics
are
> still in a state of flux while the Harmony specs are mapped to real-world
> metrics. Once they're nailed down, we can coalesce the like-valued items. Are
> you confident that there will be no more changes?

Not 100% confident, but I think at most one or two values might change at this
point.  I'd go ahead and collapse them.

https://codereview.chromium.org/2758323002/diff/490001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.h (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.h:154: views::TestViewsDelegate*
views_delegate();
On 2017/04/04 20:28:23, kylix_rd wrote:
> On 2017/04/04 02:08:55, Peter Kasting wrote:
> > Nit: This should be defined here or else named CamelCase.
> 
> Renamed to test_views_delegate().

That addresses the second concern I had, but not this one: we use
unix_hacker_names() to indicate inlined cheap accessors, so if this is named
that way it should also be inlined.  If for some reason it doesn't make sense to
inline, I'd name it GetTestViewsDelegate() to indicate that.

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
File ui/views/layout/views_layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/490001/ui/views/layout/views_...
ui/views/layout/views_layout_delegate.h:58: // enumeration element.
On 2017/04/04 20:28:23, kylix_rd wrote:
> On 2017/04/04 02:08:56, Peter Kasting wrote:
> > Basically, I don't like saying "between elements" here since not all
distances
> > are between elements, and I don't like either of the comment saying they
> return
> > an "XXX metric" since that's the name of the identifying enum rather than
the
> > actual thing-being-returned-itself.
> 
> What about "edge(s)"? That is, essentially, what the distances are between.
For
> width/height, it is between the top/bottom or left/right edges. Between
> controls, it would be the top edge of one to the bottom edge of another.

It seems at best unnecessary to me, but if you feel it improves things, go for
it.

[Peter Kasting, 2017-04-06 08:13:36]

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_ash_unittest.cc
(right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_ash_unittest.cc:34:
base::MakeUnique<ChromeViewsLayoutDelegate>());
On 2017/04/04 20:28:22, kylix_rd wrote:
> On 2017/04/04 02:08:51, Peter Kasting wrote:
> > I'm wondering how long this set could theoretically persist.

Answer: only for one test.  This entire class is completely destroyed at the end
of every individual test.

The gtest FAQ even notes this and basically says everybody should be using
constructors/destructors to do everything, except in a few cases (e.g. needing
to call virtual functions on the current object):
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

So the thing I was worried about is not a problem.  The only question is whether
some sort of helper object like I suggested would make the code read more
nicely.  Up to you whether that's worth trying to restructure things to do.

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/chromeo...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/chromeo...
chrome/browser/chromeos/options/vpn_config_view.cc:519:
views::DistanceMetric::RELATED_CONTROL_HORIZONTAL));
Seems like all these views::DistanceMetric:: values in all files could drop the
DistanceMetric:: qualifier, because being in a non-class enum introduces the
symbols to the containing scope.  That would reduce verbiage quite a bit.

(For the typography stuff we avoided introducing things directly to views:: by
placing them inside a style:: namespace.  The similar route here would be to
place the contents of layout_delegate.h in a layout:: namespace, which would
mean LayoutDelegate could potentially just be layout::Delegate to reduce
redundancy?  sky might have thoughts here.)

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_ash_unittest.cc
(right):

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_ash_unittest.cc:15:
#include "ui/views/test/test_views_delegate.h"
Nit: This #include seems unnecessary?

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.h:97: //     
base::MakeUnique<ChromeLayoutDelegate>();
Don't add this block

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_layout_delegate.cc:15:
ChromeLayoutDelegate* layout_delegate_ = nullptr;
You didn't answer my previous question about whether "Once I've verified that
any calls to ChromeViewsLayoutDelegate::Get() will never return null, then I'll
feel more comfortable with removing this global and coalescing it with the one
from ViewsLayoutGlobal::Get()" was something you planned to do before landing
this patch; if not it should probably have a TODO (I'd like this not to stick
around too long)

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_layout_delegate.h:15: enum class
ChromeInsetsMetric {
Seems like we don't even need this class or the chrome-side-specific
GetInsetsMetric() function, if we're not adding any new values on this side.

(Do you think we're imminently going to add new values?)

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_layout_delegate.h:32: // Margin between
the edge of a dialog and the left, right, or bottom of a
Nit: Maybe blank line above this?

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/harmony_layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_layout_delegate.h:8: #include <memory>
Nit: Why this #include?

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_layout_delegate.h:13: #include
"ui/views/layout/grid_layout.h"
Nit: insets.h and grid_layout.h shouldn't be necessary, even from an IWYU
perspective, since the only references to them are in overrides, and thus the
types must already be available (and if they disappear from the base class,
they'll necessarily disappear from here too, so this is pure maintenance
burden).

The IWYU clang folks agree with this, though they've had challenges
implementing; see
https://github.com/include-what-you-use/include-what-you-use/issues/105 .

https://codereview.chromium.org/2758323002/diff/590001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/2758323002/diff/590001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.cc:18: #include
"chrome/browser/ui/views/harmony/chrome_layout_delegate.h"
Can this test really be compiled without TOOLKIT_VIEWS?  Because the existing
file implied it could be, but these changes definitely won't work.

(1) You'll need to restructure the platform-specific-includes below like:

#if defined(TOOLKIT_VIEWS)
#include "chrome/browser/ui/views/harmony/chrome_layout_delegate.h"

#if defined(OS_CHROMEOS)
... the #includes you have in it now
#else
... the #includes in it now
#include "ui/views/test/test_views_delegate.h"
#endif
#endif

(2) You'll need to move the set_layout_delegate() call below into the
TOOLKIT_VIEWS block just above it.

(3) You'll need to write test_views_delegate() like: 

#if defined(TOOLKIT_VIEWS)
views::TestViewsDelegate* BrowserWithTestWindowTest::test_views_delegate() {
#if defined(OS_CHROMEOS)
  return ash_test_helper_->test_views_delegate();
#else
  return views_test_helper_->test_views_delegate();
#endif
}
#endif

[kylix_rd, 2017-04-06 16:48:19]

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:66: #if !defined(OS_WIN)
On 2017/04/05 19:30:10, Peter Kasting wrote:
> On 2017/04/04 20:28:22, kylix_rd wrote:
> > On 2017/04/04 02:08:51, Peter Kasting wrote:
> > > Nit: Can now probably kill this #ifdef by initializing the remaining
> Win-only
> > > bits in the class declaration
> > 
> > Not sure how to initialize weak_factory_ within the class declaration.
> 
> = base::WeakPtrFactory<ChromeViewsDelegate>(this);
> 
> ?

That didn't work. "error: call to deleted constructor of ..."

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.h:98: new
ChromeViewsLayoutDelegate());
On 2017/04/04 02:08:52, Peter Kasting wrote:
> This block should not have been added, I think.

Correct. It's gone in the newer patches.

https://codereview.chromium.org/2758323002/diff/490001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.h (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.h:154: views::TestViewsDelegate*
views_delegate();
On 2017/04/05 19:30:10, Peter Kasting wrote:
> On 2017/04/04 20:28:23, kylix_rd wrote:
> > On 2017/04/04 02:08:55, Peter Kasting wrote:
> > > Nit: This should be defined here or else named CamelCase.
> > 
> > Renamed to test_views_delegate().
> 
> That addresses the second concern I had, but not this one: we use
> unix_hacker_names() to indicate inlined cheap accessors, so if this is named
> that way it should also be inlined.  If for some reason it doesn't make sense
to
> inline, I'd name it GetTestViewsDelegate() to indicate that.

Hmmm. I was looking more for consistency with the other test classes which also
have test_views_delegate().

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/chromeo...
File chrome/browser/chromeos/options/vpn_config_view.cc (right):

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/chromeo...
chrome/browser/chromeos/options/vpn_config_view.cc:519:
views::DistanceMetric::RELATED_CONTROL_HORIZONTAL));
On 2017/04/06 08:13:36, Peter Kasting wrote:
> Seems like all these views::DistanceMetric:: values in all files could drop
the
> DistanceMetric:: qualifier, because being in a non-class enum introduces the
> symbols to the containing scope.  That would reduce verbiage quite a bit.

I was looking to ensure consistency (by example) with the use of the
ChromeDistanceMetric enumeration.
 
> (For the typography stuff we avoided introducing things directly to views:: by
> placing them inside a style:: namespace.  The similar route here would be to
> place the contents of layout_delegate.h in a layout:: namespace, which would
> mean LayoutDelegate could potentially just be layout::Delegate to reduce
> redundancy?  sky might have thoughts here.)

If we push all the LayoutDelegate classes into a separate "layout" namespace,
should we not also place the LayoutManager and descendants into that namespace
as well? That seems like a good candidate for a separate refactoring pass.

In that same vein, should the ChromeLayoutDelegate and HarmonyLayoutDelegate be
placed into the "layout", some other namespace, or left in the global namespace?

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_ash_unittest.cc
(right):

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_ash_unittest.cc:15:
#include "ui/views/test/test_views_delegate.h"
On 2017/04/06 08:13:36, Peter Kasting wrote:
> Nit: This #include seems unnecessary?

It seems you're right. Done.

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.h:97: //     
base::MakeUnique<ChromeLayoutDelegate>();
On 2017/04/06 08:13:36, Peter Kasting wrote:
> Don't add this block

It's gone in subsequent patches.

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_layout_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_layout_delegate.cc:15:
ChromeLayoutDelegate* layout_delegate_ = nullptr;
On 2017/04/06 08:13:36, Peter Kasting wrote:
> You didn't answer my previous question about whether "Once I've verified that
> any calls to ChromeViewsLayoutDelegate::Get() will never return null, then
I'll
> feel more comfortable with removing this global and coalescing it with the one
> from ViewsLayoutGlobal::Get()" was something you planned to do before landing
> this patch; if not it should probably have a TODO (I'd like this not to stick
> around too long)

Sorry 'bout that. Lots of comments and things to work through and I just missed
that one. Once I get all the 'bots and tests green, I'll do a pass to remove
this. I'm relying on the 'bots to ensure the health of the other platforms and
all the tests so I don't have to setup all those platforms and tests
environments.

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_layout_delegate.h:15: enum class
ChromeInsetsMetric {
On 2017/04/06 08:13:36, Peter Kasting wrote:
> (Do you think we're imminently going to add new values?)

As I worked on this patch, I saw several places in the code that would likely
require a chrome-side-specific Inset. I didn't change them at this point since
this is more about the underlying mechanism. I suppose it all depends on what is
a reasonable notion of "imminently". I would say its likely within the next
couple of weeks.

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_layout_delegate.h:32: // Margin between
the edge of a dialog and the left, right, or bottom of a
On 2017/04/06 08:13:36, Peter Kasting wrote:
> Nit: Maybe blank line above this?

Done.

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/harmony_layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_layout_delegate.h:8: #include <memory>
On 2017/04/06 08:13:36, Peter Kasting wrote:
> Nit: Why this #include?

Probably an over zealous copy operation as I initially populated this file.
Removed.

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_layout_delegate.h:13: #include
"ui/views/layout/grid_layout.h"
On 2017/04/06 08:13:36, Peter Kasting wrote:
> Nit: insets.h and grid_layout.h shouldn't be necessary, even from an IWYU
> perspective, since the only references to them are in overrides, and thus the
> types must already be available (and if they disappear from the base class,
> they'll necessarily disappear from here too, so this is pure maintenance
> burden).
> 
> The IWYU clang folks agree with this, though they've had challenges
> implementing; see
> https://github.com/include-what-you-use/include-what-you-use/issues/105 .

Good to know. It's a subtle exception to the IWYU rule.

https://codereview.chromium.org/2758323002/diff/590001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/2758323002/diff/590001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.cc:18: #include
"chrome/browser/ui/views/harmony/chrome_layout_delegate.h"
On 2017/04/06 08:13:36, Peter Kasting wrote:
> Can this test really be compiled without TOOLKIT_VIEWS?  Because the existing
> file implied it could be, but these changes definitely won't work.
> 
> (1) You'll need to restructure the platform-specific-includes below like:
> 
> #if defined(TOOLKIT_VIEWS)
> #include "chrome/browser/ui/views/harmony/chrome_layout_delegate.h"
> 
> #if defined(OS_CHROMEOS)
> ... the #includes you have in it now
> #else
> ... the #includes in it now
> #include "ui/views/test/test_views_delegate.h"
> #endif
> #endif
> 
> (2) You'll need to move the set_layout_delegate() call below into the
> TOOLKIT_VIEWS block just above it.
> 
> (3) You'll need to write test_views_delegate() like: 
> 
> #if defined(TOOLKIT_VIEWS)
> views::TestViewsDelegate* BrowserWithTestWindowTest::test_views_delegate() {
> #if defined(OS_CHROMEOS)
>   return ash_test_helper_->test_views_delegate();
> #else
>   return views_test_helper_->test_views_delegate();
> #endif
> }
> #endif

Done.

[kylix_rd, 2017-04-06 16:48:54]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 16:50:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 17:26:03]

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

[commit-bot: I haz the power, 2017-04-06 17:26:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kylix_rd, 2017-04-06 17:39:57]

Patchset #13 (id:610001) has been deleted

[kylix_rd, 2017-04-06 17:40:17]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 17:42:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 19:22:56]

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

[commit-bot: I haz the power, 2017-04-06 19:22:59]

Dry run: This issue passed the CQ dry run.

[sky, 2017-04-06 19:59:24]

One random naming suggestion.

https://codereview.chromium.org/2758323002/diff/630001/ui/views/layout/layout...
File ui/views/layout/layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/630001/ui/views/layout/layout...
ui/views/layout/layout_delegate.h:49: class VIEWS_EXPORT LayoutDelegate {
This class allows looking up layout and typography. How about naming it
LayoutProvider, which is consistent with TypographyProvider.

[Peter Kasting, 2017-04-06 20:05:33]

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:66: #if !defined(OS_WIN)
On 2017/04/06 16:48:19, kylix_rd wrote:
> On 2017/04/05 19:30:10, Peter Kasting wrote:
> > On 2017/04/04 20:28:22, kylix_rd wrote:
> > > On 2017/04/04 02:08:51, Peter Kasting wrote:
> > > > Nit: Can now probably kill this #ifdef by initializing the remaining
> > Win-only
> > > > bits in the class declaration
> > > 
> > > Not sure how to initialize weak_factory_ within the class declaration.
> > 
> > = base::WeakPtrFactory<ChromeViewsDelegate>(this);
> > 
> > ?
> 
> That didn't work. "error: call to deleted constructor of ..."

Weird, I don't know what that error means in this context.  Oh well, not worth
worrying about.

https://codereview.chromium.org/2758323002/diff/490001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.h (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.h:154: views::TestViewsDelegate*
views_delegate();
On 2017/04/06 16:48:19, kylix_rd wrote:
> On 2017/04/05 19:30:10, Peter Kasting wrote:
> > On 2017/04/04 20:28:23, kylix_rd wrote:
> > > On 2017/04/04 02:08:55, Peter Kasting wrote:
> > > > Nit: This should be defined here or else named CamelCase.
> > > 
> > > Renamed to test_views_delegate().
> > 
> > That addresses the second concern I had, but not this one: we use
> > unix_hacker_names() to indicate inlined cheap accessors, so if this is named
> > that way it should also be inlined.  If for some reason it doesn't make
sense
> to
> > inline, I'd name it GetTestViewsDelegate() to indicate that.
> 
> Hmmm. I was looking more for consistency with the other test classes which
also
> have test_views_delegate().

That's fine, just inline it then :)

The actual function definition is simple enough that inlining seems reasonable.

[kylix_rd, 2017-04-07 14:07:59]

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:66: #if !defined(OS_WIN)
On 2017/04/06 20:05:33, Peter Kasting wrote:
> On 2017/04/06 16:48:19, kylix_rd wrote:
> > On 2017/04/05 19:30:10, Peter Kasting wrote:
> > > On 2017/04/04 20:28:22, kylix_rd wrote:
> > > > On 2017/04/04 02:08:51, Peter Kasting wrote:
> > > > > Nit: Can now probably kill this #ifdef by initializing the remaining
> > > Win-only
> > > > > bits in the class declaration
> > > > 
> > > > Not sure how to initialize weak_factory_ within the class declaration.
> > > 
> > > = base::WeakPtrFactory<ChromeViewsDelegate>(this);
> > > 
> > > ?
> > 
> > That didn't work. "error: call to deleted constructor of ..."
> 
> Weird, I don't know what that error means in this context.  Oh well, not worth
> worrying about.

The implicit constructor is explicitly deleted with the
DISALLOW_IMPLICIT_CONSTRUCTORS() macro on it's declaration:
https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?type=cs&l=284

https://codereview.chromium.org/2758323002/diff/490001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.h (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.h:154: views::TestViewsDelegate*
views_delegate();
On 2017/04/06 20:05:33, Peter Kasting wrote:
> On 2017/04/06 16:48:19, kylix_rd wrote:
> > On 2017/04/05 19:30:10, Peter Kasting wrote:
> > > On 2017/04/04 20:28:23, kylix_rd wrote:
> > > > On 2017/04/04 02:08:55, Peter Kasting wrote:
> > > > > Nit: This should be defined here or else named CamelCase.
> > > > 
> > > > Renamed to test_views_delegate().
> > > 
> > > That addresses the second concern I had, but not this one: we use
> > > unix_hacker_names() to indicate inlined cheap accessors, so if this is
named
> > > that way it should also be inlined.  If for some reason it doesn't make
> sense
> > to
> > > inline, I'd name it GetTestViewsDelegate() to indicate that.
> > 
> > Hmmm. I was looking more for consistency with the other test classes which
> also
> > have test_views_delegate().
> 
> That's fine, just inline it then :)
> 
> The actual function definition is simple enough that inlining seems
reasonable.

I'll need to verify that including test_views_delegate.h in the header doesn't
cause any other issues. Right now, TestViewsDelegate is forward declared here in
the header and the .cc file includes the full definition.

https://codereview.chromium.org/2758323002/diff/630001/ui/views/layout/layout...
File ui/views/layout/layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/630001/ui/views/layout/layout...
ui/views/layout/layout_delegate.h:49: class VIEWS_EXPORT LayoutDelegate {
On 2017/04/06 19:59:24, sky wrote:
> This class allows looking up layout and typography. How about naming it
> LayoutProvider, which is consistent with TypographyProvider.

I like that suggestion. From an observational perspective, "delegates" tend to
be embedded/mixed-in/inherited with other classes. In this instance, it's a
global, stand-alone singleton. "Provider" is a better adjective that conveys
this distinction.

[Peter Kasting, 2017-04-07 18:02:39]

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:66: #if !defined(OS_WIN)
On 2017/04/07 14:07:58, kylix_rd wrote:
> On 2017/04/06 20:05:33, Peter Kasting wrote:
> > On 2017/04/06 16:48:19, kylix_rd wrote:
> > > On 2017/04/05 19:30:10, Peter Kasting wrote:
> > > > On 2017/04/04 20:28:22, kylix_rd wrote:
> > > > > On 2017/04/04 02:08:51, Peter Kasting wrote:
> > > > > > Nit: Can now probably kill this #ifdef by initializing the remaining
> > > > Win-only
> > > > > > bits in the class declaration
> > > > > 
> > > > > Not sure how to initialize weak_factory_ within the class declaration.
> > > > 
> > > > = base::WeakPtrFactory<ChromeViewsDelegate>(this);
> > > > 
> > > > ?
> > > 
> > > That didn't work. "error: call to deleted constructor of ..."
> > 
> > Weird, I don't know what that error means in this context.  Oh well, not
worth
> > worrying about.
> 
> The implicit constructor is explicitly deleted with the
> DISALLOW_IMPLICIT_CONSTRUCTORS() macro on it's declaration:
> https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?type=cs&l=284

Yeah, but we're calling the explicit (one-arg) constructor.

Unless this requires the availability of the copy constructor as well... I bet
that's it.

I bet this could be done like this:

  base::WeakPtrFactory<ChromeViewsDelegate> weak_factory_{this};

...which would fit the variable initialization guide saying brace-initialization
is fine when other methods won't work or are inconvenient.

[kylix_rd, 2017-04-07 18:27:59]

Description was changed from

==========
Broke out layout metric information from ViewsDelegate to ViewsLayoutDelegate

BUG=687349
==========

to

==========
Broke out layout metric information from ViewsDelegate to ViewsLayoutDelegate

BUG=687349
TBR=msw@chromium.org
==========

[kylix_rd, 2017-04-07 18:27:59]

kylixrd@chromium.org changed reviewers:
+ msw@chromium.org

[kylix_rd, 2017-04-07 18:29:00]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 18:29:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kylix_rd, 2017-04-07 18:33:36]

Added msw@chromium.org to TBR for the rename in 

components/constrained_window/constrained_window_views_unittest.cc

https://codereview.chromium.org/2758323002/diff/490001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.h (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.h:154: views::TestViewsDelegate*
views_delegate();
On 2017/04/07 14:07:59, kylix_rd wrote:
> On 2017/04/06 20:05:33, Peter Kasting wrote:
> > On 2017/04/06 16:48:19, kylix_rd wrote:
> > > On 2017/04/05 19:30:10, Peter Kasting wrote:
> > > > On 2017/04/04 20:28:23, kylix_rd wrote:
> > > > > On 2017/04/04 02:08:55, Peter Kasting wrote:
> > > > > > Nit: This should be defined here or else named CamelCase.
> > > > > 
> > > > > Renamed to test_views_delegate().
> > > > 
> > > > That addresses the second concern I had, but not this one: we use
> > > > unix_hacker_names() to indicate inlined cheap accessors, so if this is
> named
> > > > that way it should also be inlined.  If for some reason it doesn't make
> > sense
> > > to
> > > > inline, I'd name it GetTestViewsDelegate() to indicate that.
> > > 
> > > Hmmm. I was looking more for consistency with the other test classes which
> > also
> > > have test_views_delegate().
> > 
> > That's fine, just inline it then :)
> > 
> > The actual function definition is simple enough that inlining seems
> reasonable.
> 
> I'll need to verify that including test_views_delegate.h in the header doesn't
> cause any other issues. Right now, TestViewsDelegate is forward declared here
in
> the header and the .cc file includes the full definition.

Done.

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_layout_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/590001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_layout_delegate.h:15: enum class
ChromeInsetsMetric {
On 2017/04/06 16:48:19, kylix_rd wrote:
> On 2017/04/06 08:13:36, Peter Kasting wrote:
> > (Do you think we're imminently going to add new values?)
> 
> As I worked on this patch, I saw several places in the code that would likely
> require a chrome-side-specific Inset. I didn't change them at this point since
> this is more about the underlying mechanism. I suppose it all depends on what
is
> a reasonable notion of "imminently". I would say its likely within the next
> couple of weeks.

I went ahead and removed the override for GetInsetsMetric(). My reasoning for
keeping in it was that it would serve as a proper boilerplate for any additions
required on the Chrome side. However, the GetDistanceMetric() override clearly
demonstrates this, so my concern is a bit flat. This is regardless of which enum
approach we settle on.

[commit-bot: I haz the power, 2017-04-07 18:57:04]

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

[commit-bot: I haz the power, 2017-04-07 18:57:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2017-04-07 19:02:50]

constrained_window_views_unittest.cc lgtm, didn't look elsewhere.

[kylix_rd, 2017-04-07 19:34:33]

Patchset #14 (id:650001) has been deleted

[kylix_rd, 2017-04-07 19:34:51]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 19:35:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 20:07:01]

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

[commit-bot: I haz the power, 2017-04-07 20:07:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kylix_rd, 2017-04-07 20:11:14]

Patchset #14 (id:670001) has been deleted

[kylix_rd, 2017-04-07 20:11:44]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 20:12:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 20:17:29]

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

[commit-bot: I haz the power, 2017-04-07 20:17:31]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kylix_rd, 2017-04-11 15:43:48]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 15:44:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kylix_rd, 2017-04-11 15:59:28]

Patchset #15 (id:690001) has been deleted

[kylix_rd, 2017-04-11 15:59:47]

Patchset #14 (id:670001) has been deleted

[commit-bot: I haz the power, 2017-04-11 16:24:43]

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

[commit-bot: I haz the power, 2017-04-11 16:24:45]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kylix_rd, 2017-04-11 16:32:20]

Patchset #14 (id:710001) has been deleted

[kylix_rd, 2017-04-11 16:33:35]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 16:34:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 17:45:19]

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

[commit-bot: I haz the power, 2017-04-11 17:45:21]

Dry run: This issue passed the CQ dry run.

[kylix_rd, 2017-04-12 00:03:33]

The next patch will have the requested name change from LayoutDelegate to
LayoutProvider (along with descendants). It will also have the changes to the
enums to conform to the pattern set forth in style/typography.h &
style/typography_provider.h

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/2758323002/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/chrome_views_delegate.cc:66: #if !defined(OS_WIN)
On 2017/04/07 18:02:39, Peter Kasting wrote:
> On 2017/04/07 14:07:58, kylix_rd wrote:
> > On 2017/04/06 20:05:33, Peter Kasting wrote:
> > > On 2017/04/06 16:48:19, kylix_rd wrote:
> > > > On 2017/04/05 19:30:10, Peter Kasting wrote:
> > > > > On 2017/04/04 20:28:22, kylix_rd wrote:
> > > > > > On 2017/04/04 02:08:51, Peter Kasting wrote:
> > > > > > > Nit: Can now probably kill this #ifdef by initializing the
remaining
> > > > > Win-only
> > > > > > > bits in the class declaration
> > > > > > 
> > > > > > Not sure how to initialize weak_factory_ within the class
declaration.
> > > > > 
> > > > > = base::WeakPtrFactory<ChromeViewsDelegate>(this);
> > > > > 
> > > > > ?
> > > > 
> > > > That didn't work. "error: call to deleted constructor of ..."
> > > 
> > > Weird, I don't know what that error means in this context.  Oh well, not
> worth
> > > worrying about.
> > 
> > The implicit constructor is explicitly deleted with the
> > DISALLOW_IMPLICIT_CONSTRUCTORS() macro on it's declaration:
> > https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?type=cs&l=284
> 
> Yeah, but we're calling the explicit (one-arg) constructor.
> 
> Unless this requires the availability of the copy constructor as well... I bet
> that's it.
> 
> I bet this could be done like this:
> 
>   base::WeakPtrFactory<ChromeViewsDelegate> weak_factory_{this};
> 
> ...which would fit the variable initialization guide saying
brace-initialization
> is fine when other methods won't work or are inconvenient.

That worked.

[kylix_rd, 2017-04-12 13:12:04]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 13:12:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 13:25:12]

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

[commit-bot: I haz the power, 2017-04-12 13:25:14]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[kylix_rd, 2017-04-12 13:40:27]

Patchset #15 (id:750001) has been deleted

[kylix_rd, 2017-04-12 13:40:47]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 13:41:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kylix_rd, 2017-04-12 15:03:37]

Finished refactoring of Insets and Distance enums along with change from
LayoutDelegate to LayoutProvider.

This should be very close to the final revision.

[commit-bot: I haz the power, 2017-04-12 17:22:43]

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

[commit-bot: I haz the power, 2017-04-12 17:22:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2017-04-12 21:37:44]

LGTM with one substantial comment

Thanks for the endless slog here!  I don't know whether it makes sense to pursue
namespacing some of this stuff as views::layout:: (a la the style:: namespace)
as a followup -- I'll leave that decision to you.

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/collected_cookies_views.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/collected_cookies_views.cc:319: if
(ChromeLayoutProvider::Get()->UseExtraDialogPadding())
Nit: Pull layout provider out to a temp for reuse?

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/collected_cookies_views.cc:399:
DISTANCE_UNRELATED_CONTROL_VERTICAL));
Nit: Pull vertical distance out to a temp for reuse?

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/collected_cookies_views.cc:462:
DISTANCE_UNRELATED_CONTROL_VERTICAL));
Nit: Pull vertical distance out to a temp for reuse?

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/cookie_info_view.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/cookie_info_view.cc:153:
provider->GetDistanceMetric(DISTANCE_DIALOG_BUTTON_MARGIN));
Nit: Use temp for side inset value?

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc
(right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:47:
column_set->AddPaddingColumn(
I think this is a bad merge -- this code got deleted on trunk since one of your
previous patchsets.

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
(right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:272:
ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
Nit: Move earlier, then reuse this temp above

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/global_error_bubble_view.cc:129:
layout->AddPaddingRow(0, ChromeLayoutProvider::Get()->GetDistanceMetric(
Nit: I guess this could be LayoutProvider::Get()?

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_layout_provider.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_layout_provider.cc:34: case
views::DISTANCE_RELATED_CONTROL_VERTICAL:
Nit: I think it would be less fragile, maintenance-wise, to remove the explicit
values here and instead have a default case that makes the call-to-base-class. 
Since the base class still NOTREACHED()s on unknown values, we don't lose any
runtime safety that way.

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_layout_provider.h (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_layout_provider.h:18:
DISTANCE_DIALOG_BUTTON_MARGIN = views::VIEWS_DISTANCE_END,
Nit: Alphabetize?

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_layout_provider.h:88: // types, e.g. by
adding a new LayoutDistance value that means what you need.
Nit: LayoutDistance value -> distance metric

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/harmony_layout_provider.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_layout_provider.cc:77:
views::GridLayout::Alignment
Nit: Blank line above

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/layout_provider_unittest.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_provider_unittest.cc:198:
ChromeLayoutProvider layout_delegate;
Nit: |layout_provider|

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_provider_unittest.cc:248:
std::unique_ptr<views::LayoutProvider> layout_delegate =
Nit: |layout_provider|; can just be created on the stack as above, no need for
unique_ptr

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
(right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:109:
ChromeLayoutProvider* delegate = ChromeLayoutProvider::Get();
Nit: |provider|

https://codereview.chromium.org/2758323002/diff/770001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.h (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.h:38: 
Nit: Either remove this blank line or add one above the closing #endif (I'd do
the former)

https://codereview.chromium.org/2758323002/diff/770001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.h:47: namespace views {
Nit: I suggest placing this stuff above the nested #ifdef OS_CHROMEOS above

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/test_vie...
File ui/views/test/test_views_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/test_vie...
ui/views/test/test_views_delegate.h:43: void
set_layout_provider(std::unique_ptr<LayoutProvider> layout_provider) {
Nit: Might want some comments about why this object is managed the way it is,
e.g. "For convenience, we create a layout provider by default, but embedders
that use their own layout provider subclasses may need to set those classes as
the layout providers for their tests."

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/test_vie...
File ui/views/test/test_views_delegate_aura.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/test_vie...
ui/views/test/test_views_delegate_aura.cc:22:
layout_provider_(base::MakeUnique<LayoutProvider>()) {}
Nit: Can we avoid the need to copy this code into both the _aura and _mac files
by initializing all these in the declaration?

[sky, 2017-04-12 22:43:58]

LGTM - I'm not sure why you felt the need to rename ViewsDelegate
members/accessors to test_views_delegate. I would be inclined to leave them as
views_delegate_. But if you feel strongly, leave what you have.

https://codereview.chromium.org/2758323002/diff/770001/ash/test/ash_test_help...
File ash/test/ash_test_helper.h (right):

https://codereview.chromium.org/2758323002/diff/770001/ash/test/ash_test_help...
ash/test/ash_test_helper.h:153: std::unique_ptr<AshTestViewsDelegate>
test_views_delegate_;
optional: Personally I wouldn't bother with changing this name. views_delegate_
is fine.

https://codereview.chromium.org/2758323002/diff/770001/ui/views/layout/layout...
File ui/views/layout/layout_provider.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/ui/views/layout/layout...
ui/views/layout/layout_provider.cc:16: static LayoutProvider* g_layout_delegate
= nullptr;
We generally don't use static in cases like this.

https://codereview.chromium.org/2758323002/diff/770001/ui/views/layout/layout...
ui/views/layout/layout_provider.cc:20: g_layout_delegate = this;
DCHECK(g_layout_delegate == nullptr)?

https://codereview.chromium.org/2758323002/diff/770001/ui/views/layout/layout...
File ui/views/layout/layout_provider.h (right):

https://codereview.chromium.org/2758323002/diff/770001/ui/views/layout/layout...
ui/views/layout/layout_provider.h:72: static LayoutProvider* Get();
Please document this never returns null.

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/scoped_v...
File ui/views/test/scoped_views_test_helper.h (right):

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/scoped_v...
ui/views/test/scoped_views_test_helper.h:42: };
remove trailing ;

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/scoped_v...
ui/views/test/scoped_views_test_helper.h:49: std::unique_ptr<TestViewsDelegate>
test_views_delegate_;
optional: leave existing name.

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/test_vie...
File ui/views/test/test_views_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/test_vie...
ui/views/test/test_views_delegate.h:43: void
set_layout_provider(std::unique_ptr<LayoutProvider> layout_provider) {
Given this runs a destructor I wouldn't inline it. e.g. call this
SetLayoutProvider and move implementation to .cc.

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/views_te...
File ui/views/test/views_test_base.h (right):

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/views_te...
ui/views/test/views_test_base.h:52: TestViewsDelegate* test_views_delegate()
const {
optional: leave existing name.

[Peter Kasting, 2017-04-12 22:48:00]

https://codereview.chromium.org/2758323002/diff/770001/ash/test/ash_test_help...
File ash/test/ash_test_helper.h (right):

https://codereview.chromium.org/2758323002/diff/770001/ash/test/ash_test_help...
ash/test/ash_test_helper.h:153: std::unique_ptr<AshTestViewsDelegate>
test_views_delegate_;
On 2017/04/12 22:43:57, sky wrote:
> optional: Personally I wouldn't bother with changing this name.
views_delegate_
> is fine.

I asked for these changes, because the callers accessing these call methods that
only exist on TestViewsDelegate and not ViewsDelegate, and I was getting
confused reading them.

[sky, 2017-04-12 22:48:59]

Fair enough.

On Wed, Apr 12, 2017 at 3:47 PM, <pkasting@chromium.org> wrote:

>
> https://codereview.chromium.org/2758323002/diff/770001/
> ash/test/ash_test_helper.h
> File ash/test/ash_test_helper.h (right):
>
> https://codereview.chromium.org/2758323002/diff/770001/
> ash/test/ash_test_helper.h#newcode153
> ash/test/ash_test_helper.h:153: std::unique_ptr<AshTestViewsDelegate>
> test_views_delegate_;
> On 2017/04/12 22:43:57, sky wrote:
> > optional: Personally I wouldn't bother with changing this name.
> views_delegate_
> > is fine.
>
> I asked for these changes, because the callers accessing these call
> methods that only exist on TestViewsDelegate and not ViewsDelegate, and
> I was getting confused reading them.
>
> https://codereview.chromium.org/2758323002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kylix_rd, 2017-04-13 15:00:23]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 15:00:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 15:35:53]

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

[commit-bot: I haz the power, 2017-04-13 15:35:55]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[kylix_rd, 2017-04-13 15:39:43]

Patchset #16 (id:790001) has been deleted

[kylix_rd, 2017-04-13 15:40:16]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 15:40:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 16:45:41]

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

[kylix_rd, 2017-04-13 16:45:43]

Nearly all the feedback should be addressed now. Will land soon.

> I don't know whether it makes sense to pursue namespacing
> some of this stuff as views::layout:: (a la the
> style::namespace) as a followup -- I'll leave that
> decision to you.

I'd like to do that in a followup. The downside I see is that the enums in
layout_provider.h would then be in views::layout::, but the enums in
chrome_layout_provider.h would be in something like chrome::layout::, right? Is
there ever a case where the embedder would inject into the views::layout::
namespace?

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/collected_cookies_views.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/collected_cookies_views.cc:319: if
(ChromeLayoutProvider::Get()->UseExtraDialogPadding())
On 2017/04/12 21:37:43, Peter Kasting wrote:
> Nit: Pull layout provider out to a temp for reuse?

Done.

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/collected_cookies_views.cc:399:
DISTANCE_UNRELATED_CONTROL_VERTICAL));
On 2017/04/12 21:37:43, Peter Kasting wrote:
> Nit: Pull vertical distance out to a temp for reuse?

Done.

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/collected_cookies_views.cc:462:
DISTANCE_UNRELATED_CONTROL_VERTICAL));
On 2017/04/12 21:37:43, Peter Kasting wrote:
> Nit: Pull vertical distance out to a temp for reuse?

Done.

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/cookie_info_view.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/cookie_info_view.cc:153:
provider->GetDistanceMetric(DISTANCE_DIALOG_BUTTON_MARGIN));
On 2017/04/12 21:37:43, Peter Kasting wrote:
> Nit: Use temp for side inset value?

Done.

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc
(right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:47:
column_set->AddPaddingColumn(
On 2017/04/12 21:37:43, Peter Kasting wrote:
> I think this is a bad merge -- this code got deleted on trunk since one of
your
> previous patchsets.

Done.

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
(right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:272:
ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
On 2017/04/12 21:37:43, Peter Kasting wrote:
> Nit: Move earlier, then reuse this temp above

Done.

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/global_error_bubble_view.cc:129:
layout->AddPaddingRow(0, ChromeLayoutProvider::Get()->GetDistanceMetric(
On 2017/04/12 21:37:43, Peter Kasting wrote:
> Nit: I guess this could be LayoutProvider::Get()?

Technically, yes. ChromeLayoutProvider already had to be used in
ShouldShowWindowIcon() above, so the same is here for consistency.

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_layout_provider.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_layout_provider.cc:34: case
views::DISTANCE_RELATED_CONTROL_VERTICAL:
On 2017/04/12 21:37:43, Peter Kasting wrote:
> Nit: I think it would be less fragile, maintenance-wise, to remove the
explicit
> values here and instead have a default case that makes the call-to-base-class.

> Since the base class still NOTREACHED()s on unknown values, we don't lose any
> runtime safety that way.

Done.

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/chrome_layout_provider.h (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/chrome_layout_provider.h:18:
DISTANCE_DIALOG_BUTTON_MARGIN = views::VIEWS_DISTANCE_END,
On 2017/04/12 21:37:44, Peter Kasting wrote:
> Nit: Alphabetize?

Done.

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/harmony_layout_provider.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_layout_provider.cc:77:
views::GridLayout::Alignment
On 2017/04/12 21:37:44, Peter Kasting wrote:
> Nit: Blank line above

Done.

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/layout_provider_unittest.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_provider_unittest.cc:198:
ChromeLayoutProvider layout_delegate;
On 2017/04/12 21:37:44, Peter Kasting wrote:
> Nit: |layout_provider|

Done.

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/layout_provider_unittest.cc:248:
std::unique_ptr<views::LayoutProvider> layout_delegate =
On 2017/04/12 21:37:44, Peter Kasting wrote:
> Nit: |layout_provider|; can just be created on the stack as above, no need for
> unique_ptr

Not really. It needs to create the HarmonyLayoutProvicer (the lines above force
Harmony mode). This exercises the CreateLayoutProvider() provider selection code
as well.

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
(right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc:109:
ChromeLayoutProvider* delegate = ChromeLayoutProvider::Get();
On 2017/04/12 21:37:44, Peter Kasting wrote:
> Nit: |provider|

Done.

https://codereview.chromium.org/2758323002/diff/770001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.h (right):

https://codereview.chromium.org/2758323002/diff/770001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.h:38: 
On 2017/04/12 21:37:44, Peter Kasting wrote:
> Nit: Either remove this blank line or add one above the closing #endif (I'd do
> the former)

Done.

https://codereview.chromium.org/2758323002/diff/770001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.h:47: namespace views {
On 2017/04/12 21:37:44, Peter Kasting wrote:
> Nit: I suggest placing this stuff above the nested #ifdef OS_CHROMEOS above

Done.

https://codereview.chromium.org/2758323002/diff/770001/ui/views/layout/layout...
File ui/views/layout/layout_provider.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/ui/views/layout/layout...
ui/views/layout/layout_provider.cc:16: static LayoutProvider* g_layout_delegate
= nullptr;
On 2017/04/12 22:43:57, sky (OOO) wrote:
> We generally don't use static in cases like this.

Done.

https://codereview.chromium.org/2758323002/diff/770001/ui/views/layout/layout...
ui/views/layout/layout_provider.cc:20: g_layout_delegate = this;
On 2017/04/12 22:43:57, sky (OOO) wrote:
> DCHECK(g_layout_delegate == nullptr)?

This cannot be done because there are cases where a default base layout provider
is created and then later a more specific one may be created. This will ensure
that the "last one created, wins". See the unit tests where
set_layout_provider() is called.

https://codereview.chromium.org/2758323002/diff/770001/ui/views/layout/layout...
File ui/views/layout/layout_provider.h (right):

https://codereview.chromium.org/2758323002/diff/770001/ui/views/layout/layout...
ui/views/layout/layout_provider.h:72: static LayoutProvider* Get();
On 2017/04/12 22:43:57, sky (OOO) wrote:
> Please document this never returns null.

Done.

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/scoped_v...
File ui/views/test/scoped_views_test_helper.h (right):

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/scoped_v...
ui/views/test/scoped_views_test_helper.h:42: };
On 2017/04/12 22:43:58, sky (OOO) wrote:
> remove trailing ;

Done.

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/test_vie...
File ui/views/test/test_views_delegate.h (right):

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/test_vie...
ui/views/test/test_views_delegate.h:43: void
set_layout_provider(std::unique_ptr<LayoutProvider> layout_provider) {
On 2017/04/12 21:37:44, Peter Kasting wrote:
> Nit: Might want some comments about why this object is managed the way it is,
> e.g. "For convenience, we create a layout provider by default, but embedders
> that use their own layout provider subclasses may need to set those classes as
> the layout providers for their tests."

Done.

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/test_vie...
ui/views/test/test_views_delegate.h:43: void
set_layout_provider(std::unique_ptr<LayoutProvider> layout_provider) {
On 2017/04/12 22:43:58, sky (OOO) wrote:
> Given this runs a destructor I wouldn't inline it. e.g. call this
> SetLayoutProvider and move implementation to .cc.

There are two separate .cc/mm files, test_views_delegate_aura.cc and
test_views_delegate_mac.mm. I'd have to add a platform-agnostic .cc file or
duplicate the code in each one.

I'd rather push this to a separate clean up CL.

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/test_vie...
File ui/views/test/test_views_delegate_aura.cc (right):

https://codereview.chromium.org/2758323002/diff/770001/ui/views/test/test_vie...
ui/views/test/test_views_delegate_aura.cc:22:
layout_provider_(base::MakeUnique<LayoutProvider>()) {}
On 2017/04/12 21:37:44, Peter Kasting wrote:
> Nit: Can we avoid the need to copy this code into both the _aura and _mac
files
> by initializing all these in the declaration?

Done.

[commit-bot: I haz the power, 2017-04-13 16:45:43]

Dry run: This issue passed the CQ dry run.

[kylix_rd, 2017-04-13 16:46:12]

Patchset #16 (id:790001) has been deleted

[kylix_rd, 2017-04-13 16:46:31]

The CQ bit was checked by kylixrd@chromium.org

[kylix_rd, 2017-04-13 16:46:31]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
benwells@chromium.org, oshima@chromium.org, msw@chromium.org,
pkasting@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2758323002/#ps810001
(title: "Final feedback addressed")

[commit-bot: I haz the power, 2017-04-13 16:47:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 17:16:01]

CQ is committing da patch.

Bot data: {"patchset_id": 810001, "attempt_start_ts": 1492101991022850,
"parent_rev": "1262351d14e829049153dfe0b8f14b21c4b3b909", "commit_rev":
"4e8cac4101f13be9f86b455f06ec7b46fc707e93"}

[commit-bot: I haz the power, 2017-04-13 17:21:18]

CQ is committing da patch.

Bot data: {"patchset_id": 810001, "attempt_start_ts": 1492101991022850,
"parent_rev": "1262351d14e829049153dfe0b8f14b21c4b3b909", "commit_rev":
"4e8cac4101f13be9f86b455f06ec7b46fc707e93"}

[commit-bot: I haz the power, 2017-04-13 17:21:26]

Description was changed from

==========
Broke out layout metric information from ViewsDelegate to ViewsLayoutDelegate

BUG=687349
TBR=msw@chromium.org
==========

to

==========
Broke out layout metric information from ViewsDelegate to ViewsLayoutDelegate

BUG=687349
TBR=msw@chromium.org

Review-Url: https://codereview.chromium.org/2758323002
Cr-Commit-Position: refs/heads/master@{#464448}
Committed:
https://chromium.googlesource.com/chromium/src/+/4e8cac4101f13be9f86b455f06ec...
==========

[commit-bot: I haz the power, 2017-04-13 17:21:35]

Committed patchset #16 (id:810001) as
https://chromium.googlesource.com/chromium/src/+/4e8cac4101f13be9f86b455f06ec...

[Bret, 2017-04-13 23:16:32]

FYI: I just pulled this and I'm not getting Harmony layouts any more... it looks
like ChromeLayoutProvider::CreateLayoutProvider() is being called before
MaterialDesignController::Initialize() and so it's not picking up the
--secondary-ui-md flag. I can work around it but I'm not sure how to fix it.

[kylix_rd, 2017-04-14 16:57:06]

On 2017/04/13 23:16:32, Bret Sepulveda wrote:
> FYI: I just pulled this and I'm not getting Harmony layouts any more... it
looks
> like ChromeLayoutProvider::CreateLayoutProvider() is being called before
> MaterialDesignController::Initialize() and so it's not picking up the
> --secondary-ui-md flag. I can work around it but I'm not sure how to fix it.

Ugh... I'll take a look.

[kylix_rd, 2017-04-14 17:04:43]

On 2017/04/14 16:57:06, kylix_rd wrote:
> On 2017/04/13 23:16:32, Bret Sepulveda wrote:
> > FYI: I just pulled this and I'm not getting Harmony layouts any more... it
> looks
> > like ChromeLayoutProvider::CreateLayoutProvider() is being called before
> > MaterialDesignController::Initialize() and so it's not picking up the
> > --secondary-ui-md flag. I can work around it but I'm not sure how to fix it.
> 
> Ugh... I'll take a look.

In c/b/ui/views/chrome_browser_main_extra_parts_views.cc, move the
CreateLayoutProvider() call block to
ChromeBrowserMainExtraPartsViews::PreCreateThreads().