Use background positioning origin for repeating backgrounds

Use background positioning origin for repeating backgrounds

When support for 4-value syntax for background-position was added,
only the non-repeating case was actually tested and render support for
the non-top-left anchor points were missing.

This patch adds render support and a simple test for the three anchor
positions that wasn't supported before for repeated/tiled
backgrounds.

BUG=488707, 521481
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200718

[davve, 2015-08-14 07:02:21]

davve@opera.com changed reviewers:
+ jchaffraix@chromium.org

[davve, 2015-08-14 07:02:21]

PTAL.

Related to this BoxPainter::calculateBackgroundImageGeometry()
looks like a potential candidate for refactoring. One idea is to move
the function to a hypothetical BackgroundImageGeometry::calculate(). That
way BackgroundImageGeometry could be made more class-like (with
smaller an API) rather than the struct-like functionality it provides
today. WDYT?

There is also the TODO about calculateBackgroundImageGeometry used in
different phases (inside and outside paint). Perhaps that could be
addressed too.

[davve, 2015-08-17 19:27:43]

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

[davve, 2015-08-17 19:27:43]

Philip, are you able to review perhaps? Or point me in the direction of other
paint experts that love backgrounds and such...

[pdr., 2015-08-18 06:41:08]

I love background images! Or, well, that's a lie, but I do love them more when
they are painted correctly.

This code could use some refactoring, but that's a bit out of scope for this
patch. I like the fix you have, just one question about the spacefill cases.

https://codereview.chromium.org/1287293002/diff/1/Source/core/paint/BoxPainte...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1287293002/diff/1/Source/core/paint/BoxPainte...
Source/core/paint/BoxPainter.cpp:722: computedXPosition =
roundedMinimumValueForLength(Length(), availableWidth);
I think we need to account for the origin for SpaceFill as well.

In https://drafts.csswg.org/css-backgrounds-3/#the-background-repeat under
'space' it says the background position should be used which would include the
origin.

[davve, 2015-08-18 06:52:16]

https://codereview.chromium.org/1287293002/diff/1/Source/core/paint/BoxPainte...
File Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/1287293002/diff/1/Source/core/paint/BoxPainte...
Source/core/paint/BoxPainter.cpp:722: computedXPosition =
roundedMinimumValueForLength(Length(), availableWidth);
On 2015/08/18 06:41:08, pdr wrote:
> I think we need to account for the origin for SpaceFill as well.
> 
> In https://drafts.csswg.org/css-backgrounds-3/#the-background-repeat under
> 'space' it says the background position should be used which would include the
> origin.

Not sure I follow. Spec says for space: "The value of ‘background-position’ for
this direction is ignored, unless there is not enough space for two copies of
the image in this direction .."

the 'unless' part covered by falling back to NoRepeatFill a couple of lines
down.

[davve, 2015-08-18 06:55:43]

On 2015/08/18 06:41:08, pdr wrote:

> This code could use some refactoring, but that's a bit out of scope for this
> patch. I like the fix you have, just one question about the spacefill cases.

Yes, I've started small with
http://code.google.com/p/chromium/issues/detail?id=521481 but I have some bigger
patches locally too. I'll uploaded it or them later today.

Perhaps you like to review https://codereview.chromium.org/1297913003/ too?

[pdr., 2015-08-18 06:57:23]

On 2015/08/18 at 06:52:16, davve wrote:
>
https://codereview.chromium.org/1287293002/diff/1/Source/core/paint/BoxPainte...
> File Source/core/paint/BoxPainter.cpp (right):
> 
>
https://codereview.chromium.org/1287293002/diff/1/Source/core/paint/BoxPainte...
> Source/core/paint/BoxPainter.cpp:722: computedXPosition =
roundedMinimumValueForLength(Length(), availableWidth);
> On 2015/08/18 06:41:08, pdr wrote:
> > I think we need to account for the origin for SpaceFill as well.
> > 
> > In https://drafts.csswg.org/css-backgrounds-3/#the-background-repeat under
> > 'space' it says the background position should be used which would include
the
> > origin.
> 
> Not sure I follow. Spec says for space: "The value of ‘background-position’
for this direction is ignored, unless there is not enough space for two copies
of the image in this direction .."
> 
> the 'unless' part covered by falling back to NoRepeatFill a couple of lines
down.

Oh indeed, you're right. The "space >= 0" check is not the easiest to understand
at a glance :/

LGTM

[pdr., 2015-08-18 07:07:47]

On 2015/08/18 at 06:55:43, davve wrote:
> On 2015/08/18 06:41:08, pdr wrote:
> 
> > This code could use some refactoring, but that's a bit out of scope for this
> > patch. I like the fix you have, just one question about the spacefill cases.
> 
> Yes, I've started small with
http://code.google.com/p/chromium/issues/detail?id=521481 but I have some bigger
patches locally too. I'll uploaded it or them later today.

We actually have a fairly big paint team these days. I think we're all
auto-cc'ed on patches in core/paint but folks are known to be shy about signing
up for background image patches :) Feel free to pick a few to add as reviewers
(me, chrishtr, wangxianzhu, fs, etc) on these.

> 
> Perhaps you like to review https://codereview.chromium.org/1297913003/ too?

Love it. LGTM'd

[davve, 2015-08-18 07:11:48]

The CQ bit was checked by davve@opera.com

[commit-bot: I haz the power, 2015-08-18 07:12:00]

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

[commit-bot: I haz the power, 2015-08-18 10:13:05]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200718