Native themes: Add menu separator part

Native themes: Add menu separator part

This CL:
* Adds themeable menu separators on platforms other than Windows
* Removes menu_separator_win
* Adds gtk-drawn menu separators on Linux


Besides refactoring, menu separators on Windows, CrOS, and Mac should be
unchanged.

BUG=132847
R=erg@chromium.org,sky@chromium.org

Review-Url: https://codereview.chromium.org/2655553003
Cr-Commit-Position: refs/heads/master@{#446911}
Committed: https://chromium.googlesource.com/chromium/src/+/e3bc84e363afba49f43e8fd391dc8d1da5a80d8c

[Tom (Use chromium acct), 2017-01-25 02:14:10]

Patchset #2 (id:20001) has been deleted

[Tom (Use chromium acct), 2017-01-25 02:14:52]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 02:15:52]

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

[Tom (Use chromium acct), 2017-01-25 02:18:08]

Patchset #2 (id:40001) has been deleted

[Tom (Use chromium acct), 2017-01-25 02:20:00]

Patchset #2 (id:60001) has been deleted

[Tom (Use chromium acct), 2017-01-25 02:24:38]

Description was changed from

==========
Native themes: Add menu separator part

BUG=132847
==========

to

==========
Native themes: Add menu separator part

This CL:
* Adds themeable menu separators on platforms other than Windows
* Removes menu_separator_win
* Adds gtk-drawn menu separators on Linux


Besides refactoring, menu separators on Windows, CrOS, and Mac should be
identical.

BUG=132847
R=erg@chromium.org,sky@chromium.org
==========

[Tom (Use chromium acct), 2017-01-25 02:24:38]

thomasanderson@google.com changed reviewers:
+ erg@chromium.org, sky@chromium.org

[Tom (Use chromium acct), 2017-01-25 02:24:51]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 02:25:27]

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

[Tom (Use chromium acct), 2017-01-25 02:37:20]

reviewers PTAL

erg: libgtkui
sky: everything else

https://codereview.chromium.org/2655553003/diff/80001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtkui/native_theme_gtk3.cc (right):

https://codereview.chromium.org/2655553003/diff/80001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtkui/native_theme_gtk3.cc:128: case
ui::NativeTheme::kColorId_HoverMenuButtonBorderColor:
erg: It's unclear if we want to do this for the vertical separators, or just
return SK_ColorTRANSPARENT.

The context menu in firefox doesn't put vertical separators, but maybe this
doesn't make sense for chrome?
See https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c54
wdyt?

https://codereview.chromium.org/2655553003/diff/80001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtkui/native_theme_gtk3.cc:402: void
NativeThemeGtk3::PaintMenuSeparator(
erg: Just so you know, this lets gtk override chrome when deciding the border
size.

For example, in my testing, Chrome usually wants a border thickness of 1px, but
Ambiance needs 2px for its two-tone separator.

[Tom (Use chromium acct), 2017-01-25 02:38:52]

Description was changed from

==========
Native themes: Add menu separator part

This CL:
* Adds themeable menu separators on platforms other than Windows
* Removes menu_separator_win
* Adds gtk-drawn menu separators on Linux


Besides refactoring, menu separators on Windows, CrOS, and Mac should be
identical.

BUG=132847
R=erg@chromium.org,sky@chromium.org
==========

to

==========
Native themes: Add menu separator part

This CL:
* Adds themeable menu separators on platforms other than Windows
* Removes menu_separator_win
* Adds gtk-drawn menu separators on Linux


Besides refactoring, menu separators on Windows, CrOS, and Mac should be
unchanged.

BUG=132847
R=erg@chromium.org,sky@chromium.org
==========

[Tom (Use chromium acct), 2017-01-25 02:42:19]

Patchset #2 (id:80001) has been deleted

[Tom (Use chromium acct), 2017-01-25 02:42:24]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 02:43:11]

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

[Tom (Use chromium acct), 2017-01-25 02:43:39]

Patchset #1 (id:1) has been deleted

[Tom (Use chromium acct), 2017-01-25 02:44:40]

https://codereview.chromium.org/2655553003/diff/100001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtkui/native_theme_gtk3.cc (right):

https://codereview.chromium.org/2655553003/diff/100001/chrome/browser/ui/libg...
chrome/browser/ui/libgtkui/native_theme_gtk3.cc:128: case
ui::NativeTheme::kColorId_HoverMenuButtonBorderColor:
erg: It's unclear if we want to do this for the vertical separators, or just
return SK_ColorTRANSPARENT.

The context menu in firefox doesn't put vertical separators, but maybe this
doesn't make sense for chrome?
See https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c54
wdyt?

https://codereview.chromium.org/2655553003/diff/100001/chrome/browser/ui/libg...
chrome/browser/ui/libgtkui/native_theme_gtk3.cc:402: void
NativeThemeGtk3::PaintMenuSeparator(
erg: Just so you know, this lets gtk override chrome when deciding the border
size.

For example, in my testing, Chrome usually wants a border thickness of 1px, but
Ambiance needs 2px for its two-tone separator.

[commit-bot: I haz the power, 2017-01-25 04:08:08]

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

[commit-bot: I haz the power, 2017-01-25 04:08:08]

Dry run: This issue passed the CQ dry run.

[Elliot Glaysher, 2017-01-25 18:42:09]

lgtm

https://codereview.chromium.org/2655553003/diff/100001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtkui/native_theme_gtk3.cc (right):

https://codereview.chromium.org/2655553003/diff/100001/chrome/browser/ui/libg...
chrome/browser/ui/libgtkui/native_theme_gtk3.cc:128: case
ui::NativeTheme::kColorId_HoverMenuButtonBorderColor:
On 2017/01/25 02:44:40, Tom Anderson wrote:
> erg: It's unclear if we want to do this for the vertical separators, or just
> return SK_ColorTRANSPARENT.
> 
> The context menu in firefox doesn't put vertical separators, but maybe this
> doesn't make sense for chrome?
> See https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c54
> wdyt?

I'm pretty sure that we want the vertical separators. The firefox buttons are
icons, ours are mostly text.

[sky, 2017-01-25 21:57:22]

https://codereview.chromium.org/2655553003/diff/100001/ui/native_theme/native...
File ui/native_theme/native_theme_base.cc (right):

https://codereview.chromium.org/2655553003/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_base.cc:784: const MenuSeparatorExtraParams&
menu_separator) const {
Why is there rect and menu_separator.paint_rect? AFAICT you never use rect.
Seems better to use rect as an arg.

[Tom (Use chromium acct), 2017-01-25 22:25:44]

https://codereview.chromium.org/2655553003/diff/100001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtkui/native_theme_gtk3.cc (right):

https://codereview.chromium.org/2655553003/diff/100001/chrome/browser/ui/libg...
chrome/browser/ui/libgtkui/native_theme_gtk3.cc:128: case
ui::NativeTheme::kColorId_HoverMenuButtonBorderColor:
On 2017/01/25 18:42:09, Elliot Glaysher wrote:
> On 2017/01/25 02:44:40, Tom Anderson wrote:
> > erg: It's unclear if we want to do this for the vertical separators, or just
> > return SK_ColorTRANSPARENT.
> > 
> > The context menu in firefox doesn't put vertical separators, but maybe this
> > doesn't make sense for chrome?
> > See https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c54
> > wdyt?
> 
> I'm pretty sure that we want the vertical separators. The firefox buttons are
> icons, ours are mostly text.

Acknowledged.

https://codereview.chromium.org/2655553003/diff/100001/ui/native_theme/native...
File ui/native_theme/native_theme_base.cc (right):

https://codereview.chromium.org/2655553003/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_base.cc:784: const MenuSeparatorExtraParams&
menu_separator) const {
On 2017/01/25 21:57:22, sky wrote:
> Why is there rect and menu_separator.paint_rect? AFAICT you never use rect.
> Seems better to use rect as an arg.

rect is the region the paint context is clipped to.  paint_rect is contained in
rect, and is used as a suggestion for where to paint the separator.

rect is used in NativeThemeGtk3, where menu_separator.paint_rect is not.  The
reason for this is Chrome usually wants to render a 1px border, but Gtk borders
might have a minimum size of 2px (eg. when borders are two-tone).

https://codereview.chromium.org/2655553003/diff/100001/ui/native_theme/native...
File ui/native_theme/native_theme_win.h (right):

https://codereview.chromium.org/2655553003/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_win.h:133: const MenuSeparatorExtraParams& extra)
const;
sky: Do you think I should get rid of the extra args I added here and just pass
in extra.paint_rect as rect?

[sky, 2017-01-25 22:40:10]

https://codereview.chromium.org/2655553003/diff/100001/ui/native_theme/native...
File ui/native_theme/native_theme_win.h (right):

https://codereview.chromium.org/2655553003/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_win.h:133: const MenuSeparatorExtraParams& extra)
const;
On 2017/01/25 22:25:44, Tom Anderson wrote:
> sky: Do you think I should get rid of the extra args I added here and just
pass
> in extra.paint_rect as rect?

Yes, I think that would be less confusing.

[Tom (Use chromium acct), 2017-01-25 22:48:06]

Patchset #2 (id:120001) has been deleted

[Tom (Use chromium acct), 2017-01-25 22:48:59]

Patchset #2 (id:140001) has been deleted

[Tom (Use chromium acct), 2017-01-25 22:49:43]

https://codereview.chromium.org/2655553003/diff/100001/ui/native_theme/native...
File ui/native_theme/native_theme_win.h (right):

https://codereview.chromium.org/2655553003/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_win.h:133: const MenuSeparatorExtraParams& extra)
const;
On 2017/01/25 22:40:10, sky wrote:
> On 2017/01/25 22:25:44, Tom Anderson wrote:
> > sky: Do you think I should get rid of the extra args I added here and just
> pass
> > in extra.paint_rect as rect?
> 
> Yes, I think that would be less confusing.

Done.

[sky, 2017-01-26 00:42:32]

LGTM

[Tom (Use chromium acct), 2017-01-26 04:57:03]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 04:57: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-01-26 05:30:01]

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

[commit-bot: I haz the power, 2017-01-26 05:30:02]

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_...)

[Tom (Use chromium acct), 2017-01-28 04:15:49]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-28 04:15:58]

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-01-28 05:39:23]

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

[commit-bot: I haz the power, 2017-01-28 05:39:24]

Dry run: This issue passed the CQ dry run.

[Tom (Use chromium acct), 2017-01-28 06:38:00]

The CQ bit was checked by thomasanderson@google.com

[Tom (Use chromium acct), 2017-01-28 06:38:01]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2655553003/#ps180001
(title: "item->separator")

[commit-bot: I haz the power, 2017-01-28 06:38:07]

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-01-28 06:42:01]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485585480742910,
"parent_rev": "825d96514c3f038ba7575a8eab31f36d05d16f2d", "commit_rev":
"e3bc84e363afba49f43e8fd391dc8d1da5a80d8c"}

[commit-bot: I haz the power, 2017-01-28 06:43:08]

Description was changed from

==========
Native themes: Add menu separator part

This CL:
* Adds themeable menu separators on platforms other than Windows
* Removes menu_separator_win
* Adds gtk-drawn menu separators on Linux


Besides refactoring, menu separators on Windows, CrOS, and Mac should be
unchanged.

BUG=132847
R=erg@chromium.org,sky@chromium.org
==========

to

==========
Native themes: Add menu separator part

This CL:
* Adds themeable menu separators on platforms other than Windows
* Removes menu_separator_win
* Adds gtk-drawn menu separators on Linux


Besides refactoring, menu separators on Windows, CrOS, and Mac should be
unchanged.

BUG=132847
R=erg@chromium.org,sky@chromium.org

Review-Url: https://codereview.chromium.org/2655553003
Cr-Commit-Position: refs/heads/master@{#446911}
Committed:
https://chromium.googlesource.com/chromium/src/+/e3bc84e363afba49f43e8fd391dc...
==========

[commit-bot: I haz the power, 2017-01-28 06:43:08]

Committed patchset #3 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/e3bc84e363afba49f43e8fd391dc...