189519 - (background-size) Implement CSS3's background-size

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

CSS3 will need this ability for the 'background-size' property.

Comment 1

[ArronM (:paper)]

I'm willing (and planning) on doing the Windows end of this.  Specifically, a
TileScaledImage function in nsImageWin.  We should really consider caching
scaled images.  No exactly blocking this bug, but without it, it's going to be
sloooow rendering.

Comment 2

[John Mellor (Jomel)]

This would be useful for extensions to make theme-independent layout changes (and could be handy for themes too).

Comment 3

[Andrés Delfino]

Whiteboard should include: [parity-Konqueror] [parity-Opera] [parity-Safari].

Comment 4

[Eric Wahlforss]

This is a super-useful property. very much hoping that this will be implemented in firefox soon.

Comment 5

[Jez]

Adding self to CC list.  I'd like to see this implemented.  Obviously one can already size IMGs so you'd think this would be relatively straightforward to implement.

Comment 6

[Jeff Walden [:Waldo]]

Staking out a claim to implementing this once bug 322475 is complete and most likely once I finish up a few other things I need to do, but no guarantees.

For the moment, any person whose initials are or are not MV is warned away from this bug -- go find something else to implement.  :-P

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Something that doesn't crash (sometimes, and asserts like mad regardless) and doesn't attempt to make rendering correct

Here's a checkpoint of what I have hacked together so far.  It just attempts to implement parsing and value storage without actually hooking up those values to actual rendering, but it's still quite buggy in that respect.  Among others this patch (atop 0c250e6e2062) results in the following assertions scads of times during use:

###!!! ASSERTION: Null pointer while computing size: 'val', file /Users/jwalden/moz/background-size/layout/style/nsCSSDataBlock.cpp, line 746
###!!! ASSERTION: Null pointer while compressing: 'val', file /Users/jwalden/moz/background-size/layout/style/nsCSSDataBlock.cpp, line 852
###!!! ASSERTION: oops: 'val', file /Users/jwalden/moz/background-size/layout/style/nsCSSDataBlock.cpp, line 325
###!!! ASSERTION: oops: 'val', file /Users/jwalden/moz/background-size/layout/style/nsCSSDataBlock.cpp, line 549

layout/style/test/test_value_storage.html makes it crash, presumably because it's actually trying to use the mis-initialized -moz-background-size data that's hitting the unexpected null pointers noted in the assertions (I've verified for several of those that they occur on data for that property).

Comment 8

[Jeff Walden [:Waldo]]

This obvious (in hindsight) diff fixes the asserts, for anyone else who wants to play along with a patch that doesn't work (thanks to bz for the eye on what was causing the error):

diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
--- a/layout/style/nsCSSParser.cpp
+++ b/layout/style/nsCSSParser.cpp
@@ -6014,16 +6014,17 @@ CSSParserImpl::ParseBackground()
     if (!bgitem.mLastItem && ExpectSymbol(',', PR_TRUE)) {
       continue;
     }
     if (!ExpectEndProperty()) {
       break;
     }
 
     mTempData.mColor.mBackPosition = positionHead;
+    mTempData.mColor.mBackSize = sizeHead;
     for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(simpleValues); ++i) {
       nsCSSValueList **source = static_cast<nsCSSValueList**>(
         mTempData.PropertyAt(simpleValues[i].propID));
       *source = simpleHeads[i];
     }
 
     mTempData.SetPropertyBit(eCSSProperty_background_image);
     mTempData.SetPropertyBit(eCSSProperty_background_repeat);

Comment 9

[Jeff Walden [:Waldo]]

Attachment: WIP v2

Haven't started the rendering part yet, want to get the DOM and style rule parts correct first -- but there are still a handful of failures to address that happily all look like the same single failure.  This test, unmodified or reduced as in this patch, is representative of that failure:

layout/style/test/test_compute_data_with_start_struct.html

Not sure what I've missed here, still looking...

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Secondary patch to implement rendering portion

I changed my mind and went ahead and hacked rendering together a bit.  :-)

This falls over hard as we clip the image-rendering area after painting the background color (so our background painting area for the image doesn't match the background painting area defined by CSS3), 'cover' may or may not work fully as specified, my handling of an omitted second size is likely whack, and there's probably still more wrong, but this works at least a little bit in minor javascript:-based testing!  Probably time to start writing reftests and get a spec clarification or two as needed...

0. data:text/html,%3Cstyle%3E%20body%20%7B%20background%3A%20url(http%3A%2F%2Fwhereswalden.com%2Fwp-content%2Fuploads%2F2009%2F01%2F0105.jpg)%20contain%20no-repeat%20fixed%3B%20%7D%20%3C%2Fstyle%3E

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Round one: fight!

Okay, I think this is good to go finally.  It's got a slew of reftests and a bunch of CSS DOM tests (property_database.js and friends are awesome), and everything checks out when I compare against a webkit nightly (with appropriate s/-moz-/-webkit-/), as far as that goes -- it doesn't look like they implement contain/cover.

This includes some effort to parse background-size as part of the background shorthand, but that part of the spec's in flux, and I implemented a slight misreading of the spec (in particular, '/' isn't necessary before an item that can be unambiguously parsed as size, in which case no position is allowed in the shorthand, and '/ size' must immediately follow position if it's present at all -- spec isn't clear about immediately or merely after) before noticing it wanted something different.  I don't know what we'd want to do about a fluxy shorthand syntax with respect to this patch; multiple backgrounds does suggest some precedent for landing it and adjusting as needed later, 

The reftests themselves are probably the most painful part of this, and I could easily have made mistakes in some of them.  I'm not sure what there is to do about making it easier to review so many tests for a property that has semi-intricate interactions, unfortunately.  Feel free to suggest someone else if that optimizes everyone's time better...

Comment 12

[Boris Zbarsky [:bzbarsky]]

dbaron is probably a better reviewer for this; if nothing else he might actually be familiar with the spec here, which I'm not.

That said, you might want to have roc look over the NSCoordMultiply changes.  They look somewhat wrong to me:

1) The return value in the float case still needs to be an integer-valued float, since we don't plan to allow non-integral nscoord values even when we use a float for it.
2) It's changing the rounding behavior in the integer case if aCoord is negative.  The function currently truncates towards zero, while your code would floor.  Was this a purposeful change?

And in all cases, I'm not sure you want to do the multiple floorf() calls that PR_MIN will do for you there...  I really wish it were a template function instead of a macro.  :(

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Round two

(In reply to comment #12)
> dbaron is probably a better reviewer for this[...]  That said, you might want to have roc look over the NSCoordMultiply changes. 

Okay to both.

> 1) The return value in the float case still needs to be an integer-valued
> float, since we don't plan to allow non-integral nscoord values even when we
> use a float for it.

Hm, didn't know that, changed.

> 2) It's changing the rounding behavior in the integer case if aCoord is
> negative.  The function currently truncates towards zero, while your code
> would floor.  Was this a purposeful change?

It wasn't purposeful, but it's also not a "change", because NSCoordMultiply is unused, and this patch renames the method to indicate saturating functionality -- so being compatible with previous (versus "adjacent") behavior isn't an issue.

I've noticed further differences between what I implemented and what I intended to implement with respect to the background shorthand, so I ripped out support for specifying background-size via the shorthand; it can be added when a final syntax is determined.  In the meantime we're no worse than any other browser here, as neither WebKit nor Opera support a CSS spec revision regarding background-size that's all that recent (contain/cover aren't implemented, for example, and a single size is handled differently from how it's currently specified) and at least the former (didn't check latter) omits size from the shorthand.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

dbaron will have to review the style system changes.

+  NS_ABORT_IF_FALSE(mType < Dimension::eCount, "bad mType for this");
+  NS_ABORT_IF_FALSE(aOther.mType < Dimension::eCount, "bad mType for aOther");

Nothing particularly serious is going to happen if these asserts go wrong. Can we make them NS_ASSERTION so that if things do go wrong, we'll be able to keep on running the test suite?

+  struct Size;
+  friend struct Size;
+  struct Size {

You can just put "friend struct Size" after "struct Size {", can't you?

+      if (aLayer.mSize.mWidth.mType == NS_STYLE_BG_SIZE_COVER)
+        scaleX = scaleY = PR_MAX(scaleFitX, scaleFitY);
+      else if (scaleFitX > scaleFitY)
+        scaleX = scaleY = scaleFitY;
+      else
+        scaleX = scaleY = scaleFitX;

Like to have {} around these statements

+      else if (scaleFitX > scaleFitY)
+        scaleX = scaleY = scaleFitY;
+      else
+        scaleX = scaleY = scaleFitX;

Why not just scaleX = scaleY = PR_MIN(scaleFitX, scaleFitY)?

Otherwise, this looks fantastic! I feel quite self-satisfied that the refactoring of image drawing I did for 1.9.1 has made this very easy. The pixel-snapping should even work correctly out of the box --- but have you tested it with zooming?

Comment 15

[Jeff Walden [:Waldo]]

[2009-05-11 16:06:04] <Waldo> roc: so about zoom testing
[2009-05-11 16:06:15] <Waldo> roc: pointers to what exactly you mean?
[2009-05-11 16:07:13] <roc> Waldo: ctrl-+
[2009-05-11 16:07:40] <roc> fire up some test pages with background-size and especially repeating tiles ... then zoom in and out and see if you can spot any strange artifacts
[2009-05-11 16:08:54] <Waldo> roc: well, it works, but I'm really thinking of reftest zoom testing; basically if you don't see it in a test in a patch, it's safe to assume I didn't test it
[2009-05-11 16:09:18] <Waldo> or rather, you should, because I might not have tested it and you have no way of knowing
[2009-05-11 16:09:35] <Waldo> and if I did, I should have been rigorous and done so in an automated manner
[2009-05-11 16:10:07] <roc> well
[2009-05-11 16:10:18] <roc> we have automated tests for scaled tiled images already
[2009-05-11 16:11:14] <roc> you can easily write zooming tests, just add reftest-zoom="2.5" or whatever to your reftest's root element
[2009-05-11 16:11:26] <Waldo> interesting, didn't know about that
[2009-05-11 16:11:39] <roc> hehe
[2009-05-11 16:11:42] <roc> glad to be of service
[2009-05-11 16:12:08] <Waldo> so just dump that on one of the body tests, == compare, do something similar-ish for one of the others
[2009-05-11 16:12:24] <roc> sure
[2009-05-11 16:12:59] <roc> I'm just really pleased the rendering code came out so elegantly
[2009-05-11 16:13:09] <Waldo> roc: about pixel snapping, what would I look for to check correctness/incorrectness exactly?
[2009-05-11 16:13:27] <roc> well
[2009-05-11 16:14:03] <roc> if you have a black image then basically no matter what combination of coordinates, sizes, and zooms you use, you should get crisp black edges, no gray
[2009-05-11 16:14:58] * Waldo ponders how to check that in a reftest
[2009-05-11 16:15:21] <roc> you can't check that exactly
[2009-05-11 16:15:48] <roc> you can have a test that uses reftest-zoom and another test that doesn't
[2009-05-11 16:15:54] <roc> I mean a reference that doesn't
[2009-05-11 16:16:02] <roc> but most of these paths are already tested
[2009-05-11 16:16:10] <roc> I'm not sure how many extra tests you need

Comment 16

[Jeff Walden [:Waldo]]

Also, perf-followup-bugfodder (like, really really REALLY not-glacially but fossilizingly-slow), assuming debug-buildness doesn't affect perf here by several orders of magnitude:

1. javascript:var%20s%20%3D%20document.body.style%3B%20s.background%20%3D%20%22url(http%3A%2F%2Fwhereswalden.com%2Fwp-content%2Fuploads%2F2009%2F01%2F0105.jpg)%22%3B%20s.MozBackgroundSize%20%3D%20%2217%25%22%3B%20%5B%5D.v
2. Resize the window.

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Round three, with zooming/snapping reftests and some space optimization of the Size struct

(In reply to comment #14)
> +  NS_ABORT_IF_FALSE(mType < Dimension::eCount, "bad mType for this");
> +  NS_ABORT_IF_FALSE(aOther.mType < Dimension::eCount, "bad mType for aOther");
> 
> Nothing particularly serious is going to happen if these asserts go wrong. Can
> we make them NS_ASSERTION so that if things do go wrong, we'll be able to keep
> on running the test suite?

I don't want to have to distinguish between invariants that are dangerous and ones that aren't, and I don't want others unintentionally violating them without knowing it.  This way, they never break -- just as assertions, they might.  Also, I doubt any of these is likely to go inadvertently wrong anyway.


> You can just put "friend struct Size" after "struct Size {", can't you?

Perhaps, just following existing style for similar structs in there, would rather do that unless dbaron thinks otherwise.

> +      else if (scaleFitX > scaleFitY)
> +        scaleX = scaleY = scaleFitY;
> +      else
> +        scaleX = scaleY = scaleFitX;
> 
> Why not just scaleX = scaleY = PR_MIN(scaleFitX, scaleFitY)?

No reason, brain just wasn't thinking it through that way, agree it's no less sensible this way, probably moreso.


> The pixel-snapping should even work correctly out of the box --- but have you
> tested it with zooming?

Not really, but I have now with two added reftests.  Unfortunately, the pixel-snapping one seems to demonstrate a flaw.  From the reftest.list updates:

# There seems to be a pixel-snapping problem here, where the repeated background
# image isn't being snapped at its boundaries.  Note that the boundaries within
# the image aren't the issue, because they're being obscured to avoid sampling
# algorithm dependencies (at least assuming the sampling algorithm in use
# doesn't sample too far astray from the boundaries).
fails == background-size-zoom-repeat.html background-size-zoom-repeat-ref.html

I'll attach the HTML reftest comparison in a sec to show exactly how it went wrong beyond just a description.

Comment 18

[Jeff Walden [:Waldo]]

Attachment: reftest HTML comparison file with screenshots for the failing test

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #17)
> (In reply to comment #14)
> > +  NS_ABORT_IF_FALSE(mType < Dimension::eCount, "bad mType for this");
> > +  NS_ABORT_IF_FALSE(aOther.mType < Dimension::eCount, "bad mType for aOther");
> > 
> > Nothing particularly serious is going to happen if these asserts go wrong.
> > Can we make them NS_ASSERTION so that if things do go wrong, we'll be able
> > to keep on running the test suite?
> 
> I don't want to have to distinguish between invariants that are dangerous and
> ones that aren't,

Alright, I'll distinguish them for you: these ones aren't.

> and I don't want others unintentionally violating them without knowing it.

They'll know, NS_ASSERTION prints.

> This way, they never break -- just as assertions, they might.

They'll break or not break just the same.

Fatal assertions here have no real benefit and a real cost. Please don't use them.

I think the test is wrong, i.e. the failure is correct behaviour. In the reference, the left edge of the blue rectangles and the right edge of the green rectangles are expected to be snapped to a pixel boundary, since they're independent background images. In the testcase there's just one image being stretched across that boundary so we expect to be sampling multiple pixels at the boundary and getting a blurry edge.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

When we discussed this on IRC a while ago, I relented; I think it's OK to use fatal assertions for easily locally verified checks like these.

It's still not OK IMHO for assertions that detect complex bugs of low or unknown severity.

Comment 21

[Michael Ventnor]

Has there been any progress on this? I'm wondering if a new patch is in the works so I could build on top of this for bug 479220.

Comment 22

[Jeff Walden [:Waldo]]

Yeah, problem being dbaron wants more reftests for interactions of this with the other background properties, and I haven't gotten to it yet, hoping to do so over this weekend.  Our current implementation relies on the image being its intrinsic size in a couple places where that's not warranted (most notably with respect to background-position), and that needs to be addressed first -- probably means I need to reorder/rewrite how backgrounds are painted.

Comment 23

[Jeff Walden [:Waldo]]

Attachment: With more reftests

Actually, I think the background-position problem was the only one latent here, and the reorganization wasn't all that bad.  I did have to work through a large comment to explain to my own satisfaction the ordering constraints in determining the proper painting calculation process, however, which should aid readability and help with any future changes here.

I tried writing some tests of sized backgrounds on inlines and doing equals comparisons on them, but I encountered aliasing problems not due to background stretching, so I omitted them.  I'm leery of spending too much time on it in any case as background-break, I predict, will not be part of the final spec.  (Frankly, I wonder why inlines were even allowed to have background images in the first place given the lack of specification of rendering behavior and the depth of complexity of the variations we support.)  I added not-equals tests to show that a size changes rendering and that size plus different background-break results in distinct renderings, which I think is a reasonable gloss of inline testing.

I'm still not quite clear on the zooming test, so it's still a fails == that needs to be resolved one way or the other here.  The rest of the tests are complex but understandable, although reading and understanding all of them will be a bit of a pain.

I think I've gotten rid of all the merging mistakes that (non-fatally) percolated into the patch over time, but do watch for extraneous, clearly-unintentional changes relating to fallback colors and such.

Comment 24

[Jeff Walden [:Waldo]]

Probably should get this formally on the radar, beyond the half-stated understanding one might glean from platform weekly meeting minutes...

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 386966 [details] [diff] [review]
With more reftests

dom/interfaces/css/nsIDOMNSCSS2Properties.idl
  - please add to the end of the interface

gfx/public/nsCoord.h

+                    "you probably didn't mean to do this, but if you actually "
+                    "did, create a new method that doesn't assert to handle "
+                    "it so that users who don't will get sanity checks");

I'm having trouble parsing this message.


nsStyleStruct.h:

+      // For both mWidth and mHeight or for neither; these two values must

It should be clearer that the "For both ... " also applies only to the
first two values.

+      eCount

Call this eDimensionType_COUNT or something a little more different from
the real values.

in CSSParserImpl::ParseBackground(), it might be worth having a
|pairValues| array like |simpleValues| to avoid copying code, but I
won't hold you to it.  But at least stick a blank line where I should
have put one before the allocation of |positionItem|?

-    // -moz-background-clip to background-clip, -moz-background-origin
-    // to background-origin, change their value names to *-box, and add
-    // support for content-box on background-clip.
+    // -moz-background-clip to background-clip, -moz-background-origin to
+    // background-origin, support background-origin: content-box, and change
+    // their value names to *-box

Either don't change this comment or change it to say that we'd need to
adjust this code for the new rules in the spec now that content-box has
been dropped from background-clip.

-        aItem.mClip = aItem.mOrigin;

removing this isn't right either; the shorthand does set both clip and
origin when possible.  But you don't have to deal with that in this patch.

+PRBool
+CSSParserImpl::ParseBackgroundSize()
+{
+  nsCSSValuePairList *head = nsnull, **tail = &head;
+  nsCSSValuePair valuePair;

Switch the order of these two declarations to match the other two
functions that this is similar to.

Going back to nsCSSDeclaration:
-             pair->mXValue.GetUnit() != eCSSUnit_Initial)) {
-          // Only output a Y value if it's different from the X value
+             pair->mXValue.GetUnit() != eCSSUnit_Initial) ||
+            (aProperty == eCSSProperty__moz_background_size &&
+             pair->mXValue.GetUnit() != eCSSUnit_Inherit &&
+             pair->mXValue.GetUnit() != eCSSUnit_Initial &&
+             pair->mXValue.GetUnit() != eCSSUnit_Enumerated)) {
+          // Only output a Y value if it's different from the X value,

Couldn't you just check (in general) if the YValue's unit is not Null?
(Checking this with an && with the current condition (in its entirety)
ought to fix extra spaces in the serialization of counter-increment and
counter-reset, no?

And back to nsCSSParser:

+  if (!ParseEnum(xValue, nsCSSProps::kBackgroundSizeKTable))
+      return PR_FALSE;

Indent is 2 spaces.

You'll need to merge nsCSSPropList.h to the new style.

+        nsROCSSPrimitiveValue* val = GetROCSSPrimitiveValue();
+        if (val)
+            val->SetIdent(keyword);
+        if (!val || !valueList->AppendCSSValue(val)) {
+          delete valueList;
+          delete val;
+          return NS_ERROR_OUT_OF_MEMORY;
+        }

Move the SetIdent call to after the out-of-memory return to save the
null-check.

+        if (size.mWidthType == nsStyleBackground::Size::eAuto) {
+            valX->SetIdent(eCSSKeyword_auto);
+        } else if (size.mWidthType == nsStyleBackground::Size::ePercentage) {
+            valX->SetPercent(size.mWidth.mFloat);
+        } else {
+            NS_ABORT_IF_FALSE(size.mWidthType == nsStyleBackground::Size::eLength,
+                              "bad mWidthType");
+            valX->SetAppUnits(size.mWidth.mCoord);
+        }
+
+        if (size.mHeightType == nsStyleBackground::Size::eAuto) {
+            valY->SetIdent(eCSSKeyword_auto);
+        } else if (size.mHeightType == nsStyleBackground::Size::ePercentage) {
+            valY->SetPercent(size.mHeight.mFloat);
+        } else {
+            NS_ABORT_IF_FALSE(size.mHeightType == nsStyleBackground::Size::eLength,
+                              "bad mHeightType");
+            valY->SetAppUnits(size.mHeight.mCoord);
+        }


Indent is 2 spaces.

nsRuleNode:

+  nsStyleBackground::Size::Dimension  nsStyleBackground::Size::* result;

no need for the double-space

+#ifdef DEBUG
+        const nsCSSValue &widthValue = aSpecifiedValue->mXValue;
+        NS_ABORT_IF_FALSE(widthValue.GetUnit() != eCSSUnit_Inherit &&
+                          widthValue.GetUnit() != eCSSUnit_Initial,
+                          "initial/inherit should already have been handled");
+        NS_ABORT_IF_FALSE(widthValue.GetUnit() == eCSSUnit_Enumerated &&
+                          (widthValue.GetIntValue() == NS_STYLE_BG_SIZE_CONTAIN ||
+                           widthValue.GetIntValue() == NS_STYLE_BG_SIZE_COVER),
+                          "null height value not corresponding to allowable "
+                          "non-null width value");
+#endif

Use braces for scope inside the ifdef (and indent 2)

+#ifdef DEBUG
+    if (size.mWidthType == nsStyleBackground::Size::eContain ||
+        size.mWidthType == nsStyleBackground::Size::eCover) {
+      NS_ABORT_IF_FALSE(size.mWidthType == size.mHeightType,
+                        "contain/cover apply to both dimensions or to neither");
+    }
+#endif

Just write this as a single statement:
NS_ABORT_IF_FALSE((widthType != contain && widthType != cover) || ...

In nsStyleBackground::Size::operator==:

+  return mWidthType == aOther.mWidthType &&
+         mHeightType == aOther.mHeightType &&
+         (mWidthType == eAuto ||
+          mWidthType == eContain ||
+          mWidthType == eCover ||
+          (mWidthType == ePercentage
+           ? mWidth.mFloat == aOther.mWidth.mFloat
+           : mWidth.mCoord == aOther.mWidth.mCoord)) &&
+         (mHeightType == eAuto ||
+          mHeightType == eContain ||
+          mHeightType == eCover ||
+          (mHeightType == ePercentage
+           ? mHeight.mFloat == aOther.mHeight.mFloat
+           : mHeight.mCoord == aOther.mHeight.mCoord));

I think this would be clearer (and maybe a little more efficient
since it has fewer type checks) as:

{
  if (mWidthType != aOther.mWidthType || mHeighType != aOther.mHeightType)
    return PR_FALSE;

  if (mWidthType == ePercentage) {
    if (mWidth.mFloat != aOther.mWidth.mFloat)
      return PR_FALSE;
  } else if (mWidthType == eLength) {
    if (mWidth.mCoord != aOther.mWidth.mCoord)
      return PR_FALSE;
  }

  if (mHeightType == ePercentage) {
    if (mHeight.mFloat != aOther.mHeight.mFloat)
      return PR_FALSE;
  } else if (mHeightType == eLength) {
    if (mHeight.mCoord != aOther.mHeight.mCoord)
      return PR_FALSE;
  }

  return PR_TRUE;
}

In property_database.js, could you also test (in other_values):
"25% 50px", "3em 40%"

And now, going back to nsCSSRendering:

In ScaleDimension:

+      return aDimension.mFloat * (double(aAvailLength) / aLength);

I think it's clearer to cast both values to double rather than just one.
(same for the eLength case).

Also, were you intending to do the intermediate math as doubles and then
shorten to float?  It seems better to either use floats or doubles
throughout.

+   * The background properties we need to keep in mind when drawing backgrounds

when drawing background layers

+   * These properties have the following dependencies upon each other when
+   * determining used values:

I'd avoid the term "used values" here.

+   *   background-size
+   *     depends upon background-break (for the background positioning area for
+   *     resolving percentages), background-image (for the image's intrinsic
+   *     size), background-repeat (if that value is 'round'), and
+   *     background-clip (for the background painting area, when
+   *     background-repeat is 'round')

This should be background-origin rather than background-clip, I think.

+  switch (aLayer.mSize.mWidthType) {
+    case NS_STYLE_BG_SIZE_CONTAIN:
+    case NS_STYLE_BG_SIZE_COVER: {

these should use nsStyleBackground::Size::* constants.

+      if (aLayer.mSize.mWidthType == NS_STYLE_BG_SIZE_COVER) {
+        scaleX = scaleY = PR_MAX(scaleFitX, scaleFitY);
+      } else {
+        scaleX = scaleY = PR_MIN(scaleFitX, scaleFitY);
+      }

You could be clever and write:

  // Use the larger for cover and the smaller for contain.
  if ((aLayer.mSize.mWidthType == NS_STYLE_BG_SIZE_COVER) ==
      (scaleFitX > scaleFitY)) {
    scaleX = scaleY = scaleFitX;
  } else {
    scaleX = scaleY = scaleFitY;
  }

In the default case, I think it's better to do:

if (width auto) {
  if (height auto) {
    ...
  } else {
    ...
  }
} else {
  if (height auto) {
    ...
  } else {
    ...
  }
}

so that all 4 cases are indented the same amount and look symmetric.

Also, given this code, I think ScaleDimension shouldn't handle eAuto,
but should instead assert when given eAuto.

The reftest.list looks good based on the names of the tests and the
comments; I don't see the need to review the tests themselves.

r=dbaron with that

Comment 26

[Jeff Walden [:Waldo]]

(In reply to comment #25)
> (From update of attachment 386966 [details] [diff] [review])
> dom/interfaces/css/nsIDOMNSCSS2Properties.idl
>   - please add to the end of the interface

Done.


> I'm having trouble parsing this message.

What I meant was that if the caller expected a negative scale to scale by that magnitude and then flip the sign, they should do that manually.  I can imagine there might be such cases, but they're hard to imagine, and it seems more likely the user doesn't mean to scale by a negative value and would consider it a bug -- so the message is to tell the person who actually wants negative scales to be handled to do it himself (saturate to zero, flip the sign, etc.).  Maybe that's too difficult to communicate; for now I'm just going to make the message say that negative scales are verboten, in the interests of landing this.


> It should be clearer that the "For both ... " also applies only to the
> first two values.

Linebreak, also clarified.

> Call this eDimensionType_COUNT or something a little more different from
> the real values.

Done, with that name.


> in CSSParserImpl::ParseBackground(), it might be worth having a
> |pairValues| array like |simpleValues| to avoid copying code, but I
> won't hold you to it.  But at least stick a blank line where I should
> have put one before the allocation of |positionItem|?

Blank line added; the struct-template-looping doesn't really roll off my fingers very easily, and I'm hesitant to try something that complex at the moment when this isn't landed.


> Either don't change this comment or change it to say that we'd need to
> adjust this code for the new rules in the spec now that content-box has
> been dropped from background-clip.

Didn't change, it'll be adjusted on its own pretty soon since thismodule is almost finalized.


> removing this isn't right either; the shorthand does set both clip and
> origin when possible.  But you don't have to deal with that in this patch.

Indeed, not dealt with


> Switch the order of these two declarations to match the other two
> functions that this is similar to.

Done.


> Couldn't you just check (in general) if the YValue's unit is not Null?

*If* I understand this, this would require changing mHeight to be eCSSUnit_Null when mWidth is enumerated -- which you didn't seem to say elsewhere, so I'm not sure I understand the exact concern, so I left this as is for now.


> Indent is 2 spaces.

Done here, elsewhere.  SpiderMonkey is corrupting my soul.  (I don't even like four-space indents and prefer two!)


> You'll need to merge nsCSSPropList.h to the new style.

Done as I was updating/building my tree for this every several days or so to keep things easy.


> Move the SetIdent call to after the out-of-memory return to save the
> null-check.

Done, good call.


> no need for the double-space

Okay.


> Use braces for scope inside the ifdef (and indent 2)

Sure.


> Just write this as a single statement:
> NS_ABORT_IF_FALSE((widthType != contain && widthType != cover) || ...

Done.


> I think this would be clearer (and maybe a little more efficient
> since it has fewer type checks) as:

True, was trying to parallel the other member-equality methods, probably shouldn't have done so.


> In property_database.js, could you also test (in other_values):
> "25% 50px", "3em 40%"

Done.


> I think it's clearer to cast both values to double rather than just one.
> (same for the eLength case).
> 
> Also, were you intending to do the intermediate math as doubles and then
> shorten to float?  It seems better to either use floats or doubles
> throughout.

Casted dividend, divisor, and scale to double; intent was doubles throughout since the extra processor time is negligible and it's plausible there might be a visible rendering difference if you round just wrong, seemed rendering easily won the tradeoff.


> when drawing background layers

Fixt.


> I'd avoid the term "used values" here.

Changed to "rendering"; I tried to match the CSS term of art, might have missed it slightly, probably better not to even try.



> This should be background-origin rather than background-clip, I think.

Ah, yes, good catch.


> these should use nsStyleBackground::Size::* constants.

Okay.


> You could be clever and write:
> 
>   // Use the larger for cover and the smaller for contain.
>   if ((aLayer.mSize.mWidthType == NS_STYLE_BG_SIZE_COVER) ==
>       (scaleFitX > scaleFitY)) {
>     scaleX = scaleY = scaleFitX;
>   } else {
>     scaleX = scaleY = scaleFitY;
>   }

Too clever, that takes too long to understand if I read it.


> In the default case, I think it's better to do:

Sure.


> Also, given this code, I think ScaleDimension shouldn't handle eAuto,
> but should instead assert when given eAuto.

Done.


> The reftest.list looks good based on the names of the tests and the
> comments; I don't see the need to review the tests themselves.

Cool!

Landing in m-c momentarily after I verify my changes and see a reftest run that doesn't have any regressions...

Comment 27

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/mozilla-central/rev/88e95d638b35

I'll write up a web-tech post about this sometime in the next few weeks or so; I'll try to write up MDC documentation for the property itself as well.  If I don't get to both within that time I'll add dev-doc-needed, but for now I think I can take care of it easily enough.

Comment 28

[Jeff Walden [:Waldo]]

https://developer.mozilla.org/en/CSS/-moz-background-size

The web-tech post is written, just going to get feedback from a few people before posting it.

On second thought, I'll leave dev-doc-needed here so that this gets added to the right new-in-3.6 pages; how it works I can easily handle, but I'm probably not the best person to know exactly how it should be publicized.

Comment 29

[Jeff Walden [:Waldo]]

https://developer.mozilla.org/web-tech/2009/08/04/background-images-no-longer-restricted-to-original-size-explore-the-space-with-background-size/

Comment 30

[Eric Shepherd [:sheppy]]

This is linked from Firefox 3.6 for developers, so marking this off.