Support subpixel layout for borders.

Support subpixel layout of borders.

This cl implements subpixel layout of borders.  Painting of subpixel
borders now uses rounding to nearest CSS pixel, and is aligned with
Edge.  Handling of device pixel ratios > 1 isn't implemented here,
and is better covered in a separate task.

This does not implement subpixel borders for tables, as that interacts
with table layout width calculations, which would also need to be
adapted to subpixels.  Again, that's better dealt with in a separate
task.

A number of test cases had to be rebaselined, due to minor changes in
layout and/or painting.  Most of these changes appear in tests that uses
zooming, and implicitly have fractional borders.

svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm now looks a bit
broken, but that's due to an unrelated issue with aspect ratio and
subpixels (in LayoutPart, if I recall correctly).

BUG=227192
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2640143005
Cr-Commit-Position: refs/heads/master@{#449943}
Committed: https://chromium.googlesource.com/chromium/src/+/934becac5daa91ea979fb66e4ae21761ca11ebc9

[Karl Øygard, 2017-01-20 10:26:58]

Description was changed from

==========
Support subpixel layout for borders.

This does not implement subpixel borders for tables, as that interacts
with table layout, which would also need to be adapted to sub pixels.
That's better dealt with in a separate task.

Painting of subpixel borders uses rounding to nearest CSS pixel, and is
aligned with Edge.  Support for devices with pixels > CSS pixels should
be covered in a separate task.

A number of test cases had to be rebaselined, due to minor changes in
layout and/or painting.  Most of these changes appear in tests that uses
zooming, and implicitly have fractional borders.

BUG=227192
==========

to

==========
Support subpixel layout for borders.

This does not implement subpixel borders for tables, as that interacts
with table layout, which would also need to be adapted to sub pixels.
That's better dealt with in a separate task.

Painting of subpixel borders uses rounding to nearest CSS pixel, and is
aligned with Edge.  Support for devices with pixels > CSS pixels should
be covered in a separate task.

A number of test cases had to be rebaselined, due to minor changes in
layout and/or painting.  Most of these changes appear in tests that uses
zooming, and implicitly have fractional borders.

BUG=227192
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Karl Øygard, 2017-01-20 10:50:12]

The CQ bit was checked by karlo@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 10:50:28]

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-20 12:37:01]

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

[commit-bot: I haz the power, 2017-01-20 12:37:04]

Dry run: This issue passed the CQ dry run.

[Karl Øygard, 2017-01-20 15:05:53]

Description was changed from

==========
Support subpixel layout for borders.

This does not implement subpixel borders for tables, as that interacts
with table layout, which would also need to be adapted to sub pixels.
That's better dealt with in a separate task.

Painting of subpixel borders uses rounding to nearest CSS pixel, and is
aligned with Edge.  Support for devices with pixels > CSS pixels should
be covered in a separate task.

A number of test cases had to be rebaselined, due to minor changes in
layout and/or painting.  Most of these changes appear in tests that uses
zooming, and implicitly have fractional borders.

BUG=227192
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Support subpixel layout of borders.

This cl implements subpixel layout of borders.  Painting of subpixel
borders uses rounding to nearest CSS pixel, and is aligned with Edge.
Handling of device pixel ratios > 1 needs work on the paint side,
and is better covered in a separate task.

This does not implement subpixel borders for tables, as that interacts
with table layout, which would also need to be adapted to subpixels.
Again, that's better dealt with in a separate task.

A number of test cases had to be rebaselined, due to minor changes in
layout and/or painting.  Most of these changes appear in tests that uses
zooming, and implicitly have fractional borders.

svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm now looks a bit
broken, but that's due to an unrelated issue with aspect ratio and
subpixels (in LayoutPart, if I recall correctly).

BUG=227192
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Karl Øygard, 2017-01-20 15:10:55]

Description was changed from

==========
Support subpixel layout of borders.

This cl implements subpixel layout of borders.  Painting of subpixel
borders uses rounding to nearest CSS pixel, and is aligned with Edge.
Handling of device pixel ratios > 1 needs work on the paint side,
and is better covered in a separate task.

This does not implement subpixel borders for tables, as that interacts
with table layout, which would also need to be adapted to subpixels.
Again, that's better dealt with in a separate task.

A number of test cases had to be rebaselined, due to minor changes in
layout and/or painting.  Most of these changes appear in tests that uses
zooming, and implicitly have fractional borders.

svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm now looks a bit
broken, but that's due to an unrelated issue with aspect ratio and
subpixels (in LayoutPart, if I recall correctly).

BUG=227192
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Support subpixel layout of borders.

This cl implements subpixel layout of borders.  Painting of subpixel
borders now uses rounding to nearest CSS pixel, and is aligned with
Edge.  Handling of device pixel ratios > 1 needs work on the paint
side, and is better covered in a separate task.

This does not implement subpixel borders for tables, as that interacts
with table layout width calculations, which would also need to be
adapted to subpixels.  Again, that's better dealt with in a separate
task.

A number of test cases had to be rebaselined, due to minor changes in
layout and/or painting.  Most of these changes appear in tests that uses
zooming, and implicitly have fractional borders.

svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm now looks a bit
broken, but that's due to an unrelated issue with aspect ratio and
subpixels (in LayoutPart, if I recall correctly).

BUG=227192
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Karl Øygard, 2017-01-20 15:10:55]

karlo@opera.com changed reviewers:
+ eae@chromium.org, pdr@chromium.org

[Karl Øygard, 2017-01-23 09:40:33]

Description was changed from

==========
Support subpixel layout of borders.

This cl implements subpixel layout of borders.  Painting of subpixel
borders now uses rounding to nearest CSS pixel, and is aligned with
Edge.  Handling of device pixel ratios > 1 needs work on the paint
side, and is better covered in a separate task.

This does not implement subpixel borders for tables, as that interacts
with table layout width calculations, which would also need to be
adapted to subpixels.  Again, that's better dealt with in a separate
task.

A number of test cases had to be rebaselined, due to minor changes in
layout and/or painting.  Most of these changes appear in tests that uses
zooming, and implicitly have fractional borders.

svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm now looks a bit
broken, but that's due to an unrelated issue with aspect ratio and
subpixels (in LayoutPart, if I recall correctly).

BUG=227192
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Support subpixel layout of borders.

This cl implements subpixel layout of borders.  Painting of subpixel
borders now uses rounding to nearest CSS pixel, and is aligned with
Edge.  Handling of device pixel ratios > 1 isn't implemented here,
and is better covered in a separate task.

This does not implement subpixel borders for tables, as that interacts
with table layout width calculations, which would also need to be
adapted to subpixels.  Again, that's better dealt with in a separate
task.

A number of test cases had to be rebaselined, due to minor changes in
layout and/or painting.  Most of these changes appear in tests that uses
zooming, and implicitly have fractional borders.

svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm now looks a bit
broken, but that's due to an unrelated issue with aspect ratio and
subpixels (in LayoutPart, if I recall correctly).

BUG=227192
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Karl Øygard, 2017-01-23 09:41:17]

ptal

[pdr., 2017-01-24 06:57:31]

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSProperties.in:214: border-bottom-width
interpolable, initial=initialBorderWidth, converter=convertLineWidth<float>
Should outline-width be changed in a similar way (as a followup)?

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSProperties.in:215: border-collapse
inherited, independent, keyword_only, keywords=[separate|collapse],
initial_keyword=separate
Some of the SVG test results seem like regressions (e.g.,
third_party/WebKit/LayoutTests/platform/mac/svg/zoom/page/zoom-background-images-expected.png).

Similarly, the zoom test results seem suspect to me (e.g.,
third_party/WebKit/LayoutTests/platform/win/fast/forms/color/color-suggestion-picker-appearance-zoom125-expected.png).
Shouldn't these get strictly better?

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp (right):

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:283: borderWidth =
roundf(borderWidth);
Should we divide then round, or is this needed to match other rounding?

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/BorderValue.h (right):

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/BorderValue.h:81: float width() const {
return m_width; }
I'm a little worried about the compatibility of this change with other browsers.
I did some testing and the landscape looks like:
FF: matches our current behavior
WebKit: matches our current behavior
Edge: matches the proposed behavior

Do you think it makes sense to try contacting the other browsers to see if we
can move unilaterally on this? I wasn't able to find any spec language around
this. I think it would be worth contacting a spec author just to ask since
border width is a little special to begin with.

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/BorderValue.h:88: float m_width;
What range of values does this need to store? I don't think we should
prematurely optimize this, but an option is to used a fixed-point representation
like LayoutUnit.

[pdr., 2017-01-24 19:52:12]

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/BorderValue.h (right):

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/BorderValue.h:88: float m_width;
On 2017/01/24 at 06:57:31, pdr. wrote:
> What range of values does this need to store? I don't think we should
prematurely optimize this, but an option is to used a fixed-point representation
like LayoutUnit.

Looking at this a bit closer... border width is only storing an unsigned short
value (aka 8 bits) but has 26 bits reserved below. I think it would be possible
to use LayoutUnit's fixed point approach while still having the same range of
values for this width, and without increasing the size of this object.

[Karl Øygard, 2017-01-24 22:42:58]

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSProperties.in:214: border-bottom-width
interpolable, initial=initialBorderWidth, converter=convertLineWidth<float>
On 2017/01/24 06:57:31, pdr. wrote:
> Should outline-width be changed in a similar way (as a followup)?

Yes, that would make sense.  I debated including outline-width and
column-rule-width in this patch, since their underlying data structures
now use floats, but opted against it since the changes are already quite
large.

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSProperties.in:215: border-collapse
inherited, independent, keyword_only, keywords=[separate|collapse],
initial_keyword=separate
On 2017/01/24 06:57:31, pdr. wrote:
> Some of the SVG test results seem like regressions (e.g.,
>
third_party/WebKit/LayoutTests/platform/mac/svg/zoom/page/zoom-background-images-expected.png).
> 
> Similarly, the zoom test results seem suspect to me (e.g.,
>
third_party/WebKit/LayoutTests/platform/win/fast/forms/color/color-suggestion-picker-appearance-zoom125-expected.png).
> Shouldn't these get strictly better?

I'll recheck and verify that the new results are as expected.

The SVG tests in particular are a little tricky, since their views have int
sizes, and don't always fit well in with subpixel layout.  That was an
issue before this patch as well, but subpixel borders may in some cases
make it more obvious.

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp (right):

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/BoxBorderPainter.cpp:283: borderWidth =
roundf(borderWidth);
On 2017/01/24 06:57:31, pdr. wrote:
> Should we divide then round, or is this needed to match other rounding?
This should be fine as it is.  The following division is just to place things
correctly for strokeRect().

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/BorderValue.h (right):

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/BorderValue.h:81: float width() const {
return m_width; }
On 2017/01/24 06:57:31, pdr. wrote:
> I'm a little worried about the compatibility of this change with other
browsers.
> I did some testing and the landscape looks like:
> FF: matches our current behavior
> WebKit: matches our current behavior
> Edge: matches the proposed behavior
> 
> Do you think it makes sense to try contacting the other browsers to see if we
> can move unilaterally on this? I wasn't able to find any spec language around
> this. I think it would be worth contacting a spec author just to ask since
> border width is a little special to begin with.

I'm trying to figure out how WebKit and FF are handling subpixel layout--it's
not completely obvious.  FF doesn't appear to support subpixel borders, while
WebKit floors to nearest device pixel. I'll dig some more.

I aligned the code with Edge, for the simple reason that Blink's implementation
of subpixel layout is currently most similar to Edge.  I agree that this may
warrant an opinion from a spec author, I'll see what I can do about that.

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/BorderValue.h:88: float m_width;
On 2017/01/24 19:52:12, pdr. wrote:
> On 2017/01/24 at 06:57:31, pdr. wrote:
> > What range of values does this need to store? I don't think we should
> prematurely optimize this, but an option is to used a fixed-point
representation
> like LayoutUnit.
> 
> Looking at this a bit closer... border width is only storing an unsigned short
> value (aka 8 bits) but has 26 bits reserved below. I think it would be
possible
> to use LayoutUnit's fixed point approach while still having the same range of
> values for this width, and without increasing the size of this object.

Unless I overlooked something, outline-width uses unsigned short, while borders
use unsigned int.  I don't really see a very good reason to allow borders to be
that large, though, so I agree that it might be worthwhile to use an approach
similar to LayoutUnit.

Should I do that here, or is that better suited for a separate patch?

[eae, 2017-01-24 23:57:12]

This looks great!

Could you take my test fast/sub-pixel/sub-pixel-border.html from
https://codereview.chromium.org/2566593002/ and add that to your change? More
test coverage is always better :)

LGTM w/caveats

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/NinePieceImageGrid.cpp (right):

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/NinePieceImageGrid.cpp:23: return
valueForLength(borderSlice.length(), LayoutUnit(boxExtent)).toInt();
This should probably be .round() now

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/NinePieceImageGrid.cpp:28:
valueForLength(slice, LayoutUnit(maximum)).toInt());
...and here.

[pdr., 2017-01-25 00:35:50]

On 2017/01/24 at 22:42:58, karlo wrote:
> values for this width, and without increasing the size of this object.
> 
> Unless I overlooked something, outline-width uses unsigned short, while
borders
> use unsigned int.  I don't really see a very good reason to allow borders to
be
> that large, though, so I agree that it might be worthwhile to use an approach
> similar to LayoutUnit.
> 
> Should I do that here, or is that better suited for a separate patch?

Because of how common BorderValue is, can you try to fit into the current
object's size in this patch? If that turns out to be too hard, I'd be okay with
doing perf&memory tests to prove the regression is okay, but I think it's worth
trying for this patch.

Btw, I talked with Tab Atkins who is familiar with the specs in this area and he
was supportive of this change.

Btw 2, I also think this patch looks great. I should have mentioned that in my
review :)

[Karl Øygard, 2017-01-26 19:49:50]

The CQ bit was checked by karlo@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 19:50:55]

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 21:04:47]

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

[commit-bot: I haz the power, 2017-01-26 21:04:49]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Karl Øygard, 2017-01-27 20:06:29]

On 2017/01/25 00:35:50, pdr. wrote:
> Because of how common BorderValue is, can you try to fit into the current
> object's size in this patch? If that turns out to be too hard, I'd be okay
with
> doing perf&memory tests to prove the regression is okay, but I think it's
worth
> trying for this patch.

I changed the BorderValue width to be fixed point, in the same manner as
LayoutUnit, with six bits for the fraction.  This leaves 20 bits for the integer
part, making 1048575 the largest border supported.  I believe that's
sufficient, agreed?

> Btw, I talked with Tab Atkins who is familiar with the specs in this area and
he
> was supportive of this change.

Excellent, thanks for the help!

[Karl Øygard, 2017-01-27 20:18:42]

On 2017/01/24 23:57:12, eae wrote:
> This looks great!
> 
> Could you take my test fast/sub-pixel/sub-pixel-border.html from
> https://codereview.chromium.org/2566593002/ and add that to your change? More
> test coverage is always better :)

Yes, proper test was missing here.  Butchered and added your test. 

Thanks!

[Karl Øygard, 2017-01-27 20:19:28]

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSProperties.in:215: border-collapse
inherited, independent, keyword_only, keywords=[separate|collapse],
initial_keyword=separate
On 2017/01/24 06:57:31, pdr. wrote:
> Some of the SVG test results seem like regressions (e.g.,
>
third_party/WebKit/LayoutTests/platform/mac/svg/zoom/page/zoom-background-images-expected.png).

Yes, something's amiss here, investigating.

> Similarly, the zoom test results seem suspect to me (e.g.,
>
third_party/WebKit/LayoutTests/platform/win/fast/forms/color/color-suggestion-picker-appearance-zoom125-expected.png).
> Shouldn't these get strictly better?

They look mostly fine to me, what do you see that you don't like?  The form
fields just shift around a bit, I assumed due to borders that now become
larger.  I'll admit I didn't investigate closely--I'm not sure how and where
these pickers are implemented.

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/NinePieceImageGrid.cpp (right):

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/NinePieceImageGrid.cpp:23: return
valueForLength(borderSlice.length(), LayoutUnit(boxExtent)).toInt();
On 2017/01/24 23:57:12, eae wrote:
> This should probably be .round() now

Done.

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/NinePieceImageGrid.cpp:28:
valueForLength(slice, LayoutUnit(maximum)).toInt());
On 2017/01/24 23:57:12, eae wrote:
> ...and here.

Done.

[pdr., 2017-01-27 20:28:45]

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/BorderEdge.cpp (right):

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/BorderEdge.cpp:16: m_width(edgeWidth) {
Can width be negative? If not, can you DCHECK that it's positive?

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/BorderValue.h (right):

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/BorderValue.h:35: static const int
kBorderWidthFractionalBits = 6;
Can you add a comment above here describing the fixed point representation in a
little more detail, for folks encountering this for this first time? Maybe note
that it is just like LayoutUnit.h.

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/BorderValue.h:38: 
Could you add a short unittest for the new fixed point behavior? Basically just
copy LayoutUnitTest.cpp and assert the basic width get/set behavior.

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/BorderValue.h:90: bool widthEquals(float
width) const {
Is this used over checking width() to avoid the perf hit of a divide? If so, can
you add a short comment mentioning that?

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/BorderValue.h:99: return
static_cast<unsigned>(std::min<float>(width, kMaxForBorderWidth) *
Is this safe when width is negative? Could you add a DCHECK that width >= 0?

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/BorderValue.h:106: unsigned m_width : 26; 
// Fixed point width
WDYT of using a typedef to make this a little more clear?
I'm thinking something like this at the top of the file:
using FixedPointBorderWidth = unsigned;

Then:
static FixedPointBorderWidth widthToFixedPoint(float width) { ...
and
FixedPointBorderWidth m_width : 26;

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:108:
group.access()->variable.setWidth(value)
Nit: should this line be indented?

[pdr., 2017-01-27 20:31:15]

On 2017/01/27 at 20:19:28, karlo wrote:
> > Similarly, the zoom test results seem suspect to me (e.g.,
> >
third_party/WebKit/LayoutTests/platform/win/fast/forms/color/color-suggestion-picker-appearance-zoom125-expected.png).
> > Shouldn't these get strictly better?
> 
> They look mostly fine to me, what do you see that you don't like?  The form
> fields just shift around a bit, I assumed due to borders that now become
> larger.  I'll admit I didn't investigate closely--I'm not sure how and where
> these pickers are implemented.

Before this patch there was a dark border around the picker, but now the bottom
border is cut off. Just guessing; I wonder if a clip needs adjusting somewhere?

[Karl Øygard, 2017-01-28 16:46:27]

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/2640143005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSProperties.in:215: border-collapse
inherited, independent, keyword_only, keywords=[separate|collapse],
initial_keyword=separate
On 2017/01/27 20:19:27, karlo wrote:

>
third_party/WebKit/LayoutTests/platform/mac/svg/zoom/page/zoom-background-images-expected.png).
> 
> Yes, something's amiss here, investigating.

I discovered one issue with repeating background images and sub-pixel
rendering.  This issue was already present, but was much less likely to be
triggered before.  I've created a separate bug report for that  in
https://bugs.chromium.org/p/chromium/issues/detail?id=686435

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/BorderValue.h (right):

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/BorderValue.h:90: bool widthEquals(float
width) const {
On 2017/01/27 20:28:44, pdr. wrote:
> Is this used over checking width() to avoid the perf hit of a divide? If so,
can
> you add a short comment mentioning that?

We have to do this, since width now loses precision.  Comparing a float
value with the value from width() may give incorrect results, since
the difference may now be less than what we can represent.

I'll try to add a comment to that effect.

[Karl Øygard, 2017-02-09 10:22:22]

The CQ bit was checked by karlo@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 10:22:33]

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-02-09 12:00:15]

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

[commit-bot: I haz the power, 2017-02-09 12:00:19]

Dry run: This issue passed the CQ dry run.

[Karl Øygard, 2017-02-09 14:22:42]

On 2017/01/27 20:31:15, pdr. wrote:
> On 2017/01/27 at 20:19:28, karlo wrote:
> > They look mostly fine to me, what do you see that you don't like?  The form
> > fields just shift around a bit, I assumed due to borders that now become
> > larger.  I'll admit I didn't investigate closely--I'm not sure how and where
> > these pickers are implemented.
> 
> Before this patch there was a dark border around the picker, but now the
bottom
> border is cut off. Just guessing; I wonder if a clip needs adjusting
somewhere?

This appears to be an existing issue, caused by some unfortunate assumptions
and interactions in Source/web/resources/colorSuggestionPicker.js and friends.

The color picker is a separate window, and the contents of the window is
generated in ColorChooserPopupUIController.cpp.  colorSuggestionPicker.js
resizes the window to exactly fit the content, but the zoom factor is applied
afterwards, causing an additional reflow (see WebPagePopupImpl.cpp:329).
There is no guarantee that the zoomed contents will fit the window any more,
and most of the time it doesn't.

Without subpixel borders, the content will most of the time shrink, leaving
the window too big, which is obvious opening
color-suggestion-picker-appearance-zoom125.html in Linux Chrome.
color-suggestion-picker-appearance-zoom200.html is vice versa one pixel
too large, cutting off one pixel of the bottom border.

With subpixel borders, probability increases that that the content will be too
large.  Moving the application of the zoom factor to before loading the
window contents resolves the issue, but I haven't tested it extensively.

What would you advise?

[Karl Øygard, 2017-02-10 12:53:18]

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/BorderEdge.cpp (right):

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/BorderEdge.cpp:16: m_width(edgeWidth) {
On 2017/01/27 20:28:44, pdr. wrote:
> Can width be negative? If not, can you DCHECK that it's positive?

Done.

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/BorderValue.h (right):

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/BorderValue.h:99: return
static_cast<unsigned>(std::min<float>(width, kMaxForBorderWidth) *
On 2017/01/27 20:28:44, pdr. wrote:
> Is this safe when width is negative? Could you add a DCHECK that width >= 0?

Done.

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/BorderValue.h:106: unsigned m_width : 26; 
// Fixed point width
On 2017/01/27 20:28:44, pdr. wrote:
> WDYT of using a typedef to make this a little more clear?
> I'm thinking something like this at the top of the file:
> using FixedPointBorderWidth = unsigned;
> 
> Then:
> static FixedPointBorderWidth widthToFixedPoint(float width) { ...
> and
> FixedPointBorderWidth m_width : 26;

I agree and tried, but the check-webkit-style presubmit disagrees;
bitfields have to be declared unsigned.

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2640143005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:108:
group.access()->variable.setWidth(value)
On 2017/01/27 20:28:44, pdr. wrote:
> Nit: should this line be indented?

I agree, but the formatter disagrees...

[pdr., 2017-02-10 22:41:33]

On 2017/02/09 at 14:22:42, karlo wrote:
> On 2017/01/27 20:31:15, pdr. wrote:
> > On 2017/01/27 at 20:19:28, karlo wrote:
> > > They look mostly fine to me, what do you see that you don't like?  The
form
> > > fields just shift around a bit, I assumed due to borders that now become
> > > larger.  I'll admit I didn't investigate closely--I'm not sure how and
where
> > > these pickers are implemented.
> > 
> > Before this patch there was a dark border around the picker, but now the
bottom
> > border is cut off. Just guessing; I wonder if a clip needs adjusting
somewhere?
> 
> This appears to be an existing issue, caused by some unfortunate assumptions
> and interactions in Source/web/resources/colorSuggestionPicker.js and friends.
> 
> The color picker is a separate window, and the contents of the window is
> generated in ColorChooserPopupUIController.cpp.  colorSuggestionPicker.js
> resizes the window to exactly fit the content, but the zoom factor is applied
> afterwards, causing an additional reflow (see WebPagePopupImpl.cpp:329).
> There is no guarantee that the zoomed contents will fit the window any more,
> and most of the time it doesn't.
> 
> Without subpixel borders, the content will most of the time shrink, leaving
> the window too big, which is obvious opening
> color-suggestion-picker-appearance-zoom125.html in Linux Chrome.
> color-suggestion-picker-appearance-zoom200.html is vice versa one pixel
> too large, cutting off one pixel of the bottom border.
> 
> With subpixel borders, probability increases that that the content will be too
> large.  Moving the application of the zoom factor to before loading the
> window contents resolves the issue, but I haven't tested it extensively.
> 
> What would you advise?

Thanks for the investigation. This seems like a regression so I think we should
fix it, but as a followup. Can you file a bug and we can iterate on your
zoom-before-load patch?

[pdr., 2017-02-10 22:42:31]

LGTM! Great patch.

Lets iterate on the SVG and color picker issues as followups.

[Karl Øygard, 2017-02-12 16:39:09]

On 2017/02/10 22:42:31, pdr. wrote:
> LGTM! Great patch.
> 
> Lets iterate on the SVG and color picker issues as followups.

I've created an issue for the color picker in
https://bugs.chromium.org/p/chromium/issues/detail?id=691375

Many, many thanks for the help, greatly appreciated.  Let's hope the patch
sticks!

[Karl Øygard, 2017-02-13 11:27:32]

The CQ bit was checked by karlo@opera.com

[Karl Øygard, 2017-02-13 11:27:34]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2640143005/#ps70001
(title: "Rebaselined tests.")

[commit-bot: I haz the power, 2017-02-13 11:27:43]

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-13 13:52:17]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1486985252992630,
"parent_rev": "d6df828820be4b0f22b9bed959fb2bd93f84b331", "commit_rev":
"934becac5daa91ea979fb66e4ae21761ca11ebc9"}

[commit-bot: I haz the power, 2017-02-13 13:52:57]

Description was changed from

==========
Support subpixel layout of borders.

This cl implements subpixel layout of borders.  Painting of subpixel
borders now uses rounding to nearest CSS pixel, and is aligned with
Edge.  Handling of device pixel ratios > 1 isn't implemented here,
and is better covered in a separate task.

This does not implement subpixel borders for tables, as that interacts
with table layout width calculations, which would also need to be
adapted to subpixels.  Again, that's better dealt with in a separate
task.

A number of test cases had to be rebaselined, due to minor changes in
layout and/or painting.  Most of these changes appear in tests that uses
zooming, and implicitly have fractional borders.

svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm now looks a bit
broken, but that's due to an unrelated issue with aspect ratio and
subpixels (in LayoutPart, if I recall correctly).

BUG=227192
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Support subpixel layout of borders.

This cl implements subpixel layout of borders.  Painting of subpixel
borders now uses rounding to nearest CSS pixel, and is aligned with
Edge.  Handling of device pixel ratios > 1 isn't implemented here,
and is better covered in a separate task.

This does not implement subpixel borders for tables, as that interacts
with table layout width calculations, which would also need to be
adapted to subpixels.  Again, that's better dealt with in a separate
task.

A number of test cases had to be rebaselined, due to minor changes in
layout and/or painting.  Most of these changes appear in tests that uses
zooming, and implicitly have fractional borders.

svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm now looks a bit
broken, but that's due to an unrelated issue with aspect ratio and
subpixels (in LayoutPart, if I recall correctly).

BUG=227192
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2640143005
Cr-Commit-Position: refs/heads/master@{#449943}
Committed:
https://chromium.googlesource.com/chromium/src/+/934becac5daa91ea979fb66e4ae2...
==========

[commit-bot: I haz the power, 2017-02-13 13:52:59]

Committed patchset #5 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/934becac5daa91ea979fb66e4ae2...

[Stephen Chennney, 2017-04-28 15:28:46]

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

[Stephen Chennney, 2017-04-28 15:28:48]

svg/zoom/page/zoom-background-images-expected.png shows the background not fit
correctly to the border painting.

Other tests with filled backgrounds show similar effects. These were important,
and should not have been landed.