Last Comment Bug 548372 - Implement background-repeat: [space | round]
: Implement background-repeat: [space | round]
Status: RESOLVED FIXED
: css3, dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Other Branch
: All All
: -- normal with 14 votes (vote)
: mozilla49
Assigned To: Ethan Lin[:ethlin]
:
Mentors:
http://dev.w3.org/csswg/css3-backgrou...
: 589678 589679 589680 589687 603852 (view as bug list)
Depends on: 548375 1275957 1275966
Blocks: css3-background css3test 1276582 ietestcenter 720531 1244609 1258626 1276432
  Show dependency treegraph
 
Reported: 2010-02-24 11:50 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2016-05-30 01:22 PDT (History)
45 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
background-repeat patch 1 reftests (35.91 KB, patch)
2012-01-12 19:33 PST, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat patch 1 reftests v2 (27.04 KB, patch)
2012-01-16 15:42 PST, Lazar Sumar [:nonsensickle]
roc: review+
Details | Diff | Splinter Review
background-repeat patch 1 reftests v3 (26.18 KB, patch)
2012-01-16 17:14 PST, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat patch 1 reftests v4 (25.06 KB, patch)
2012-01-16 18:27 PST, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat parser changes patch v1 (4.58 KB, patch)
2012-01-18 20:51 PST, Lazar Sumar [:nonsensickle]
dbaron: review+
Details | Diff | Splinter Review
background-repeat patch 1 reftests v5 (18.04 KB, patch)
2012-01-19 23:08 PST, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat patch 1 reftests v5 (18.04 KB, patch)
2012-01-19 23:10 PST, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat round rendering patch v1 (10.73 KB, patch)
2012-01-19 23:16 PST, Lazar Sumar [:nonsensickle]
roc: review+
Details | Diff | Splinter Review
background-repeat round rendering patch v2 (10.76 KB, patch)
2012-01-22 15:06 PST, Lazar Sumar [:nonsensickle]
bugzilla: review+
Details | Diff | Splinter Review
background-repeat round rendering patch v2.1 (10.77 KB, patch)
2012-01-22 15:20 PST, Lazar Sumar [:nonsensickle]
bugzilla: review+
Details | Diff | Splinter Review
background-repeat patch 1 reftests v6 (25.81 KB, patch)
2012-01-22 15:35 PST, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat patch 1 reftests v7 (35.07 KB, patch)
2012-01-23 20:32 PST, Lazar Sumar [:nonsensickle]
dbaron: review+
Details | Diff | Splinter Review
background-repeat space rendering patch (incomplete) (25.29 KB, patch)
2012-02-16 19:40 PST, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat space rendering patch v1 (29.71 KB, patch)
2012-02-19 12:05 PST, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat space rendering patch v1 (29.58 KB, patch)
2012-02-19 16:07 PST, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat space rendering patch v1.3 (29.63 KB, patch)
2012-02-19 19:49 PST, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat space rendering patch v1.4 (26.13 KB, patch)
2012-02-21 13:57 PST, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat parser changes patch v2 (5.69 KB, patch)
2012-02-23 13:15 PST, Lazar Sumar [:nonsensickle]
bugzilla: review+
Details | Diff | Splinter Review
background-repeat patch 1 tests v8 (4.79 KB, patch)
2012-02-23 13:18 PST, Lazar Sumar [:nonsensickle]
bugzilla: review+
Details | Diff | Splinter Review
background-repeat patch 1 tests v8 (35.79 KB, patch)
2012-02-23 13:20 PST, Lazar Sumar [:nonsensickle]
bugzilla: review+
Details | Diff | Splinter Review
background-repeat parser changes patch v2 (5.55 KB, patch)
2012-02-23 14:51 PST, Lazar Sumar [:nonsensickle]
bugzilla: review+
Details | Diff | Splinter Review
background-repeat space rendering patch v1.5 (28.08 KB, patch)
2012-02-23 16:00 PST, Lazar Sumar [:nonsensickle]
bugzilla: review-
Details | Diff | Splinter Review
background-repeat parser changes patch v2.1 (4.84 KB, patch)
2012-02-23 17:27 PST, Lazar Sumar [:nonsensickle]
bugzilla: review+
Details | Diff | Splinter Review
background-repeat space rendering patch v1.6 (28.08 KB, patch)
2012-03-04 13:15 PST, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat patch 1 tests v9 (34.98 KB, patch)
2013-06-15 15:21 PDT, Lazar Sumar [:nonsensickle]
roc: review+
Details | Diff | Splinter Review
background-repeat patch 2 parser changes v3 (4.84 KB, patch)
2013-06-15 15:24 PDT, Lazar Sumar [:nonsensickle]
dbaron: review+
Details | Diff | Splinter Review
background-repeat patch 3 round rendering v3 (10.82 KB, patch)
2013-06-15 15:25 PDT, Lazar Sumar [:nonsensickle]
roc: review+
Details | Diff | Splinter Review
background-repeat patch 4 space rendering v2 (32.91 KB, patch)
2013-06-15 15:27 PDT, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat patch 4 space rendering v2 (32.91 KB, patch)
2013-06-15 15:29 PDT, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat patch 4 space rendering v2.1 (32.79 KB, patch)
2013-06-22 04:00 PDT, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat patch 4 space rendering v2.2 (33.18 KB, patch)
2013-06-22 06:59 PDT, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat patch 3 round rendering v3.1 (10.82 KB, patch)
2013-06-27 03:20 PDT, Lazar Sumar [:nonsensickle]
bugzilla: review+
Details | Diff | Splinter Review
background-repeat patch 4 space rendering v2.3 (34.04 KB, patch)
2013-06-28 03:03 PDT, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat patch 2 parser changes v3.1 (4.86 KB, patch)
2014-04-06 01:56 PDT, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat patch 1 tests v9.1 (35.33 KB, patch)
2014-04-06 01:57 PDT, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
background-repeat patch 1 (39.37 KB, patch)
2016-05-06 02:09 PDT, Ethan Lin[:ethlin]
no flags Details | Diff | Splinter Review
Part1. parser changes (5.17 KB, patch)
2016-05-06 02:16 PDT, Ethan Lin[:ethlin]
cam: review+
Details | Diff | Splinter Review
Part2. Handle rendering of background-repeat: round (7.27 KB, patch)
2016-05-06 02:18 PDT, Ethan Lin[:ethlin]
no flags Details | Diff | Splinter Review
Part3. Handle rendering of background-repeat: space (31.72 KB, patch)
2016-05-06 02:20 PDT, Ethan Lin[:ethlin]
no flags Details | Diff | Splinter Review
Part2. Handle rendering of background-repeat: round/space (29.63 KB, patch)
2016-05-11 01:51 PDT, Ethan Lin[:ethlin]
no flags Details | Diff | Splinter Review
Part2. Handle rendering of background-repeat: round/space (29.72 KB, patch)
2016-05-12 02:21 PDT, Ethan Lin[:ethlin]
no flags Details | Diff | Splinter Review
Part3. Add test case (29.44 KB, patch)
2016-05-12 02:24 PDT, Ethan Lin[:ethlin]
cam: review+
Details | Diff | Splinter Review
Part2. Handle rendering of background-repeat: round/space (27.63 KB, patch)
2016-05-17 03:07 PDT, Ethan Lin[:ethlin]
mstange: review+
Details | Diff | Splinter Review
Part3. Add test case for round / space (carry r+: heycam) (52.38 KB, patch)
2016-05-17 03:11 PDT, Ethan Lin[:ethlin]
no flags Details | Diff | Splinter Review
Part2. Handle rendering of background-repeat: round/space (carry r+: mstange) (996.83 KB, patch)
2016-05-18 02:08 PDT, Ethan Lin[:ethlin]
no flags Details | Diff | Splinter Review
Part3. Add test case for round / space (carry r+: heycam) (200.37 KB, patch)
2016-05-18 02:12 PDT, Ethan Lin[:ethlin]
no flags Details | Diff | Splinter Review
Part2. Handle rendering of background-repeat: round/space (carry r+: mstange) (27.45 KB, patch)
2016-05-18 02:28 PDT, Ethan Lin[:ethlin]
no flags Details | Diff | Splinter Review
Part3. Add test case for round / space (carry r+: heycam) (43.03 KB, patch)
2016-05-18 02:31 PDT, Ethan Lin[:ethlin]
mstange: review+
Details | Diff | Splinter Review
testcase that shows that we need to fill the fill rect even to the top/left of the dest rect (2.29 KB, text/html)
2016-05-18 10:39 PDT, Markus Stange [:mstange]
no flags Details
Part2. Handle rendering of background-repeat: round/space (carry r+: mstange) (27.82 KB, patch)
2016-05-18 23:09 PDT, Ethan Lin[:ethlin]
no flags Details | Diff | Splinter Review
Part3. Add test case for round / space (carry r+: heycam, mstange) (46.46 KB, patch)
2016-05-18 23:16 PDT, Ethan Lin[:ethlin]
mstange: review+
Details | Diff | Splinter Review
Part2. Handle rendering of background-repeat: round/space (carry r+: mstange) (26.94 KB, patch)
2016-05-19 19:32 PDT, Ethan Lin[:ethlin]
no flags Details | Diff | Splinter Review
Par4. Add 'round, space' in shorthand property test (2.22 KB, patch)
2016-05-22 18:54 PDT, Ethan Lin[:ethlin]
cam: review+
Details | Diff | Splinter Review
Part2. Handle rendering of background-repeat: round/space (carry r+: mstange) (26.90 KB, patch)
2016-05-25 09:29 PDT, Ethan Lin[:ethlin]
no flags Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2010-02-24 11:50:29 PST
These are two new CSS3 values for background-repeat.  'space' roughly says compute the number of instances of the background image that can be drawn without clipping, then space them evenly in the relevant dimension.  'round' roughly says draw the background image at a size so that an exact integer number of instances of the background image fit.

(The spec language is far more precise than this, of course, and should be referred to for implementation, but this is the basic flavor.)

Note that background-size will also need to be updated to accommodate these values, because the two properties are tightly intertwined for these two background-repeat values.
Comment 1 Boris Zbarsky [:bz] 2010-08-22 20:30:44 PDT
*** Bug 589680 has been marked as a duplicate of this bug. ***
Comment 2 Boris Zbarsky [:bz] 2010-08-22 20:35:04 PDT
*** Bug 589687 has been marked as a duplicate of this bug. ***
Comment 3 Ryan Flint [:rflint] (ping via IRC for reviews) 2010-08-22 21:02:18 PDT
*** Bug 589679 has been marked as a duplicate of this bug. ***
Comment 4 philippe (part-time) 2010-08-23 05:16:35 PDT
*** Bug 589678 has been marked as a duplicate of this bug. ***
Comment 5 Boris Zbarsky [:bz] 2010-10-12 19:27:35 PDT
*** Bug 603852 has been marked as a duplicate of this bug. ***
Comment 6 Lazar Sumar [:nonsensickle] 2011-12-20 17:58:16 PST
Marc, are you working on this currently? If not, I would like to take it.
Comment 7 Lazar Sumar [:nonsensickle] 2012-01-12 19:33:44 PST
Created attachment 588289 [details] [diff] [review]
background-repeat patch 1 reftests

Adding tests for the new functionality.

David, would you like to check that what I have matches the spec on

dev.w3.org/csswg/css4-background/#the-background-repeat

I'm moving onto writing the actual patch.
Comment 8 Lazar Sumar [:nonsensickle] 2012-01-12 20:32:18 PST
Comment on attachment 588289 [details] [diff] [review]
background-repeat patch 1 reftests

Hey Rob, I'll try and cut down on the number of reviews I send flying to David. Thanks for volunteering :)
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-16 13:37:22 PST
Comment on attachment 588289 [details] [diff] [review]
background-repeat patch 1 reftests

Review of attachment 588289 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/backgrounds/background-repeat-round-1-ref.html
@@ +20,5 @@
> +If ‘background-repeat’ is ‘round’ for one dimension only and if ‘background-size’ 
> +is ‘auto’ for the other dimension, then there is a third step: that other 
> +dimension is scaled so that the original aspect ratio is restored.
> +
> +Lazar Sumar <lsumar@mozilla.com>, 13th Jan 2012, Bug 548372

I don't think we need all this. A URL reference will suffice. Same in the other tests.

A comment in each test saying what situation it's testing would be very helpful, however.
Comment 10 Lazar Sumar [:nonsensickle] 2012-01-16 15:42:52 PST
Created attachment 589053 [details] [diff] [review]
background-repeat patch 1 reftests v2

> I don't think we need all this. A URL reference will suffice. Same in the other 
> tests.

done.

> A comment in each test saying what situation it's testing would be very 
> helpful, however.

done.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-16 16:25:14 PST
Comment on attachment 589053 [details] [diff] [review]
background-repeat patch 1 reftests v2

Review of attachment 589053 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Lose the "Lazar Sumar <lsumar@mozilla.com>, 13th Jan 2012, Bug 548372" lines. This information belongs in hg.
Comment 12 Lazar Sumar [:nonsensickle] 2012-01-16 17:14:45 PST
Created attachment 589071 [details] [diff] [review]
background-repeat patch 1 reftests v3

> Lose the "Lazar Sumar <lsumar@mozilla.com>, 13th Jan 2012, Bug 548372" lines.

done.
Comment 13 Lazar Sumar [:nonsensickle] 2012-01-16 18:27:35 PST
Created attachment 589082 [details] [diff] [review]
background-repeat patch 1 reftests v4

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37 in bug 548375)
> Don't repeat the contents of these rules. Share the width/height/image 
> properties in their own rule body > div > div or something like that. same for 
> the other test.

(In reply to Lazar Sumar [:lsumar] from comment #38 in bug 548375)

> Done. And were you referring to the tests for bug 548372 as well?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39 in bug 548375)

> Yes, actually.

done.
Comment 14 Lazar Sumar [:nonsensickle] 2012-01-16 23:23:25 PST
Added dependence on bug 548375 because it will be easier to make some changes on top of the patch I already did there.
Comment 15 Lazar Sumar [:nonsensickle] 2012-01-18 20:51:39 PST
Created attachment 589769 [details] [diff] [review]
background-repeat parser changes patch v1

These are only the necessary parser changes. 

I'm still working on the rendering changes and will put them in a separate patch for roc to review.

Jeff, could you please review these in the meantime?
Comment 16 Lazar Sumar [:nonsensickle] 2012-01-18 20:52:48 PST
Comment on attachment 589769 [details] [diff] [review]
background-repeat parser changes patch v1

It seems I have to keep the +bmo in order to ask for review...
Comment 17 Lazar Sumar [:nonsensickle] 2012-01-19 16:48:50 PST
Behavior of background-repeat with background-size cover and contain. 
IRC discussion transcript:

 <lsumar> hi David
 <lsumar> I've got a question about the spec for round/space in background-repeat
 <lsumar> what happens if you define background-size: cover; background-repeat: space?
 <lsumar> and the image is 32x32 on an 80x80 element
 <dbaron> I would think background-size processing happens first
 <dbaron> so if you use background-size: cover then background-repeat doesn't have anything interesting to do
 <dbaron> oh, except background-repeat: round influences background-size computation
 <dbaron> I guess it's not clear to me what background-repeat: space does when there isn't enough room for one image
 <dbaron> Oh, I guess it is clear... it just follows the rule for "less than enough room for 2 images to fit"
 <dbaron> so in the particular case you describe you'd just end up with one 80x80 scaled tile
 <lsumar> k, so I will assume that the background-size processing happens first in the case of background-repeat: space.
 <lsumar> thanks :)

Computation of background-size will come first and computation for background-repeat will follow.
Comment 18 Lazar Sumar [:nonsensickle] 2012-01-19 23:08:45 PST
Created attachment 590120 [details] [diff] [review]
background-repeat patch 1 reftests v5

Math error. Rounding up...
Comment 19 Lazar Sumar [:nonsensickle] 2012-01-19 23:10:53 PST
Created attachment 590121 [details] [diff] [review]
background-repeat patch 1 reftests v5

Wrong file. This is v5.
Comment 20 Lazar Sumar [:nonsensickle] 2012-01-19 23:16:38 PST
Created attachment 590124 [details] [diff] [review]
background-repeat round rendering patch v1

Proposed implementation of 'round' keyword rendering. 
Passes local reftests for the round keyword. Fails reftests for space keyword.
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-22 13:46:26 PST
Comment on attachment 590124 [details] [diff] [review]
background-repeat round rendering patch v1

Review of attachment 590124 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great!

::: layout/base/nsCSSRendering.cpp
@@ +2425,5 @@
>  
>    // We can skip painting the background color if a background image is opaque.
>    if (drawBackgroundColor &&
> +      bg->BottomLayer().mRepeat.mXRepeat & (NS_STYLE_BG_REPEAT_REPEAT | 
> +                                            NS_STYLE_BG_REPEAT_ROUND) &&

Put parens around the &-expression to make sure the precedence is right.

@@ +4166,5 @@
> + * http://dev.w3.org/csswg/css3-background/#the-background-size
> + */
> +static nscoord
> +ComputeBackgroundRepeadRound(const nscoord& currentSize, 
> +                             const nscoord& positioningSize) {

Just pass nscoord values

@@ +4168,5 @@
> +static nscoord
> +ComputeBackgroundRepeadRound(const nscoord& currentSize, 
> +                             const nscoord& positioningSize) {
> +  float X = currentSize;
> +  float W = positioningSize;

lowercase

@@ +4170,5 @@
> +                             const nscoord& positioningSize) {
> +  float X = currentSize;
> +  float W = positioningSize;
> +  
> +  float a = NS_roundf(W/X);

Add an assertion that currentSize must be greater than zero
Comment 22 Lazar Sumar [:nonsensickle] 2012-01-22 15:06:44 PST
Created attachment 590594 [details] [diff] [review]
background-repeat round rendering patch v2

(In response to review by Robert O'Callahan (:roc) (Mozilla Corporation) in comment #21)

> Put parens around the &-expression to make sure the precedence is right.

Done. In the future, should I explicitly state precedence for readability? In this case the C++ operator precedence would result in the intended logic.
  ref 1 - http://en.cppreference.com/w/cpp/language/operator_precedence
  ref 2 - http://en.wikipedia.org/wiki/Operators_in_C_and_C%2B%2B#Operator_precedence

> Just pass nscoord values

Done.

> lowercase

Done.

> Add an assertion that currentSize must be greater than zero.

Done.
Comment 23 Lazar Sumar [:nonsensickle] 2012-01-22 15:20:59 PST
Created attachment 590597 [details] [diff] [review]
background-repeat round rendering patch v2.1

It's better style not to evaluate floats (even if zero) for assertions.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-22 15:29:49 PST
(In reply to Lazar Sumar [:lsumar] from comment #22)
> Done. In the future, should I explicitly state precedence for readability?

There are some "non-obvious" precedences that we want parents for. Any mix of && and || for example, and anything involving & and |.
Comment 25 Lazar Sumar [:nonsensickle] 2012-01-22 15:35:35 PST
Created attachment 590598 [details] [diff] [review]
background-repeat patch 1 reftests v6

Added a few more reftests for background-repeat: round. In order for the round keyword to be fully specified by reftests one more needs to be added to define its interaction with the background-origin and background-clip properties.

Note: The way the background-repeat: round is rendered when the 
      background-position property is not set to "left top" or "0 0" is not 
      very clear in the spec. For consistency with background-size: cover and
      contain the background-position has been allowed to have an effect. I 
      have raised a question on the www-style mailing list and am waiting to 
      get it accepted.
Comment 26 Lazar Sumar [:nonsensickle] 2012-01-23 19:17:50 PST
The specification for background-repeat space states two things which will make the implementation have an ugly special case. The statements are:

  1. "The image is repeated as often as will fit ... without being clipped"
  2. "The first and last images touch the edges of the area"

The following case will be ambiguous as to the way it is implemented and has potential to be differently implemented in different browsers.

Example case (I will add this to the reftests):

width: 105px;
height: 105px;
background-size: 32px 32px;
background-repeat: space;

Three images can fit in the positioning area and the spacing between them is 4.5px which means that one has to be 4px and the other 5px. Which one?

I will implement an even spacing between all images with the last image's spacing accounting for the discrepancy. Exact details will be explained when the patch is ready.
Comment 27 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-23 19:24:20 PST
(In reply to Lazar Sumar [:lsumar] from comment #26)
> I will implement an even spacing between all images with the last image's
> spacing accounting for the discrepancy. Exact details will be explained when
> the patch is ready.

That sounds good. I don't think this matters too much. Browsers already differ in how they round in some situations and mostly it doesn't affect Web authors.
Comment 28 Lazar Sumar [:nonsensickle] 2012-01-23 20:32:02 PST
Created attachment 590988 [details] [diff] [review]
background-repeat patch 1 reftests v7

Added tests for space special case. Tests still don't fully define behavior.

Missing tests: use background-clip, background-origin to test for repetition of pattern beyond background positioning area and into the background clip area.
Comment 29 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-25 14:21:54 PST
Comment on attachment 589769 [details] [diff] [review]
background-repeat parser changes patch v1

Review of attachment 589769 [details] [diff] [review]:
-----------------------------------------------------------------

This isn't really an area where I generally do reviews, so it's a little bit of a stretch for me.  But not much at all, happy to do it if needed.  I think technically I don't have review powers here, but I assume someone suggested me to review this to take load off someone with more complex reviews?

The context in the nsRuleNode.cpp in the patch doesn't correspond to the current state of that file in m-i, so I don't know whether those changes make sense or not.  Could you post an updated version of the patch which corresponds to the current tree, and/or point out to me the prerequisite patches to this one?

::: layout/base/nsStyleConsts.h
@@ +298,5 @@
>  #define NS_STYLE_BG_REPEAT_REPEAT_X                 0x01
>  #define NS_STYLE_BG_REPEAT_REPEAT_Y                 0x02
>  #define NS_STYLE_BG_REPEAT_REPEAT                   0x03
> +#define NS_STYLE_BG_REPEAT_SPACE                    0x04
> +#define NS_STYLE_BG_REPEAT_ROUND                    0x08

So, no-repeat (no-repeat no-repeat) is a bitfield.  repeat-x (repeat no-repeat) is also a bitfield.  repeat-y (no-repeat repeat) is a bitfield.  repeat (repeat repeat) is a bitfield.  And space/round are...what?  From the other patch it looks like background-repeat is a keyword pair, in which case having space/round not be bitfieldy might not matter.  But it would seem to me that things shouldn't be inconsistent, or misleading, on the point.

Why not have repeat/space/round/no-repeat as the components of each keyword pair, with no misleading bitfield-ness at all to it?  (Serialization code would have to recognize the cases where a pair decays to a single keyword, but that doesn't seem difficult.  We have to do similar things in other places when serializing already.)

::: layout/style/nsRuleNode.cpp
@@ +4838,5 @@
> +      NS_ASSERTION(value == NS_STYLE_BG_REPEAT_NO_REPEAT || 
> +                   value == NS_STYLE_BG_REPEAT_REPEAT || 
> +                   value == NS_STYLE_BG_REPEAT_SPACE || 
> +                   value == NS_STYLE_BG_REPEAT_ROUND,
> +                   "Unexpected value");

This explanation string doesn't add anything to the assertion, just restates it.  Use MOZ_ASSERT instead so that you can remove it.

@@ +4863,5 @@
> +      NS_ASSERTION(value == NS_STYLE_BG_REPEAT_NO_REPEAT || 
> +                   value == NS_STYLE_BG_REPEAT_REPEAT || 
> +                   value == NS_STYLE_BG_REPEAT_SPACE || 
> +                   value == NS_STYLE_BG_REPEAT_ROUND,
> +                   "Unexpected value");

Same here.
Comment 30 Lazar Sumar [:nonsensickle] 2012-01-25 15:52:24 PST
(In reply to Jeff Walden (remove +bmo to email) from comment #29)

> This isn't really an area where I generally do reviews, so it's a little bit
> of a stretch for me.  But not much at all, happy to do it if needed.  I
> think technically I don't have review powers here, but I assume someone
> suggested me to review this to take load off someone with more complex
> reviews?

It was my own initiative to not bother dbaron and bz too much with reviews. 
I've so far asked Zack (on separate bugs) and yourself for feedback. Thanks.

> The context in the nsRuleNode.cpp in the patch doesn't correspond to the
> current state of that file in m-i, so I don't know whether those changes
> make sense or not.  Could you post an updated version of the patch which
> corresponds to the current tree, and/or point out to me the prerequisite
> patches to this one?

Yes there is. I thought I mentioned it in the comment #14 and marked a
dependence on the bug 548375 in comment #13. It is probably not ideal for 
you to have to look through those... Can you recommend a good place to put
a link to the patches that precede this one?

The patches this bug depends on are in the bug 548375. They are applied in
the following sequence:

background-repeat patch 1 tests v4           (bug 548375)
background-repeat patch 1 implementation v6  (bug 548375)
background-repeat patch 1 reftests v7        (this bug 548372)
background-repeat parser changes patch v1    (this bug 548372)
background-repeat round rendering patch v2.1 (this bug 548372)

I will post a new patch addressing your comments, after addressing the feedback I got on bug 548375.

> This explanation string doesn't add anything to the assertion, just restates
> it.  Use MOZ_ASSERT instead so that you can remove it.

I didn't know about MOZ_ASSERT. Noted.
Comment 31 Lazar Sumar [:nonsensickle] 2012-02-16 19:40:18 PST
Created attachment 598106 [details] [diff] [review]
background-repeat space rendering patch (incomplete)

This is a proposed implementation of the spacing keyword. 
Currently the background-position is not treated correctly and I'm working on fixing that but the rest of the functionality is the way I want it.

The spacing of the images is done by rendering the image to an intermediate surface in CSS pixel space and then tiling.

At certain zoom levels this method leaves some unwanted artifacts in the very last pixel touching the boundary due to the sampling of neighborhood pixels. I will try to fix it in the next patch using an anchor point in the intermediate surface which corresponds to the bottom right corner of the image.

There is an edge case where the number of pixels available for spacing is a non-integer value. This case is handled by splitting the image into 4 quadrants with different spacing. Top left quadrant is spaced with the ceiling value while the bottom left quadrant is spaced with the floored value. The top right and bottom right quadrants space images with a mix of floor and ceil values which make them line up with the top left and bottom right quadrants.

Roc, how does it look?
Comment 32 Lazar Sumar [:nonsensickle] 2012-02-19 12:05:00 PST
Created attachment 598685 [details] [diff] [review]
background-repeat space rendering patch v1

The reftests need to be updated before a push to the try server. Masking needs to be done in the reftest 4.

The artifact which is present during zooming, referred to in comment #31, still isn't fixed.

There is a correction to the wording in the last paragraph of comment #31.

> ...Top left quadrant is spaced with the ceiling value while the bottom left 
> quadrant is spaced with the floored value. The top right and bottom right 
> quadrants space images with a mix of floor and ceil values which make them 
> line up with the top left and bottom right quadrants.

This should read:

Top left quadrant is spaced with the ceiling value while the bottom RIGHT quadrant is spaced with the floored value. The top right and bottom LEFT quadrants space images with a mix of floor and ceil values which make them line up with the top left and bottom right quadrants.
Comment 33 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-19 14:06:13 PST
Comment on attachment 598685 [details] [diff] [review]
background-repeat space rendering patch v1

Review of attachment 598685 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.h
@@ +1111,5 @@
>     *                            occurs, or it might be the image's size if
>     *                            the image is a vector image being rendered at
>     *                            that size.)
> +   *   @param aSpacingPixels    The total number of pixels that are available 
> +   *                            for use as spacing between the images.

If this is a whole number of pixels, it should be an nsIntSize. And you should say whether it's device pixels or CSS pixels. And you should describe how this affects rendering.

@@ +1114,5 @@
> +   *   @param aSpacingPixels    The total number of pixels that are available 
> +   *                            for use as spacing between the images.
> +   *   @param aImageCount       This represents the number of images that can
> +   *                            fit into the positioning area without being 
> +   *                            clipped.

Should definitely be nsIntSize too I think. But why do we need this here? Can't the caller just adjust aDest and aFill to handle 'round'?

Move these parameter descriptions down after aFill where they belong.
Comment 34 Lazar Sumar [:nonsensickle] 2012-02-19 16:07:46 PST
Created attachment 598728 [details] [diff] [review]
background-repeat space rendering patch v1

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #33)
> Comment on attachment 598685 [details] [diff] [review]
> background-repeat space rendering patch v1
> 
> Review of attachment 598685 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsLayoutUtils.h
> @@ +1111,5 @@
> >     *                            occurs, or it might be the image's size if
> >     *                            the image is a vector image being rendered at
> >     *                            that size.)
> > +   *   @param aSpacingPixels    The total number of pixels that are available 
> > +   *                            for use as spacing between the images.
> 
> If this is a whole number of pixels, it should be an nsIntSize. And you
> should say whether it's device pixels or CSS pixels. And you should describe
> how this affects rendering.

This is in app units and that is why I've used nsSize. It seems to be the convention.

> @@ +1114,5 @@
> > +   *   @param aSpacingPixels    The total number of pixels that are available 
> > +   *                            for use as spacing between the images.
> > +   *   @param aImageCount       This represents the number of images that can
> > +   *                            fit into the positioning area without being 
> > +   *                            clipped.
> 
> Should definitely be nsIntSize too I think. But why do we need this here?
> Can't the caller just adjust aDest and aFill to handle 'round'?

aImageCount was changed to nsIntSize.

This is to handle the 'space' keyword. It is necessary because the fill area does not represent the area in which the calculation is being done. It is to do with the way background-origin and background-clip properties affect backgrounds.

> Move these parameter descriptions down after aFill where they belong.

Done.
Comment 35 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-19 18:52:11 PST
Comment on attachment 598728 [details] [diff] [review]
background-repeat space rendering patch v1

Review of attachment 598728 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.h
@@ +1116,5 @@
>     *   @param aFill             The area to be filled with copies of the
>     *                            image.
> +   *   @param aSpacingPixels    The total number of pixels that are available 
> +   *                            for use as spacing between the images in app
> +   *                            units.

Let's not call it pixels, since it's appunits :-). So aSpacing, and "the total number of appunits to use as spacing between the images".

It's still not clear where the spacing goes in the rendering. Is the idea that we draw an image at aDest and then repeat it aImageCount.width times to the right and aImageCount.height times down, with aSpacing total horizontal and vertical space between the repetitions?
Comment 36 Lazar Sumar [:nonsensickle] 2012-02-19 19:49:04 PST
Created attachment 598748 [details] [diff] [review]
background-repeat space rendering patch v1.3

> Let's not call it pixels, since it's appunits :-). So aSpacing, and "the total 
> number of appunits to use as spacing between the images".

Done.

> It's still not clear where the spacing goes in the rendering. 

That is the number of pixels that are 'left-over' after the width of all the images that can fit is summed up.

> Is the idea that we draw an image at aDest and then repeat it 
> aImageCount.width times to the right and aImageCount.height times down, 
> with aSpacing total horizontal and vertical space between the repetitions?

Yes.
Comment 37 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-19 19:58:13 PST
(In reply to Lazar Sumar [:lsumar] from comment #36)
> > Is the idea that we draw an image at aDest and then repeat it 
> > aImageCount.width times to the right and aImageCount.height times down, 
> > with aSpacing total horizontal and vertical space between the repetitions?
> 
> Yes.

This needs to be made clear in the documentation for DrawBackgroundImage.
Comment 38 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-19 20:26:45 PST
Comment on attachment 598748 [details] [diff] [review]
background-repeat space rendering patch v1.3

Review of attachment 598748 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +159,5 @@
>    static bool s3DTransformPrefCached = false;
>  
>    if (!s3DTransformPrefCached) {
>      s3DTransformPrefCached = true;
> +    mozilla::Preferences::AddBoolVarCache(&s3DTransformsEnabled,

This hunk (and several more below) don't seem to change anything. Just remove them from the patch.

@@ +3606,5 @@
> +
> +static IntermediateSurfaceParameters
> +ComputeIntermediateSurfaceParameters(const nsRect&     aDest,
> +                                     const nsRect&     aFill,
> +                                     const nsMargin&   aPadding)

Need to document what this does.

@@ +3838,5 @@
> +static void
> +ComputeIndividualImagePadding(const nsSize&    aSpacing,
> +                              const nsIntSize& aImageCount,
> +                              nsIntSize&       aOutPadding,
> +                              nsIntSize&       aOutRemainder)

This could just work on one axis and then you could call it twice, once for x and once for y.

Shouldn't aOutRemainder be in appunits?

@@ +3846,5 @@
> +    spacingX = nsPresContext::AppUnitsToFloatCSSPixels(aSpacing.width);
> +    spacingX /= aImageCount.width-1;
> +    aOutPadding.width = floorf(spacingX);
> +    float remX = spacingX - floorf(spacingX);
> +    aOutRemainder.width = NS_round(remX * (aImageCount.width - 1));

Probably a bit more accurate and efficient to set aOutRemainder.width to aSpacing.width - aOutPadding.width*(aImageCount.width - 1)
Comment 39 Lazar Sumar [:nonsensickle] 2012-02-21 13:57:38 PST
Created attachment 599344 [details] [diff] [review]
background-repeat space rendering patch v1.4

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #38)
> ::: layout/base/nsLayoutUtils.cpp
> @@ +159,5 @@
> >    static bool s3DTransformPrefCached = false;
> >  
> >    if (!s3DTransformPrefCached) {
> >      s3DTransformPrefCached = true;
> > +    mozilla::Preferences::AddBoolVarCache(&s3DTransformsEnabled,
> 
> This hunk (and several more below) don't seem to change anything. Just
> remove them from the patch.

Whoops, auto trailing space trimming -> off. Removed.

> @@ +3606,5 @@
> > +
> > +static IntermediateSurfaceParameters
> > +ComputeIntermediateSurfaceParameters(const nsRect&     aDest,
> > +                                     const nsRect&     aFill,
> > +                                     const nsMargin&   aPadding)
> 
> Need to document what this does.

Done.

> @@ +3838,5 @@
> > +static void
> > +ComputeIndividualImagePadding(const nsSize&    aSpacing,
> > +                              const nsIntSize& aImageCount,
> > +                              nsIntSize&       aOutPadding,
> > +                              nsIntSize&       aOutRemainder)
> 
> This could just work on one axis and then you could call it twice, once for
> x and once for y.

Done.

> Shouldn't aOutRemainder be in appunits?
> 
> @@ +3846,5 @@
> > +    spacingX = nsPresContext::AppUnitsToFloatCSSPixels(aSpacing.width);
> > +    spacingX /= aImageCount.width-1;
> > +    aOutPadding.width = floorf(spacingX);
> > +    float remX = spacingX - floorf(spacingX);
> > +    aOutRemainder.width = NS_round(remX * (aImageCount.width - 1));
> 
> Probably a bit more accurate and efficient to set aOutRemainder.width to
> aSpacing.width - aOutPadding.width*(aImageCount.width - 1)

Done.

--------------------------------------------

I've also refactored some stuff we talked about in person. The ComputeImageRenderingQuadrants function has been redone in a less clear but more efficient way. I'll add a comment to it in the next iteration...

A lot has still been left uncommented.

Also, please note Bug 600207 because that issue is present in this bug as well. 
Reason:
The SVG images are drawn to an intermediate surface in image space and this causes pixelation to occur where it shouldn't...
Fix:
Create the intermediate surface in device space.

The gfxUtils::DrawPixelSnapped function can be used instead of the gfxSurfaceDrawable::Draw function in this patch, so having a fix in Bug 600207 could help simplify the fix for this bug.
Comment 40 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-21 15:00:27 PST
Comment on attachment 599344 [details] [diff] [review]
background-repeat space rendering patch v1.4

Review of attachment 599344 [details] [diff] [review]:
-----------------------------------------------------------------

Can you add documentation to the rest of the places where you know you need it, before I review this again? :-)

::: layout/base/nsLayoutUtils.cpp
@@ +3856,5 @@
>  
> +/* ComputeIndividualImagePadding is a helper function that computes the number
> + * of pixels that each image is padded with, in image space, in order to fill
> + * gaps between aImageCount images evenly. aSpacing represents the total number
> + * of app units that are available to be used as spacing.

Document aOutPadding and aOutRemainder.

@@ +3867,5 @@
> +{
> +  if (aImageCount >= 2) {
> +    float spacing = 0.0f;
> +    spacing = nsPresContext::AppUnitsToFloatCSSPixels(aSpacing);
> +    spacing /= aImageCount-1;

spaces around '-' (everywhere)

@@ +3870,5 @@
> +    spacing = nsPresContext::AppUnitsToFloatCSSPixels(aSpacing);
> +    spacing /= aImageCount-1;
> +    aOutPadding = floorf(spacing);
> +    aOutRemainder = aSpacing-nsPresContext::CSSPixelsToAppUnits(aOutPadding)*
> +                    (aImageCount-1);

Break lines near lower-precedence operators, so after '-'

::: layout/base/nsLayoutUtils.h
@@ +1126,5 @@
> +   *                            and y (height) directions. These will be divided
> +   *                            evenly among all the gaps that appear when
> +   *                            drawing aImageCount images.
> +   *   @param aImageCount       Only effects rendering if sufficient aSpacing is
> +   *                            provided. Represents the number of images that

I'm not sure what this means. If aSpacing was zero, aImageCount could still affect rendering by limiting the number of tiles that are shown. Right?
Comment 41 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-02-21 20:40:30 PST
Comment on attachment 589769 [details] [diff] [review]
background-repeat parser changes patch v1

># HG changeset patch
># Date 1326942871 -46800
># User Lazar Sumar  <lsumar@mozilla.com>
># Parent 2045f6c8033e791f326b81ac78ad25fe3665033a
>Bug 548372 - Implemented background-repeat round and space

"Implemented" -> "Implement"

"background-repeat round and space" -> "round and space values of background-repeat CSS property"

>diff --git a/layout/base/nsStyleConsts.h b/layout/base/nsStyleConsts.h
>--- a/layout/base/nsStyleConsts.h
>+++ b/layout/base/nsStyleConsts.h
>@@ -293,16 +293,18 @@ static inline mozilla::css::Side operato
> #define NS_STYLE_BG_POSITION_LEFT    (1<<3)
> #define NS_STYLE_BG_POSITION_RIGHT   (1<<4)
> 
> // See nsStyleBackground
> #define NS_STYLE_BG_REPEAT_NO_REPEAT                0x00
> #define NS_STYLE_BG_REPEAT_REPEAT_X                 0x01
> #define NS_STYLE_BG_REPEAT_REPEAT_Y                 0x02
> #define NS_STYLE_BG_REPEAT_REPEAT                   0x03
>+#define NS_STYLE_BG_REPEAT_SPACE                    0x04
>+#define NS_STYLE_BG_REPEAT_ROUND                    0x08

I don't see any reason this needs to be bitmask-like anymore.  I'd say just use 4 and 5.

>diff --git a/layout/style/nsCSSProps.cpp b/layout/style/nsCSSProps.cpp
>--- a/layout/style/nsCSSProps.cpp
>+++ b/layout/style/nsCSSProps.cpp
>@@ -626,16 +626,18 @@ const PRInt32 nsCSSProps::kBackgroundPos
>   eCSSKeyword_UNKNOWN,-1
> };
> 
> const PRInt32 nsCSSProps::kBackgroundRepeatKTable[] = {
>   eCSSKeyword_no_repeat,  NS_STYLE_BG_REPEAT_NO_REPEAT,
>   eCSSKeyword_repeat,     NS_STYLE_BG_REPEAT_REPEAT,
>   eCSSKeyword_repeat_x,   NS_STYLE_BG_REPEAT_REPEAT_X,
>   eCSSKeyword_repeat_y,   NS_STYLE_BG_REPEAT_REPEAT_Y,
>+  eCSSKeyword_round,      NS_STYLE_BG_REPEAT_ROUND,
>+  eCSSKeyword_space,      NS_STYLE_BG_REPEAT_SPACE,
>   eCSSKeyword_UNKNOWN,-1
> };

Once you address my comments on bug 548375, you'll need this change in 2 places.

>     case eCSSUnit_Enumerated:
>       value = aSpecifiedValue->mYValue.GetIntValue();
>-      switch (value) {
>-        case NS_STYLE_BG_REPEAT_NO_REPEAT:
>-        case NS_STYLE_BG_REPEAT_REPEAT:
>-          aComputedValue.mYRepeat = value;
>-          break;
>-        default:
>-          NS_NOTREACHED("Unexpected value");
>-          break;
>-      }
>+      
>+      NS_ASSERTION(value == NS_STYLE_BG_REPEAT_NO_REPEAT || 
>+                   value == NS_STYLE_BG_REPEAT_REPEAT || 
>+                   value == NS_STYLE_BG_REPEAT_SPACE || 
>+                   value == NS_STYLE_BG_REPEAT_ROUND,
>+                   "Unexpected value");
>+      
>+      aComputedValue.mYRepeat = value;

I asked for pretty much this structural change in my review of bug 548375, so if you do that there this will be less of a change :-)

r=dbaron with those changes and also additions of some tests to layout/style/test/property_database.js
Comment 42 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-02-21 20:42:46 PST
Comment on attachment 590988 [details] [diff] [review]
background-repeat patch 1 reftests v7

># HG changeset patch
># Date 1327379230 -46800
># User Lazar Sumar  <lsumar@mozilla.com>
># Parent a4244164c90bd69cfd3340660301302f58bbffd7
>Bug 548372 - Added reftests for background-repeat round and space

Oh, wait, this has property_database.js changes.  You should change the summary of the patch from "reftests" -> "tests".  Also both of the same changes I suggested for the commit message of the previous patch.

>+spec at: dev.w3.org/csswg/css4-background/#the-background-repeat

Cite css3-background, not css4-background.  And put http:// on the front.

r=dbaron with that, though I didn't go through the tests closely
Comment 43 Lazar Sumar [:nonsensickle] 2012-02-23 13:15:44 PST
Created attachment 600151 [details] [diff] [review]
background-repeat parser changes patch v2

(In reply to David Baron [:dbaron] from comment #41)
> Comment on attachment 589769 [details] [diff] [review]
> background-repeat parser changes patch v1
> 
> ># HG changeset patch
> ># Date 1326942871 -46800
> ># User Lazar Sumar  <lsumar@mozilla.com>
> ># Parent 2045f6c8033e791f326b81ac78ad25fe3665033a
> >Bug 548372 - Implemented background-repeat round and space
> 
> "Implemented" -> "Implement"

Corrected.

> "background-repeat round and space" -> "round and space values of
> background-repeat CSS property"

Corrected.

> >diff --git a/layout/base/nsStyleConsts.h b/layout/base/nsStyleConsts.h
> >--- a/layout/base/nsStyleConsts.h
> >+++ b/layout/base/nsStyleConsts.h
> >@@ -293,16 +293,18 @@ static inline mozilla::css::Side operato
> > #define NS_STYLE_BG_POSITION_LEFT    (1<<3)
> > #define NS_STYLE_BG_POSITION_RIGHT   (1<<4)
> > 
> > // See nsStyleBackground
> > #define NS_STYLE_BG_REPEAT_NO_REPEAT                0x00
> > #define NS_STYLE_BG_REPEAT_REPEAT_X                 0x01
> > #define NS_STYLE_BG_REPEAT_REPEAT_Y                 0x02
> > #define NS_STYLE_BG_REPEAT_REPEAT                   0x03
> >+#define NS_STYLE_BG_REPEAT_SPACE                    0x04
> >+#define NS_STYLE_BG_REPEAT_ROUND                    0x08
> 
> I don't see any reason this needs to be bitmask-like anymore.  I'd say just
> use 4 and 5.

Done.

> >diff --git a/layout/style/nsCSSProps.cpp b/layout/style/nsCSSProps.cpp
> >--- a/layout/style/nsCSSProps.cpp
> >+++ b/layout/style/nsCSSProps.cpp
> >@@ -626,16 +626,18 @@ const PRInt32 nsCSSProps::kBackgroundPos
> >   eCSSKeyword_UNKNOWN,-1
> > };
> > 
> > const PRInt32 nsCSSProps::kBackgroundRepeatKTable[] = {
> >   eCSSKeyword_no_repeat,  NS_STYLE_BG_REPEAT_NO_REPEAT,
> >   eCSSKeyword_repeat,     NS_STYLE_BG_REPEAT_REPEAT,
> >   eCSSKeyword_repeat_x,   NS_STYLE_BG_REPEAT_REPEAT_X,
> >   eCSSKeyword_repeat_y,   NS_STYLE_BG_REPEAT_REPEAT_Y,
> >+  eCSSKeyword_round,      NS_STYLE_BG_REPEAT_ROUND,
> >+  eCSSKeyword_space,      NS_STYLE_BG_REPEAT_SPACE,
> >   eCSSKeyword_UNKNOWN,-1
> > };
> 
> Once you address my comments on bug 548375, you'll need this change in 2
> places.

Both are done.

> >     case eCSSUnit_Enumerated:
> >       value = aSpecifiedValue->mYValue.GetIntValue();
> >-      switch (value) {
> >-        case NS_STYLE_BG_REPEAT_NO_REPEAT:
> >-        case NS_STYLE_BG_REPEAT_REPEAT:
> >-          aComputedValue.mYRepeat = value;
> >-          break;
> >-        default:
> >-          NS_NOTREACHED("Unexpected value");
> >-          break;
> >-      }
> >+      
> >+      NS_ASSERTION(value == NS_STYLE_BG_REPEAT_NO_REPEAT || 
> >+                   value == NS_STYLE_BG_REPEAT_REPEAT || 
> >+                   value == NS_STYLE_BG_REPEAT_SPACE || 
> >+                   value == NS_STYLE_BG_REPEAT_ROUND,
> >+                   "Unexpected value");
> >+      
> >+      aComputedValue.mYRepeat = value;
> 
> I asked for pretty much this structural change in my review of bug 548375,
> so if you do that there this will be less of a change :-)

Done.

> r=dbaron with those changes and also additions of some tests to
> layout/style/test/property_database.js

They have been included in the patch 590988: background-repeat patch 1 reftests v7, though as you point out in comment #42, they have been incorrectly named.
Comment 44 Lazar Sumar [:nonsensickle] 2012-02-23 13:18:38 PST
Created attachment 600154 [details] [diff] [review]
background-repeat patch 1 tests v8

(In reply to David Baron [:dbaron] from comment #42)
> Comment on attachment 590988 [details] [diff] [review]
> background-repeat patch 1 reftests v7
> 
> ># HG changeset patch
> ># Date 1327379230 -46800
> ># User Lazar Sumar  <lsumar@mozilla.com>
> ># Parent a4244164c90bd69cfd3340660301302f58bbffd7
> >Bug 548372 - Added reftests for background-repeat round and space
> 
> Oh, wait, this has property_database.js changes.  You should change the
> summary of the patch from "reftests" -> "tests".  Also both of the same
> changes I suggested for the commit message of the previous patch.

Done.

> >+spec at: dev.w3.org/csswg/css4-background/#the-background-repeat
> 
> Cite css3-background, not css4-background.  And put http:// on the front.

Done.

> r=dbaron with that, though I didn't go through the tests closely

Done.
Comment 45 Lazar Sumar [:nonsensickle] 2012-02-23 13:20:35 PST
Created attachment 600155 [details] [diff] [review]
background-repeat patch 1 tests v8

(In reply to Lazar Sumar [:lsumar] from comment #44)
> Created attachment 600154 [details] [diff] [review]
> background-repeat patch 1 tests v8

Wrong patch attached.

Corrected.
Comment 46 Lazar Sumar [:nonsensickle] 2012-02-23 14:51:12 PST
Created attachment 600188 [details] [diff] [review]
background-repeat parser changes patch v2
Comment 47 Lazar Sumar [:nonsensickle] 2012-02-23 16:00:16 PST
Created attachment 600218 [details] [diff] [review]
background-repeat space rendering patch v1.5

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #40)
> Comment on attachment 599344 [details] [diff] [review]
> background-repeat space rendering patch v1.4
> 
> Review of attachment 599344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you add documentation to the rest of the places where you know you need
> it, before I review this again? :-)

Done.

> ::: layout/base/nsLayoutUtils.cpp
> @@ +3856,5 @@
> >  
> > +/* ComputeIndividualImagePadding is a helper function that computes the number
> > + * of pixels that each image is padded with, in image space, in order to fill
> > + * gaps between aImageCount images evenly. aSpacing represents the total number
> > + * of app units that are available to be used as spacing.
> 
> Document aOutPadding and aOutRemainder.

Done.

> @@ +3867,5 @@
> > +{
> > +  if (aImageCount >= 2) {
> > +    float spacing = 0.0f;
> > +    spacing = nsPresContext::AppUnitsToFloatCSSPixels(aSpacing);
> > +    spacing /= aImageCount-1;
> 
> spaces around '-' (everywhere)

Done. As many as I could find.

> @@ +3870,5 @@
> > +    spacing = nsPresContext::AppUnitsToFloatCSSPixels(aSpacing);
> > +    spacing /= aImageCount-1;
> > +    aOutPadding = floorf(spacing);
> > +    aOutRemainder = aSpacing-nsPresContext::CSSPixelsToAppUnits(aOutPadding)*
> > +                    (aImageCount-1);
> 
> Break lines near lower-precedence operators, so after '-'

I changed a few things so this is nicer now.

> ::: layout/base/nsLayoutUtils.h
> @@ +1126,5 @@
> > +   *                            and y (height) directions. These will be divided
> > +   *                            evenly among all the gaps that appear when
> > +   *                            drawing aImageCount images.
> > +   *   @param aImageCount       Only effects rendering if sufficient aSpacing is
> > +   *                            provided. Represents the number of images that
> 
> I'm not sure what this means. If aSpacing was zero, aImageCount could still
> affect rendering by limiting the number of tiles that are shown. Right?

The problem was the name. aImageCount was only ever used to determine the number of gaps and has been renamed to better describe its purpose. Sorry for the confusion.

I believe that I have added comments to every relevant function and structure I've added so I will be marking r?(roc).
Comment 48 Lazar Sumar [:nonsensickle] 2012-02-23 16:13:11 PST
Comment on attachment 600218 [details] [diff] [review]
background-repeat space rendering patch v1.5

(In reply to Lazar Sumar [:lsumar] from comment #47)
> Created attachment 600218 [details] [diff] [review]
> background-repeat space rendering patch v1.5

An additional reminder that the bug 600207 problem will be duplicated here too because I still haven't moved the temporary surface from image space to device space.

I'm hence canceling the review and will come back when I fix both. Still, roc, if you have time could you give me feedback on what's already there?
Comment 49 Lazar Sumar [:nonsensickle] 2012-02-23 17:27:12 PST
Created attachment 600260 [details] [diff] [review]
background-repeat parser changes patch v2.1

Rebasing for changes in bug 548375 attachment #600254 [details] [diff] [review].
Comment 50 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-27 01:38:58 PST
Comment on attachment 600218 [details] [diff] [review]
background-repeat space rendering patch v1.5

Review of attachment 600218 [details] [diff] [review]:
-----------------------------------------------------------------

I actually think we should land this before fixing bug 600207, if it's ready.

::: layout/base/nsCSSRendering.cpp
@@ +4366,5 @@
>        nsLayoutUtils::DrawBackgroundImage(&aRenderingContext, mImageContainer,
>            nsIntSize(nsPresContext::AppUnitsToIntCSSPixels(mSize.width),
>                      nsPresContext::AppUnitsToIntCSSPixels(mSize.height)),
>            graphicsFilter,
> +          aDest, aFill, aSpacing, aGapCount, aAnchor, aDirty, drawFlags);

So we're not supporting 'space' for gradients and -moz-element, but we should. That can be a separate patch or bug.

::: layout/base/nsLayoutUtils.cpp
@@ +3637,5 @@
> +                    nsPresContext::AppUnitsToIntCSSPixels(dest.width),
> +                    nsPresContext::AppUnitsToIntCSSPixels(dest.height));
> +  nsIntSize surfIntSize =
> +    nsIntSize(nsPresContext::AppUnitsToIntCSSPixels(surfSize.width),
> +              nsPresContext::AppUnitsToIntCSSPixels(surfSize.height));

These calls to AppUnitsToIntCSSPixels round to the nearest pixel. Is that really what you want? It's not clear to me what happens to the errors from such rounding. Or since you're creating a temporary surface in image space, are these guaranteed to be integer multiples of CSS pixels already? If so, please assert that here.

@@ +4011,5 @@
>                                     const nsRect&       aDirty,
>                                     PRUint32            aImageFlags)
>  {
>    SAMPLE_LABEL("layout", "nsLayoutUtils::DrawBackgroundImage");
> +  nsresult res;

'rv'

@@ +4029,5 @@
> +                                       aGraphicsFilter, dest,
> +                                       quadFill[i], quadPadding[i],
> +                                       aAnchor + translation, aDirty,
> +                                       aImageSize, aImageFlags);
> +    if (res != NS_OK) {

NS_FAILED(rv)

::: layout/base/nsLayoutUtils.h
@@ +1105,5 @@
> +   * at aDest and then repeated aImageCount.width times to the right and 
> +   * aImageCount.height times down, with the total horizontal and vertical 
> +   * spacing between all the repetitions being equal to aSpacing.
> +   * TotalArea = aImageCount * aImageSize + aSpacing (ignoring app unit to css
> +   *                                                  unit conversions).

Update this comment to aGapCount

@@ +1138,5 @@
>                                        GraphicsFilter      aGraphicsFilter,
>                                        const nsRect&       aDest,
>                                        const nsRect&       aFill,
> +                                      const nsSize&       aSpacing,
> +                                      const nsIntSize&    aImageCount,

Rename this to aGapCount
Comment 51 Lazar Sumar [:nonsensickle] 2012-03-04 13:15:40 PST
Created attachment 602760 [details] [diff] [review]
background-repeat space rendering patch v1.6

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #50)
> Comment on attachment 600218 [details] [diff] [review]
> background-repeat space rendering patch v1.5
> 
> Review of attachment 600218 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I actually think we should land this before fixing bug 600207, if it's ready.

I think that the basic version is ready, though there are some minor issues. There are some zoom problems that might be caused by rounding errors you picked up below.

> ::: layout/base/nsCSSRendering.cpp
> @@ +4366,5 @@
> >        nsLayoutUtils::DrawBackgroundImage(&aRenderingContext, mImageContainer,
> >            nsIntSize(nsPresContext::AppUnitsToIntCSSPixels(mSize.width),
> >                      nsPresContext::AppUnitsToIntCSSPixels(mSize.height)),
> >            graphicsFilter,
> > +          aDest, aFill, aSpacing, aGapCount, aAnchor, aDirty, drawFlags);
> 
> So we're not supporting 'space' for gradients and -moz-element, but we
> should. That can be a separate patch or bug.

How would you do it for gradients? I thought that gradients had the size equivalent to that of the element which when used with the rules in the spec means they cannot be spaced... I might be misunderstanding something though.

> ::: layout/base/nsLayoutUtils.cpp
> @@ +3637,5 @@
> > +                    nsPresContext::AppUnitsToIntCSSPixels(dest.width),
> > +                    nsPresContext::AppUnitsToIntCSSPixels(dest.height));
> > +  nsIntSize surfIntSize =
> > +    nsIntSize(nsPresContext::AppUnitsToIntCSSPixels(surfSize.width),
> > +              nsPresContext::AppUnitsToIntCSSPixels(surfSize.height));
> 
> These calls to AppUnitsToIntCSSPixels round to the nearest pixel. Is that
> really what you want? It's not clear to me what happens to the errors from
> such rounding. Or since you're creating a temporary surface in image space,
> are these guaranteed to be integer multiples of CSS pixels already? If so,
> please assert that here.

aDest's size is propagated from the nsCSSRenderer so it might or might not be (depending if background-size has been defined). As for the aDest's position I think that that doesn't matter so much as we do need to 'snap' the image into place somewhere (and I'm always using an anchor of 0,0). I'm not sure if I can mitigate these errors at all so long as my intermediate surface is in image space.

The padding is guaranteed to be an even multiple of CSS pixels. 

Is there a better way to assert this then
NS_ASSERTION(AppUnitsToIntCSSPixels(X) == AppUnitsToFloatCSSPixels(X)) ?
Should I use an epsilon value?

Assertion has not been added to this revision, pending question above.

> @@ +4011,5 @@
> >                                     const nsRect&       aDirty,
> >                                     PRUint32            aImageFlags)
> >  {
> >    SAMPLE_LABEL("layout", "nsLayoutUtils::DrawBackgroundImage");
> > +  nsresult res;
> 
> 'rv'

Done.

> @@ +4029,5 @@
> > +                                       aGraphicsFilter, dest,
> > +                                       quadFill[i], quadPadding[i],
> > +                                       aAnchor + translation, aDirty,
> > +                                       aImageSize, aImageFlags);
> > +    if (res != NS_OK) {
> 
> NS_FAILED(rv)

I didn't know about that one. Thanks.

> ::: layout/base/nsLayoutUtils.h
> @@ +1105,5 @@
> > +   * at aDest and then repeated aImageCount.width times to the right and 
> > +   * aImageCount.height times down, with the total horizontal and vertical 
> > +   * spacing between all the repetitions being equal to aSpacing.
> > +   * TotalArea = aImageCount * aImageSize + aSpacing (ignoring app unit to css
> > +   *                                                  unit conversions).
> 
> Update this comment to aGapCount

Done.

> @@ +1138,5 @@
> >                                        GraphicsFilter      aGraphicsFilter,
> >                                        const nsRect&       aDest,
> >                                        const nsRect&       aFill,
> > +                                      const nsSize&       aSpacing,
> > +                                      const nsIntSize&    aImageCount,
> 
> Rename this to aGapCount

Done.
Comment 52 Lazar Sumar [:nonsensickle] 2012-03-14 00:59:13 PDT
(In reply to Lazar Sumar [:lsumar] from comment #51)
> Created attachment 602760 [details] [diff] [review]
> background-repeat space rendering patch v1.6
> 
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February
> 21 to March 17) from comment #50)
> > Comment on attachment 600218 [details] [diff] [review]
> > background-repeat space rendering patch v1.5
> > 
> > Review of attachment 600218 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: layout/base/nsLayoutUtils.cpp
> > @@ +3637,5 @@
> > > +                    nsPresContext::AppUnitsToIntCSSPixels(dest.width),
> > > +                    nsPresContext::AppUnitsToIntCSSPixels(dest.height));
> > > +  nsIntSize surfIntSize =
> > > +    nsIntSize(nsPresContext::AppUnitsToIntCSSPixels(surfSize.width),
> > > +              nsPresContext::AppUnitsToIntCSSPixels(surfSize.height));
> > 
> > These calls to AppUnitsToIntCSSPixels round to the nearest pixel. Is that
> > really what you want? It's not clear to me what happens to the errors from
> > such rounding. Or since you're creating a temporary surface in image space,
> > are these guaranteed to be integer multiples of CSS pixels already? If so,
> > please assert that here.
> 
> aDest's size is propagated from the nsCSSRenderer so it might or might not
> be (depending if background-size has been defined). As for the aDest's
> position I think that that doesn't matter so much as we do need to 'snap'
> the image into place somewhere (and I'm always using an anchor of 0,0). I'm
> not sure if I can mitigate these errors at all so long as my intermediate
> surface is in image space.
> 
> The padding is guaranteed to be an even multiple of CSS pixels. 
> 
> Is there a better way to assert this then
> NS_ASSERTION(AppUnitsToIntCSSPixels(X) == AppUnitsToFloatCSSPixels(X)) ?
> Should I use an epsilon value?
> 
> Assertion has not been added to this revision, pending question above.

This is the point where I've intentionally allowed rounding errors to happen. I tried to defer them to the last possible moment but I think that they must happen here. At this point I am specifying the region where the image is to be drawn on the destination surface which is in image space. This means that any rounding error that occurs is being applied at the very last possible moment, just before rendering to the intermediate surface. I've thought about how I can mitigate this and the only way I could think of, other than moving it to device space, was to adjust the anchor point according to the quadrant that I am rendering.
Comment 53 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-25 18:51:34 PDT
Comment on attachment 602760 [details] [diff] [review]
background-repeat space rendering patch v1.6

Review of attachment 602760 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +3640,5 @@
> + * @return See notes for IntermediateSurfaceParameters.
> + */
> +static IntermediateSurfaceParameters
> +ComputeIntermediateSurfaceParameters(const nsSize&     aImageSize,
> +                                     const nsMargin&   aPadding)

This can be an nsIntSize in CSS pixels since it's always a whole number of pixels (and top-left is 0,0).

@@ +3917,5 @@
> + */
> +inline nscoord
> +BisectSpacingAxis(nscoord  aImageSize,
> +                  nscoord  aAxisOffset, nscoord  aAxisLength,
> +                  PRInt32  aSpacing, nscoord  aRemainder,

don't bother indenting anything on these two lines

@@ +3958,5 @@
> +                               const nsRect&    aFill,
> +                               const nsSize&    aSpacing,
> +                               const nsIntSize& aGapCount,
> +                               nsRect*          aOutFill,
> +                               nsMargin*        aOutPadding)

Let's not call this 'padding'. Padding has a specific meaning in CSS. Let's call it aOutBetweenImageGap or something like that.

Also, since the top/left is always 0,0, I think it's a good idea to make this an nsSize instead of an nsMargin. And I think it should be an nsIntSize and in pixels since it's always a whole number of pixels.

::: layout/base/nsLayoutUtils.h
@@ +1117,5 @@
>     *                            occurs, or it might be the image's size if
>     *                            the image is a vector image being rendered at
>     *                            that size.)
>     *   @param aDest             The position and scaled area where one copy of
>     *                            the image should be drawn.

Does aDest include a gap, or does aDest include only the image?

I don't see anything in nsCSSRendering to reduce aDest when there's a gap, so I assume it includes the gap. But then we use it as the image size in BisectSpacingAxis. I must be missing something, but the documentation here needs to be improved anyway.
Comment 54 Lazar Sumar [:nonsensickle] 2013-06-15 15:21:46 PDT
Created attachment 763165 [details] [diff] [review]
background-repeat patch 1 tests v9

Removing bitrot.
Comment 55 Lazar Sumar [:nonsensickle] 2013-06-15 15:24:57 PDT
Created attachment 763166 [details] [diff] [review]
background-repeat patch 2 parser changes v3

Removing bitrot.
Comment 56 Lazar Sumar [:nonsensickle] 2013-06-15 15:25:59 PDT
Created attachment 763167 [details] [diff] [review]
background-repeat patch 3 round rendering v3

Removing bitrot.
Comment 57 Lazar Sumar [:nonsensickle] 2013-06-15 15:27:00 PDT
Created attachment 763168 [details] [diff] [review]
background-repeat patch 4 space rendering v2

Removing bitrot.
Comment 58 Lazar Sumar [:nonsensickle] 2013-06-15 15:29:52 PDT
Created attachment 763169 [details] [diff] [review]
background-repeat patch 4 space rendering v2

Whoops, forgot to overwrite the obsolete patches.

Note: The current state of the patches fails the reftests in patch 1. I'm currently working on resolving the issue.
Comment 59 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-16 19:41:39 PDT
Comment on attachment 763165 [details] [diff] [review]
background-repeat patch 1 tests v9

Review of attachment 763165 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this needs re-review.
Comment 60 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-16 19:42:53 PDT
Thanks for updating the patches!!!
Comment 61 Lazar Sumar [:nonsensickle] 2013-06-22 04:00:01 PDT
Created attachment 766271 [details] [diff] [review]
background-repeat patch 4 space rendering v2.1

Passes reftests

Still need to address Roc's comments from 2012-06-26.
Comment 62 Lazar Sumar [:nonsensickle] 2013-06-22 06:59:13 PDT
Created attachment 766292 [details] [diff] [review]
background-repeat patch 4 space rendering v2.2

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #53)
> Comment on attachment 602760 [details] [diff] [review]
> background-repeat space rendering patch v1.6
> 
> Review of attachment 602760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsLayoutUtils.cpp
> @@ +3640,5 @@
> > + * @return See notes for IntermediateSurfaceParameters.
> > + */
> > +static IntermediateSurfaceParameters
> > +ComputeIntermediateSurfaceParameters(const nsSize&     aImageSize,
> > +                                     const nsMargin&   aPadding)
> 
> This can be an nsIntSize in CSS pixels since it's always a whole number of
> pixels (and top-left is 0,0).

Done.

> @@ +3917,5 @@
> > + */
> > +inline nscoord
> > +BisectSpacingAxis(nscoord  aImageSize,
> > +                  nscoord  aAxisOffset, nscoord  aAxisLength,
> > +                  PRInt32  aSpacing, nscoord  aRemainder,
> 
> don't bother indenting anything on these two lines

I'm not sure what you mean by don't indent? Don't inden't at all or don't indent as much? I've reshuffled it a little, I hope that's enough.

> @@ +3958,5 @@
> > +                               const nsRect&    aFill,
> > +                               const nsSize&    aSpacing,
> > +                               const nsIntSize& aGapCount,
> > +                               nsRect*          aOutFill,
> > +                               nsMargin*        aOutPadding)
> 
> Let's not call this 'padding'. Padding has a specific meaning in CSS. Let's
> call it aOutBetweenImageGap or something like that.
> 
> Also, since the top/left is always 0,0, I think it's a good idea to make
> this an nsSize instead of an nsMargin. And I think it should be an nsIntSize
> and in pixels since it's always a whole number of pixels.

I picked nsMargin so that spacing can be added around the image. However, I agree that this is unnecessary. Changed to nsIntSize.

> ::: layout/base/nsLayoutUtils.h
> @@ +1117,5 @@
> >     *                            occurs, or it might be the image's size if
> >     *                            the image is a vector image being rendered at
> >     *                            that size.)
> >     *   @param aDest             The position and scaled area where one copy of
> >     *                            the image should be drawn.
> 
> Does aDest include a gap, or does aDest include only the image?
> 
> I don't see anything in nsCSSRendering to reduce aDest when there's a gap,
> so I assume it includes the gap. But then we use it as the image size in
> BisectSpacingAxis. I must be missing something, but the documentation here
> needs to be improved anyway.

aDest is the image itself with position and size information. It does not include the spacing/gap. The gap is propagated, separately to aDest, down to nsLayoutUtils which deals with it.
Comment 63 Lazar Sumar [:nonsensickle] 2013-06-22 07:01:25 PDT
Comment on attachment 763166 [details] [diff] [review]
background-repeat patch 2 parser changes v3

Review of attachment 763166 [details] [diff] [review]:
-----------------------------------------------------------------

This patch has been unbitrotted. I am not sure what the protocol with the review's is in this case so I'm re-requesting one for all patches.
Comment 64 Lazar Sumar [:nonsensickle] 2013-06-22 07:02:55 PDT
Comment on attachment 763167 [details] [diff] [review]
background-repeat patch 3 round rendering v3

Review of attachment 763167 [details] [diff] [review]:
-----------------------------------------------------------------

Hi again Roc. I know you passed the tests review as ok but I'm not sure if the review's are required for unbitrotting files so I'm requesting one anyway. This should not have changed from before. Thanks in advance.
Comment 65 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-25 20:38:21 PDT
Comment on attachment 763167 [details] [diff] [review]
background-repeat patch 3 round rendering v3

Review of attachment 763167 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +4584,5 @@
> + * http://dev.w3.org/csswg/css3-background/#the-background-size
> + */
> +static nscoord
> +ComputeBackgroundRepeadRound(nscoord currentSize, 
> +                             nscoord positioningSize) {

Repeat, not Repead
Comment 66 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-25 21:17:39 PDT
Comment on attachment 766292 [details] [diff] [review]
background-repeat patch 4 space rendering v2.2

Review of attachment 766292 [details] [diff] [review]:
-----------------------------------------------------------------

It's looking good.

::: layout/base/nsLayoutUtils.cpp
@@ +4008,5 @@
>                 aSVGContext, imgIContainer::FRAME_CURRENT, aImageFlags);
>    return NS_OK;
>  }
>  
> +/* IntermediateSurfaceParameters holds the return value of the 

/** for all doc-comments

@@ +4035,5 @@
> + * creating and drawing to an intermediate surface which represents a padded
> + * image.
> + * @param  aImageSize     - Image size in App Units.
> + * @param  aImageSpacing  - The padding, in app units, to be added around the image.
> + * @return See notes for IntermediateSurfaceParameters.

aImageSpace should be an nsSize given it's in appunits.

@@ +4055,5 @@
> +  
> +  return IntermediateSurfaceParameters(cssDest, subimage, surfIntSize);
> +}
> +
> +/* Draws the image to an intermediate surface, in image space, adding the

Use /** for a doc-comment

@@ +4067,5 @@
> +                             imgIContainer*      aImage,
> +                             GraphicsFilter      aGraphicsFilter,
> +                             const nsRect&       aDest,
> +                             const nsRect&       aFill,
> +                             const nsIntSize&    aImageSpacing,

Should be nsSize given it's in appunits

@@ +4113,5 @@
> +  if (!drawingParams.mShouldDraw)
> +    return NS_OK;
> +
> +  gfxContextMatrixAutoSaveRestore saveMatrix(ctx);
> +  if (drawingParams.mResetCTM) { // TODO: Check validity of this statement!!!

I think this is right; remove the comment.

@@ +4165,5 @@
>                               gfxASurface::ImageFormatARGB32, aFilter);
>  }
>  
> +/* ComputeImageAxisPadding is a helper function that computes the number
> + * of pixels that each image is padded with, in image space, in order to fill

Start the comment with /** since it's a doc-comment

@@ +4183,5 @@
> +                        nscoord& aOutRemainder)
> +{
> +  if (aGapCount > 0) {
> +    float spacing = 0.0f;
> +    spacing = nsPresContext::AppUnitsToFloatCSSPixels(aTotalSpacingUnits) / aGapCount;

Fuse these into a single statement. Also, this should probably be a double.

@@ +4203,5 @@
> + *   @aRemainder         - see ComputeImageAxisPadding @param aOutRemainder.
> + *   @aOutImageSpacing1  - Represents the spacing in the first region. If spacing can
> + *                         be performed, this is never zero.
> + *   @aOutImageSpacing2  - Represents the spacing in the second region. Zero if
> + *                         spacing can be done evenly in the first region.

Document aAxisOffset/aAxisLength

@@ +4209,5 @@
> + *             If the axis doesn't need to be split it returns the end point.
> + */
> +inline nscoord
> +BisectSpacingAxis(nscoord  aImageSize, nscoord  aAxisOffset, nscoord  aAxisLength,
> +                  PRInt32  aTotalSpacingUnits, nscoord  aRemainder,

Remove all the double-spaces on these lines.

@@ +4210,5 @@
> + */
> +inline nscoord
> +BisectSpacingAxis(nscoord  aImageSize, nscoord  aAxisOffset, nscoord  aAxisLength,
> +                  PRInt32  aTotalSpacingUnits, nscoord  aRemainder,
> +                  PRInt32& aOutImageSpacing1, PRInt32& aOutImageSpacing2)

These should be nscoord since they're in appunits.

@@ +4250,5 @@
> +                               const nsRect&    aFill,
> +                               const nsSize&    aTotalSpacingUnits,
> +                               const nsIntSize& aGapCount,
> +                               nsRect*          aOutFill,
> +                               nsIntSize*       aOutImageSpacing)

aOutImageSpace should be nsSize* since it's in appunits

@@ +4418,5 @@
>    if (UseBackgroundNearestFiltering()) {
>      aGraphicsFilter = gfxPattern::FILTER_NEAREST;
>    }
>  
> +  nsresult rv;

I know we discussed this before, but I think we should have a fast path here so that when aGapCount is zero in each direction, we just call DrawImageInternal the way we did before.
Comment 67 Lazar Sumar [:nonsensickle] 2013-06-27 03:20:17 PDT
Created attachment 768240 [details] [diff] [review]
background-repeat patch 3 round rendering v3.1

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #65)
> Comment on attachment 763167 [details] [diff] [review]
> background-repeat patch 3 round rendering v3
> 
> Review of attachment 763167 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsCSSRendering.cpp
> @@ +4584,5 @@
> > + * http://dev.w3.org/csswg/css3-background/#the-background-size
> > + */
> > +static nscoord
> > +ComputeBackgroundRepeadRound(nscoord currentSize, 
> > +                             nscoord positioningSize) {
> 
> Repeat, not Repead

Done. (Obvious copy paste since I managed to propagate it twice).

Leaving as review+
Comment 68 Lazar Sumar [:nonsensickle] 2013-06-28 03:03:40 PDT
Created attachment 768854 [details] [diff] [review]
background-repeat patch 4 space rendering v2.3

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #66)
> Comment on attachment 766292 [details] [diff] [review]
> background-repeat patch 4 space rendering v2.2
> 
> Review of attachment 766292 [details] [diff] [review]:
> -----------------------------------------------------------------

> ::: layout/base/nsLayoutUtils.cpp
> @@ +4008,5 @@
> >                 aSVGContext, imgIContainer::FRAME_CURRENT, aImageFlags);
> >    return NS_OK;
> >  }
> >  
> > +/* IntermediateSurfaceParameters holds the return value of the 
> 
> /** for all doc-comments

Done for all stated and all not stated but present in the patch (please check, it's late and I could have missed one).

> @@ +4035,5 @@
> > + * creating and drawing to an intermediate surface which represents a padded
> > + * image.
> > + * @param  aImageSize     - Image size in App Units.
> > + * @param  aImageSpacing  - The padding, in app units, to be added around the image.
> > + * @return See notes for IntermediateSurfaceParameters.
> 
> aImageSpace should be an nsSize given it's in appunits.

I have not done this yet. Previously I was asked to make all things that should be whole numbers into an nsIntSize regardless of app units. I thought that aImageSpacing is one of those so I will have to go and check.

> @@ +4067,5 @@
> > +                             imgIContainer*      aImage,
> > +                             GraphicsFilter      aGraphicsFilter,
> > +                             const nsRect&       aDest,
> > +                             const nsRect&       aFill,
> > +                             const nsIntSize&    aImageSpacing,
> 
> Should be nsSize given it's in appunits

Again, I will have to check if this has to be an integer.

> @@ +4113,5 @@
> > +  if (!drawingParams.mShouldDraw)
> > +    return NS_OK;
> > +
> > +  gfxContextMatrixAutoSaveRestore saveMatrix(ctx);
> > +  if (drawingParams.mResetCTM) { // TODO: Check validity of this statement!!!
> 
> I think this is right; remove the comment.

Comment removed, statement left in.

> @@ +4183,5 @@
> > +                        nscoord& aOutRemainder)
> > +{
> > +  if (aGapCount > 0) {
> > +    float spacing = 0.0f;
> > +    spacing = nsPresContext::AppUnitsToFloatCSSPixels(aTotalSpacingUnits) / aGapCount;
> 
> Fuse these into a single statement. Also, this should probably be a double.

That was a way to keep to the 80 char limit. Nonsense removed.
 
> @@ +4203,5 @@
> > + *   @aRemainder         - see ComputeImageAxisPadding @param aOutRemainder.
> > + *   @aOutImageSpacing1  - Represents the spacing in the first region. If spacing can
> > + *                         be performed, this is never zero.
> > + *   @aOutImageSpacing2  - Represents the spacing in the second region. Zero if
> > + *                         spacing can be done evenly in the first region.
> 
> Document aAxisOffset/aAxisLength

Both documented but I'm gonna see if I can remove the aAxisLength. It seems to merely acts as a clamp to truncate remainders (which shouldn't be present at this point anyway).

> @@ +4209,5 @@
> > + *             If the axis doesn't need to be split it returns the end point.
> > + */
> > +inline nscoord
> > +BisectSpacingAxis(nscoord  aImageSize, nscoord  aAxisOffset, nscoord  aAxisLength,
> > +                  PRInt32  aTotalSpacingUnits, nscoord  aRemainder,
> 
> Remove all the double-spaces on these lines.

Done.

> @@ +4210,5 @@
> > + */
> > +inline nscoord
> > +BisectSpacingAxis(nscoord  aImageSize, nscoord  aAxisOffset, nscoord  aAxisLength,
> > +                  PRInt32  aTotalSpacingUnits, nscoord  aRemainder,
> > +                  PRInt32& aOutImageSpacing1, PRInt32& aOutImageSpacing2)
> 
> These should be nscoord since they're in appunits.

Not done. See previous comments relating to nsSize vs nsIntSize.

> @@ +4250,5 @@
> > +                               const nsRect&    aFill,
> > +                               const nsSize&    aTotalSpacingUnits,
> > +                               const nsIntSize& aGapCount,
> > +                               nsRect*          aOutFill,
> > +                               nsIntSize*       aOutImageSpacing)
> 
> aOutImageSpace should be nsSize* since it's in appunits

Not done. See previous comments on nsSize vs nsIntSize.

> @@ +4418,5 @@
> >    if (UseBackgroundNearestFiltering()) {
> >      aGraphicsFilter = gfxPattern::FILTER_NEAREST;
> >    }
> >  
> > +  nsresult rv;
> 
> I know we discussed this before, but I think we should have a fast path here
> so that when aGapCount is zero in each direction, we just call
> DrawImageInternal the way we did before.

Agreed. I've refactored since so I can't remember why it isn't there but it definitely should be. Done.
Comment 69 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-06-28 17:17:17 PDT
Comment on attachment 763166 [details] [diff] [review]
background-repeat patch 2 parser changes v3

r=dbaron if you add some tests to property_database.js for the new values, both in the background-repeat property and the background shorthand (as I said in comment 41)
Comment 70 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-29 06:05:05 PDT
(In reply to Lazar Sumar [:lsumar] from comment #68)
> I have not done this yet. Previously I was asked to make all things that
> should be whole numbers into an nsIntSize regardless of app units. I thought
> that aImageSpacing is one of those so I will have to go and check.

Sorry for the confusion. What I meant was that aImageSpacing should be nsIntSize *and* be in CSS pixels.
Comment 71 Lazar Sumar [:nonsensickle] 2013-07-23 14:10:40 PDT
It is unlikely that I will be able to get this completed in the next 5 weeks. After that time I'll start work on this again. Someone else can take it in the meantime if need be.
Comment 72 Florian Bender 2014-02-01 12:15:50 PST
Lazar, do you still want to fix this? Otherwise I'd reset the assignee so somebody else can volunteer.
Comment 73 Lazar Sumar [:nonsensickle] 2014-02-09 15:16:15 PST
(In reply to Florian Bender from comment #72)
> Lazar, do you still want to fix this? Otherwise I'd reset the assignee so
> somebody else can volunteer.

Hi Florian, considering the time taken I'm embarrassed to hold onto it much longer. If I manage to update the patches this week I'll be back but for now I'll let someone else take it.
Comment 74 Florian Bender 2014-02-16 13:03:56 PST
Do you have any updated patches addressing the review, or at least rebased patches? That would help a lot if anybody else is willing to take this bug.
Comment 75 Lazar Sumar [:nonsensickle] 2014-03-14 17:50:30 PDT
(In reply to Florian Bender from comment #74)
> Do you have any updated patches addressing the review, or at least rebased
> patches? That would help a lot if anybody else is willing to take this bug.

Not at this stage. When I do I will put them up.
Comment 76 Lazar Sumar [:nonsensickle] 2014-04-06 01:56:46 PDT
Created attachment 8402280 [details] [diff] [review]
background-repeat patch 2 parser changes v3.1
Comment 77 Lazar Sumar [:nonsensickle] 2014-04-06 01:57:31 PDT
Created attachment 8402281 [details] [diff] [review]
background-repeat patch 1 tests v9.1
Comment 78 Lazar Sumar [:nonsensickle] 2014-04-06 02:03:21 PDT
(In reply to Florian Bender from comment #74)
> Do you have any updated patches addressing the review, or at least rebased
> patches? That would help a lot if anybody else is willing to take this bug.

Hi Florian, I've updated the parser and test patches, however the round keyword implementation will require a little more of my time than I can afford tonight.

I will say that the changes that have introduced bit-rot in the round patch came from the fix for Bug 700926. They properly implement the computations outlined at http://www.w3.org/TR/css3-background/#the-background-size, that I myself was trying to follow.

With that fix, the round keyword patch should become smaller but I don't have time right now to do the work. I'll come back to it in a week or so.

Still, I'm leaving the bug for others to attack if they wish.
Comment 79 Florian Bender 2014-11-22 05:58:51 PST
Lazar, did you have a change to look at this again? :)
Comment 80 Lazar Sumar [:nonsensickle] 2014-12-14 18:54:53 PST
(In reply to Florian Bender from comment #79)
> Lazar, did you have a change to look at this again? :)

Hi Florian, not yet. I'm hoping to take a second look when I go on holiday next week, unless someone beats me to it.
Comment 81 Lazar Sumar [:nonsensickle] 2014-12-15 16:13:08 PST
Florian, are you aware of any other bugs that will be changing the background image code in the near future?
Comment 82 Florian Bender 2014-12-22 10:04:40 PST
There's Bug 700926 lurking around but since there was no activity for some time, I presume there will be none for the next weeks as well. I don't know of any other bugs, but :seth (or :dbaron?) may know more.
Comment 83 Seth Fowler [:seth] [:s2h] 2015-03-11 13:55:34 PDT
Sorry for taking so long to respond; somehow I missed this needinfo. (That's what I get for using needinfo's as a TODO list.)

I really don't have an answer, though. It depends what you consider the "background image" code to be. There's been a huge amount of churn in image-related code recently. You will probably need to rebase.
Comment 84 Mark Straver 2015-06-15 17:50:28 PDT
I know this bug is a little stale, but I've had a jab at implementing this locally based on the existing patches (with the necessary edits, of course).

My observations:
* With HWA off, everything works just fine and dandy!
* With HWA on, Round is fine, but space is problematic:
In DrawImageWithSpacingInternal, currentSurface->CreateSimilarSurface crashes because ctx->CurrentSurface() comes back as null (Azure isn't backed by surfaces, IIRC, so that would only work in cairo)

Something tells me there should be an easy way around this...
Comment 85 yiorsi 2015-07-09 00:40:13 PDT
No one to fix it?
Comment 86 yiorsi 2015-07-09 00:41:33 PDT Comment hidden (spam)
Comment 87 Mark Straver 2015-07-10 05:50:32 PDT
Maybe someone who has enough knowledge of what exactly is available in this kind of context can have a look at this? What are our options? An off-screen surface in main memory? Creating a new surface/target from scratch? Some DX magic?
Comment 88 Mark Straver 2015-12-01 16:24:58 PST
I guess nobody has any idea how to solve this, then? :)

To refresh/explain the problem:

The problem is the following code in DrawImageWithSpacingInternal, where there's the following code:

  nsRefPtr<gfxASurface> currentSurface = ctx->CurrentSurface();

  // Construct the intermediate surface (in image space)
  nsRefPtr<gfxASurface> tmpSurface =
    currentSurface->CreateSimilarSurface(gfxASurface::CONTENT_COLOR_ALPHA,
                                         srfParams.mSurfaceSize);

With HWA on, regardless of specific mode used, gfxcontext ctx->CurrentSurface() returns a nullptr because of:

already_AddRefed<gfxASurface>
gfxContext::CurrentSurface(gfxFloat *dx, gfxFloat *dy)
{
  if (mCairo) {
    cairo_surface_t *s = cairo_get_group_target(mCairo);
    if (s == mSurface->CairoSurface()) {
        if (dx && dy)
            cairo_surface_get_device_offset(s, dx, dy);
        nsRefPtr<gfxASurface> ret = mSurface;
        return ret.forget();
    }

    if (dx && dy)
        cairo_surface_get_device_offset(s, dx, dy);
    return gfxASurface::Wrap(s);
  } else {
    if (dx && dy) {
      *dx = *dy = 0;
    }
    // An Azure context doesn't have a surface backing it. <<***
    return nullptr;
  }
}

Which in turn makes CreateSimilarSurface crash (no checks + no surface to make a similar copy of). We have no surface, period.
So for D2D and friends, this needs a different approach, or creating a new drawing surface to send off when filled, correct?
Comment 89 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2015-12-01 20:34:21 PST
Probably you want to focus on using the newer DrawTarget-based APIs in gfx/2d/2D.h rather than the older gfxContext/gfxASurface APIs.  There's an ongoing effort to convert to those.

That said, I'm not particularly sure of specifics, but others might be able to advise better.  I'd recommend using needinfo? to get people's attention, though.

That said, if you're going to ask questions about patches you're working on, it's also best to post those patches so that the people you're asking can see the code you're asking about.
Comment 90 Mark Straver 2015-12-02 04:59:13 PST
The patches are those posted here in this bug (patch 4 v2.3) -- I barely had to edit anything apart from the interface to surrounding code and it works as-is with this one exception.
Converting it to a different API is always possible of course; is the conversion process documented somewhere? Are both the APIs documented somewhere?

I'm also not sure who to NI for this, I don't know who in general works on what in the current organization - there seems to be some churn in tasks and people.
Comment 91 Lazar Sumar [:nonsensickle] 2016-01-26 13:30:18 PST
I've had a quick chat with Matt W. and he suggested the following:

> <nonsensickle> Basically, I used a cairo intermediate surface to render an image with padding around it (spacing)
> <nonsensickle> and then told cairo to repeat this image across a canvas...
> <nonsensickle> how would you do that in Azure?
> <mattwoodrow> DrawTarget::CreateSimilarDrawTarget
> <mattwoodrow> draw into that, and use DrawTarget::Snapshot to convert your temporary DT into a SourceSurface. Then you probably want FillRect to draw it, constructing a SurfacePattern around the SourceSurface with the REPEAT extend mode

Which should help anyone picking up this work.
Comment 92 Ethan Lin[:ethlin] 2016-03-22 02:10:20 PDT
I will try to study this bug.
Comment 93 percyley 2016-03-31 21:20:17 PDT
(In reply to Ethan Lin[:ethlin] from comment #92)
> I will try to study this bug.

Good news!
Comment 94 Ethan Lin[:ethlin] 2016-05-06 02:09:49 PDT
Created attachment 8749579 [details] [diff] [review]
background-repeat patch 1

Test cases.
Comment 95 Ethan Lin[:ethlin] 2016-05-06 02:16:20 PDT
Created attachment 8749580 [details] [diff] [review]
Part1. parser changes

Add round and space values to background-repeat.
Comment 96 Ethan Lin[:ethlin] 2016-05-06 02:18:13 PDT
Created attachment 8749581 [details] [diff] [review]
Part2. Handle rendering of background-repeat: round

Handle rendering if round.
Comment 97 Ethan Lin[:ethlin] 2016-05-06 02:20:05 PDT
Created attachment 8749582 [details] [diff] [review]
Part3. Handle rendering of background-repeat: space

Handle rendering of space.
Comment 98 Astley Chen [:astley] (UTC+8) 2016-05-06 02:25:36 PDT
Change status to assigned since Ethan Lin is now working on this bug.
Comment 99 Cameron McCormack (:heycam) 2016-05-08 23:52:01 PDT
Comment on attachment 8749580 [details] [diff] [review]
Part1. parser changes

Review of attachment 8749580 [details] [diff] [review]:
-----------------------------------------------------------------

r=me assuming you adjust property_database.js to add the new values (this can be in a separate part that adds overall tests for the feature).
Comment 100 percyley 2016-05-09 02:50:44 PDT
Implement border-image-repeat space keyword
https://bugzilla.mozilla.org/show_bug.cgi?id=720531
Comment 101 Markus Stange [:mstange] 2016-05-09 16:31:08 PDT
Comment on attachment 8749581 [details] [diff] [review]
Part2. Handle rendering of background-repeat: round

Review of attachment 8749581 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't read the spec closely yet, so I'll need to do another review pass after I've done that and after my comments have been fixed.

This needs lots of reftests before it can land. Are you working on those?

::: layout/base/nsCSSRendering.cpp
@@ +2018,5 @@
>    if (aDrawBackgroundColor &&
> +      (bg->BottomLayer().mRepeat.mXRepeat & (NS_STYLE_IMAGELAYER_REPEAT_REPEAT |
> +                                             NS_STYLE_IMAGELAYER_REPEAT_ROUND)) &&
> +      (bg->BottomLayer().mRepeat.mYRepeat & (NS_STYLE_IMAGELAYER_REPEAT_REPEAT |
> +                                             NS_STYLE_IMAGELAYER_REPEAT_ROUND)) &&

Uhh, the values of NS_STYLE_IMAGELAYER_REPEAT_* are not powers of two. I don't think this works.

@@ +3236,5 @@
> + * http://dev.w3.org/csswg/css3-background/#the-background-size
> + */
> +static nscoord
> +ComputeBackgroundRepeatRound(nscoord currentSize,
> +                             nscoord positioningSize)

These arguments need the "a" prefix to conform with the style guide.

@@ +3239,5 @@
> +ComputeBackgroundRepeatRound(nscoord currentSize,
> +                             nscoord positioningSize)
> +{
> +  float x = currentSize;
> +  float w = positioningSize;

I don't think there is value in using the variable names "x" and "w" to match the spec. The spec only uses them because it describes the horizontal dimension as an example.

How about this:

float repeatCount = NS_roundf(float(aPositioningSize) / float(aCurrentSize));
if (repeatCount < 1.0f) {
  return aPositioningSize;
}
return nscoord(NS_lround(float(aPositioningSize) / repeatCount)));

@@ +3379,5 @@
>                                                     bgPositionSize,
>                                                     aLayer.mSize);
> +  uint8_t xRepeat = aLayer.mRepeat.mXRepeat;
> +  uint8_t yRepeat = aLayer.mRepeat.mYRepeat;
> +  bool hasTwoRoundKeywords = xRepeat == NS_STYLE_IMAGELAYER_REPEAT_ROUND &&

Maybe call this isRepeatRoundInBothDimensions

@@ +3383,5 @@
> +  bool hasTwoRoundKeywords = xRepeat == NS_STYLE_IMAGELAYER_REPEAT_ROUND &&
> +                             yRepeat == NS_STYLE_IMAGELAYER_REPEAT_ROUND;
> +
> +  // calculate the rounded size only if the background-size computation
> +  // returned a correct size for the image.

Why don't you instead move this below the early return?

I assume there's a reason for this. Can you add a comment that explains what we need to do in the case where e.g. imageSize.width > 0 && imageSize.height == 0 && xRepeat == NS_STYLE_IMAGELAYER_REPEAT_ROUND ? Are you adding a reftest for that case?
Comment 102 Markus Stange [:mstange] 2016-05-09 17:09:00 PDT
Comment on attachment 8749582 [details] [diff] [review]
Part3. Handle rendering of background-repeat: space

Review of attachment 8749582 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +6835,5 @@
> + *   @return - returns the point on the axis where the two regions meet.
> + *             If the axis doesn't need to be split it returns the end point.
> + */
> +inline nscoord
> +BisectSpacingAxis(nscoord aImageSize, nscoord aAxisOffset, nscoord aAxisLength,

I don't really understand why we need all of this. Couldn't we, when we draw the image, just loop over the x and y repetitions and draw the image one by one, at round((totalSize - imageSize) / numberOfRepetitions)?
Comment 103 Markus Stange [:mstange] 2016-05-09 17:10:05 PDT
It would be easier for me to understand some decisions you made in the code if the patch came with reftests that require the behavior that you chose.
Comment 104 Ethan Lin[:ethlin] 2016-05-10 20:10:35 PDT
(In reply to Markus Stange [:mstange] from comment #102)
> Comment on attachment 8749582 [details] [diff] [review]
> Part3. Handle rendering of background-repeat: space
> 
> Review of attachment 8749582 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsLayoutUtils.cpp
> @@ +6835,5 @@
> > + *   @return - returns the point on the axis where the two regions meet.
> > + *             If the axis doesn't need to be split it returns the end point.
> > + */
> > +inline nscoord
> > +BisectSpacingAxis(nscoord aImageSize, nscoord aAxisOffset, nscoord aAxisLength,
> 
> I don't really understand why we need all of this. Couldn't we, when we draw
> the image, just loop over the x and y repetitions and draw the image one by
> one, at round((totalSize - imageSize) / numberOfRepetitions)?

For this part I used the original patch's calculations. I will try to simplify the calculations and draw the image one by one.
Comment 105 Ethan Lin[:ethlin] 2016-05-11 01:51:08 PDT
Created attachment 8751180 [details] [diff] [review]
Part2. Handle rendering of background-repeat: round/space

Addressed :mstange's comments. I simplified the calculation of spacing. I will add some test cases.
Comment 106 Ethan Lin[:ethlin] 2016-05-12 01:36:05 PDT
(In reply to Markus Stange [:mstange] from comment #101)
> Comment on attachment 8749581 [details] [diff] [review]
> Part2. Handle rendering of background-repeat: round
> 
> Review of attachment 8749581 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I haven't read the spec closely yet, so I'll need to do another review pass
> after I've done that and after my comments have been fixed.
> 
> This needs lots of reftests before it can land. Are you working on those?
> 
> ::: layout/base/nsCSSRendering.cpp
> @@ +2018,5 @@
> >    if (aDrawBackgroundColor &&
> > +      (bg->BottomLayer().mRepeat.mXRepeat & (NS_STYLE_IMAGELAYER_REPEAT_REPEAT |
> > +                                             NS_STYLE_IMAGELAYER_REPEAT_ROUND)) &&
> > +      (bg->BottomLayer().mRepeat.mYRepeat & (NS_STYLE_IMAGELAYER_REPEAT_REPEAT |
> > +                                             NS_STYLE_IMAGELAYER_REPEAT_ROUND)) &&
> 
> Uhh, the values of NS_STYLE_IMAGELAYER_REPEAT_* are not powers of two. I
> don't think this works.
> 
Right, I fixed it.

> @@ +3236,5 @@
> > + * http://dev.w3.org/csswg/css3-background/#the-background-size
> > + */
> > +static nscoord
> > +ComputeBackgroundRepeatRound(nscoord currentSize,
> > +                             nscoord positioningSize)
> 
> These arguments need the "a" prefix to conform with the style guide.
Okay.

> @@ +3239,5 @@
> > +ComputeBackgroundRepeatRound(nscoord currentSize,
> > +                             nscoord positioningSize)
> > +{
> > +  float x = currentSize;
> > +  float w = positioningSize;
> 
> I don't think there is value in using the variable names "x" and "w" to
> match the spec. The spec only uses them because it describes the horizontal
> dimension as an example.
> 
> How about this:
> 
> float repeatCount = NS_roundf(float(aPositioningSize) / float(aCurrentSize));
> if (repeatCount < 1.0f) {
>   return aPositioningSize;
> }
> return nscoord(NS_lround(float(aPositioningSize) / repeatCount)));
> 
Okay, thanks.

> @@ +3379,5 @@
> >                                                     bgPositionSize,
> >                                                     aLayer.mSize);
> > +  uint8_t xRepeat = aLayer.mRepeat.mXRepeat;
> > +  uint8_t yRepeat = aLayer.mRepeat.mYRepeat;
> > +  bool hasTwoRoundKeywords = xRepeat == NS_STYLE_IMAGELAYER_REPEAT_ROUND &&
> 
> Maybe call this isRepeatRoundInBothDimensions
> 
Okay, will do.

> @@ +3383,5 @@
> > +  bool hasTwoRoundKeywords = xRepeat == NS_STYLE_IMAGELAYER_REPEAT_ROUND &&
> > +                             yRepeat == NS_STYLE_IMAGELAYER_REPEAT_ROUND;
> > +
> > +  // calculate the rounded size only if the background-size computation
> > +  // returned a correct size for the image.
> 
> Why don't you instead move this below the early return?
> 
> I assume there's a reason for this. Can you add a comment that explains what
> we need to do in the case where e.g. imageSize.width > 0 && imageSize.height
> == 0 && xRepeat == NS_STYLE_IMAGELAYER_REPEAT_ROUND ? Are you adding a
> reftest for that case?

In my new patch, I move this calculation to ComputeDrawnSizeForBackground. That should be a better place not so confused.
Comment 107 Ethan Lin[:ethlin] 2016-05-12 02:21:06 PDT
Created attachment 8751647 [details] [diff] [review]
Part2. Handle rendering of background-repeat: round/space

Address :mstange's comments. I simplified some calculations, especially the space calculations. And I also use a simple loop to do the space rendering. If there is any logic needs test case to explain, I can write test cases for that.
Comment 108 Ethan Lin[:ethlin] 2016-05-12 02:24:00 PDT
Created attachment 8751648 [details] [diff] [review]
Part3. Add test case

I add test cases for round/space, including property_database.js.
Comment 109 Cameron McCormack (:heycam) 2016-05-12 17:42:38 PDT
Comment on attachment 8751648 [details] [diff] [review]
Part3. Add test case

Review of attachment 8751648 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the property_database.js change.  mstange, can you look at the reftests, since you're reviewing the functionality patch too?
Comment 110 Markus Stange [:mstange] 2016-05-12 17:43:49 PDT
Sure.
Comment 111 Cameron McCormack (:heycam) 2016-05-12 17:51:15 PDT
Ethan, could you send a mail to dev-platform with an "intent to implement (and ship soon)" mail for this?  I think it's fine for this feature to ship without a pref, since it's been in the spec for a while (at CR) and Chrome, Safari and Edge all support it.
Comment 112 Ethan Lin[:ethlin] 2016-05-16 02:39:13 PDT
(In reply to Cameron McCormack (:heycam) from comment #111)
> Ethan, could you send a mail to dev-platform with an "intent to implement
> (and ship soon)" mail for this?  I think it's fine for this feature to ship
> without a pref, since it's been in the spec for a while (at CR) and Chrome,
> Safari and Edge all support it.

Okay, I will do this.
Comment 113 Markus Stange [:mstange] 2016-05-16 14:15:36 PDT
Comment on attachment 8751648 [details] [diff] [review]
Part3. Add test case

Review of attachment 8751648 [details] [diff] [review]:
-----------------------------------------------------------------

We also need tests for the combinations of "space" in one dimension with "repeat" or "round" in the other dimension.
Comment 114 Markus Stange [:mstange] 2016-05-16 14:19:23 PDT
Comment on attachment 8751647 [details] [diff] [review]
Part2. Handle rendering of background-repeat: round/space

Review of attachment 8751647 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +2012,5 @@
>      }
>    }
>  
>    // We can skip painting the background color if a background image is opaque.
> +  bool xFullRepeat = bg->BottomLayer().mRepeat.mXRepeat == NS_STYLE_IMAGELAYER_REPEAT_REPEAT ||

Please store bg->BottomLayer().mRepeat instead of repeating it four times.

@@ +3199,5 @@
> + * property which can be found at
> + * http://dev.w3.org/csswg/css3-background/#the-background-size
> + */
> +static nscoord
> +ComputeBackgroundRepeatRound(nscoord aCurrentSize,

Let's call this ComputeRoundedSize. In the comment above, please mention that this returns the adjusted size of the background image.

@@ +3247,5 @@
> +    imageSize = nsImageRenderer::ComputeConcreteSize(specifiedSize,
> +                                                     aIntrinsicSize,
> +                                                     aBgPositioningArea);
> +  }
> +

I think it would be good idea to quote the spec in a comment at this point.

"If ‘background-repeat’ is ‘round’ for one (or both) dimensions, there is a second step. The UA must scale the image in that dimension (or both dimensions) so that it fits a whole number of times in the background positioning area."
"If ‘background-repeat’ is ‘round’ for one dimension only and if ‘background-size’ is ‘auto’ for the other dimension, then there is a third step: that other dimension is scaled so that the original aspect ratio is restored."

@@ +3259,5 @@
> +                                                   aBgPositioningArea.width);
> +    if (!isRepeatRoundInBothDimensions &&
> +        aLayerSize.mHeightType == nsStyleImageLayers::Size::DimensionType::eAuto) {
> +      // Restore intrinsic rato
> +      if (aIntrinsicSize.mRatio.height) {

I think you need to check mRatio.width instead, because that's what you're dividing by.

@@ +3262,5 @@
> +      // Restore intrinsic rato
> +      if (aIntrinsicSize.mRatio.height) {
> +        imageSize.height =
> +          NSCoordSaturatingNonnegativeMultiply(imageSize.width,
> +                                               double(aIntrinsicSize.mRatio.height) / aIntrinsicSize.mRatio.width);

NSCoordSaturatingNonnegativeMultiply accepts a float as the second argument, so this should also use float instead of double.
Try separating out the division into its own variable so that you don't overflow the line length limit by that much.

@@ +3275,5 @@
> +                                                    aBgPositioningArea.height);
> +    if (!isRepeatRoundInBothDimensions &&
> +        aLayerSize.mWidthType == nsStyleImageLayers::Size::DimensionType::eAuto) {
> +      // Restore intrinsic rato
> +      if (aIntrinsicSize.mRatio.width) {

same comment as above but with width&height reversed

@@ +3458,5 @@
>  
>    state.mDestArea = nsRect(imageTopLeft + aBorderArea.TopLeft(), imageSize);
>    state.mFillArea = state.mDestArea;
>    int repeatX = aLayer.mRepeat.mXRepeat;
>    int repeatY = aLayer.mRepeat.mYRepeat;

You can move these two variables up a little to before the call to ComputeDrawnSizeForBackground, and use them in that call.

@@ +3510,1 @@
>        repeatMode = ExtendMode::REPEAT;

This is wrong.

Say you have a 10x10 image in a 50x50 positioning area, with repeatX==REPEAT and repeatY==ROUND. The former sets repeatMode to REPEAT_X, and new you set repeatMode to REPEAT_Y. The image will end up only repeating vertically, won't it? Please add a test for this.

::: layout/base/nsCSSRendering.h
@@ +350,5 @@
> +  /**
> +   * mGapCount represents the number of gaps that mSpacing needs to be
> +   *           distributed across. Always equal to the number of images - 1.
> +   */
> +  nsIntSize mGapCount;

Instead of computing mSpacing and mGapCount, can you compute an nsSize mRepeatSize? You'd set that to the image size plus one gap, and that'll be the value you'll multiply by the index in the drawing loop.

I'm going to backpedal on my last comment about the rounding. I think rounding the gap size (or rather image size + gap size) to app units shouldn't be a problem. Or do you have a case where it creates problems?

::: layout/base/nsLayoutUtils.cpp
@@ +6894,5 @@
> +                             aDirty, &svgContext, aImageFlags, aExtendMode);
> +  }
> +
> +  /* Image spacing path */
> +  DrawResult result = DrawResult::SUCCESS;

The declaration of this variable can be moved down to the place where you set it.

@@ +6931,5 @@
> +      }
> +    }
> +  }
> +
> +  return result;

And then this can just return DrawResult::SUCCESS.
Comment 115 Markus Stange [:mstange] 2016-05-16 15:06:58 PDT
Comment on attachment 8751647 [details] [diff] [review]
Part2. Handle rendering of background-repeat: round/space

Review of attachment 8751647 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +6923,5 @@
> +      if (remainSpace.height) {
> +        dest.y += std::min(j, remainSpace.height) * nsPresContext::AppUnitsPerCSSPixel();
> +      }
> +      result = DrawImageInternal(aContext, aPresContext, aImage, aGraphicsFilter,
> +                                 dest, dest, aAnchor, dest, &svgContext,

The last "dest" needs to be aDirty. Otherwise you'll draw outside the dirty rect.
Comment 116 Ethan Lin[:ethlin] 2016-05-17 03:03:07 PDT
(In reply to Markus Stange [:mstange] from comment #114)
> Comment on attachment 8751647 [details] [diff] [review]
> Part2. Handle rendering of background-repeat: round/space
> 
> Review of attachment 8751647 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsCSSRendering.cpp
> @@ +2012,5 @@
> >      }
> >    }
> >  
> >    // We can skip painting the background color if a background image is opaque.
> > +  bool xFullRepeat = bg->BottomLayer().mRepeat.mXRepeat == NS_STYLE_IMAGELAYER_REPEAT_REPEAT ||
> 
> Please store bg->BottomLayer().mRepeat instead of repeating it four times.
> 
Okay.

> @@ +3199,5 @@
> > + * property which can be found at
> > + * http://dev.w3.org/csswg/css3-background/#the-background-size
> > + */
> > +static nscoord
> > +ComputeBackgroundRepeatRound(nscoord aCurrentSize,
> 
> Let's call this ComputeRoundedSize. In the comment above, please mention
> that this returns the adjusted size of the background image.
> 

Okay.

> @@ +3247,5 @@
> > +    imageSize = nsImageRenderer::ComputeConcreteSize(specifiedSize,
> > +                                                     aIntrinsicSize,
> > +                                                     aBgPositioningArea);
> > +  }
> > +
> 
> I think it would be good idea to quote the spec in a comment at this point.
> 
> "If ‘background-repeat’ is ‘round’ for one (or both) dimensions, there is a
> second step. The UA must scale the image in that dimension (or both
> dimensions) so that it fits a whole number of times in the background
> positioning area."
> "If ‘background-repeat’ is ‘round’ for one dimension only and if
> ‘background-size’ is ‘auto’ for the other dimension, then there is a third
> step: that other dimension is scaled so that the original aspect ratio is
> restored."
> 

I will add the comment here.

> @@ +3259,5 @@
> > +                                                   aBgPositioningArea.width);
> > +    if (!isRepeatRoundInBothDimensions &&
> > +        aLayerSize.mHeightType == nsStyleImageLayers::Size::DimensionType::eAuto) {
> > +      // Restore intrinsic rato
> > +      if (aIntrinsicSize.mRatio.height) {
> 
> I think you need to check mRatio.width instead, because that's what you're
> dividing by.

Right.

> 
> @@ +3262,5 @@
> > +      // Restore intrinsic rato
> > +      if (aIntrinsicSize.mRatio.height) {
> > +        imageSize.height =
> > +          NSCoordSaturatingNonnegativeMultiply(imageSize.width,
> > +                                               double(aIntrinsicSize.mRatio.height) / aIntrinsicSize.mRatio.width);
> 
> NSCoordSaturatingNonnegativeMultiply accepts a float as the second argument,
> so this should also use float instead of double.
> Try separating out the division into its own variable so that you don't
> overflow the line length limit by that much.
> 

Okay.

> @@ +3275,5 @@
> > +                                                    aBgPositioningArea.height);
> > +    if (!isRepeatRoundInBothDimensions &&
> > +        aLayerSize.mWidthType == nsStyleImageLayers::Size::DimensionType::eAuto) {
> > +      // Restore intrinsic rato
> > +      if (aIntrinsicSize.mRatio.width) {
> 
> same comment as above but with width&height reversed
> 
> @@ +3458,5 @@
> >  
> >    state.mDestArea = nsRect(imageTopLeft + aBorderArea.TopLeft(), imageSize);
> >    state.mFillArea = state.mDestArea;
> >    int repeatX = aLayer.mRepeat.mXRepeat;
> >    int repeatY = aLayer.mRepeat.mYRepeat;
> 
> You can move these two variables up a little to before the call to
> ComputeDrawnSizeForBackground, and use them in that call.
> 
> @@ +3510,1 @@
> >        repeatMode = ExtendMode::REPEAT;
> 
> This is wrong.
> 
> Say you have a 10x10 image in a 50x50 positioning area, with repeatX==REPEAT
> and repeatY==ROUND. The former sets repeatMode to REPEAT_X, and new you set
> repeatMode to REPEAT_Y. The image will end up only repeating vertically,
> won't it? Please add a test for this.
>

Right. I'll add a test case for it.  

> ::: layout/base/nsCSSRendering.h
> @@ +350,5 @@
> > +  /**
> > +   * mGapCount represents the number of gaps that mSpacing needs to be
> > +   *           distributed across. Always equal to the number of images - 1.
> > +   */
> > +  nsIntSize mGapCount;
> 
> Instead of computing mSpacing and mGapCount, can you compute an nsSize
> mRepeatSize? You'd set that to the image size plus one gap, and that'll be
> the value you'll multiply by the index in the drawing loop.
> 
> I'm going to backpedal on my last comment about the rounding. I think
> rounding the gap size (or rather image size + gap size) to app units
> shouldn't be a problem. Or do you have a case where it creates problems?
> 

There is no problem. I will use mRepeatSize to replace mSpacing and mGapCount.

> ::: layout/base/nsLayoutUtils.cpp
> @@ +6894,5 @@
> > +                             aDirty, &svgContext, aImageFlags, aExtendMode);
> > +  }
> > +
> > +  /* Image spacing path */
> > +  DrawResult result = DrawResult::SUCCESS;
> 
> The declaration of this variable can be moved down to the place where you
> set it.
> 
> @@ +6931,5 @@
> > +      }
> > +    }
> > +  }
> > +
> > +  return result;
> 
> And then this can just return DrawResult::SUCCESS.

Okay, I will change it.
Comment 117 Ethan Lin[:ethlin] 2016-05-17 03:07:40 PDT
Created attachment 8753300 [details] [diff] [review]
Part2. Handle rendering of background-repeat: round/space

Address :mstange's comments.
Comment 118 Ethan Lin[:ethlin] 2016-05-17 03:11:01 PDT
Created attachment 8753306 [details] [diff] [review]
Part3. Add test case for round / space (carry r+: heycam)

I add some more test cases for 'round repeat', 'repeat round', 'space round', 'round space', 'space repeat', 'repeat space'.
Comment 119 Markus Stange [:mstange] 2016-05-17 10:04:48 PDT
Comment on attachment 8753300 [details] [diff] [review]
Part2. Handle rendering of background-repeat: round/space

Review of attachment 8753300 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! r+ with the rest of the comments addressed.

::: layout/base/nsCSSRendering.cpp
@@ +3296,5 @@
> + */
> +static void
> +ComputeBackgroundRepeatSpacing(nscoord aImageDimension,
> +                               nscoord aAvailableSpace,
> +                               nscoord& aRepeatSize) {

Let's call this ComputeSpacedRepeatSize, and return the repeat size instead of using an outparam.

@@ +3300,5 @@
> +                               nscoord& aRepeatSize) {
> +  float ratio = aAvailableSpace / aImageDimension;
> +
> +  if (ratio < 2.0f) { // if you can't repeat at least twice, then don't repeat.
> +    aRepeatSize = 0;

I'd prefer this case to return aImageDimension. The caller can check for the repeat size being larger than the image size instead of checking whether it's greater than zero.
And then you won't need the breaks in the drawing loop later.

::: layout/base/nsCSSRendering.h
@@ +339,5 @@
>     */
>    nsPoint mAnchor;
> +  /**
> +   * The background-repeat property space keyword computation the
> +   * repeat size which is image size plus spacing.

This is not a real sentence. Maybe you wanted "computes" instead of "computation"?

::: layout/base/nsLayoutUtils.cpp
@@ +6892,5 @@
> +                             aGraphicsFilter, aDest, aFill, aAnchor,
> +                             aDirty, &svgContext, aImageFlags, aExtendMode);
> +  }
> +
> +  for (int32_t i = aFill.x; i < aFill.x + aFill.width; i += aRepeatSize.width) {

You can replace aFill.x + aFill.width with aFill.XMost(). Same for y.

@@ +6899,5 @@
> +      DrawResult result = DrawImageInternal(aContext, aPresContext, aImage, aGraphicsFilter,
> +                                            dest, dest, aAnchor, aDirty, &svgContext,
> +                                            aImageFlags, ExtendMode::CLAMP);
> +      if (result != DrawResult::SUCCESS)
> +        return result;

Please add braces.
Comment 120 Markus Stange [:mstange] 2016-05-17 12:12:29 PDT
Comment on attachment 8753306 [details] [diff] [review]
Part3. Add test case for round / space (carry r+: heycam)

Review of attachment 8753306 [details] [diff] [review]:
-----------------------------------------------------------------

I wonder if these tests could be simpler if you used flexbox instead of floats. Flexbox has built-in mechanisms to distribute space evenly if the children don't fill the box completely, and you'd be able to reuse code between the horizontal and the vertical tests simply by changing flex-flow from "row nowrap" to "column nowrap".

::: layout/reftests/w3c-css/submitted/background/background-repeat-round-1-ref.html
@@ +5,5 @@
> +    <title>CSS Background: background-repeat: background image repeat</title>
> +    <link rel="author" title="Ethan Lin" href="mailto:ethlin@mozilla.com">
> +    <link rel="author" title="Mozilla" href="https://www.mozilla.org">
> +    <style type="text/css">
> +      #outer {

You have multiple elements with this ID. It works but it's not great, please use classes instead. So you'll have <div class="outer"> and .outer {...} .

@@ +8,5 @@
> +    <style type="text/css">
> +      #outer {
> +        width: 72px;
> +        height: 72px;
> +        border: 1px solid black;

You could apply this border to .inner, then you wouldn't need the outer elements. Oh, and you'd need to set background-origin and background-clip to padding-box for equal results.

::: layout/reftests/w3c-css/submitted/background/background-repeat-round-1a.html
@@ +25,5 @@
> +  <body>
> +    <div id="outer"><div id="inner"></div></div>
> +    <div id="outer"><div id="inner"></div></div>
> +    <div id="outer"><div id="inner"></div></div>
> +    <div id="outer"><div id="inner"></div></div>

Why is this repeated four times? I don't see a difference between these elements in any of the tests.

::: layout/reftests/w3c-css/submitted/background/background-repeat-round-2-ref.html
@@ +13,5 @@
> +    }
> +    #inner {
> +      top: 0px;
> +      height: 36px;
> +      float: top;

Both "top: 0px" and "float: top" don't do anything; the latter is invalid and the former doesn't have any effect unless you set position to a non-default value.
Comment 121 Ethan Lin[:ethlin] 2016-05-18 02:08:39 PDT
Created attachment 8753735 [details] [diff] [review]
Part2. Handle rendering of background-repeat: round/space (carry r+: mstange)

Address :mstange's comments.
Comment 122 Ethan Lin[:ethlin] 2016-05-18 02:12:52 PDT
Created attachment 8753747 [details] [diff] [review]
Part3. Add test case for round / space (carry r+: heycam)

I remove all redundant div to simplify test cases. I also use display:flex to create the reference for space.
Comment 123 Ethan Lin[:ethlin] 2016-05-18 02:20:10 PDT
Comment on attachment 8753735 [details] [diff] [review]
Part2. Handle rendering of background-repeat: round/space (carry r+: mstange)

Review of attachment 8753735 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +6893,5 @@
> +                             aDirty, &svgContext, aImageFlags, aExtendMode);
> +  }
> +
> +  for (int32_t i = aDest.x; i < aFill.XMost(); i += aRepeatSize.width) {
> +    for (int32_t j = aDest.y; j < aFill.YMost(); j += aRepeatSize.height) {

Markus,
There was a border overlap problem when I tried my test cases in Part3. So I changed the initial value to aDest.x/aDest.y from aFill.x/aFill.y. Please let me know if you think it's wrong.
Comment 124 Ethan Lin[:ethlin] 2016-05-18 02:28:06 PDT
Created attachment 8753755 [details] [diff] [review]
Part2. Handle rendering of background-repeat: round/space (carry r+: mstange)

Attached wrong patch. Correct it.
Comment 125 Ethan Lin[:ethlin] 2016-05-18 02:31:19 PDT
Created attachment 8753761 [details] [diff] [review]
Part3. Add test case for round / space (carry r+: heycam)

Attach correct patch.
In this patch I remove all redundant div to simplify test cases. I also use display:flex to create the reference for space.
Comment 126 Markus Stange [:mstange] 2016-05-18 10:18:40 PDT
Comment on attachment 8753761 [details] [diff] [review]
Part3. Add test case for round / space (carry r+: heycam)

Review of attachment 8753761 [details] [diff] [review]:
-----------------------------------------------------------------

These tests are looking quite good now, thanks.

I found one more bug with your patch that I'd like you to add a reftest for. Testcase upcoming.
Comment 127 Markus Stange [:mstange] 2016-05-18 10:39:52 PDT
Created attachment 8753957 [details]
testcase that shows that we need to fill the fill rect even to the top/left of the dest rect

(In reply to Ethan Lin[:ethlin] from comment #123)
> Comment on attachment 8753735 [details] [diff] [review]
> Part2. Handle rendering of background-repeat: round/space (carry r+: mstange)
> 
> Review of attachment 8753735 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsLayoutUtils.cpp
> @@ +6893,5 @@
> > +                             aDirty, &svgContext, aImageFlags, aExtendMode);
> > +  }
> > +
> > +  for (int32_t i = aDest.x; i < aFill.XMost(); i += aRepeatSize.width) {
> > +    for (int32_t j = aDest.y; j < aFill.YMost(); j += aRepeatSize.height) {
> 
> Markus,
> There was a border overlap problem when I tried my test cases in Part3. So I
> changed the initial value to aDest.x/aDest.y from aFill.x/aFill.y. Please
> let me know if you think it's wrong.

This is more correct than what we had before, but still not completely correct.

And your tests caught it because you were adding a border without adding background-clip: padding-box, so the background now gets repeated under the border. (That's why I suggested background-clip: padding-box in my review comment, but it's good that you didn't do it because it made you find this bug.)

It turns out that the fill rect can extend beyond the dest rect even to the top left, as shown in this testcase. We need to repeat into those areas, too.

You can compute the first tile that intersects the fill rect like this:
nsPoint firstTilePos = aDest.TopLeft +
  nsPoint(floor(double(aFill.x - aDest.x) / aRepeatSize.width)) * aRepeatSize.width,
          floor(double(aFill.y - aDest.y) / aRepeatSize.height)) * aRepeatSize.height);
Comment 128 Markus Stange [:mstange] 2016-05-18 10:41:25 PDT
That was supposed to be "aDest.TopLeft()".
Comment 129 Ethan Lin[:ethlin] 2016-05-18 23:09:41 PDT
Created attachment 8754222 [details] [diff] [review]
Part2. Handle rendering of background-repeat: round/space (carry r+: mstange)

Address :mstange's comment. Fix the tile position.
Comment 130 Ethan Lin[:ethlin] 2016-05-18 23:16:36 PDT
Created attachment 8754224 [details] [diff] [review]
Part3. Add test case for round / space (carry r+: heycam, mstange)

Address mstange's comment. Add a test case for space with translucent border.
Comment 131 Ethan Lin[:ethlin] 2016-05-18 23:46:02 PDT
Comment on attachment 8754224 [details] [diff] [review]
Part3. Add test case for round / space (carry r+: heycam, mstange)

I added a new test case 'background-repeat-space-8' for the border problem.
Comment 132 Markus Stange [:mstange] 2016-05-19 11:11:22 PDT
Comment on attachment 8754222 [details] [diff] [review]
Part2. Handle rendering of background-repeat: round/space (carry r+: mstange)

Review of attachment 8754222 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.h
@@ +1716,5 @@
>    /**
>     * Draw a background image.  The image's dimensions are as specified in aDest;
>     * the image itself is not consulted to determine a size.
>     * See https://wiki.mozilla.org/Gecko:Image_Snapping_and_Rendering
> +   * When aGapCount and aSpacing are provided the image is

This comment needs to be fixed.

@@ +1740,3 @@
>     *   @param aFill             The area to be filled with copies of the image.
> +   *   @param aRepeatSize       The number of app units that can be used for repeating
> +   *                            image with space.

I'd say "The distance between the positions of two subsequent repeats of the image. Sizes larger than aDest.Size() create gaps between the images."
Comment 133 Markus Stange [:mstange] 2016-05-19 11:12:40 PDT
Comment on attachment 8754224 [details] [diff] [review]
Part3. Add test case for round / space (carry r+: heycam, mstange)

Review of attachment 8754224 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Comment 134 Ethan Lin[:ethlin] 2016-05-19 19:32:31 PDT
Created attachment 8754621 [details] [diff] [review]
Part2. Handle rendering of background-repeat: round/space (carry r+: mstange)

Fix comments.
Comment 135 Ethan Lin[:ethlin] 2016-05-22 18:54:46 PDT
Created attachment 8755246 [details] [diff] [review]
Par4. Add 'round, space' in shorthand property test

There was a try server fail in shorthand property test. So I add round, space in the test.
Comment 137 Carsten Book [:Tomcat] 2016-05-24 08:36:57 PDT
part 2 failed to apply:

1 out of 8 hunks FAILED -- saving rejects to file layout/base/nsCSSRendering.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug54372-p2.patch
Tomcats-MacBook-Pro-2:mozilla-inbound Tomcat$
Comment 138 Ethan Lin[:ethlin] 2016-05-25 09:29:28 PDT
Created attachment 8756255 [details] [diff] [review]
Part2. Handle rendering of background-repeat: round/space (carry r+: mstange)

Rebase the patch.
Comment 141 Sebastian Zartner [:sebo] 2016-05-26 16:28:21 PDT
Updated the compatibility info here:
https://developer.mozilla.org/en-US/docs/Web/CSS/background-repeat

And added a note about it here:
https://developer.mozilla.org/en-US/Firefox/Releases/49

Sebastian
Comment 142 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2016-05-27 23:38:52 PDT
These reftests in layout/reftests/w3c-css/submitted/ don't have the link rel="match" that will cause them to be recognized as reftests by the W3C harness:

Missing link for ./background/background-repeat-space-2.html
Missing link for ./background/background-repeat-space-3.html
Missing link for ./background/background-repeat-space-4.html
Missing link for ./background/background-repeat-space-5.html
Missing link for ./background/background-repeat-space-6.html
Missing link for ./background/background-repeat-space-7.html
Missing link for ./background/background-repeat-space-8.html
Comment 143 Ethan Lin[:ethlin] 2016-05-28 02:39:08 PDT
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #142)
> These reftests in layout/reftests/w3c-css/submitted/ don't have the link
> rel="match" that will cause them to be recognized as reftests by the W3C
> harness:
> 
> Missing link for ./background/background-repeat-space-2.html
> Missing link for ./background/background-repeat-space-3.html
> Missing link for ./background/background-repeat-space-4.html
> Missing link for ./background/background-repeat-space-5.html
> Missing link for ./background/background-repeat-space-6.html
> Missing link for ./background/background-repeat-space-7.html
> Missing link for ./background/background-repeat-space-8.html

Sorry for that. I will take care the reftest content next time. Patch is upcoming.
Comment 144 Ethan Lin[:ethlin] 2016-05-28 03:02:23 PDT
I open bug 1276432 to fix the wrong links.

Note You need to log in before you can comment on or make changes to this bug.