Turn down BackgroundImageGeometry aggressive LayoutUnit conversion by a notch

Turn down BackgroundImageGeometry aggressive LayoutUnit conversion by a notch

https://codereview.chromium.org/1456813002 was landed with the best of
intentions. While still a progression, it turned out to be a little
aggressive about the LayoutUnit conversion. The patch kept the notion
of flooring our tile size, but moved that operation to the end of the
background image geometry calculation. This ends up causing trouble
on even relatively simple pages with scaled background images.

We continue to get the benefit of calculating offsets in LayoutUnits
from the previous patch, but this will fix the issue of using pre-floored
tile size values by applying the sub-pixel heuristic before using the
calculated tile size for other calculations.

BUG=565926
Committed: https://crrev.com/9c60333a01e2cad5fa21318c39c8a330244b3d56
Cr-Commit-Position: refs/heads/master@{#363805}

[leviw_travelin_and_unemployed, 2015-12-08 03:54:41]

Description was changed from

==========
Turn down BackgroundImageGeometry aggressive LayoutUnit conversion by a notch

https://codereview.chromium.org/1456813002 was landed with the best of
intentions. While still a progression, it turned out to be a little
aggressive about the LayoutUnit conversion. The patch kept the notion
of flooring our tile size, but moved that operation to the end of the
background image geometry calculation. This ends up causing trouble
on even relatively simple pages with scaled background images.

We continue to get the benefit of calculating offsets in LayoutUnits
from the previous patch, but this will fix the issue of using pre-floored
tile size values.

BUG=565926
==========

to

==========
Turn down BackgroundImageGeometry aggressive LayoutUnit conversion by a notch

https://codereview.chromium.org/1456813002 was landed with the best of
intentions. While still a progression, it turned out to be a little
aggressive about the LayoutUnit conversion. The patch kept the notion
of flooring our tile size, but moved that operation to the end of the
background image geometry calculation. This ends up causing trouble
on even relatively simple pages with scaled background images.

We continue to get the benefit of calculating offsets in LayoutUnits
from the previous patch, but this will fix the issue of using pre-floored
tile size values by applying the sub-pixel heuristic before using the
calculated tile size for other calculations.

BUG=565926
==========

[leviw_travelin_and_unemployed, 2015-12-08 03:54:41]

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

[leviw_travelin_and_unemployed, 2015-12-08 04:03:29]

Fallout from sub-pixel sprites, round #1!

http://i.imgur.com/Cm9W6fR.jpg

[pdr., 2015-12-08 17:36:45]

On 2015/12/08 at 04:03:29, leviw wrote:
> Fallout from sub-pixel sprites, round #1!
> 
> http://i.imgur.com/Cm9W6fR.jpg

This patch looks great but css2.1/20110323/background-intrinsic-004.htm looks
like a real failure. Does the test need updating?

[leviw_travelin_and_unemployed, 2015-12-08 18:39:26]

On 2015/12/08 at 17:36:45, pdr wrote:
> On 2015/12/08 at 04:03:29, leviw wrote:
> > Fallout from sub-pixel sprites, round #1!
> > 
> > http://i.imgur.com/Cm9W6fR.jpg
> 
> This patch looks great but css2.1/20110323/background-intrinsic-004.htm looks
like a real failure. Does the test need updating?

Ahhh yes, *that* test. You had me worried for a second!

This result is actually reverting that particular test to its *previous* state
of failure (before my tiling patch from last week) from what I'll now consider
its *intermediate* state of failure. I've never seen it entirely green :(

So allow me to rest you concerns that it's somehow more or less broken than
before.







Okay, you've had a moment to consider how great our tests are. WDYT about
removing it entirely?

[pdr., 2015-12-08 18:45:02]

On 2015/12/08 at 18:39:26, leviw wrote:
> On 2015/12/08 at 17:36:45, pdr wrote:
> > On 2015/12/08 at 04:03:29, leviw wrote:
> > > Fallout from sub-pixel sprites, round #1!
> > > 
> > > http://i.imgur.com/Cm9W6fR.jpg
> > 
> > This patch looks great but css2.1/20110323/background-intrinsic-004.htm
looks like a real failure. Does the test need updating?
> 
> Ahhh yes, *that* test. You had me worried for a second!
> 
> This result is actually reverting that particular test to its *previous* state
of failure (before my tiling patch from last week) from what I'll now consider
its *intermediate* state of failure. I've never seen it entirely green :(
> 
> So allow me to rest you concerns that it's somehow more or less broken than
before.
> 
> 
> 
> 
> 
> 
> 
> Okay, you've had a moment to consider how great our tests are. WDYT about
removing it entirely?

Levi and I looked at this more and this patch makes us match how firefox fails
this test. LGTM

We may want to just change the test (in a followup patch) to be all green for
our & firefox's behavior.

[leviw_travelin_and_unemployed, 2015-12-08 19:59:59]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2015-12-08 20:01:06]

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

[davve, 2015-12-08 20:19:21]

LGTM

I see this is already on the way, but I only had nits and questions to offer
anyway. Feel free to ignore.

https://codereview.chromium.org/1504463002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/backgrounds/size/scaled-sprited-background.html
(right):

https://codereview.chromium.org/1504463002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/backgrounds/size/scaled-sprited-background.html:10:
<body>
nit: you may remove <body> here and in the -expected file?

https://codereview.chromium.org/1504463002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (left):

https://codereview.chromium.org/1504463002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:167: static
LayoutSize applySubPixelHeuristicToImageSize(const LayoutSize& size, const
LayoutRect& destination)
nit: If you like you can put this function in the unnamed namespace, as is done
for the other helper functions in this file.

https://codereview.chromium.org/1504463002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/1504463002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:171:
size.width().fraction() == destination.width().fraction() ?
snapSizeToPixel(size.width(), destination.x()) : size.width().floor(),
The other change below is easier to understand than this one. What was the
reason for checking only the fraction part?

[commit-bot: I haz the power, 2015-12-08 21:56:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-08 21:58:10]

Description was changed from

==========
Turn down BackgroundImageGeometry aggressive LayoutUnit conversion by a notch

https://codereview.chromium.org/1456813002 was landed with the best of
intentions. While still a progression, it turned out to be a little
aggressive about the LayoutUnit conversion. The patch kept the notion
of flooring our tile size, but moved that operation to the end of the
background image geometry calculation. This ends up causing trouble
on even relatively simple pages with scaled background images.

We continue to get the benefit of calculating offsets in LayoutUnits
from the previous patch, but this will fix the issue of using pre-floored
tile size values by applying the sub-pixel heuristic before using the
calculated tile size for other calculations.

BUG=565926
==========

to

==========
Turn down BackgroundImageGeometry aggressive LayoutUnit conversion by a notch

https://codereview.chromium.org/1456813002 was landed with the best of
intentions. While still a progression, it turned out to be a little
aggressive about the LayoutUnit conversion. The patch kept the notion
of flooring our tile size, but moved that operation to the end of the
background image geometry calculation. This ends up causing trouble
on even relatively simple pages with scaled background images.

We continue to get the benefit of calculating offsets in LayoutUnits
from the previous patch, but this will fix the issue of using pre-floored
tile size values by applying the sub-pixel heuristic before using the
calculated tile size for other calculations.

BUG=565926

Committed: https://crrev.com/9c60333a01e2cad5fa21318c39c8a330244b3d56
Cr-Commit-Position: refs/heads/master@{#363805}
==========

[commit-bot: I haz the power, 2015-12-08 21:58:11]

Patchset 4 (id:??) landed as
https://crrev.com/9c60333a01e2cad5fa21318c39c8a330244b3d56
Cr-Commit-Position: refs/heads/master@{#363805}

[leviw_travelin_and_unemployed, 2015-12-08 22:43:19]

On 2015/12/08 at 20:19:21, davve wrote:
> LGTM

Thanks for the review davve! I'll address your comments in a follow-up!

> 
> I see this is already on the way, but I only had nits and questions to offer
anyway. Feel free to ignore.
> 
>
https://codereview.chromium.org/1504463002/diff/60001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/fast/backgrounds/size/scaled-sprited-background.html
(right):
> 
>
https://codereview.chromium.org/1504463002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/fast/backgrounds/size/scaled-sprited-background.html:10:
<body>
> nit: you may remove <body> here and in the -expected file?
Sure

> 
>
https://codereview.chromium.org/1504463002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (left):
> 
>
https://codereview.chromium.org/1504463002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:167: static
LayoutSize applySubPixelHeuristicToImageSize(const LayoutSize& size, const
LayoutRect& destination)
> nit: If you like you can put this function in the unnamed namespace, as is
done for the other helper functions in this file.
I'm kind of indifferent to this approach, but I agree it makes sense for this
file to be internally consistent.

> 
>
https://codereview.chromium.org/1504463002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (right):
> 
>
https://codereview.chromium.org/1504463002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:171:
size.width().fraction() == destination.width().fraction() ?
snapSizeToPixel(size.width(), destination.x()) : size.width().floor(),
> The other change below is easier to understand than this one. What was the
reason for checking only the fraction part?
I probably could have explained this better...

Why fraction? We generally want to snap the tile the same way our renderer
snapped, and if our fractions match, that's a pretty good heuristic.
Specifically, the common case is the tile perfectly fits within a box's border,
so it doesn't match the border-box size, but it does match the padding-box, and
hence the fractions will match (as we compute borders to pixels, at least now).

I'm totally open to suggestions on improving this logic, and can add some more
info to the follow-up.