Create c/b/ui/views/layout_constants.*.

Create c/b/ui/views/layout_constants.*.

This becomes a global place to get views layout constants which need some kind
of parametrized getter, in these cases because of Material Design.  Previously
these were accessible from ThemeService, but that required having an instance of
a ThemeProvider (infeasible in some contexts) and implied these were
theme-related in some way, which isn't true.

BUG=531544
TEST=none

Committed: https://crrev.com/903dceff91f3fb0a3f48343f3366eb715b4698d8
Cr-Commit-Position: refs/heads/master@{#348967}

[Peter Kasting, 2015-09-12 00:38:45]

pkasting@chromium.org changed reviewers:
+ jonross@chromium.org, pkotwicz@chromium.org

[Peter Kasting, 2015-09-12 00:38:46]

jonross: Review
pkotwicz: OWNER for chrome/browser/themes/

[jonross, 2015-09-14 15:27:10]

A few points, but overall I like this simplification. Especially the
incorporation of the Insets method.

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
File chrome/browser/ui/views/layout_constants.cc (right):

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
chrome/browser/ui/views/layout_constants.cc:11: const int
kIconLabelViewTrailingPadding[] = {2, 8, 8};
Do we want to bring in any of the comments in theme_properties.cc to document
what each of these are for?

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
chrome/browser/ui/views/layout_constants.cc:24: const int mode =
ui::MaterialDesignController::GetMode();
This will impact unittesting. 

The MaterialDesignController keeping the mode static has let bruthig@ to
introduce an uninitialize method to allow for testing:

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
File chrome/browser/ui/views/layout_constants.h (right):

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
chrome/browser/ui/views/layout_constants.h:10: enum LayoutConstant {
Any concerns about these being in the global namespace?

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:121:
contents_rect.Inset(0, views::NonClientFrameView::kClientEdgeThickness, 0, 0);
Incorporate into OMNIBOX_DROPDOWN_TEXT?

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.cc:194:
border->set_insets(insets);
Nit: border->set_insets(GetLayoutInsets(TOOLBAR_BUTTON));

[tdanderson, 2015-09-14 15:33:08]

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

[tdanderson, 2015-09-14 15:33:09]

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
File chrome/browser/ui/views/layout_constants.h (right):

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
chrome/browser/ui/views/layout_constants.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
In https://chromiumcodereview.appspot.com/1306173002/ I want to return a default
font size based on the material flag. That's not technically a "layout constant"
so where should that live?

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
chrome/browser/ui/views/layout_constants.h:54: OMNIBOX_DROPDOWN_ICON,
nit: It would be nice to have documentation for each of these items like is done
in the LayoutConstant enum.

[tdanderson, 2015-09-14 15:37:46]

On 2015/09/14 15:33:09, tdanderson wrote:
>
https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
> File chrome/browser/ui/views/layout_constants.h (right):
> 
>
https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
> chrome/browser/ui/views/layout_constants.h:1: // Copyright 2015 The Chromium
> Authors. All rights reserved.
> In https://chromiumcodereview.appspot.com/1306173002/ I want to return a
default
> font size based on the material flag. That's not technically a "layout
constant"
> so where should that live?
> 
>
https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
> chrome/browser/ui/views/layout_constants.h:54: OMNIBOX_DROPDOWN_ICON,
> nit: It would be nice to have documentation for each of these items like is
done
> in the LayoutConstant enum.

Also, please link to bug 531544 in the CL description.

[Peter Kasting, 2015-09-14 21:06:31]

PTAL

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
File chrome/browser/ui/views/layout_constants.cc (right):

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
chrome/browser/ui/views/layout_constants.cc:11: const int
kIconLabelViewTrailingPadding[] = {2, 8, 8};
On 2015/09/14 15:27:10, jonross wrote:
> Do we want to bring in any of the comments in theme_properties.cc to document
> what each of these are for?

I put those comments in the header file to document what the enums mean.  That
seems more appropriate.

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
chrome/browser/ui/views/layout_constants.cc:24: const int mode =
ui::MaterialDesignController::GetMode();
On 2015/09/14 15:27:09, jonross wrote:
> This will impact unittesting. 
> 
> The MaterialDesignController keeping the mode static has let bruthig@ to
> introduce an uninitialize method to allow for testing:
> 
>
https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...

I'm not sure what you're asking me to do here.  The implementation of this is
the same as in ThemeProvider, where we ask the MaterialDesignController for the
mode, so I don't see how my change makes testing different than it previously
was.

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
File chrome/browser/ui/views/layout_constants.h (right):

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
chrome/browser/ui/views/layout_constants.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/09/14 15:33:09, tdanderson wrote:
> In https://chromiumcodereview.appspot.com/1306173002/ I want to return a
default
> font size based on the material flag. That's not technically a "layout
constant"
> so where should that live?

I would consider that a layout constant.  It's a constant value related to how
the control lays out its contents (specifically, the text in it).

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
chrome/browser/ui/views/layout_constants.h:10: enum LayoutConstant {
On 2015/09/14 15:27:10, jonross wrote:
> Any concerns about these being in the global namespace?

Not really, since few people should be pulling this file in.

We can always change things if it becomes a problem later.

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
chrome/browser/ui/views/layout_constants.h:54: OMNIBOX_DROPDOWN_ICON,
On 2015/09/14 15:33:08, tdanderson wrote:
> nit: It would be nice to have documentation for each of these items like is
done
> in the LayoutConstant enum.

Done.

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:121:
contents_rect.Inset(0, views::NonClientFrameView::kClientEdgeThickness, 0, 0);
On 2015/09/14 15:27:10, jonross wrote:
> Incorporate into OMNIBOX_DROPDOWN_TEXT?

Not all callers want this inclusion.

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.cc:194:
border->set_insets(insets);
On 2015/09/14 15:27:10, jonross wrote:
> Nit: border->set_insets(GetLayoutInsets(TOOLBAR_BUTTON));

Done.

[jonross, 2015-09-14 23:46:43]

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
File chrome/browser/ui/views/layout_constants.cc (right):

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
chrome/browser/ui/views/layout_constants.cc:24: const int mode =
ui::MaterialDesignController::GetMode();
On 2015/09/14 21:06:31, Peter Kasting wrote:
> On 2015/09/14 15:27:09, jonross wrote:
> > This will impact unittesting. 
> > 
> > The MaterialDesignController keeping the mode static has let bruthig@ to
> > introduce an uninitialize method to allow for testing:
> > 
> >
>
https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
> 
> I'm not sure what you're asking me to do here.  The implementation of this is
> the same as in ThemeProvider, where we ask the MaterialDesignController for
the
> mode, so I don't see how my change makes testing different than it previously
> was.

Sorry for not providing the details.

In ThemeProvider it was not a const int:
int mode = ui::MaterialDesignController::GetMode()

so that changes during unittests would be reflected on subsequent polls for
layout constants.

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
File chrome/browser/ui/views/layout_constants.h (right):

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
chrome/browser/ui/views/layout_constants.h:10: enum LayoutConstant {
On 2015/09/14 21:06:31, Peter Kasting wrote:
> On 2015/09/14 15:27:10, jonross wrote:
> > Any concerns about these being in the global namespace?
> 
> Not really, since few people should be pulling this file in.
> 
> We can always change things if it becomes a problem later.

SGTM

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:121:
contents_rect.Inset(0, views::NonClientFrameView::kClientEdgeThickness, 0, 0);
On 2015/09/14 21:06:31, Peter Kasting wrote:
> On 2015/09/14 15:27:10, jonross wrote:
> > Incorporate into OMNIBOX_DROPDOWN_TEXT?
> 
> Not all callers want this inclusion.

Acknowledged.

[Peter Kasting, 2015-09-14 23:49:43]

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
File chrome/browser/ui/views/layout_constants.cc (right):

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
chrome/browser/ui/views/layout_constants.cc:24: const int mode =
ui::MaterialDesignController::GetMode();
On 2015/09/14 23:46:43, jonross wrote:
> On 2015/09/14 21:06:31, Peter Kasting wrote:
> > On 2015/09/14 15:27:09, jonross wrote:
> > > This will impact unittesting. 
> > > 
> > > The MaterialDesignController keeping the mode static has let bruthig@ to
> > > introduce an uninitialize method to allow for testing:
> > > 
> > >
> >
>
https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
> > 
> > I'm not sure what you're asking me to do here.  The implementation of this
is
> > the same as in ThemeProvider, where we ask the MaterialDesignController for
> the
> > mode, so I don't see how my change makes testing different than it
previously
> > was.
> 
> Sorry for not providing the details.
> 
> In ThemeProvider it was not a const int:
> int mode = ui::MaterialDesignController::GetMode()
> 
> so that changes during unittests would be reflected on subsequent polls for
> layout constants.

This code is still fine from that perspective.  You're thinking of "static
const", this is non-static.

[tdanderson, 2015-09-15 14:29:01]

LGTM (but please wait for Jon's sign-off too)

[jonross, 2015-09-15 14:37:02]

On 2015/09/15 14:29:01, tdanderson wrote:
> LGTM (but please wait for Jon's sign-off too)

LGTM

[jonross, 2015-09-15 14:37:08]

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
File chrome/browser/ui/views/layout_constants.cc (right):

https://codereview.chromium.org/1338103002/diff/1/chrome/browser/ui/views/lay...
chrome/browser/ui/views/layout_constants.cc:24: const int mode =
ui::MaterialDesignController::GetMode();
On 2015/09/14 23:49:43, Peter Kasting wrote:
> On 2015/09/14 23:46:43, jonross wrote:
> > On 2015/09/14 21:06:31, Peter Kasting wrote:
> > > On 2015/09/14 15:27:09, jonross wrote:
> > > > This will impact unittesting. 
> > > > 
> > > > The MaterialDesignController keeping the mode static has let bruthig@ to
> > > > introduce an uninitialize method to allow for testing:
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
> > > 
> > > I'm not sure what you're asking me to do here.  The implementation of this
> is
> > > the same as in ThemeProvider, where we ask the MaterialDesignController
for
> > the
> > > mode, so I don't see how my change makes testing different than it
> previously
> > > was.
> > 
> > Sorry for not providing the details.
> > 
> > In ThemeProvider it was not a const int:
> > int mode = ui::MaterialDesignController::GetMode()
> > 
> > so that changes during unittests would be reflected on subsequent polls for
> > layout constants.
> 
> This code is still fine from that perspective.  You're thinking of "static
> const", this is non-static.

Sorry, my mistake.

[Peter Kasting, 2015-09-15 18:59:24]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2015-09-15 19:00:11]

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

[commit-bot: I haz the power, 2015-09-15 19:16:23]

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

[commit-bot: I haz the power, 2015-09-15 19:16:24]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pkotwicz, 2015-09-15 19:45:47]

lgtm

[pkotwicz, 2015-09-15 19:45:55]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2015-09-15 19:46:43]

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

[commit-bot: I haz the power, 2015-09-15 19:58:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-15 19:59:09]

Patchset 2 (id:??) landed as
https://crrev.com/903dceff91f3fb0a3f48343f3366eb715b4698d8
Cr-Commit-Position: refs/heads/master@{#348967}

[commit-bot: I haz the power, 2015-09-23 12:48:28]

Patchset 2 (id:??) landed as
https://crrev.com/903dceff91f3fb0a3f48343f3366eb715b4698d8
Cr-Commit-Position: refs/heads/master@{#348967}