Fix for phase in background tiling.

Fix for phase in background tiling.

When computing the phase for background image tiling using snapped
coordinates, we get very incorrect results, and unstable results,
when the background-position is large (because the division of
the position by the tile width magnifies the rounding artifacts).
This patch changes the phase calculation to compute the phase using
unsnapped values then re-apply the fractional component to the
snapped values. This results in stable, near-pixel perfect results.
Given very large background-positions, we can't do much better.
There's a new test and some results changed by a pixel or so.

There is some code refactoring to reduce what is becoming a very
unwieldy method. Sorry that makes it harder to review.

R=leviw@chromium.org
BUG=593079
Committed: https://crrev.com/41f3d062c90c1a5d67228e6e061c7e35fd4d62fd
Cr-Commit-Position: refs/heads/master@{#384706}

[Stephen Chennney, 2016-03-31 21:16:40]

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

[commit-bot: I haz the power, 2016-03-31 21:16:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847983003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847983003/1

[Stephen Chennney, 2016-03-31 21:17:00]

It's ugly, but it works. Couldn't see a better way.

[commit-bot: I haz the power, 2016-03-31 21:26:15]

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

[commit-bot: I haz the power, 2016-03-31 21:26:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Stephen Chennney, 2016-04-01 16:50:43]

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

[commit-bot: I haz the power, 2016-04-01 16:50:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847983003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847983003/20001

[commit-bot: I haz the power, 2016-04-01 18:20:53]

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

[commit-bot: I haz the power, 2016-04-01 18:20:54]

Dry run: This issue passed the CQ dry run.

[leviw_travelin_and_unemployed, 2016-04-01 20:16:41]

I looked at the bot runs from before you added test expectations but didn't see
where you got your whole list of failing tests. Did I miss something?

I'm a bit confused why you're using const LayoutUnit& refs instead of just
copying.

Anyways, this is a good change. LGTM.

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

https://codereview.chromium.org/1847983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:127:
NOTREACHED();
Why this change? Are we moving to using NOTREACHED() instead of
ASSERT_NOT_REACHED()?

https://codereview.chromium.org/1847983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:161: void
BackgroundImageGeometry::setNoRepeatX(const LayoutUnit& xOffset)
I find it weird to use const references for LayoutUnits. It shouldn't be any
cheaper than just copying.

https://codereview.chromium.org/1847983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:169: void
BackgroundImageGeometry::setNoRepeatY(const LayoutUnit& yOffset)
Ditto.

https://codereview.chromium.org/1847983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:186: if
(unsnappedTileSize.width()) {
This is unfortunate copy pasta.

https://codereview.chromium.org/1847983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:229: void
BackgroundImageGeometry::setSpaceX(const LayoutUnit& space, const LayoutUnit&
availableWidth, const LayoutUnit& extraOffset)
Elon Musk is gonna be pissed.

[Stephen Chennney, 2016-04-01 20:38:36]

Test failures were from a local run and the results of the first try job. I
checked them all and they are very minor (not like the one's I had while trying
to fix all my typos).

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

https://codereview.chromium.org/1847983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:127:
NOTREACHED();
On 2016/04/01 20:16:40, leviw wrote:
> Why this change? Are we moving to using NOTREACHED() instead of
> ASSERT_NOT_REACHED()?

Yes, per an email on blink-dev we're supposed to move all Blink ASSERT related
stuff to Chromium equivalents.

https://codereview.chromium.org/1847983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:161: void
BackgroundImageGeometry::setNoRepeatX(const LayoutUnit& xOffset)
On 2016/04/01 20:16:40, leviw wrote:
> I find it weird to use const references for LayoutUnits. It shouldn't be any
> cheaper than just copying.

I'll switch them back. I was just in a const groove.

https://codereview.chromium.org/1847983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:186: if
(unsnappedTileSize.width()) {
On 2016/04/01 20:16:40, leviw wrote:
> This is unfortunate copy pasta.

Yeah, but because we don't use arrays of 2 values for [x,y] we can't make them
all one method. I do only use width in this one though, and height in the next,
so it can be simplified.

https://codereview.chromium.org/1847983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:229: void
BackgroundImageGeometry::setSpaceX(const LayoutUnit& space, const LayoutUnit&
availableWidth, const LayoutUnit& extraOffset)
On 2016/04/01 20:16:40, leviw wrote:
> Elon Musk is gonna be pissed.

I never saw that coming.

[Stephen Chennney, 2016-04-01 20:39:49]

Description was changed from

==========
Fix for phase in background tiling.

When computing hte phase for background image tiling using snapped
coordinates, we get very incorrect results, and unstable results,
when the background-position is large (because the division of
the position by the tile width magnifies the roundig artifacts).
This patch changes the phase calculation to compute the phase using
unsnapped values then re-apply the fractional component to the
snapped values. This reslts in stable, near-pixel perfect results.
Given very large background-positions, we can't do much better.
There's a new test and some results changed by a pixel or so.

There is some code refactoring to reduce what is becoming a very
unwieldy method. Sorry that makes it harder to review.

R=leviw@chromium.org
BUG=593079
==========

to

==========
Fix for phase in background tiling.

When computing the phase for background image tiling using snapped
coordinates, we get very incorrect results, and unstable results,
when the background-position is large (because the division of
the position by the tile width magnifies the rounding artifacts).
This patch changes the phase calculation to compute the phase using
unsnapped values then re-apply the fractional component to the
snapped values. This results in stable, near-pixel perfect results.
Given very large background-positions, we can't do much better.
There's a new test and some results changed by a pixel or so.

There is some code refactoring to reduce what is becoming a very
unwieldy method. Sorry that makes it harder to review.

R=leviw@chromium.org
BUG=593079
==========

[Stephen Chennney, 2016-04-01 20:43:25]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2016-04-01 20:43:26]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1847983003/#ps40001
(title: "Code now better")

[commit-bot: I haz the power, 2016-04-01 20:43:46]

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

[leviw_travelin_and_unemployed, 2016-04-01 20:50:24]

On 2016/04/01 at 20:38:36, schenney wrote:
> Test failures were from a local run and the results of the first try job. I
checked them all and they are very minor (not like the one's I had while trying
to fix all my typos).
> 
>
https://codereview.chromium.org/1847983003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (right):
> 
>
https://codereview.chromium.org/1847983003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:127:
NOTREACHED();
> On 2016/04/01 20:16:40, leviw wrote:
> > Why this change? Are we moving to using NOTREACHED() instead of
> > ASSERT_NOT_REACHED()?
> 
> Yes, per an email on blink-dev we're supposed to move all Blink ASSERT related
stuff to Chromium equivalents.
> 
>
https://codereview.chromium.org/1847983003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:161: void
BackgroundImageGeometry::setNoRepeatX(const LayoutUnit& xOffset)
> On 2016/04/01 20:16:40, leviw wrote:
> > I find it weird to use const references for LayoutUnits. It shouldn't be any
> > cheaper than just copying.
> 
> I'll switch them back. I was just in a const groove.
> 
>
https://codereview.chromium.org/1847983003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:186: if
(unsnappedTileSize.width()) {
> On 2016/04/01 20:16:40, leviw wrote:
> > This is unfortunate copy pasta.
> 
> Yeah, but because we don't use arrays of 2 values for [x,y] we can't make them
all one method. I do only use width in this one though, and height in the next,
so it can be simplified.
> 
>
https://codereview.chromium.org/1847983003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:229: void
BackgroundImageGeometry::setSpaceX(const LayoutUnit& space, const LayoutUnit&
availableWidth, const LayoutUnit& extraOffset)
> On 2016/04/01 20:16:40, leviw wrote:
> > Elon Musk is gonna be pissed.
> 
> I never saw that coming.

:)

Thanks again for working on this!

[commit-bot: I haz the power, 2016-04-01 22:21:05]

Description was changed from

==========
Fix for phase in background tiling.

When computing the phase for background image tiling using snapped
coordinates, we get very incorrect results, and unstable results,
when the background-position is large (because the division of
the position by the tile width magnifies the rounding artifacts).
This patch changes the phase calculation to compute the phase using
unsnapped values then re-apply the fractional component to the
snapped values. This results in stable, near-pixel perfect results.
Given very large background-positions, we can't do much better.
There's a new test and some results changed by a pixel or so.

There is some code refactoring to reduce what is becoming a very
unwieldy method. Sorry that makes it harder to review.

R=leviw@chromium.org
BUG=593079
==========

to

==========
Fix for phase in background tiling.

When computing the phase for background image tiling using snapped
coordinates, we get very incorrect results, and unstable results,
when the background-position is large (because the division of
the position by the tile width magnifies the rounding artifacts).
This patch changes the phase calculation to compute the phase using
unsnapped values then re-apply the fractional component to the
snapped values. This results in stable, near-pixel perfect results.
Given very large background-positions, we can't do much better.
There's a new test and some results changed by a pixel or so.

There is some code refactoring to reduce what is becoming a very
unwieldy method. Sorry that makes it harder to review.

R=leviw@chromium.org
BUG=593079
==========

[commit-bot: I haz the power, 2016-04-01 22:21:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-01 22:23:31]

Description was changed from

==========
Fix for phase in background tiling.

When computing the phase for background image tiling using snapped
coordinates, we get very incorrect results, and unstable results,
when the background-position is large (because the division of
the position by the tile width magnifies the rounding artifacts).
This patch changes the phase calculation to compute the phase using
unsnapped values then re-apply the fractional component to the
snapped values. This results in stable, near-pixel perfect results.
Given very large background-positions, we can't do much better.
There's a new test and some results changed by a pixel or so.

There is some code refactoring to reduce what is becoming a very
unwieldy method. Sorry that makes it harder to review.

R=leviw@chromium.org
BUG=593079
==========

to

==========
Fix for phase in background tiling.

When computing the phase for background image tiling using snapped
coordinates, we get very incorrect results, and unstable results,
when the background-position is large (because the division of
the position by the tile width magnifies the rounding artifacts).
This patch changes the phase calculation to compute the phase using
unsnapped values then re-apply the fractional component to the
snapped values. This results in stable, near-pixel perfect results.
Given very large background-positions, we can't do much better.
There's a new test and some results changed by a pixel or so.

There is some code refactoring to reduce what is becoming a very
unwieldy method. Sorry that makes it harder to review.

R=leviw@chromium.org
BUG=593079

Committed: https://crrev.com/41f3d062c90c1a5d67228e6e061c7e35fd4d62fd
Cr-Commit-Position: refs/heads/master@{#384706}
==========

[commit-bot: I haz the power, 2016-04-01 22:23:32]

Patchset 3 (id:??) landed as
https://crrev.com/41f3d062c90c1a5d67228e6e061c7e35fd4d62fd
Cr-Commit-Position: refs/heads/master@{#384706}