Use NativeTheme to draw vertical menu separators.

Use NativeTheme to draw vertical menu separators.

Use NativeTheme to draw vertical menu separators ("menu button border") 
so that they look like native horizontal separators instead of being
subtly different.

For now the GTK theme doesn't have a way to draw vertical separators,
so the old way of doing things (SkCanvas::drawRect) is retained, just
moved from app_menu.cc to native_theme_gtk3.cc.

BUG=710159
TBR=sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2813703003
Cr-Commit-Position: refs/heads/master@{#464930}
Committed: https://chromium.googlesource.com/chromium/src/+/944eab72bd2c478b03195680ef033b1d88aa85cc

[Evan Stade, 2017-04-10 21:47:04]

Description was changed from

==========
wip - fix painting of app menu button separators

BUG=
==========

to

==========
Use NativeTheme to draw vertical menu separators.

Use NativeTheme to draw vertical menu separators ("menu button border") 
so that they look like native horizontal separators instead of being
subtly different.

For now the GTK theme doesn't have a way to draw vertical separators,
so the old way of doing things (SkCanvas::drawRect) is retained, just
moved from app_menu.cc to native_theme_gtk3.cc.

BUG=710159
==========

[Evan Stade, 2017-04-10 21:47:05]

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

[Evan Stade, 2017-04-10 21:49:33]

I haven't checked whether this works on Mac and Windows. It does work on Chrome
OS and due to the bandaid* works on GTK. Peter and Trent: it would be great if
you could check whether mac and windows need a similar band-aid.

*see CL desc

https://codereview.chromium.org/2813703003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu.cc (left):

https://codereview.chromium.org/2813703003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu.cc:179: static SkColor
BorderColor(View* view, views::Button::ButtonState state) {
note that this function was only ever called with state == STATE_NORMAL so the
other two colors were never actually used.

[Tom Anderson, 2017-04-10 22:00:09]

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

[Tom Anderson, 2017-04-10 22:00:10]

libgtkui lgtm
Thanks for the cleanup!  Will see if I can add the gtk3 impl once this lands

[Peter Kasting, 2017-04-10 22:08:35]

It looks like PaintMenuSeparator() on windows is implemented in two ways: one
which works on a canvas and definitely will not do the right thing (it always
draws a horizontal line), and one which uses the native theme engine to draw a
MENU_POPUPSEPARATOR, which I suspect also is only horizontal but I'm not sure
about.

[tapted, 2017-04-11 02:48:06]

mac doesn't need an entire bandaid, but it's probably an opportune time to fix a
subtle glitch :)

https://codereview.chromium.org/2813703003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu.cc (right):

https://codereview.chromium.org/2813703003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu.cc:167: gfx::Rect separator_bounds =
gfx::Rect(0, 0, 1, h);
The `1` (i.e. width) on this line should be
MenuConfig::instance().separator_thickness.

https://codereview.chromium.org/2813703003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu.cc:173: bounds.Inset(gfx::Insets(0, 1,
0, 0));
This `1` should be separator_thickness too, so that the background doesn't paint
over the separator.

[Evan Stade, 2017-04-11 17:53:12]

On 2017/04/10 22:08:35, Peter Kasting wrote:
> It looks like PaintMenuSeparator() on windows is implemented in two ways: one
> which works on a canvas and definitely will not do the right thing (it always
> draws a horizontal line), and one which uses the native theme engine to draw a
> MENU_POPUPSEPARATOR, which I suspect also is only horizontal but I'm not sure
> about.

It seems like the version of NativeThemeWin::PaintMenuSeparator that uses
drawLine is the only one that's called. I think the HDC version of that and a
few others has been dead since 2014:
https://chromium.googlesource.com/chromium/src/+/5ed1d964d453ae38835af00ca47e...

Thus I've removed the HDC one and updated the implementation to support drawing
a vertical line. This implementation is untested as I don't have a Windows
machine.

https://codereview.chromium.org/2813703003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu.cc (right):

https://codereview.chromium.org/2813703003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu.cc:167: gfx::Rect separator_bounds =
gfx::Rect(0, 0, 1, h);
On 2017/04/11 02:48:06, tapted wrote:
> The `1` (i.e. width) on this line should be
> MenuConfig::instance().separator_thickness.
> 

Done.

https://codereview.chromium.org/2813703003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu.cc:173: bounds.Inset(gfx::Insets(0, 1,
0, 0));
On 2017/04/11 02:48:06, tapted wrote:
> This `1` should be separator_thickness too, so that the background doesn't
paint
> over the separator.

Ick. This is reserving space in DIP, but we may only want to paint a single
pixel for the separator (<1 dip). So it seems like we should paint the bg first
and the separator on top. Only this doesn't work on GTK because the background
isn't a solid color but itself has a border.

I've taken your suggestion for now but we'll have to keep working on this.

[Evan Stade, 2017-04-11 17:53:18]

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

[commit-bot: I haz the power, 2017-04-11 17:54: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-04-11 18:32:36]

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

[commit-bot: I haz the power, 2017-04-11 18:32:37]

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

[tapted, 2017-04-12 00:31:17]

Those app_menu.cc changes and the mac perspective lgtm . Removing that HDC
drawing stuff also sounds right (also: yay!) - there's just a compile error in
native_theme_win to resolve.

https://codereview.chromium.org/2813703003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu.cc (right):

https://codereview.chromium.org/2813703003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu.cc:173: bounds.Inset(gfx::Insets(0, 1,
0, 0));
On 2017/04/11 17:53:12, Evan Stade wrote:
> On 2017/04/11 02:48:06, tapted wrote:
> > This `1` should be separator_thickness too, so that the background doesn't
> paint
> > over the separator.
> 
> Ick. This is reserving space in DIP, but we may only want to paint a single
> pixel for the separator (<1 dip). So it seems like we should paint the bg
first
> and the separator on top. Only this doesn't work on GTK because the background
> isn't a solid color but itself has a border.
> 
> I've taken your suggestion for now but we'll have to keep working on this.

Acknowledged.

[Evan Stade, 2017-04-12 15:35:57]

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

[Evan Stade, 2017-04-12 15:35:57]

+bsep, can you help me test this on Windows? Basically just making sure that the
app menu separators (between rows and next to cut/copy/paste/zoom buttons) paint
correctly.

[Evan Stade, 2017-04-12 15:36:04]

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

[commit-bot: I haz the power, 2017-04-12 15:36:29]

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 16:50:58]

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

[commit-bot: I haz the power, 2017-04-12 16:50:59]

Dry run: This issue passed the CQ dry run.

[Bret, 2017-04-12 19:55:34]

On 2017/04/12 15:35:57, Evan Stade wrote:
> +bsep, can you help me test this on Windows? Basically just making sure that
the
> app menu separators (between rows and next to cut/copy/paste/zoom buttons)
paint
> correctly.

The horizontal ones between rows work. At 1.5x dsf the vertical ones disappear,
but at 1x and 2x they show up just fine. I don't see anything obviously wrong
with your code so I can't say why. I can help debug it if you want.

[Evan Stade, 2017-04-12 20:56:30]

On 2017/04/12 19:55:34, Bret Sepulveda wrote:
> On 2017/04/12 15:35:57, Evan Stade wrote:
> > +bsep, can you help me test this on Windows? Basically just making sure that
> the
> > app menu separators (between rows and next to cut/copy/paste/zoom buttons)
> paint
> > correctly.
> 
> The horizontal ones between rows work. At 1.5x dsf the vertical ones
disappear,
> but at 1x and 2x they show up just fine. I don't see anything obviously wrong
> with your code so I can't say why. I can help debug it if you want.

I would very much appreciate that.

[Peter Kasting, 2017-04-13 00:22:23]

I agree that removing the old HDC_based methods for these is safe.

LGTM on windows once you and Bret figure out the problem there.

On 2017/04/12 19:55:34, Bret Sepulveda wrote:
> On 2017/04/12 15:35:57, Evan Stade wrote:
> > +bsep, can you help me test this on Windows? Basically just making sure that
> the
> > app menu separators (between rows and next to cut/copy/paste/zoom buttons)
> paint
> > correctly.
> 
> The horizontal ones between rows work. At 1.5x dsf the vertical ones
disappear,
> but at 1x and 2x they show up just fine. I don't see anything obviously wrong
> with your code so I can't say why. I can help debug it if you want.

Maybe we're trying to draw a non-antialiased line right on the border between
two pixels and it's getting lost somehow?  If we manually unscaled, snapped the
coordinates to pixel centers, and then draw with an explicit 1 px thickness,
would that help?

[Evan Stade, 2017-04-14 16:29:09]

I think I fixed the issue. I copied the Windows code to ChromeOS to debug, and
it turns out we can fix it just by using floating point as that means our
calculations align to pixel centers (similar to when we add 0.5f to coords).

Here are some notes (for future work):
a) NativeTheme::Paint expects an SkCanvas which has a scaling transform applied
for the DSF. Therefore we can't call UndoDeviceScaleFactor. We may need to
change NativeTheme to take a gfx::Canvas.
b) We are (already were) using a hairline stroke width, i.e. always 1px. This is
important because we can't undo the dsf. Setting a stroke width of 1 leads to a
blurry line (scaled to match DSF).
c) I don't know what width the separators should actually be on Windows. Maybe
1dip and not 1px, as we do on CrOS. Would have to check what native menus do.
This patch leaves them alone.

[Evan Stade, 2017-04-14 16:33:54]

On 2017/04/14 16:29:09, Evan Stade wrote:
> I think I fixed the issue. I copied the Windows code to ChromeOS to debug, and
> it turns out we can fix it just by using floating point as that means our
> calculations align to pixel centers (similar to when we add 0.5f to coords).
> 
> Here are some notes (for future work):
> a) NativeTheme::Paint expects an SkCanvas which has a scaling transform
applied
> for the DSF. Therefore we can't call UndoDeviceScaleFactor. We may need to
> change NativeTheme to take a gfx::Canvas.
> b) We are (already were) using a hairline stroke width, i.e. always 1px. This
is
> important because we can't undo the dsf. Setting a stroke width of 1 leads to
a
> blurry line (scaled to match DSF).
> c) I don't know what width the separators should actually be on Windows. Maybe
> 1dip and not 1px, as we do on CrOS. Would have to check what native menus do.
> This patch leaves them alone.

p.s. I also saw some missing horizontal separators at certain fractional scales
(depends on exact layout and scale factor, so it's probably not directly
reproducible in Windows but would nevertheless show up at some combo of scale
and menu layout). This patch fixes those as well.

[Peter Kasting, 2017-04-14 16:38:34]

LGTM but consider filing a followup bug on the canvas type conversion stuff you
mentioned

[Bret, 2017-04-14 18:59:27]

On 2017/04/14 16:29:09, Evan Stade wrote:
> I think I fixed the issue. I copied the Windows code to ChromeOS to debug, and
> it turns out we can fix it just by using floating point as that means our
> calculations align to pixel centers (similar to when we add 0.5f to coords).

I just retested it and the vertical spearators show up now. LGTM

[Evan Stade, 2017-04-14 19:05:17]

On 2017/04/14 18:59:27, Bret Sepulveda wrote:
> On 2017/04/14 16:29:09, Evan Stade wrote:
> > I think I fixed the issue. I copied the Windows code to ChromeOS to debug,
and
> > it turns out we can fix it just by using floating point as that means our
> > calculations align to pixel centers (similar to when we add 0.5f to coords).
> 
> I just retested it and the vertical spearators show up now. LGTM

thanks!

filed crbug.com/711744

[Evan Stade, 2017-04-14 19:05:21]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-04-14 19:05:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
thomasanderson@chromium.org, tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/2813703003/#ps100001
(title: "use rectf")

[commit-bot: I haz the power, 2017-04-14 19:05:42]

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-14 19:14:43]

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

[commit-bot: I haz the power, 2017-04-14 19:14:44]

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

[Evan Stade, 2017-04-14 22:39:51]

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

[Evan Stade, 2017-04-14 22:39:52]

+sadrul for 	ui/base/models/menu_separator_types.h

[Evan Stade, 2017-04-17 15:21:30]

Description was changed from

==========
Use NativeTheme to draw vertical menu separators.

Use NativeTheme to draw vertical menu separators ("menu button border") 
so that they look like native horizontal separators instead of being
subtly different.

For now the GTK theme doesn't have a way to draw vertical separators,
so the old way of doing things (SkCanvas::drawRect) is retained, just
moved from app_menu.cc to native_theme_gtk3.cc.

BUG=710159
==========

to

==========
Use NativeTheme to draw vertical menu separators.

Use NativeTheme to draw vertical menu separators ("menu button border") 
so that they look like native horizontal separators instead of being
subtly different.

For now the GTK theme doesn't have a way to draw vertical separators,
so the old way of doing things (SkCanvas::drawRect) is retained, just
moved from app_menu.cc to native_theme_gtk3.cc.

BUG=710159
TBR=sadrul@chromium.org
==========

[Evan Stade, 2017-04-17 15:21:36]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-04-17 15:21:49]

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-17 16:39:09]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492442496368050,
"parent_rev": "a18bf7ffecba6583139316459a2652139fb08cc4", "commit_rev":
"944eab72bd2c478b03195680ef033b1d88aa85cc"}

[commit-bot: I haz the power, 2017-04-17 16:39:22]

Description was changed from

==========
Use NativeTheme to draw vertical menu separators.

Use NativeTheme to draw vertical menu separators ("menu button border") 
so that they look like native horizontal separators instead of being
subtly different.

For now the GTK theme doesn't have a way to draw vertical separators,
so the old way of doing things (SkCanvas::drawRect) is retained, just
moved from app_menu.cc to native_theme_gtk3.cc.

BUG=710159
TBR=sadrul@chromium.org
==========

to

==========
Use NativeTheme to draw vertical menu separators.

Use NativeTheme to draw vertical menu separators ("menu button border") 
so that they look like native horizontal separators instead of being
subtly different.

For now the GTK theme doesn't have a way to draw vertical separators,
so the old way of doing things (SkCanvas::drawRect) is retained, just
moved from app_menu.cc to native_theme_gtk3.cc.

BUG=710159
TBR=sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2813703003
Cr-Commit-Position: refs/heads/master@{#464930}
Committed:
https://chromium.googlesource.com/chromium/src/+/944eab72bd2c478b03195680ef03...
==========

[commit-bot: I haz the power, 2017-04-17 16:39:23]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/944eab72bd2c478b03195680ef03...