Improve appearance of FocusableBorder at fractional scale factors by

Improve appearance of FocusableBorder at fractional scale factors by
sharing more code with MD/harmony version of FocusableBorder.

BUG=691816
Review-Url: https://codereview.chromium.org/2693943003
Cr-Commit-Position: refs/heads/master@{#450583}
Committed: https://chromium.googlesource.com/chromium/src/+/a137561d0d58aa4003e3aa97acfaf749c0b62196

[Evan Stade, 2017-02-14 00:18:48]

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

[Peter Kasting, 2017-02-14 00:30:32]

https://codereview.chromium.org/2693943003/diff/1/ui/views/controls/focusable...
File ui/views/controls/focusable_border.cc (right):

https://codereview.chromium.org/2693943003/diff/1/ui/views/controls/focusable...
ui/views/controls/focusable_border.cc:50:
ui::MaterialDesignController::IsSecondaryUiMaterial() ? 1 : dsf;
It seems like the old pre-Harmony code was trying to draw a 2 DIP border, and
the new code draws something like a 1 DIP border.  Don't we want a 2* in here?

I also wonder if this shouldn't be gfx::ToRoundedInt().  It looks like you're
truncating-to-int, which seems a bit surprising.  If that's really what we want,
we probably want an explicit ToFlooredInt() + a comment about why flooring is
correct.

[Evan Stade, 2017-02-14 00:37:47]

https://codereview.chromium.org/2693943003/diff/1/ui/views/controls/focusable...
File ui/views/controls/focusable_border.cc (right):

https://codereview.chromium.org/2693943003/diff/1/ui/views/controls/focusable...
ui/views/controls/focusable_border.cc:50:
ui::MaterialDesignController::IsSecondaryUiMaterial() ? 1 : dsf;
On 2017/02/14 00:30:32, Peter Kasting wrote:
> It seems like the old pre-Harmony code was trying to draw a 2 DIP border, and
> the new code draws something like a 1 DIP border.  Don't we want a 2* in here?

That was just a hacky way to get a 1dip border. 1dip was inside the rect, 1
outside (and therefore outside the view, i.e. not visible). Instead, we use the
correct stroke width and inset the bounds by half the stroke.

> 
> I also wonder if this shouldn't be gfx::ToRoundedInt().  It looks like you're
> truncating-to-int, which seems a bit surprising.  If that's really what we
want,
> we probably want an explicit ToFlooredInt() + a comment about why flooring is
> correct.

I've been assuming flooring is correct for dip-sized strokes, but I'll ping
Sebastien.

[Peter Kasting, 2017-02-14 00:45:59]

https://codereview.chromium.org/2693943003/diff/1/ui/views/controls/focusable...
File ui/views/controls/focusable_border.cc (right):

https://codereview.chromium.org/2693943003/diff/1/ui/views/controls/focusable...
ui/views/controls/focusable_border.cc:50:
ui::MaterialDesignController::IsSecondaryUiMaterial() ? 1 : dsf;
On 2017/02/14 00:37:47, Evan Stade wrote:
> On 2017/02/14 00:30:32, Peter Kasting wrote:
> > It seems like the old pre-Harmony code was trying to draw a 2 DIP border,
and
> > the new code draws something like a 1 DIP border.  Don't we want a 2* in
here?
> 
> That was just a hacky way to get a 1dip border. 1dip was inside the rect, 1
> outside (and therefore outside the view, i.e. not visible). Instead, we use
the
> correct stroke width and inset the bounds by half the stroke.

I see.

Why was the old code producing poorer-looking output than the new code?  Was it
because we were drawing the path without AA but not rounding or flooring the
path width, so on some edges the fractional pixel would result in filling the
whole next pixel, and in some it wouldn't?  In which case, the reason this is
better is basically because of the use of "int" in this line.

[Evan Stade, 2017-02-14 16:40:42]

On 2017/02/14 00:45:59, Peter Kasting wrote:
>
https://codereview.chromium.org/2693943003/diff/1/ui/views/controls/focusable...
> File ui/views/controls/focusable_border.cc (right):
> 
>
https://codereview.chromium.org/2693943003/diff/1/ui/views/controls/focusable...
> ui/views/controls/focusable_border.cc:50:
> ui::MaterialDesignController::IsSecondaryUiMaterial() ? 1 : dsf;
> On 2017/02/14 00:37:47, Evan Stade wrote:
> > On 2017/02/14 00:30:32, Peter Kasting wrote:
> > > It seems like the old pre-Harmony code was trying to draw a 2 DIP border,
> and
> > > the new code draws something like a 1 DIP border.  Don't we want a 2* in
> here?
> > 
> > That was just a hacky way to get a 1dip border. 1dip was inside the rect, 1
> > outside (and therefore outside the view, i.e. not visible). Instead, we use
> the
> > correct stroke width and inset the bounds by half the stroke.
> 
> I see.
> 
> Why was the old code producing poorer-looking output than the new code?  Was
it
> because we were drawing the path without AA but not rounding or flooring the
> path width, so on some edges the fractional pixel would result in filling the
> whole next pixel, and in some it wouldn't?  In which case, the reason this is
> better is basically because of the use of "int" in this line.

precisely. This technique was already applied in MD but I guess I didn't want to
risk touching non-MD at the time.

Sebastien thinks that we should round down (i.e. floor) on dip-defined strokes.
I can apply the explicit flooring function you mentioned.

[Evan Stade, 2017-02-14 22:59:28]

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

[Evan Stade, 2017-02-14 22:59:46]

Description was changed from

==========
Improve appearance of FocusableBorder at fractional scale factors by
sharing more code with MD/harmony version of FocusableBorder.

BUG=691816
==========

to

==========
Improve appearance of FocusableBorder at fractional scale factors by
sharing more code with MD/harmony version of FocusableBorder.

BUG=691816
==========

[Evan Stade, 2017-02-14 22:59:46]

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

[Evan Stade, 2017-02-14 22:59:50]

added gfx::ToFlooredInt, +sky for OWNERS

[commit-bot: I haz the power, 2017-02-14 23:00:23]

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

[sky, 2017-02-15 00:37:06]

LGTM

[Peter Kasting, 2017-02-15 01:06:13]

LGTM

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

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

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

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-02-15 01:24:02]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-02-15 01:25: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, 2017-02-15 06:11:40]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487121842573740,
"parent_rev": "ab1a9331a441fa315edb669486ac1964788e47c7", "commit_rev":
"a137561d0d58aa4003e3aa97acfaf749c0b62196"}

[commit-bot: I haz the power, 2017-02-15 06:12:53]

Description was changed from

==========
Improve appearance of FocusableBorder at fractional scale factors by
sharing more code with MD/harmony version of FocusableBorder.

BUG=691816
==========

to

==========
Improve appearance of FocusableBorder at fractional scale factors by
sharing more code with MD/harmony version of FocusableBorder.

BUG=691816

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

[commit-bot: I haz the power, 2017-02-15 06:12:54]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a137561d0d58aa4003e3aa97acfa...