Top Chrome MD cleanup - get rid of most of GetLayoutInsets.

Top Chrome MD cleanup - get rid of most of GetLayoutInsets.

BUG=660445
Committed: https://crrev.com/90509e4e9bf1be47bd971792af7ab764f2b29489
Cr-Commit-Position: refs/heads/master@{#435529}

[Evan Stade, 2016-11-22 17:51:58]

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

[commit-bot: I haz the power, 2016-11-22 17:52:11]

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

[Evan Stade, 2016-11-22 17:55:38]

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

[Evan Stade, 2016-11-22 17:55:38]

Mostly I just tried to consolidate equal values, but sometimes there were parts
of an Insets which were used in multiple files (e.g. top/bottom), and parts
which were not (left/right only used in one file). In those cases I decided to
split the values but admittedly it's a bit of a judgment call.

[commit-bot: I haz the power, 2016-11-22 19:13:17]

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

[commit-bot: I haz the power, 2016-11-22 19:13:18]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-11-22 22:12:08]

LGTM

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/layout_co...
File chrome/browser/ui/layout_constants.h (right):

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/layout_co...
chrome/browser/ui/layout_constants.h:64: // Spacing inside toolbar button,
between its border and image.
Nit: Spacing -> Padding

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:411: const
int pad = GetLayoutConstant(AVATAR_ICON_PADDING);
Nit: "avoid abbreviation" (
http://google.github.io/styleguide/cppguide.html#General_Naming_Rules ) suggests
using "padding" instead of "pad" for variable and constant names like this. 
(Multiple files)

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

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:152:
contents_rect.Inset(gfx::Insets(kPopupVerticalPadding, 0));
Nit: contents_rect.Inset(0, kPopupVerticalPadding);

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

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:695: : 4;
Nit: Shorter:

  using MD = ui::MaterialDesignController;
  const int kIconPadding = (MD::GetMode() == MD::MATERIAL_HYBRID) ? 8 : 4;

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

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_view.cc:113: : 4;
Nit: Shorter:

  using MD = ui::MaterialDesignController;
  return (MD::GetMode() == MD::MATERIAL_HYBRID) ? 8 : 4;

[Evan Stade, 2016-11-29 03:01:45]

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/layout_co...
File chrome/browser/ui/layout_constants.h (right):

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/layout_co...
chrome/browser/ui/layout_constants.h:64: // Spacing inside toolbar button,
between its border and image.
On 2016/11/22 22:12:07, Peter Kasting wrote:
> Nit: Spacing -> Padding

Done.

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:411: const
int pad = GetLayoutConstant(AVATAR_ICON_PADDING);
On 2016/11/22 22:12:08, Peter Kasting wrote:
> Nit: "avoid abbreviation" (
> http://google.github.io/styleguide/cppguide.html#General_Naming_Rules )
suggests
> using "padding" instead of "pad" for variable and constant names like this. 
> (Multiple files)

Pad is actually a synonym of padding (and much older to boot). Granted,
"padding" is more idiomatic in this context. How strongly do you feel about
this?

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

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:152:
contents_rect.Inset(gfx::Insets(kPopupVerticalPadding, 0));
On 2016/11/22 22:12:08, Peter Kasting wrote:
> Nit: contents_rect.Inset(0, kPopupVerticalPadding);

I strongly dislike that Rect's Inset methods mess with the order of parameters*,
so I try to avoid ever using them and hope one day to delete them (just leaving
the gfx::Insets version). I am confident this difference causes unmentioned
headaches if not outright bugs, and I am constantly in fear of making the
mistake every time I deal with insets because I can never quite remember which
method it is that rearranges the arguments. 

*compared to gfx::Insets and the Border factory functions

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

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:695: : 4;
On 2016/11/22 22:12:08, Peter Kasting wrote:
> Nit: Shorter:
> 
>   using MD = ui::MaterialDesignController;
>   const int kIconPadding = (MD::GetMode() == MD::MATERIAL_HYBRID) ? 8 : 4;

done (with slight adjustments)

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

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_view.cc:113: : 4;
On 2016/11/22 22:12:08, Peter Kasting wrote:
> Nit: Shorter:
> 
>   using MD = ui::MaterialDesignController;
>   return (MD::GetMode() == MD::MATERIAL_HYBRID) ? 8 : 4;

done (with slight adjustments)

[Evan Stade, 2016-11-29 03:01:50]

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

[commit-bot: I haz the power, 2016-11-29 03:02:34]

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

[Peter Kasting, 2016-11-29 03:38:28]

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:411: const
int pad = GetLayoutConstant(AVATAR_ICON_PADDING);
On 2016/11/29 03:01:45, Evan Stade wrote:
> On 2016/11/22 22:12:08, Peter Kasting wrote:
> > Nit: "avoid abbreviation" (
> > http://google.github.io/styleguide/cppguide.html#General_Naming_Rules )
> suggests
> > using "padding" instead of "pad" for variable and constant names like this. 
> > (Multiple files)
> 
> Pad is actually a synonym of padding (and much older to boot). Granted,
> "padding" is more idiomatic in this context. How strongly do you feel about
> this?

Do we use it very much elsewhere in views?  I certainly read it as an
abbreviation, but if it's a common term in the codebase, then I don't object. 
If not, and we use "padding" most places today, it seems like it would be better
to use that.

I am willing to leave this to your judgment.

[commit-bot: I haz the power, 2016-11-29 03:38:50]

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

[commit-bot: I haz the power, 2016-11-29 03:38:51]

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_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Evan Stade, 2016-11-30 00:39:22]

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/2523783002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:411: const
int pad = GetLayoutConstant(AVATAR_ICON_PADDING);
On 2016/11/29 03:38:28, Peter Kasting wrote:
> On 2016/11/29 03:01:45, Evan Stade wrote:
> > On 2016/11/22 22:12:08, Peter Kasting wrote:
> > > Nit: "avoid abbreviation" (
> > > http://google.github.io/styleguide/cppguide.html#General_Naming_Rules )
> > suggests
> > > using "padding" instead of "pad" for variable and constant names like
this. 
> > > (Multiple files)
> > 
> > Pad is actually a synonym of padding (and much older to boot). Granted,
> > "padding" is more idiomatic in this context. How strongly do you feel about
> > this?
> 
> Do we use it very much elsewhere in views?  I certainly read it as an
> abbreviation, but if it's a common term in the codebase, then I don't object. 
> If not, and we use "padding" most places today, it seems like it would be
better
> to use that.
> 
> I am willing to leave this to your judgment.

I can see both sides. Padding is much more common, but personally I don't think
pad detracts from understanding and it makes things more concise.

[Evan Stade, 2016-11-30 00:39:40]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-11-30 00:39:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2523783002/#ps30013
(title: "fix typo")

[commit-bot: I haz the power, 2016-11-30 00:41:03]

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, 2016-11-30 02:43:08]

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

[commit-bot: I haz the power, 2016-11-30 02:43:09]

Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Evan Stade, 2016-11-30 23:58:50]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-11-30 23:59:31]

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, 2016-12-01 02:23:12]

CQ is committing da patch.

Bot data: {"patchset_id": 30013, "attempt_start_ts": 1480550330649580,
"parent_rev": "0b24a26f20cd94d944a10f4db606e5a17dcf1570", "commit_rev":
"b699658c76b0a6b0a65b1d2a35996d2628b8f2c6"}

[commit-bot: I haz the power, 2016-12-01 02:23:37]

Committed patchset #3 (id:30013)

[commit-bot: I haz the power, 2016-12-01 02:26:21]

Description was changed from

==========
Top Chrome MD cleanup - get rid of most of GetLayoutInsets.

BUG=660445
==========

to

==========
Top Chrome MD cleanup - get rid of most of GetLayoutInsets.

BUG=660445

Committed: https://crrev.com/90509e4e9bf1be47bd971792af7ab764f2b29489
Cr-Commit-Position: refs/heads/master@{#435529}
==========

[commit-bot: I haz the power, 2016-12-01 02:26:22]

Patchset 3 (id:??) landed as
https://crrev.com/90509e4e9bf1be47bd971792af7ab764f2b29489
Cr-Commit-Position: refs/heads/master@{#435529}