Make sure line widths in the range of 0px and 1px are rounded up to 1px for visibility.

Make sure line widths in the range of 0px and 1px are rounded up to 1px for visibility.

Currently if a border/outline computes to less than 1px, convertLineWidth
casts the value to unsigned int or short and hence returns a 0. This is
inconsistent with FF and IE. This patch fixes that issue by performing
all computation on a double and returning a 1 if computeLength returns a
value between 0 and 1. Otherwise it returns a type casted result of
computeLength. It fixes this in both StyleBuilder and
AnimatableStyleBuilder. It also rebaselines the relevant tests.

BUG=229428, 382483
Committed: https://crrev.com/f73426be35a4b02a6d029407db459a87bc5e4688
Cr-Commit-Position: refs/heads/master@{#357305}

[nainar, 2015-10-20 05:15:59]

nainar@chromium.org changed reviewers:
+ sashab@chromium.org, timloh@chromium.org

[nainar, 2015-10-20 05:15:59]

PTAL?

Thanks!

[sashab, 2015-10-20 23:42:46]

I don't think this is actually what we want. Some code (like animations code)
needs line widths to round down to 0px to finish animating (see the failing
layout tests).

Not sure about the right thing to do here... Better ask tim :)

[nainar, 2015-10-21 07:20:26]

nainar@chromium.org changed reviewers:
+ alancutter@chromium.org

[nainar, 2015-10-21 07:20:26]

PTAL?

Thanks!

[nainar, 2015-10-21 07:21:49]

Description was changed from

==========
Make sure line widths in the range of 0px and 1px are rounded up to 1px for
visibility.

Currently if a border/outline is set to be less than 1px, convertLineWidth
casts the value to unsigned int or short and hence returns a 0. This is
inconsistent with FF and IE. This patch fixes that issue be performing
all computation on a double and returning a 1 if computeLength returns a
value between 0 and 1. Otherwise it returns a type casted result of
computeLength.

BUG=229428
==========

to

==========
Make sure line widths in the range of 0px and 1px are rounded up to 1px for
visibility.

Currently if a border/outline is set to be less than 1px, convertLineWidth
casts the value to unsigned int or short and hence returns a 0. This is
inconsistent with FF and IE. This patch fixes that issue be performing
all computation on a double and returning a 1 if computeLength returns a
value between 0 and 1. Otherwise it returns a type casted result of
computeLength. It fixes this in both StyleBuilder and 
AnimatableStyleBuilder. It also rebaselines the relevant tests.

BUG=229428
==========

[alancutter (OOO until 2018), 2015-10-27 04:10:32]

https://codereview.chromium.org/1412103005/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/round-up-all-line-width-less-than-1-greater-than-0.html
(right):

https://codereview.chromium.org/1412103005/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/round-up-all-line-width-less-than-1-greater-than-0.html:10:
var expectedValues = ["0px", "1px", "1px", "1px", "1px", "1px", "1px", "2px",
"1px", "1px", "2px", "2px"]
I would make this a dictionary of input: expected pairs to make the association
between values clearer and less error prone.

https://codereview.chromium.org/1412103005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.h
(right):

https://codereview.chromium.org/1412103005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.h:141: if
(state.style()->effectiveZoom() < 1.0f && result < 1.0) {
Is this code path redundant now?

https://codereview.chromium.org/1412103005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.h:148: return
primitiveValue.computeLength<T>(state.cssToLengthConversionData());
Can we just call clampTo<T>() instead of calling computeLength() twice?

[nainar, 2015-10-27 05:30:18]

PTAL?

Thanks!

https://codereview.chromium.org/1412103005/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/round-up-all-line-width-less-than-1-greater-than-0.html
(right):

https://codereview.chromium.org/1412103005/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/round-up-all-line-width-less-than-1-greater-than-0.html:10:
var expectedValues = ["0px", "1px", "1px", "1px", "1px", "1px", "1px", "2px",
"1px", "1px", "2px", "2px"]
Done.

https://codereview.chromium.org/1412103005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.h
(right):

https://codereview.chromium.org/1412103005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.h:141: if
(state.style()->effectiveZoom() < 1.0f && result < 1.0) {
Done.

https://codereview.chromium.org/1412103005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.h:148: return
primitiveValue.computeLength<T>(state.cssToLengthConversionData());
Done.

[nainar, 2015-10-28 03:45:34]

Description was changed from

==========
Make sure line widths in the range of 0px and 1px are rounded up to 1px for
visibility.

Currently if a border/outline is set to be less than 1px, convertLineWidth
casts the value to unsigned int or short and hence returns a 0. This is
inconsistent with FF and IE. This patch fixes that issue be performing
all computation on a double and returning a 1 if computeLength returns a
value between 0 and 1. Otherwise it returns a type casted result of
computeLength. It fixes this in both StyleBuilder and 
AnimatableStyleBuilder. It also rebaselines the relevant tests.

BUG=229428
==========

to

==========
Make sure line widths in the range of 0px and 1px are rounded up to 1px for
visibility.

Currently if a border/outline computes to less than 1px, convertLineWidth
casts the value to unsigned int or short and hence returns a 0. This is
inconsistent with FF and IE. This patch fixes that issue be performing
all computation on a double and returning a 1 if computeLength returns a
value between 0 and 1. Otherwise it returns a type casted result of
computeLength. It fixes this in both StyleBuilder and 
AnimatableStyleBuilder. It also rebaselines the relevant tests.

BUG=229428
==========

[nainar, 2015-10-28 22:58:29]

Description was changed from

==========
Make sure line widths in the range of 0px and 1px are rounded up to 1px for
visibility.

Currently if a border/outline computes to less than 1px, convertLineWidth
casts the value to unsigned int or short and hence returns a 0. This is
inconsistent with FF and IE. This patch fixes that issue be performing
all computation on a double and returning a 1 if computeLength returns a
value between 0 and 1. Otherwise it returns a type casted result of
computeLength. It fixes this in both StyleBuilder and 
AnimatableStyleBuilder. It also rebaselines the relevant tests.

BUG=229428
==========

to

==========
Make sure line widths in the range of 0px and 1px are rounded up to 1px for
visibility.

Currently if a border/outline computes to less than 1px, convertLineWidth
casts the value to unsigned int or short and hence returns a 0. This is
inconsistent with FF and IE. This patch fixes that issue by performing
all computation on a double and returning a 1 if computeLength returns a
value between 0 and 1. Otherwise it returns a type casted result of
computeLength. It fixes this in both StyleBuilder and 
AnimatableStyleBuilder. It also rebaselines the relevant tests.

BUG=229428
==========

[nainar, 2015-10-29 03:59:40]

Alan, 

Made the changes you asked for, PTAL?

Thanks!

https://codereview.chromium.org/1412103005/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/round-up-all-line-width-less-than-1-greater-than-0.html
(right):

https://codereview.chromium.org/1412103005/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/round-up-all-line-width-less-than-1-greater-than-0.html:9:
var values = [{actual:"0px", expected:"0px"}, {actual:"0.9px", expected:"1px"},
{actual:"1px", expected:"1px"}, {actual:"0.2px", expected:"1px"},
{actual:"0.9pt", expected:"1px"}, {actual:"1pt", expected:"1px"},
{actual:"0.2pt", expected:"1px"}, {actual:"1.9pt", expected:"2px"},
{actual:"1pt", expected:"1px"}, {actual:"1.2pt", expected:"1px"},
{actual:"2.1px", expected:"2px"}, {actual:"2.9px", expected:"2px"}];
actual -> input
Each entry on a new line

https://codereview.chromium.org/1412103005/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/round-up-all-line-width-less-than-1-greater-than-0.html:15:
div.textContent = "Div with outline of " + value + " computes to  " +
getComputedStyle(div).outlineWidth;
remove text and display: none

[nainar, 2015-10-29 04:07:55]

https://codereview.chromium.org/1412103005/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css/round-up-all-line-width-less-than-1-greater-than-0.html
(right):

https://codereview.chromium.org/1412103005/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/round-up-all-line-width-less-than-1-greater-than-0.html:9:
var values = [
Indented

[alancutter (OOO until 2018), 2015-10-29 04:10:05]

lgtm

[Timothy Loh, 2015-11-02 02:52:08]

timloh@chromium.org changed reviewers:
+ eae@chromium.org

[Timothy Loh, 2015-11-02 02:52:09]

Probably OK but I'll defer to Emil who might have opinions on sub-pixel stuff :)

[eae, 2015-11-02 03:02:02]

https://codereview.chromium.org/1412103005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.h
(right):

https://codereview.chromium.org/1412103005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.h:138: double
result =
primitiveValue.computeLength<double>(state.cssToLengthConversionData());
I'd argue that a border that is specified to be 0.25 should round to zero rather
than one. Today that doesn't really matter but with the move to use full page
zoom for high-dpi 0.5 would map to 1 device pixel on high DPI. At the very least
I'd like us to clamp it to device pixels or zoom adjusted CSS pixels instead of
raw CSS pixels.

[eae, 2015-11-02 03:14:46]

LGTM

https://codereview.chromium.org/1412103005/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.h
(right):

https://codereview.chromium.org/1412103005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.h:138: double
result =
primitiveValue.computeLength<double>(state.cssToLengthConversionData());
It's probably more important to match the other browsers here than to match our
own legacy behavior. Let's give this a try. Please add a comment about hi-dpi
support though and reference bug 485650.

[nainar, 2015-11-02 03:27:01]

The CQ bit was checked by nainar@chromium.org

[nainar, 2015-11-02 03:27:01]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/1412103005/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-11-02 03:27:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412103005/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412103005/200001

[nainar, 2015-11-02 04:08:08]

Description was changed from

==========
Make sure line widths in the range of 0px and 1px are rounded up to 1px for
visibility.

Currently if a border/outline computes to less than 1px, convertLineWidth
casts the value to unsigned int or short and hence returns a 0. This is
inconsistent with FF and IE. This patch fixes that issue by performing
all computation on a double and returning a 1 if computeLength returns a
value between 0 and 1. Otherwise it returns a type casted result of
computeLength. It fixes this in both StyleBuilder and 
AnimatableStyleBuilder. It also rebaselines the relevant tests.

BUG=229428
==========

to

==========
Make sure line widths in the range of 0px and 1px are rounded up to 1px for
visibility.

Currently if a border/outline computes to less than 1px, convertLineWidth
casts the value to unsigned int or short and hence returns a 0. This is
inconsistent with FF and IE. This patch fixes that issue by performing
all computation on a double and returning a 1 if computeLength returns a
value between 0 and 1. Otherwise it returns a type casted result of
computeLength. It fixes this in both StyleBuilder and 
AnimatableStyleBuilder. It also rebaselines the relevant tests.

BUG=229428, 382483
==========

[commit-bot: I haz the power, 2015-11-02 04:25:21]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-11-02 04:26:59]

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