Adjust BackgroundWith1PxBorder to better handle fractional scale factors

Adjust BackgroundWith1PxBorder to better handle fractional scale factors

This fixes blurriness in the omnibox border on surface tablets.

BUG=627469
Committed: https://crrev.com/eb39becd04cdefd6aac776b659358458aa7ace23
Cr-Commit-Position: refs/heads/master@{#406371}

[Evan Stade, 2016-07-13 16:39:32]

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

[Bret, 2016-07-13 22:42:50]

lgtm. I'm not an owner, however

[Evan Stade, 2016-07-14 17:06:38]

On 2016/07/13 22:42:50, Bret Sepulveda wrote:
> lgtm. I'm not an owner, however

thanks. I'll just wait for pkasting as there doesn't seem to be any pressure to
fix this for m53.

[Peter Kasting, 2016-07-18 19:01:53]

LGTM

https://codereview.chromium.org/2145713002/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/background_with_1_px_border.cc
(right):

https://codereview.chromium.org/2145713002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:31: //
factors. See crbug.com/627469
Nit: Rather than refer to a bug, just ensure the problem is explained
sufficiently here, e.g.

Draw the border as a 1 px thick line aligned with the inside edge of the
LOCATION_BAR_BORDER_THICKNESS region.  This line needs to be snapped to the
pixel grid, so the result of the scale-up needs to be snapped to an integer
value.  Using floor() instead of round() ensures that, for non-integral scale
factors, the border will still be drawn inside the BORDER_THICKNESS region
instead of being partially inside it.

[Evan Stade, 2016-07-18 19:15:08]

https://codereview.chromium.org/2145713002/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/background_with_1_px_border.cc
(right):

https://codereview.chromium.org/2145713002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:31: //
factors. See crbug.com/627469
On 2016/07/18 19:01:53, Peter Kasting (slow) wrote:
> Nit: Rather than refer to a bug, just ensure the problem is explained
> sufficiently here, e.g.
> 
> Draw the border as a 1 px thick line aligned with the inside edge of the
> LOCATION_BAR_BORDER_THICKNESS region.  This line needs to be snapped to the
> pixel grid, so the result of the scale-up needs to be snapped to an integer
> value.  Using floor() instead of round() ensures that, for non-integral scale
> factors, the border will still be drawn inside the BORDER_THICKNESS region
> instead of being partially inside it.

really? I prefer shorter comments to longer comments, and more context to less
context. Seems like using a bug link gives us both.

[Peter Kasting, 2016-07-18 19:21:17]

On 2016/07/18 19:15:08, Evan Stade wrote:
>
https://codereview.chromium.org/2145713002/diff/1/chrome/browser/ui/views/loc...
> File chrome/browser/ui/views/location_bar/background_with_1_px_border.cc
> (right):
> 
>
https://codereview.chromium.org/2145713002/diff/1/chrome/browser/ui/views/loc...
> chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:31: //
> factors. See crbug.com/627469
> On 2016/07/18 19:01:53, Peter Kasting (slow) wrote:
> > Nit: Rather than refer to a bug, just ensure the problem is explained
> > sufficiently here, e.g.
> > 
> > Draw the border as a 1 px thick line aligned with the inside edge of the
> > LOCATION_BAR_BORDER_THICKNESS region.  This line needs to be snapped to the
> > pixel grid, so the result of the scale-up needs to be snapped to an integer
> > value.  Using floor() instead of round() ensures that, for non-integral
scale
> > factors, the border will still be drawn inside the BORDER_THICKNESS region
> > instead of being partially inside it.
> 
> really? I prefer shorter comments to longer comments, and more context to less
> context. Seems like using a bug link gives us both.

Bug links are problematic for two reasons.

* They require you to go look elsewhere, so you're not reading the comment in
the context of looking at the code.  This makes it hard to understand the reason
for something at-a-glance and, depending on the complexity of the code and
reason, can make understanding things require repeatedly switching between code
and bug.

* Frequently bugs are long, have many comments, get spurious comments added
later, morph, etc.  Even for simple bugs it's faster and easier for the original
author to know what pertinent information should be distilled as the rationale
for the code, and this effect grows rapidly as the bug length changes.

For these reasons, I always ask that people omit bug links.

In the particular case here, much of the additional length in my comment comes
from describing exactly why floor() is the right choice and not ceil() or
round().  The linked bug didn't explain this fine detail at all.

[Evan Stade, 2016-07-18 19:28:00]

On 2016/07/18 19:21:17, Peter Kasting (slow) wrote:
> On 2016/07/18 19:15:08, Evan Stade wrote:
> >
>
https://codereview.chromium.org/2145713002/diff/1/chrome/browser/ui/views/loc...
> > File chrome/browser/ui/views/location_bar/background_with_1_px_border.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2145713002/diff/1/chrome/browser/ui/views/loc...
> > chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:31: //
> > factors. See crbug.com/627469
> > On 2016/07/18 19:01:53, Peter Kasting (slow) wrote:
> > > Nit: Rather than refer to a bug, just ensure the problem is explained
> > > sufficiently here, e.g.
> > > 
> > > Draw the border as a 1 px thick line aligned with the inside edge of the
> > > LOCATION_BAR_BORDER_THICKNESS region.  This line needs to be snapped to
the
> > > pixel grid, so the result of the scale-up needs to be snapped to an
integer
> > > value.  Using floor() instead of round() ensures that, for non-integral
> scale
> > > factors, the border will still be drawn inside the BORDER_THICKNESS region
> > > instead of being partially inside it.
> > 
> > really? I prefer shorter comments to longer comments, and more context to
less
> > context. Seems like using a bug link gives us both.
> 
> Bug links are problematic for two reasons.
> 
> * They require you to go look elsewhere, so you're not reading the comment in
> the context of looking at the code.  This makes it hard to understand the
reason
> for something at-a-glance and, depending on the complexity of the code and
> reason, can make understanding things require repeatedly switching between
code
> and bug.

Only if you want more info than what's in the rest of the comment (and usually,
you don't).

> 
> * Frequently bugs are long, have many comments, get spurious comments added
> later, morph, etc.  Even for simple bugs it's faster and easier for the
original
> author to know what pertinent information should be distilled as the rationale
> for the code, and this effect grows rapidly as the bug length changes.

I think the screenshot in the bug link is pretty valuable.

> 
> For these reasons, I always ask that people omit bug links.

I understand your point here, but bug links in comments are really common in
Chrome and if they should be discouraged that seems like something for the
broader chromium dev community to discuss and codify in the style guide.

[Peter Kasting, 2016-07-18 19:40:45]

On 2016/07/18 19:28:00, Evan Stade wrote:
> On 2016/07/18 19:21:17, Peter Kasting (slow) wrote:
> > On 2016/07/18 19:15:08, Evan Stade wrote:
> > >
> >
>
https://codereview.chromium.org/2145713002/diff/1/chrome/browser/ui/views/loc...
> > > File chrome/browser/ui/views/location_bar/background_with_1_px_border.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2145713002/diff/1/chrome/browser/ui/views/loc...
> > > chrome/browser/ui/views/location_bar/background_with_1_px_border.cc:31: //
> > > factors. See crbug.com/627469
> > > On 2016/07/18 19:01:53, Peter Kasting (slow) wrote:
> > > > Nit: Rather than refer to a bug, just ensure the problem is explained
> > > > sufficiently here, e.g.
> > > > 
> > > > Draw the border as a 1 px thick line aligned with the inside edge of the
> > > > LOCATION_BAR_BORDER_THICKNESS region.  This line needs to be snapped to
> the
> > > > pixel grid, so the result of the scale-up needs to be snapped to an
> integer
> > > > value.  Using floor() instead of round() ensures that, for non-integral
> > scale
> > > > factors, the border will still be drawn inside the BORDER_THICKNESS
region
> > > > instead of being partially inside it.
> > > 
> > > really? I prefer shorter comments to longer comments, and more context to
> less
> > > context. Seems like using a bug link gives us both.
> > 
> > Bug links are problematic for two reasons.
> > 
> > * They require you to go look elsewhere, so you're not reading the comment
in
> > the context of looking at the code.  This makes it hard to understand the
> reason
> > for something at-a-glance and, depending on the complexity of the code and
> > reason, can make understanding things require repeatedly switching between
> code
> > and bug.
> 
> Only if you want more info than what's in the rest of the comment (and
usually,
> you don't).

I've not yet had someone propose a comment to me with a bug link where they put
all the relevant info in the comment, and thus the reader didn't have to click
through to the bug link to try and figure out what was going on.  This case was
also that way.

> > * Frequently bugs are long, have many comments, get spurious comments added
> > later, morph, etc.  Even for simple bugs it's faster and easier for the
> original
> > author to know what pertinent information should be distilled as the
rationale
> > for the code, and this effect grows rapidly as the bug length changes.
> 
> I think the screenshot in the bug link is pretty valuable.

It's only meaningful if you know what problem you're looking for and why it's a
problem -- at which point usually you don't need the screenshot.  It took me a
bit of time looking at the screenshot to realize that the issue was that the
border was supposed to be 1 px; nothing on the bug said that, I just know it
because of my recent work on MD.  In two years I would have no idea, looking at
that bug, what the issue is.

Whereas, given the explanation in the comment I proposed, the screenshot is
redundant; it doesn't say anything the comment doesn't say.

> > For these reasons, I always ask that people omit bug links.
> 
> I understand your point here, but bug links in comments are really common in
> Chrome and if they should be discouraged that seems like something for the
> broader chromium dev community to discuss and codify in the style guide.

This isn't the sort of rule that we would put in the style guide regardless of
what the outcome of such a discussion would be.

I think it is reasonable to simply discuss locally.  I'm not simply saying
"never link to bugs" and walking away leaving it up to you to figure out how and
whether to comply with that.  I'm proposing a specific piece of replacement text
that I think is a clear readability win.  If you think something else would be
even clearer, such as adding a bug link after the text I propose, feel free to
suggest that (although my response there would be to ask what the bug link gives
the reader that the text doesn't already describe).

[Peter Kasting, 2016-07-18 19:41:51]

(To phrase it differently: bug links are not categorically wrong and bad style. 
They're just a clue that generally the comment could be written better, and so I
always seek to propose such an improvement.)

[Evan Stade, 2016-07-19 19:47:45]

On 2016/07/18 19:41:51, Peter Kasting (slow) wrote:
> (To phrase it differently: bug links are not categorically wrong and bad
style. 
> They're just a clue that generally the comment could be written better, and so
I
> always seek to propose such an improvement.)

OK, I guess I misunderstood you because I thought you were saying they were
universally wrong. I've adopted your suggested comment.

[Evan Stade, 2016-07-19 19:47:51]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-07-19 19:47:51]

The patchset sent to the CQ was uploaded after l-g-t-m from bsep@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2145713002/#ps20001
(title: "use pkasting comment")

[commit-bot: I haz the power, 2016-07-19 19:48:32]

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-07-19 20:34:12]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-19 20:34:27]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-19 20:35:48]

Description was changed from

==========
Adjust BackgroundWith1PxBorder to better handle fractional scale factors

This fixes blurriness in the omnibox border on surface tablets.

BUG=627469
==========

to

==========
Adjust BackgroundWith1PxBorder to better handle fractional scale factors

This fixes blurriness in the omnibox border on surface tablets.

BUG=627469

Committed: https://crrev.com/eb39becd04cdefd6aac776b659358458aa7ace23
Cr-Commit-Position: refs/heads/master@{#406371}
==========

[commit-bot: I haz the power, 2016-07-19 20:35:49]

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

[Peter Kasting, 2016-07-19 22:10:09]

On 2016/07/19 19:47:45, Evan Stade wrote:
> On 2016/07/18 19:41:51, Peter Kasting (slow) wrote:
> > (To phrase it differently: bug links are not categorically wrong and bad
> style. 
> > They're just a clue that generally the comment could be written better, and
so
> I
> > always seek to propose such an improvement.)
> 
> OK, I guess I misunderstood you because I thought you were saying they were
> universally wrong. I've adopted your suggested comment.

It's my fault, I worded things too strongly at first and was misleading.  Sorry.

I think I'll take your advice and send something to chromium-dev, but as a
suggestion, not a proposed style rule.