Last Comment Bug 113577 - Implement -moz-image-rect(): use a rectangular region of an image as CSS background image
: Implement -moz-image-rect(): use a rectangular region of an image as CSS back...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla1.9.3a1
Assigned To: Ryo Kawaguchi
:
:
Mentors:
: 191853 (view as bug list)
Depends on: background-size 479220 509681
Blocks: 507052 506826 511803
  Show dependency treegraph
 
Reported: 2001-12-05 02:02 PST by David Hyatt
Modified: 2014-04-26 02:23 PDT (History)
33 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
sample usecases for the work in progress patch (811 bytes, text/html)
2009-07-13 14:11 PDT, Ryo Kawaguchi
no flags Details
work in progress patch v1 (81.01 KB, patch)
2009-07-13 14:44 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
work in progress patch v2 (80.99 KB, patch)
2009-07-13 15:56 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
patch v3 (55.82 KB, patch)
2009-07-24 17:52 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
reftests v1 (34.13 KB, patch)
2009-07-24 17:53 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
patch v4 (55.77 KB, patch)
2009-07-27 18:55 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
patch v5 (74.80 KB, patch)
2009-08-05 22:00 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
patch v6 with reftests (125.52 KB, patch)
2009-08-11 15:55 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
add more tests to TestRect.cpp (18.16 KB, patch)
2009-08-11 15:58 PDT, Ryo Kawaguchi
roc: review+
Details | Diff | Splinter Review
patch v7 with reftests (124.35 KB, patch)
2009-08-13 16:05 PDT, Ryo Kawaguchi
dbaron: review-
Details | Diff | Splinter Review
add more tests to TestRect.cpp v2 (22.29 KB, patch)
2009-08-13 16:10 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
add more tests to TestRect.cpp v3 (22.31 KB, patch)
2009-08-13 16:21 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
patch v8 with reftests (118.54 KB, patch)
2009-08-19 15:44 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
patch v9 with reftests (118.59 KB, patch)
2009-08-19 16:52 PDT, Ryo Kawaguchi
dbaron: review+
Details | Diff | Splinter Review
checkin patch (140.52 KB, patch)
2009-08-20 18:17 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
Optimize for non-tiling subimages (16.72 KB, patch)
2009-09-03 22:48 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
Optimize for non-tiling subimages v2 (16.72 KB, patch)
2009-09-03 23:41 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review
Optimize for non-tiling subimages v3 (16.60 KB, patch)
2009-09-03 23:57 PDT, Ryo Kawaguchi
no flags Details | Diff | Splinter Review

Description David Hyatt 2001-12-05 02:02:03 PST
The background equivalent to -moz-image-region.  Works the same way but for
backgrounds instead of foreground images.
Comment 1 David Hyatt 2001-12-14 14:24:08 PST
Not going to bother.  Will keep open since I do have a partial patch I want to
attach.
Comment 2 Scott MacGregor 2004-03-04 11:41:14 PST
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.

Comment 3 Hixie (not reading bugmail) 2004-03-07 05:16:22 PST
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.
Comment 4 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2007-03-28 15:29:53 PDT
Another possible syntax is:

  background: -moz-crop(url(foo.png), 0, 24, 24, 24);
Comment 5 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-01-19 09:48:25 PST
*** Bug 191853 has been marked as a duplicate of this bug. ***
Comment 6 neil@parkwaycc.co.uk 2009-05-08 09:04:53 PDT
(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.)
Comment 7 fantasai 2009-06-15 15:02:59 PDT
See also http://dev.w3.org/csswg/css3-images/ which I drafted up after some discussion at the F2F.
Comment 8 Ryo Kawaguchi 2009-06-16 18:53:32 PDT
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.
Comment 9 Ryo Kawaguchi 2009-07-13 14:11:44 PDT
Created attachment 388314 [details]
sample usecases for the work in progress patch
Comment 10 Ryo Kawaguchi 2009-07-13 14:44:55 PDT
Created attachment 388331 [details] [diff] [review]
work in progress patch v1

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!
Comment 11 Ryo Kawaguchi 2009-07-13 15:56:11 PDT
Created attachment 388351 [details] [diff] [review]
work in progress patch v2

removed some warnings; sorry.
Comment 12 Ryo Kawaguchi 2009-07-24 17:52:03 PDT
Created attachment 390598 [details] [diff] [review]
patch v3

Supports -moz-image-rect() for 'background' and 'background-image' properties.
Comment 13 Ryo Kawaguchi 2009-07-24 17:53:55 PDT
Created attachment 390599 [details] [diff] [review]
reftests v1

Reftests for patch v3
Comment 14 Daniel Holbert [:dholbert] 2009-07-27 13:04:13 PDT
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.
Comment 15 Daniel Holbert [:dholbert] 2009-07-27 13:06:27 PDT
(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".
Comment 16 Ryo Kawaguchi 2009-07-27 18:55:34 PDT
Created attachment 390979 [details] [diff] [review]
patch v4

Addresses dholbert's review comments.
Comment 17 Ryo Kawaguchi 2009-07-28 11:57:01 PDT
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?
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-28 12:06:06 PDT
Yes, in a separate patch.

It would be nice if those could share as much code as possible with the background path.
Comment 19 Ryo Kawaguchi 2009-07-28 12:17:29 PDT
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?
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-28 12:24:51 PDT
Yes, I think we're going to land bug 189519 and bug 479220 first.
Comment 21 Ryo Kawaguchi 2009-07-28 12:34:14 PDT
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.
Comment 22 Ryo Kawaguchi 2009-08-05 22:00:13 PDT
Created attachment 392871 [details] [diff] [review]
patch v5

re-written on top of bug 189519 and bug 479220.
Comment 23 Ryo Kawaguchi 2009-08-05 22:47:56 PDT
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.
Comment 24 fantasai 2009-08-05 23:51:07 PDT
The current set of syntax choices seems fine to me. Out of curiosity, how hard would it be to do (2)?
Comment 25 Dão Gottwald [:dao] 2009-08-06 00:33:52 PDT
(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.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-06 02:10:09 PDT
(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.
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-06 02:14:29 PDT
(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);
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-06 02:15:30 PDT
(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.
Comment 29 Ryo Kawaguchi 2009-08-06 16:20:50 PDT
(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.
Comment 30 fantasai 2009-08-06 16:50:03 PDT
> 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.
Comment 31 Ryo Kawaguchi 2009-08-06 17:27:38 PDT
(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).
Comment 32 fantasai 2009-08-06 19:15:44 PDT
Only quoted strings if you want to allow other notations.
Comment 33 Ryo Kawaguchi 2009-08-11 15:55:17 PDT
Created attachment 393916 [details] [diff] [review]
patch v6 with reftests

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!
Comment 34 Ryo Kawaguchi 2009-08-11 15:58:41 PDT
Created attachment 393917 [details] [diff] [review]
add more tests to TestRect.cpp

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?
Comment 35 fantasai 2009-08-11 16:25:19 PDT
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?
Comment 36 Ryo Kawaguchi 2009-08-12 14:11:27 PDT
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.
Comment 37 Jeff Walden [:Waldo] (remove +bmo to email) 2009-08-12 14:42:58 PDT
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.
Comment 38 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-08-12 14:48:34 PDT
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.
Comment 39 Jeff Walden [:Waldo] (remove +bmo to email) 2009-08-12 14:52:48 PDT
Bleh, I guess I misread it.  I'm still not sure I like omittable commas in new properties going forward, tho.
Comment 40 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-12 21:34:04 PDT
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.
Comment 41 Ryo Kawaguchi 2009-08-13 16:05:08 PDT
Created attachment 394398 [details] [diff] [review]
patch v7 with reftests

(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.
Comment 42 Ryo Kawaguchi 2009-08-13 16:10:17 PDT
Created attachment 394399 [details] [diff] [review]
add more tests to TestRect.cpp v2

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.
Comment 43 Ryo Kawaguchi 2009-08-13 16:21:18 PDT
Created attachment 394401 [details] [diff] [review]
add more tests to TestRect.cpp v3

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;
     }
Comment 44 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-08-18 17:10:20 PDT
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.
Comment 45 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-08-18 17:10:58 PDT
Comment on attachment 393916 [details] [diff] [review]
patch v6 with reftests

Clearing this request because I reviewed the newer patch.
Comment 46 Ryo Kawaguchi 2009-08-19 15:13:22 PDT
(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.
Comment 47 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-08-19 15:37:33 PDT
(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.)
Comment 48 Ryo Kawaguchi 2009-08-19 15:44:33 PDT
Created attachment 395432 [details] [diff] [review]
patch v8 with reftests

Addresses both roc and dbaron's review comments
Adds some more reftests
Rebased on the latest patch of bug 509681
Comment 49 Ryo Kawaguchi 2009-08-19 15:47:52 PDT
Comment on attachment 395432 [details] [diff] [review]
patch v8 with reftests

removing review flag until I address the dbaron's new comment.
Comment 50 Ryo Kawaguchi 2009-08-19 16:52:33 PDT
Created attachment 395455 [details] [diff] [review]
patch v9 with reftests

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...
Comment 51 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-08-20 14:07:54 PDT
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 52 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-08-20 14:22:19 PDT
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.
Comment 53 Ryo Kawaguchi 2009-08-20 18:17:06 PDT
Created attachment 395741 [details] [diff] [review]
checkin patch

Address dbaron's comment 52.
Combined with TestRect.cpp patch.
Comment 54 Ryo Kawaguchi 2009-08-20 18:40:54 PDT
bug 511803 was created to change the transform functions to use the new
mechanism for storing CSS function values in nsCSSValue.
Comment 55 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-08-21 09:51:50 PDT
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
Comment 56 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-08-21 10:31:38 PDT
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.
Comment 57 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-08-21 13:43:18 PDT
Landed on mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/a3cda2dce88e
http://hg.mozilla.org/mozilla-central/rev/b1a05c7a6ebe
Comment 58 Bobby Holley (:bholley) (busy with Stylo) 2009-08-24 05:47:27 PDT
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?
Comment 59 Jesse Ruderman 2009-08-27 22:15:32 PDT
Should this be marked as fixed?
Comment 60 Daniel Holbert [:dholbert] 2009-09-01 14:25:05 PDT
(In reply to comment #59)
> Should this be marked as fixed?

I believe so -- resolving. (Ryo, correct me if I'm wrong.)
Comment 61 Bobby Holley (:bholley) (busy with Stylo) 2009-09-01 15:56:29 PDT
Ryo - before this bug rests in peace, care to comment on the above questions?
Comment 62 Ryo Kawaguchi 2009-09-01 16:58:06 PDT
Yes, this should be marked as fixed.
The only concern left is the lacking developer document.
Comment 63 Daniel Holbert [:dholbert] 2009-09-01 16:59:23 PDT
I think Bobby was asking about his questions from comment 58.
Comment 64 Ryo Kawaguchi 2009-09-01 17:45:33 PDT
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?
Comment 65 Bobby Holley (:bholley) (busy with Stylo) 2009-09-03 07:02:15 PDT
(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
Comment 66 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-03 15:23:05 PDT
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.
Comment 67 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-03 15:26:28 PDT
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? :-)
Comment 68 Ryo Kawaguchi 2009-09-03 22:48:54 PDT
Created attachment 398588 [details] [diff] [review]
Optimize for non-tiling subimages

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.
Comment 69 Ryo Kawaguchi 2009-09-03 23:41:36 PDT
Created attachment 398594 [details] [diff] [review]
 Optimize for non-tiling subimages v2

Moved the call of ExtractCurrentFrame() from ImageRenderer::Draw() to nsLayoutUtils::DrawImage() so that other possible callers can take advantage of it.
Comment 70 Ryo Kawaguchi 2009-09-03 23:57:12 PDT
Created attachment 398596 [details] [diff] [review]
Optimize for non-tiling subimages v3

I'm sorry for the multiple posts. Forgot to qrefresh...
Comment 71 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-04 03:34:20 PDT
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.
Comment 72 Ryo Kawaguchi 2009-09-04 14:11:05 PDT
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.
Comment 73 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-04 14:16:37 PDT
Thanks.
Comment 74 Bobby Holley (:bholley) (busy with Stylo) 2009-09-04 14:43:59 PDT
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.
Comment 75 Ryo Kawaguchi 2009-09-04 16:38:44 PDT
So, maybe I should wait until those bugs are fixed? This optimization is currently not so critical anyway.
Comment 76 Bobby Holley (:bholley) (busy with Stylo) 2009-09-04 20:30:35 PDT
probably
Comment 77 Eric Shepherd [:sheppy] 2010-10-11 06:14:26 PDT
Documented here:

https://developer.mozilla.org/en/CSS/-moz-image-rect
Comment 78 Girish Sharma [:Optimizer] 2012-03-01 05:42:07 PST
Sorry for commenting here, but does this property for background-image applies to xul elements like stack,hbox etc also ?

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