Closed Bug 113577 Opened 23 years ago Closed 15 years ago

Implement -moz-image-rect(): use a rectangular region of an image as CSS background image

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: hyatt, Assigned: rkawaguchi)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 16 obsolete files)

140.52 KB, patch
Details | Diff | Splinter Review
16.60 KB, patch
Details | Diff | Splinter Review
The background equivalent to -moz-image-region.  Works the same way but for
backgrounds instead of foreground images.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7
Not going to bother.  Will keep open since I do have a partial patch I want to
attach.
Target Milestone: mozilla0.9.7 → Future
If this were implemented in CSS, we could then simplify all of the little smiley
images (See Bug #84950) into a single image. This would be a nice ease of use
feature for themers along with reducing the number of tiny images we have for
smileys.

what we really should do, as discussed with the CSSWG last week, is:

   background: -moz-url-with-region(foo.png, 0, 24, 24, 24);

...or some such.
Another possible syntax is:

  background: -moz-crop(url(foo.png), 0, 24, 24, 24);
Assignee: hyatt → nobody
Status: ASSIGNED → NEW
QA Contact: ian → style-system
Summary: Implement -moz-background-image-region → Implement -moz-background-image-region or background-image: -moz-image-with-region()
OS: Windows 2000 → All
Hardware: x86 → All
(In reply to comment #3)
> what we really should do, as discussed with the CSSWG last week, is:
> 
>    background: -moz-url-with-region(foo.png, 0, 24, 24, 24);
> 
> ...or some such.

At least in our themes, we use separate image and region clauses so that hover effects can simply update the region. (In certain cases it is possible to apply the same hover effect to all buttons but this is not done in current projects.)
See also http://dev.w3.org/csswg/css3-images/ which I drafted up after some discussion at the F2F.
See also Jorrit Vermeiren's message in Media Fragments WG mailing list (http://lists.w3.org/Archives/Public/public-media-fragment/2009Jun/0002.html). He points out that CSS sprites, which -moz-background-image-region is trying to achieve, are not limited to CSS.

Media Fragments WG (http://www.w3.org/TR/2009/WD-media-frags-reqs-20090430/#naming-space) proposes use of URIs to specify spacial fragments of an image (e.g. foo.png#xywh=160,120,320,240); however, they mainly focus on retrieving a part of an image from origin server.
Attached patch work in progress patch v1 (obsolete) — Splinter Review
This work-in-progress patch can handle the use cases in the previously attached file.

The patch is not yet complete (I am thinking to replace imgContainerWrapper). But I appreciate any feedback at this stage as it will be very helpful for me to learn, whether it's trivial or critical. Thank you!
Attached patch work in progress patch v2 (obsolete) — Splinter Review
removed some warnings; sorry.
Attachment #388331 - Attachment is obsolete: true
Assignee: nobody → rkawaguchi
Attached patch patch v3 (obsolete) — Splinter Review
Supports -moz-image-rect() for 'background' and 'background-image' properties.
Attachment #388314 - Attachment is obsolete: true
Attachment #388351 - Attachment is obsolete: true
Attached patch reftests v1 (obsolete) — Splinter Review
Reftests for patch v3
Here are some nicpick notes, from a cursory glance over the patch.

> +++ b/layout/style/nsCSSDataBlock.cpp
> +    
> +    nsIDocument* doc = aRuleData->mPresContext->Document();

Remove the spaces on that added empty line here

> +  const PRUnichar* thisFunctionName = func->Item(0).GetStringBufferValue();
> +  

and here.

> ++ b/layout/style/nsCSSDeclaration.cpp
> +      NS_ABORT_IF_FALSE(eCSSUnit_URL == url.GetUnit() || 

Remove extra space at end-of-line here

> + * where each side takes a bare number or a percent value. 

and here.

>+++ b/layout/style/nsCSSParser.cpp
>-    if (ParseURL(aValue)) {
>+    PRBool isFunction = PR_TRUE;
>+    if (ParseURL(aValue, isFunction)) {

There's no need for the added "isFunction" variable, right?  Why not just pass in PR_TRUE directly?

If you want to be explicit about what PR_TRUE means in that context, you could just add a comment.  (Or perhaps you could keep the variable and declare it as 'const' to indicate that it's not going to change... but that still seems like overkill to me, for a boolean value that's used once. :))

This applies to all "ParseURL" calls in the patch -- there are at least 3 that I noticed.

>+  const PRUint32  numArgs = 5;

Nix the extra space between "PRUint32" and "numArgs"

>+  { // <top>, <right>, <bottom>, <left>
>+    nsDOMCSSValueList cropRectDOM(PR_TRUE, PR_TRUE);
>+
>+    NS_FOR_CSS_SIDES(side) {
>+      nsROCSSPrimitiveValue *sideValue = GetROCSSPrimitiveValue();
>+      if (!sideValue || !cropRectDOM.AppendCSSValue(sideValue)) {
>+        delete sideValue;
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
>+      SetValueToCoord(sideValue, cropRect.Get(side), nsnull, nsnull);
>+    }
[snip]
>+  } // cropRectDOM and sideValue's are deleted here.

On that last comment, maybe add " as cropRectDOM leaves scope"?  At first, I was unclear on why the sideValues would be deleted there, since they're not going out of scope at that point, or anything like that.  But after digging in I realized that it's because they're given to cropRectDOM, so they'll be deleted along with it.

Anyway, a simple clarification on the comment would make that more clear that *all* of that deletion is due to cropRectDOM going out-of-scope.
(In reply to comment #14)
> Remove the spaces on that added empty line here
> 
> > +  const PRUnichar* thisFunctionName = func->Item(0).GetStringBufferValue();
> > +  
> 
> and here.

oops, sorry, ignore that comment -- looks like that particular whitespace-on-empty-line is just due to the added single-space-indentation from "diff".
Summary: Implement -moz-background-image-region or background-image: -moz-image-with-region() → Implement -moz-image-rect(): use a sub-image as a CSS image.
Attached patch patch v4 (obsolete) — Splinter Review
Addresses dholbert's review comments.
Attachment #390598 - Attachment is obsolete: true
Attachment #390979 - Flags: review?(roc)
It should be fairly easy now to integrate -moz-image-rect() into other CSS properties such as border-image and list-style-image. Would that be useful?
Yes, in a separate patch.

It would be nice if those could share as much code as possible with the background path.
Depends on: background-size, 479220
Should I create a new bug for that patch?

Also, I just realized patch v4 heavily depends on bug 189519 and bug 479220. I think I should rewrite my patch on top of the patches for tbose bugs, correct?
Yes, I think we're going to land bug 189519 and bug 479220 first.
Attachment #390979 - Flags: review?(roc)
Comment on attachment 390979 [details] [diff] [review]
patch v4

Removing r? flag until a new patch is re-written on top of the bugs this depends on.
Blocks: 506826
Summary: Implement -moz-image-rect(): use a sub-image as a CSS image. → Implement -moz-image-rect(): use a rectangular region of an image as CSS background image
Blocks: 507052
Attached patch patch v5 (obsolete) — Splinter Review
re-written on top of bug 189519 and bug 479220.
Attachment #390979 - Attachment is obsolete: true
At this point, I'd like to finalize the syntax for the proposed CSS function.
Currently, the patch v5 adopts the following syntax:

-moz-image-rect(<uri>, <top>, <right>, <bottom>, <left>);

where <uri> is a quoted string (e.g. "foo.png"), and each side (<top>,...,<left>) is a non-negative (possibly floating-point) value with an option of percentage (%) sign.

Other options to consider are:
(1) use of url() function for <uri>
(2) allowing a non-quoted string for <uri>
(3) allowing "inherit" and "auto" keywords for each argument
(4) other function names such as -moz-crop() and -moz-url-with-region()

Please share with me your thought on this as I am going to finalize the patch for review.
The current set of syntax choices seems fine to me. Out of curiosity, how hard would it be to do (2)?
(In reply to comment #23)
> Other options to consider are:
> (1) use of url() function for <uri>

Is it documented somewhere, or is there an obvious reason why the url is part of that property at all, as opposed to -moz-image-region?

> (2) allowing a non-quoted string for <uri>

This seems to me like it should be a requirement.

> (4) other function names such as -moz-crop() and -moz-url-with-region()

-moz-image-rect sounds like an arbitrary variation of or alias for -moz-image-region, so I definitely think it's a bad name.

If it's intended for background images as this bug suggests, I think -moz-background-region without the url would be a better choice. This is how -moz-image-region works, too.

If it's intended to be a generic source for cropped images (obsoleting -moz-image-region?), I think -moz-url-with-region would be a better choice, although it's unfortunate that you would have to repeat the url each time you use that image with a different region.
(In reply to comment #25)
> If it's intended to be a generic source for cropped images (obsoleting
> -moz-image-region?),

That's right.

> I think -moz-url-with-region would be a better choice,
> although it's unfortunate that you would have to repeat the url each time you
> use that image with a different region.

We're extracting a subrectangle of an image, that's why I think -moz-image-rect is a good choice. -moz-url-with-region would be unnecessarily obscure.
(In reply to comment #25)
> Is it documented somewhere, or is there an obvious reason why the url is part
> of that property at all, as opposed to -moz-image-region?

One reason is that we could allow -moz-image-rect anywhere we currently accept an image in CSS, for example, in border-image or list-style-image.

Another reason is that it allows fallback for other browsers, e.g.
background: url(icon.png);
background: -moz-image-rect("iconmosaic.png",0,10,10,0);
(In reply to comment #23)
> Other options to consider are:
> (1) use of url() function for <uri>

I think we probably should use url() here. That makes this extensible to extract, for example, a subrectangle of a gradient, or a subrectangle of a -moz-element.
(In reply to comment #24)
> The current set of syntax choices seems fine to me. Out of curiosity, how hard
> would it be to do (2)?

Not really hard; I just need to tweak nsCSSScanner a bit.

(In reply to comment #25)
> -moz-image-rect sounds like an arbitrary variation of or alias for
> -moz-image-region, so I definitely think it's a bad name.

Another rationale for using "image" as part of the function name is the W3C's proposal of new CSS <image> type.
http://dev.w3.org/csswg/css3-images/

-moz-image-rect(<image>, <rect>)
where <image> is one of {<url>, <gradient>, <element>}

> although it's unfortunate that you would have to repeat the url each time you
> use that image with a different region.

If we allow the use of "inherit" for <uri>, the user can do something like:
li { background: -moz-image-rect(url(foo.png), 0px, 10px, 10px, 0px); }
li:hover { background: -moz-image-rect(inherit, 10px, 10px, 20px, 0px); }
Does this sound like another requirement?

(In reply to comment #28)
> I think we probably should use url() here. That makes this extensible to
> extract, for example, a subrectangle of a gradient, or a subrectangle of a
> -moz-element.

Yes, that seems a very natural extension and simplifies CSS Parser.
> If we allow the use of "inherit" for <uri>

No, we definitely do not want to do this. Also, your example isn't inheritance but cascading.

> I think we probably should use url() here.

If we want to do that, then I think we also should allow strings to be interpreted as being wrapped in url(). -moz-image-rect(url("foo.png") ...) is very verbose.
(In reply to comment #30)
> No, we definitely do not want to do this. Also, your example isn't inheritance
> but cascading.

Yes... I apparently need to study web standard.

> If we want to do that, then I think we also should allow strings to be
> interpreted as being wrapped in url(). -moz-image-rect(url("foo.png") ...) is
> very verbose.

Should we unconditionally interpret any string, whether quoted or not, as a shorthand of url()? It may become confusing when we introduce -moz-element(); people might try -moz-image-rect("#id", ...) (maybe, it's okey because anyway it doesn't work).
Only quoted strings if you want to allow other notations.
Attached patch patch v6 with reftests (obsolete) — Splinter Review
I eventually adopted the following syntax:

-moz-image-rect(url(<uri>), <top>, <right>, <bottom>, <left>)

where url() function notation can be omitted for those who are troubled by the verbosity.

Roc, can I ask a review on this patch? Thank you!
Attachment #390599 - Attachment is obsolete: true
Attachment #392871 - Attachment is obsolete: true
Attachment #393916 - Flags: review?(roc)
Attached patch add more tests to TestRect.cpp (obsolete) — Splinter Review
Add more nsRect tests and also perform the same set of tests on nsIntRect since patch v6 uses some functionality of nsIntRect that was not tested before.

Who would be a good person to ask a review on this patch?
You're probably looking for r?roc and sr?dbaron or vice versa. I assume the interpretation of the top/right/bottom/left offsets matches that of the 'clip' property?
Attachment #393916 - Flags: superreview?(dbaron)
The syntax of top/right/bottom/left follows that of rect() function for the 'clip' property. Commas are optional (and if omitted, they must be omitted consistently) for the same reason.
Comma omission isn't part of the CSS spec.  I did some archaeology recently on this, and the eventual commit that added support for omitting commas (1998-04-28, nsCSSParser.cpp 3.5) had the commit message "Allow for optional commas between clip rect elements".  I'm pretty sure this was for IE compatibility, but I'm not sure I'd be in favor of allowing commas to be omitted from new properties going forward -- not even sure about old properties, to be honest, without evidence that sites actually depend on this.  I believe WebKit for one doesn't support space-only separation, possibly Opera as well.
It is part of CSS 2.1, which says in
http://www.w3.org/TR/CSS21/visufx.html#clipping :

Authors should separate offset values with commas. User agents must support separation with commas, but may also support separation without commas (but not a combination), because a previous revision of this specification was ambiguous in this respect.
Bleh, I guess I misread it.  I'm still not sure I like omittable commas in new properties going forward, tho.
This looks really nice!!! My comments are minor.

+  PRBool ComputeActualCropRect(nsIntRect& aActualCropRect,
+                               PRBool* aIsEntireImage = nsnull) const;

Document what the return value means.

+  PRBool IsOpaque(PRBool aCheckOnlyCurrentFrame) const;

Why would you want aCheckOnlyCurrentFrame to be false here? I think the current
code in nsDisplayBackground, that returns false for animated images, is
actually a bug; we only care about the current frame, because display lists are
ephemeral; we will always paint the same frame as the one we're testing here. I
think we should get rid of the IsOpaque parameter here .

+    // Must make sure if mCropRect contains at least a pixel.
+    // XXX Is this optimization worth it? Maybe I should just return PR_FALSE.
+    nsIntRect actualCropRect;
+    PRBool rv = ComputeActualCropRect(actualCropRect);
+    NS_ASSERTION(rv, "ComputeActualCropRect() can not fail here");
+    return rv && !actualCropRect.IsEmpty();

Seems to me you can return true if the actualCropRect is empty, since every
pixel in your bounds is opaque.

+  if (drawBackgroundColor) {
+    if (aBackground.BottomLayer().mRepeat == NS_STYLE_BG_REPEAT_XY) {

Use && to reduce to a single "if"

I'd call StyleImageRenderer just "ImageRenderer".

This will need to be rebased to be on top of the patch for bug 509681, but
that's not going to be hard.

+StyleImageRenderer::PrepareImage()
+{
+  mIsReady = PR_FALSE;

Why do you need to set this again?

I didn't review the style system stuff. If you fix the above and get dbaron to
check it over, then r=me.
Attached patch patch v7 with reftests (obsolete) — Splinter Review
(In reply to comment #40)
Addressed roc's review comment.

> +  PRBool IsOpaque(PRBool aCheckOnlyCurrentFrame) const;
> 
> Why would you want aCheckOnlyCurrentFrame to be false here? I think the current
> code in nsDisplayBackground, that returns false for animated images, is
> actually a bug; we only care about the current frame, because display lists are
> ephemeral; we will always paint the same frame as the one we're testing here. I
> think we should get rid of the IsOpaque parameter here .

I agree that this check is not necessary. I was just not 100% sure that it was okey to remove it. The parameter has been removed.

> +    // Must make sure if mCropRect contains at least a pixel.
> +    // XXX Is this optimization worth it? Maybe I should just return PR_FALSE.
> +    nsIntRect actualCropRect;
> +    PRBool rv = ComputeActualCropRect(actualCropRect);
> +    NS_ASSERTION(rv, "ComputeActualCropRect() can not fail here");
> +    return rv && !actualCropRect.IsEmpty();
> 
> Seems to me you can return true if the actualCropRect is empty, since every
> pixel in your bounds is opaque.

IsOpaque() is used to check if the image covers an item beneath with opaque pixels so that the item beneath is totally invisible. So, I think if the crop rect is empty, it should return PR_FALSE just as it does when a source image can not be loaded. The comment on the function declaration was not reflecting the desired behavior of the function, so I fixed it.
I accidentally removed TestColorNames.cpp from Makefile. So, I put it back.
TestColorNames was also failing one test (NS_HexToRGB() on a transparent color), which has been fixed.
Attachment #393917 - Attachment is obsolete: true
Sorry for the repeated post.
The only change I made is:

diff --git a/gfx/tests/TestColorNames.cpp b/gfx/tests/TestColorNames.cpp
--- a/gfx/tests/TestColorNames.cpp
+++ b/gfx/tests/TestColorNames.cpp
@@ -111,17 +111,17 @@ int main(int argc, char** argv)
 
     // Check that parsing an RGB value in hex gets the right values
     PRUint8 r = NS_GET_R(rgb);
     PRUint8 g = NS_GET_G(rgb);
     PRUint8 b = NS_GET_B(rgb);
     PRUint8 a = NS_GET_A(rgb);
     if (a != PR_UINT8_MAX) {
       // NS_HexToRGB() can not handle a color with alpha channel
-      continue;
+      rgb = NS_RGB(r, g, b);
     }
     char cbuf[50];
     PR_snprintf(cbuf, sizeof(cbuf), "%02x%02x%02x", r, g, b);
     nscolor hexrgb;
     if (!NS_HexToRGB(NS_ConvertASCIItoUTF16(cbuf), &hexrgb)) {
       fail("hex conversion to color of '%s'", cbuf);
       rv = -1;
     }
Attachment #394399 - Attachment is obsolete: true
Comment on attachment 394398 [details] [diff] [review]
patch v7 with reftests

I don't think we want the nsCSSScanner changes (or
CSSParserImpl::GetURLNotInParens, CSSParserImpl::ParseAmbiguousURL,
etc.).  We should instead just support a syntax in which all URLs are
either (a) strings or (b) enclosed in the url() function.  Commas are
valid characters in URLs and frequently show up in them; having the
normal CSS syntax without quotes work most of the time but then break if
they have commas in their URLs would be confusing for authors.  Such a
syntax would also be even harder to standardize because of the way url()
is currently represented in the spec as a single token.  I'd suggest
that the syntax can be:
  -moz-image-rect(url("image.png"), ...)
  -moz-image-rect(url(image.png), ...)
  -moz-image-rect("image.png", ...)
but that you can't use an unquoted URL without url().

I think you should call your reftests directory reftests/image-rect/
rather than reftests/moz-image-rect/; we've generally avoided using
vendor prefixes (which might go away in the future) in the names of
directories.

I think that the way you're reusing the eCSSUnit_Function code that
currently stores the function name as a string is probably taking that
code a little beyond what it's meant to do.  It's something that's ok to
land as is, although we should probably fix it afterwards, and it would
be ok to fix it first.  I think instead of storing the value as a string
it should store it as eCSSUnit_Enumerated, where the enumerated values
are eCSSKeyword_* constants.  Then your new nsCSSValue::EqualsFunction
could take an nsCSSKeyword argument and do an integer comparison rather
than a string comparison.  And we wouldn't need to mess with nsIAtoms.

nsCSSParser.cpp:

In the CSS parser, ParseImage seems like too broad a description of
something that only parses background-image (not list-style-image or
generated content).  I think it's reasonable to expect that we extend
-moz-image-rect() to those, but probably not gradients.  So I think,
instead of adding ParseImage, you should add VARIANT_IMAGE_RECT and add
support for it in ParseVariant, and then change the caller of ParseImage
back to calling ParseVariant.

Also, I'd prefer if you call ParseMozImageRect just ParseImageRect.

+    // Optional commas are allowed except for the last element
+    if (3 != side) {
+      if (ExpectSymbol(',', PR_TRUE) != foundComma) {
+        NS_WARNING("inconsistent use of commas; abort parsing");
+        return PR_FALSE;
+      }
+    }

I don't think you want an NS_WARNING; that's for problems in our code,
not problems in a Web page.

I think I agree with Waldo that we shouldn't introduce a new
optional-comma syntax.  (I also think that if we were going to do that,
we'd still want the comma after the url.)  So I think it would be better
not to do that.

At all except the first of the false returns inside ParseMozImageRect,
you need to call SkipUntil(')') before returning false.  (Local style is
also to put braces around single line blocks, but this will make all
those blocks non-single-line.)

nsComputedDOMStyle.cpp:

+    SetValueToCoord(valSide, aCropRect.Get(side), nsnull, nsnull);

you don't need the ", null, null" because there are default arguments.

Also, I think it's probably cleaner to:
 * make GetMozImageRectString take an nsString& argument rather than
   nsAString&.
 * do all the assigning to that string argument at the end, with:
   aString = NS_LITERAL_STRING("-moz-image-rect(") +
             argumentString +
             NS_LITERAL_STRING(")");

You should rename nsComputedDOMStyle::SetStyleImageToDOMValue to be
nsComputedDOMStyle::SetValueToStyleImage or
SetPrimitiveValueToStyleImage.


nsRuleNode.cpp:

If you're going to make SetAbsCoord not pass dummyCanStoreInRuleTree
back to its caller, you should assert that it's still true after you
call SetCoord.  (Right now SETCOORD_LENGTH and SETCOORD_INHERIT are the
only ones that set aCanStoreInRuleTree to false, but that could change.)


nsStyleStruct.cpp:

+    // We could check if every stop color of the gradient is non-transparent:
+    // return mGradient->IsOpaque();
+    return PR_FALSE;

Skip the middle line; better not to have commented-out code, especially
to code that doesn't exist.

+  // IntersectRect() returns an empty rect if we get negative width or height

Do you have tests that test this case somewhere?

Also tests that test the behavior of negative coordinates, coordinates
larger than the image, and percentages less than 0 or greater than 100%?



nsStyleStruct.h:

+ *
+ * This struct used to be nsStyleBackground::Image, but we are moving it out of
+ * nsStyleBackground since it will be used by other CSS properties such as
+ * border-image and list-style-image as a consequence of bug 507052 and others.

Don't give the history of code in comments; it's in the version control
system if we really care.  (It might be worth noting that it's currently
used only for 'background-image' but that might be used for
'border-image', 'list-style-image', and 'content' in the future.)

+  nsStyleSides* GetCropRect() const {
+    return mCropRect;
+  }

This should assert that it's eStyleImageType_Image.



I'm going to mark this review- because I'd like to look at the
revisions.  However, I think it looks pretty good so far.

Also note that I didn't look at the changes in layout/base or
layout/reftests.
Attachment #394398 - Flags: review-
Comment on attachment 393916 [details] [diff] [review]
patch v6 with reftests

Clearing this request because I reviewed the newer patch.
(In reply to comment #44)
Thank you for your review!
I fixed all the mistakes and accepted all the suggestions.
Before uploading the latest patch, let me resolve some of my concerns.

> I think I agree with Waldo that we shouldn't introduce a new
> optional-comma syntax.  (I also think that if we were going to do that,
> we'd still want the comma after the url.)  So I think it would be better
> not to do that.
I made the separating commas mandatory.

> At all except the first of the false returns inside ParseMozImageRect,
> you need to call SkipUntil(')') before returning false.  (Local style is
> also to put braces around single line blocks, but this will make all
> those blocks non-single-line.)
I am sorry, but I could not understand the comment in the parentheses.
Could you explain in more details?

If we have the following blocks in <style> tag, my current implementation
ignores anything after -moz-image-rect() including the #test2 block,
painting both div's red. Is it the correct behavior?

div {
  background-color: red;
}
div#test1 {
  /* missing closing parenthesis */
  background-image: -moz-image-rect("foo.png", 0, 16, 16, 0;
  background-color: yellow;
}
div#test2 {
  background-color: yellow;
}

> You should rename nsComputedDOMStyle::SetStyleImageToDOMValue to be
> nsComputedDOMStyle::SetValueToStyleImage or
> SetPrimitiveValueToStyleImage.
In this function, we want to assign the value of nsStyleImage to
nsROCSSPrimitiveValue. So, shouldn't the function name be
SetStyleImageToValue or SetStyleImageToPrimitiveValue?

> +  // IntersectRect() returns an empty rect if we get negative width or height
> Do you have tests that test this case somewhere?
Yes, I added 4 unit tests in TestRect.cpp and some more reftests that test
negative width and height.

> Also tests that test the behavior of negative coordinates, coordinates
> larger than the image, and percentages less than 0 or greater than 100%?
Yes, all of them are tested by reftests.
(In reply to comment #46)
> > At all except the first of the false returns inside ParseMozImageRect,
> > you need to call SkipUntil(')') before returning false.  (Local style is
> > also to put braces around single line blocks, but this will make all
> > those blocks non-single-line.)
> I am sorry, but I could not understand the comment in the parentheses.
> Could you explain in more details?

I'm saying that the code in nsCSSParser tends to use:

  if (!ExpectSymbol(',')) {
    return PR_FALSE;
  }

rather than:

  if (!ExpectSymbol(',')
    return PR_FALSE;

but that since I'm asking you to put a second line there, it will force you to add the braces anyway.

> If we have the following blocks in <style> tag, my current implementation
> ignores anything after -moz-image-rect() including the #test2 block,
> painting both div's red. Is it the correct behavior?

Yes.  You should see the same thing for an unclosed url() or (perhaps more relevant) an unclosed attr() in the content property.

> > You should rename nsComputedDOMStyle::SetStyleImageToDOMValue to be
> > nsComputedDOMStyle::SetValueToStyleImage or
> > SetPrimitiveValueToStyleImage.
> In this function, we want to assign the value of nsStyleImage to
> nsROCSSPrimitiveValue. So, shouldn't the function name be
> SetStyleImageToValue or SetStyleImageToPrimitiveValue?

No.  "Set A to B" means that you're changing the value of A so that it equals B.  "Set A" means that you're changing the value of A; the "to B" part of the sentence is optional, and says what you're changing it to.  Grammatically, the word "set" is like the word "change": its object is the thing you are changing.  (On the other hand, "Assign A to B" means that you're changing the value of B.)
Attached patch patch v8 with reftests (obsolete) — Splinter Review
Addresses both roc and dbaron's review comments
Adds some more reftests
Rebased on the latest patch of bug 509681
Attachment #393916 - Attachment is obsolete: true
Attachment #394398 - Attachment is obsolete: true
Attachment #395432 - Flags: review?(dbaron)
Attachment #393916 - Flags: review?(roc)
Depends on: 509681
Comment on attachment 395432 [details] [diff] [review]
patch v8 with reftests

removing review flag until I address the dbaron's new comment.
Attachment #395432 - Flags: review?(dbaron)
Attached patch patch v9 with reftests (obsolete) — Splinter Review
Addresses the dbaron's last comment.

(In reply to comment #47)
> No.  "Set A to B" means that you're changing the value of A so that it equals
> B.  "Set A" means that you're changing the value of A; the "to B" part of the
> sentence is optional, and says what you're changing it to.  Grammatically, the
> word "set" is like the word "change": its object is the thing you are changing.
>  (On the other hand, "Assign A to B" means that you're changing the value of
> B.)

I see!! That really makes sense!! I was thinking "set" and "assign" are synonyms with the same grammatical role...
Attachment #395432 - Attachment is obsolete: true
Attachment #395455 - Flags: review?(dbaron)
Comment on attachment 395455 [details] [diff] [review]
patch v9 with reftests

>diff --git a/layout/style/nsCSSDeclaration.cpp b/layout/style/nsCSSDeclaration.cpp

>+    const nsCSSValue& functionName = array->Item(0);
>+    if (functionName.GetUnit() == eCSSUnit_Enumerated) {
>+      // We assume that the first argument is always of nsCSSKeyword type.
>+      const nsCSSKeyword functionId =
>+        static_cast<nsCSSKeyword>(functionName.GetIntValue());
>+
>+      nsCSSKeywords::AddRefTable();
>+      AppendASCIItoUTF16(nsCSSKeywords::GetStringValue(functionId), aResult);
>+      nsCSSKeywords::ReleaseTable();
>+    } else {
>+      AppendCSSValueToString(aProperty, functionName, aResult);
>+    }

Two issues here:

 * nsLayoutStatics already calls AddRefTable and ReleaseTable; you don't need to do that here.
 * Please file a follow-up bug to change the transform functions to the new mechanism
Comment on attachment 395455 [details] [diff] [review]
patch v9 with reftests

You should actually move this code:

>   if (!mSheetPrincipal) {
>     NS_NOTREACHED("Codepaths that expect to parse URLs MUST pass in an "
>                   "origin principal");
>     return PR_FALSE;
>   }

Into SetValueToURL so that all callers of SetValueToURL hit it.  (Making
it happen a little later is fine; just put it at the start of
SetValueToURL.)


>+    if (!ParseNonNegativeVariant(top, VARIANT_SIDE, nsnull) ||
>+        !ExpectSymbol(',', PR_TRUE))
>+      break;
>+    if (!ParseNonNegativeVariant(right, VARIANT_SIDE, nsnull) ||
>+        !ExpectSymbol(',', PR_TRUE))
>+      break;
>+    if (!ParseNonNegativeVariant(bottom, VARIANT_SIDE, nsnull) ||
>+        !ExpectSymbol(',', PR_TRUE))
>+      break;
>+    if (!ParseNonNegativeVariant(left, VARIANT_SIDE, nsnull))
>+      break;
>+
>+    if (!ExpectSymbol(')', PR_TRUE))
>+      break;

This (and the ExpectSymbol check before it) could actually all be a
single if:

 if (!a ||
     !b ||
     !c ||
     !d ||
     ...)
   break;

>+    return ParseVariant(aValue,
>+        VARIANT_HUO | VARIANT_GRADIENT | VARIANT_IMAGE_RECT, nsnull);

Line this up like this instead:
    return ParseVariant(aValue,
                        VARIANT_HUO | VARIANT_GRADIENT | VARIANT_IMAGE_RECT,
                        nsnull);


r=dbaron if you fix these issues, plus the AddRefTable/ReleaseTable from
comment 51.
Attachment #395455 - Flags: review?(dbaron) → review+
Attached patch checkin patchSplinter Review
Address dbaron's comment 52.
Combined with TestRect.cpp patch.
Attachment #394401 - Attachment is obsolete: true
Attachment #395455 - Attachment is obsolete: true
Blocks: 511803
bug 511803 was created to change the transform functions to use the new
mechanism for storing CSS function values in nsCSSValue.
No longer blocks: 511803
Blocks: 511803
I started preparing the patches for checkin.  I actually split them back into two patches again.  Also, the patch required a little bit of merging to get it current with trunk.

Then the try server build failed on Windows due to errors in TestWinTSF:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1250866381.1250871717.26771.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1250866381.1250869945.3270.gz

I think this is because of the #include change in nsCSSValue.h, so I'm testing this fix (this diff actually also includes the merging to trunk) in a second round of try server pushing:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches-clean/rev/567f29f7ea7e
Also failed reftest:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1250866381.1250874246.26185.gz
because the reftest.list was pointing to moz-image-rect/reftest.list instead of image-rect/reftest.list.

I pushed to try again with that fixed.
I just noticed this bug as I was rebasing my patch from bug 435296.

I haven't looked into the patch in much detail, so I was curious about a few things:
1) Do we reset mIsReady call PrepareImage() when we get a FrameChanged on the original image? If not, how do we handle animated images?
2) How intelligent are we about saving the image we get from ExtractCurrentFrame (soon to be ExtractFrame)? This was a major perf bottleneck for border-image (see bug 507817).
3) The new framework in this patch looks like it might be able to be used for border-image also. Has this been considered?
Should this be marked as fixed?
Target Milestone: Future → mozilla1.9.3a1
(In reply to comment #59)
> Should this be marked as fixed?

I believe so -- resolving. (Ryo, correct me if I'm wrong.)
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Ryo - before this bug rests in peace, care to comment on the above questions?
Yes, this should be marked as fixed.
The only concern left is the lacking developer document.
I think Bobby was asking about his questions from comment 58.
Yes, that's right. I'm sorry that I left them unanswered for a long time.

1) ImageRenderer class is created and discarded everything it needs to draw a background image. So, when an animated image advances its current frame, and the background needs be re-drawn, ImageRenderer is created from a scratch, which internally sets mIsReady false.

2) We are not being intelligent here, creating a temporary surface to hold a subimage every time it needs to draw a background image. We could cache the subimage, but we have to consider the trade off between speed and memory usage. I will follow bug 507817. (I heard this issue will be resolved at some point at the graphics layer, like source clipping in cairo?)

3) Is bug 507052 what you meant?
(In reply to comment #64)
> Yes, that's right. I'm sorry that I left them unanswered for a long time.
> 
> 1) ImageRenderer class is created and discarded everything it needs to draw a
> background image. So, when an animated image advances its current frame, and
> the background needs be re-drawn, ImageRenderer is created from a scratch,
> which internally sets mIsReady false.

Glad we're handling it. In terms of performance, this still really sucks. If a moz-image-rect() is animated (say, 10fps), and on screen, then we're calling ExtractCurrentFrame 10 times per second. Perhaps this needs support from Imagelib - maybe a function like ExtractSubImage() that's similar to ExtractCurrentFrame but extracts all the frames into an animate-able container? Of course, whether this is useful depends on #2 below.

> 
> 2) We are not being intelligent here, creating a temporary surface to hold a
> subimage every time it needs to draw a background image. We could cache the
> subimage, but we have to consider the trade off between speed and memory usage.

I'm pretty confident that the tradeoff is in favor of caching it. 

> I will follow bug 507817. (I heard this issue will be resolved at some point at
> the graphics layer, like source clipping in cairo?)
> 
> 3) Is bug 507052 what you meant?

yep! glad you're on top if it
Whiteboard: [doc-waiting-1.9.3]
I don't think we should add caching of extracted subimages. I don't think making -moz-image-rect faster is a high priority, and the best fix for this is for cairo to offer API (and backend support) for repeating and padding a subimage without copying it. I talked to Jeff last week and we think GL shaders can do everything we need.

What we can do, and probably should do, is change the Extract methods to not copy but return a special imgIContainer object that references the original image. Then its Draw method can check for the special case where we don't need to do any tiling, padding or resampling, and just draw directly from the original image in that case.
Actually, whenever we're not doing any tiling, the subimage Draw method can delegate to the underlying image's Draw method with appropriate parameters to control the pixels it's allowed to sample. The underlying image Draw may end up having to make its own temporary copy to avoid inappropriate sampling, but we already hit that path in various situations.

So this would be pretty easy to implement. Any volunteers? :-)
So, I worked on the roc's suggestion (comment 67).

This patch optimizes for non-tiling subimages: when possible, it relies on the subimage sampling restriction by imageFrame::Draw() so that it doesn't have to call the expensive ExtractCurrentFrame().

The patch passes all the reftests for -moz-image-rect() --- I didn't create new reftests since many of the reftests previously written for -moz-image-rect() test non-tiling subimages.
Attachment #398588 - Flags: review?(roc)
Moved the call of ExtractCurrentFrame() from ImageRenderer::Draw() to nsLayoutUtils::DrawImage() so that other possible callers can take advantage of it.
Attachment #398588 - Attachment is obsolete: true
Attachment #398594 - Flags: review?(roc)
Attachment #398588 - Flags: review?(roc)
I'm sorry for the multiple posts. Forgot to qrefresh...
Attachment #398594 - Attachment is obsolete: true
Attachment #398596 - Flags: review?(roc)
Attachment #398594 - Flags: review?(roc)
I thought I commented here but I guess I failed to...

I was thinking of having both comment #66 and comment #67. So the current code that's checked into nsCSSRendering would be unchanged. In the image library, ExtractFrame would return an object that implements imgIContainer but doesn't copy the image, just refers to a subrect of a frame of another image. That object's Draw method would check to see if tiling is required; if no tiling is required it can delegate the Draw to that other image. If tiling is required it creates a temporary copy of the subimage and draws that.

This way everyone who calls imgIContainer::ExtractFrame can get the optimization. And later, if we get smarter image drawing functionality in cairo or some other way, we may be able to avoid making that temporary copy in more cases.

But if that turns out to be hard or undesirable for some reason, your patch still looks pretty good.
I see. In fact, I have implemented at some point a class that implements imgIContainer, which internally has a pointer to the master imgIContainer and the crop rect. I can dig into my old patches to find it.

A major advantage of hiding the extraction detail under the hood of imgIContainer is that we can repeatedly call ExtractCurrentFrame() to crop a subimage out of an already cropped image while doing the actual extraction only once (lazy extraction). This behavior is very useful for the usecase:

-moz-border-image: -moz-image-rect(url(foo), 50, 100, 100, 50) 10

I will write another patch after finishing -moz-element().
I will probably do this on bug 507052.
Attachment #398596 - Flags: review?(roc)
ryo - keep in mind that a lot of things in ImageLib will change soon. Notably, bug 435296 changes a bunch of stuff, and bug 503973 will change the container interface.
So, maybe I should wait until those bugs are fixed? This optimization is currently not so critical anyway.
probably
Whiteboard: [doc-waiting-1.9.3]
Sorry for commenting here, but does this property for background-image applies to xul elements like stack,hbox etc also ?
You need to log in before you can comment on or make changes to this bug.