Jeff Walden [:Waldo] Reporter
	Description • 14 years ago

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.

	Lazar Sumar [:nonsensickle]
	Comment 6 • 13 years ago

Marc, are you working on this currently? If not, I would like to take it.

	Lazar Sumar [:nonsensickle]
	Comment 7 • 13 years ago

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.

	Lazar Sumar [:nonsensickle]
	Comment 8 • 13 years ago

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 :)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 9 • 12 years ago

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.

	Lazar Sumar [:nonsensickle]
	Comment 10 • 12 years ago

> 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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 11 • 12 years ago

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.

	Lazar Sumar [:nonsensickle]
	Comment 12 • 12 years ago

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

done.

	Lazar Sumar [:nonsensickle]
	Comment 13 • 12 years ago

(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.

	Lazar Sumar [:nonsensickle]
	Comment 14 • 12 years ago

Added dependence on bug 548375 because it will be easier to make some changes on top of the patch I already did there.

	Lazar Sumar [:nonsensickle]
	Comment 15 • 12 years ago

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?

	Lazar Sumar [:nonsensickle]
	Comment 16 • 12 years ago

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...

	Lazar Sumar [:nonsensickle]
	Comment 17 • 12 years ago

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.

	Lazar Sumar [:nonsensickle]
	Comment 18 • 12 years ago

Math error. Rounding up...

	Lazar Sumar [:nonsensickle]
	Comment 19 • 12 years ago

Wrong file. This is v5.

	Lazar Sumar [:nonsensickle]
	Comment 20 • 12 years ago

Proposed implementation of 'round' keyword rendering. 
Passes local reftests for the round keyword. Fails reftests for space keyword.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 21 • 12 years ago

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

	Lazar Sumar [:nonsensickle]
	Comment 22 • 12 years ago

(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.

	Lazar Sumar [:nonsensickle]
	Comment 23 • 12 years ago

It's better style not to evaluate floats (even if zero) for assertions.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 24 • 12 years ago

(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 |.

	Lazar Sumar [:nonsensickle]
	Comment 25 • 12 years ago

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.

	Lazar Sumar [:nonsensickle]
	Comment 26 • 12 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 27 • 12 years ago

(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.

	Lazar Sumar [:nonsensickle]
	Comment 28 • 12 years ago

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.

	Jeff Walden [:Waldo] Reporter
	Comment 29 • 12 years ago

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.

	Lazar Sumar [:nonsensickle]
	Comment 30 • 12 years ago

(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.

	Lazar Sumar [:nonsensickle]
	Comment 31 • 12 years ago

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?

	Lazar Sumar [:nonsensickle]
	Comment 32 • 12 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 33 • 12 years ago

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.

	Lazar Sumar [:nonsensickle]
	Comment 34 • 12 years ago

(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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 35 • 12 years ago

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?

	Lazar Sumar [:nonsensickle]
	Comment 36 • 12 years ago

> 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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 37 • 12 years ago

(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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 38 • 12 years ago

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)

	Lazar Sumar [:nonsensickle]
	Comment 39 • 12 years ago

(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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 40 • 12 years ago

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?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 41 • 12 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 42 • 12 years ago

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

	Lazar Sumar [:nonsensickle]
	Comment 43 • 12 years ago

(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.

	Lazar Sumar [:nonsensickle]
	Comment 44 • 12 years ago

(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.

	Lazar Sumar [:nonsensickle]
	Comment 45 • 12 years ago

(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.

	Lazar Sumar [:nonsensickle]
	Comment 47 • 12 years ago

(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).

	Lazar Sumar [:nonsensickle]
	Comment 48 • 12 years ago

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?

	Lazar Sumar [:nonsensickle]
	Comment 49 • 12 years ago

Rebasing for changes in bug 548375 attachment #600254 [details] [diff] [review].

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 50 • 12 years ago

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

	Lazar Sumar [:nonsensickle]
	Comment 51 • 12 years ago

(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.

	Lazar Sumar [:nonsensickle]
	Comment 52 • 12 years ago

(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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 53 • 12 years ago

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.

	Lazar Sumar [:nonsensickle]
	Comment 54 • 11 years ago

Removing bitrot.

	Lazar Sumar [:nonsensickle]
	Comment 55 • 11 years ago

Removing bitrot.

	Lazar Sumar [:nonsensickle]
	Comment 56 • 11 years ago

Removing bitrot.

	Lazar Sumar [:nonsensickle]
	Comment 57 • 11 years ago

Removing bitrot.

	Lazar Sumar [:nonsensickle]
	Comment 58 • 11 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 59 • 11 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 60 • 11 years ago

Thanks for updating the patches!!!

	Lazar Sumar [:nonsensickle]
	Comment 61 • 11 years ago

Passes reftests

Still need to address Roc's comments from 2012-06-26.

	Lazar Sumar [:nonsensickle]
	Comment 62 • 11 years ago

(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.

	Lazar Sumar [:nonsensickle]
	Comment 63 • 11 years ago

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.

	Lazar Sumar [:nonsensickle]
	Comment 64 • 11 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 65 • 11 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 66 • 11 years ago

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.

	Lazar Sumar [:nonsensickle]
	Comment 67 • 11 years ago

(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+

	Lazar Sumar [:nonsensickle]
	Comment 68 • 11 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 69 • 11 years ago

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)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 70 • 11 years ago

(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.

	Lazar Sumar [:nonsensickle]
	Comment 71 • 11 years ago

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.

	Florian Bender
	Comment 72 • 10 years ago

Lazar, do you still want to fix this? Otherwise I'd reset the assignee so somebody else can volunteer.

	Lazar Sumar [:nonsensickle]
	Comment 73 • 10 years ago

(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.

	Florian Bender
	Comment 74 • 10 years ago

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.

	Lazar Sumar [:nonsensickle]
	Comment 75 • 10 years ago

(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.

	Lazar Sumar [:nonsensickle]
	Comment 78 • 10 years ago

(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.

	Florian Bender
	Comment 79 • 10 years ago

Lazar, did you have a change to look at this again? :)

	Lazar Sumar [:nonsensickle]
	Comment 80 • 10 years ago

(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.

	Lazar Sumar [:nonsensickle]
	Comment 81 • 10 years ago

Florian, are you aware of any other bugs that will be changing the background image code in the near future?

	Florian Bender
	Comment 82 • 10 years ago

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.

	Seth Fowler [:seth] [:s2h]
	Comment 83 • 9 years ago

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.

	Mark Straver
	Comment 84 • 9 years ago

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...

	yiorsi
	Comment 85 • 9 years ago

No one to fix it?

	Mark Straver
	Comment 87 • 9 years ago

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?

	Mark Straver
	Comment 88 • 9 years ago

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?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 89 • 9 years ago

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.

	Mark Straver
	Comment 90 • 9 years ago

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.

	Lazar Sumar [:nonsensickle]
	Comment 91 • 8 years ago

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.

	Ethan Lin[:ethlin] Assignee
	Comment 92 • 8 years ago

I will try to study this bug.

	yisibl
	Comment 93 • 8 years ago

(In reply to Ethan Lin[:ethlin] from comment #92)
> I will try to study this bug.

Good news!

	Ethan Lin[:ethlin] Assignee
	Comment 94 • 8 years ago

Test cases.

	Ethan Lin[:ethlin] Assignee
	Comment 95 • 8 years ago

Add round and space values to background-repeat.

	Ethan Lin[:ethlin] Assignee
	Comment 96 • 8 years ago

Handle rendering if round.

	Ethan Lin[:ethlin] Assignee
	Comment 97 • 8 years ago

Handle rendering of space.

	Astley Chen (inactive)
	Comment 98 • 8 years ago

Change status to assigned since Ethan Lin is now working on this bug.

	Cameron McCormack (:heycam)
	Comment 99 • 8 years ago

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).

	yisibl
	Comment 100 • 8 years ago

Implement border-image-repeat space keyword
https://bugzilla.mozilla.org/show_bug.cgi?id=720531

	Markus Stange [:mstange]
	Comment 101 • 8 years ago

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?

	Markus Stange [:mstange]
	Comment 102 • 8 years ago

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)?

	Markus Stange [:mstange]
	Comment 103 • 8 years ago

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.

	Ethan Lin[:ethlin] Assignee
	Comment 104 • 8 years ago

(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.

	Ethan Lin[:ethlin] Assignee
	Comment 105 • 8 years ago

Addressed :mstange's comments. I simplified the calculation of spacing. I will add some test cases.

	Ethan Lin[:ethlin] Assignee
	Comment 106 • 8 years ago

(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.

	Ethan Lin[:ethlin] Assignee
	Comment 107 • 8 years ago

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.

	Ethan Lin[:ethlin] Assignee
	Comment 108 • 8 years ago

I add test cases for round/space, including property_database.js.

	Cameron McCormack (:heycam)
	Comment 109 • 8 years ago

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?

	Markus Stange [:mstange]
	Comment 110 • 8 years ago

Sure.

	Cameron McCormack (:heycam)
	Comment 111 • 8 years ago

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.

	Ethan Lin[:ethlin] Assignee
	Comment 112 • 8 years ago

(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.

	Markus Stange [:mstange]
	Comment 113 • 8 years ago

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.

	Markus Stange [:mstange]
	Comment 114 • 8 years ago

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.

	Markus Stange [:mstange]
	Comment 115 • 8 years ago

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.

	Ethan Lin[:ethlin] Assignee
	Comment 116 • 8 years ago

(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.

	Ethan Lin[:ethlin] Assignee
	Comment 117 • 8 years ago

Address :mstange's comments.

	Ethan Lin[:ethlin] Assignee
	Comment 118 • 8 years ago

I add some more test cases for 'round repeat', 'repeat round', 'space round', 'round space', 'space repeat', 'repeat space'.

	Markus Stange [:mstange]
	Comment 119 • 8 years ago

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.

	Markus Stange [:mstange]
	Comment 120 • 8 years ago

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.

	Ethan Lin[:ethlin] Assignee
	Comment 121 • 8 years ago

Address :mstange's comments.

	Ethan Lin[:ethlin] Assignee
	Comment 122 • 8 years ago

I remove all redundant div to simplify test cases. I also use display:flex to create the reference for space.

	Ethan Lin[:ethlin] Assignee
	Comment 123 • 8 years ago

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.

	Ethan Lin[:ethlin] Assignee
	Comment 124 • 8 years ago

Attached wrong patch. Correct it.

	Ethan Lin[:ethlin] Assignee
	Comment 125 • 8 years ago

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.

	Markus Stange [:mstange]
	Comment 126 • 8 years ago

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.

	Markus Stange [:mstange]
	Comment 127 • 8 years ago

(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);

	Markus Stange [:mstange]
	Comment 128 • 8 years ago

That was supposed to be "aDest.TopLeft()".

	Ethan Lin[:ethlin] Assignee
	Comment 129 • 8 years ago

Address :mstange's comment. Fix the tile position.

	Ethan Lin[:ethlin] Assignee
	Comment 130 • 8 years ago

Address mstange's comment. Add a test case for space with translucent border.

	Ethan Lin[:ethlin] Assignee
	Comment 131 • 8 years ago

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.

	Markus Stange [:mstange]
	Comment 132 • 8 years ago

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."

	Markus Stange [:mstange]
	Comment 133 • 8 years ago

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!

	Ethan Lin[:ethlin] Assignee
	Comment 134 • 8 years ago

Fix comments.

	Ethan Lin[:ethlin] Assignee
	Comment 135 • 8 years ago

There was a try server fail in shorthand property test. So I add round, space in the test.

	Ethan Lin[:ethlin] Assignee
	Comment 136 • 8 years ago

try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=48d20110c5d3&selectedJob=21205971

	Carsten Book [:Tomcat]
	Comment 137 • 8 years ago

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$

	Ethan Lin[:ethlin] Assignee
	Comment 138 • 8 years ago

Rebase the patch.

	Pulsebot
	Comment 139 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/424985ac2faf
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a8d924408cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd8dfedc0f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b9e115d053a

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 140 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/424985ac2faf
https://hg.mozilla.org/mozilla-central/rev/2a8d924408cc
https://hg.mozilla.org/mozilla-central/rev/1bd8dfedc0f6
https://hg.mozilla.org/mozilla-central/rev/3b9e115d053a

	Sebastian Zartner [:sebo]
	Comment 141 • 8 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 142 • 8 years ago

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

	Ethan Lin[:ethlin] Assignee
	Comment 143 • 8 years ago

(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.

	Ethan Lin[:ethlin] Assignee
	Comment 144 • 8 years ago

I open bug 1276432 to fix the wrong links.