Last Comment Bug 378217 - implement css3 'border-image' property
: implement css3 'border-image' property
Status: RESOLVED FIXED
[parity-webkit]
: css3
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: P1 normal with 7 votes (vote)
: ---
Assigned To: Rob Arnold [:robarnold]
: Hixie (not reading bugmail)
Mentors:
http://dev.w3.org/csswg/css3-backgrou...
Depends on: 455308 455398 445810 445911 446328 448110 448121 468473 497995 676603
Blocks: songbird 444658 css3-background
  Show dependency treegraph
 
Reported: 2007-04-20 13:24 PDT by Andrew Smith
Modified: 2011-08-04 11:29 PDT (History)
42 users (show)
vladimir: blocking1.9.1-
vladimir: wanted1.9.1+
roc: blocking1.9-
dbaron: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
what i have so far (not working) (41.68 KB, patch)
2007-05-31 17:59 PDT, Andrew Smith
no flags Details | Diff | Review
partly working (45.82 KB, patch)
2007-06-12 10:45 PDT, Andrew Smith
no flags Details | Diff | Review
mostly working (67.23 KB, patch)
2007-06-25 07:57 PDT, Andrew Smith
no flags Details | Diff | Review
patch ready for review (96.40 KB, patch)
2007-07-20 05:12 PDT, Andrew Smith
no flags Details | Diff | Review
reftests (2.95 KB, application/zip)
2007-07-20 05:14 PDT, Andrew Smith
no flags Details
patch ready for review (96.11 KB, patch)
2007-07-24 10:05 PDT, Andrew Smith
no flags Details | Diff | Review
patch ready for review (79.44 KB, patch)
2007-07-24 17:01 PDT, Andrew Smith
no flags Details | Diff | Review
patch ready for review (74.61 KB, patch)
2007-07-24 19:27 PDT, Andrew Smith
vladimir: review-
Details | Diff | Review
patch with many small fixes (75.06 KB, patch)
2007-07-31 08:01 PDT, Andrew Smith
no flags Details | Diff | Review
patch using mBorderImageWidth (71.49 KB, patch)
2007-08-01 08:36 PDT, Andrew Smith
no flags Details | Diff | Review
patch with more small fixes (104.83 KB, patch)
2007-08-08 11:40 PDT, Andrew Smith
no flags Details | Diff | Review
patch with more small fixes (104.59 KB, patch)
2007-08-08 11:46 PDT, Andrew Smith
dbaron: review-
Details | Diff | Review
patch with yet more small fixes (108.79 KB, patch)
2007-08-24 04:39 PDT, Andrew Smith
no flags Details | Diff | Review
patch with a couple more fixes (109.83 KB, patch)
2007-08-24 10:16 PDT, Andrew Smith
no flags Details | Diff | Review
patch merged to trunk, with some comments addressed (70.41 KB, patch)
2008-05-14 16:39 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
patch with more reftests (88.13 KB, patch)
2008-05-19 17:03 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
Added drawing of center image (86.63 KB, patch)
2008-05-28 13:58 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Review
patch (123.95 KB, patch)
2008-05-28 15:00 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
patch (154.16 KB, patch)
2008-05-30 16:51 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
patch (mostly gfx fixes) (76.54 KB, patch)
2008-06-11 16:23 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Review
border-image patch (68.38 KB, patch)
2008-07-03 10:58 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Review
border-image tests (54.04 KB, patch)
2008-07-03 11:00 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Review
border-image patch (67.95 KB, patch)
2008-07-03 14:32 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Review
border-image tests (54.04 KB, patch)
2008-07-03 14:32 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Review
border-image patch (68.13 KB, patch)
2008-07-09 17:40 PDT, Rob Arnold [:robarnold]
vladimir: review+
Details | Diff | Review
border-image patch (73.20 KB, patch)
2008-07-10 14:11 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Review
review in the form of a diff (11.63 KB, patch)
2008-07-15 16:18 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
tellrob: review+
Details | Diff | Review
merged patch and tests (162.94 KB, patch)
2008-07-15 20:28 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
remainder of review comments in diff form (13.76 KB, patch)
2008-07-16 10:40 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
tellrob: review+
Details | Diff | Review
uses patterns instead of the image directly (1.11 KB, patch)
2008-07-16 11:16 PDT, Rob Arnold [:robarnold]
vladimir: review+
Details | Diff | Review
merged patch and tests (165.19 KB, patch)
2008-07-16 12:03 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
dbaron: review+
Details | Diff | Review
reftest failures on mac (16.50 KB, text/plain)
2008-07-17 00:58 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
the changes I made to fix the build (6.22 KB, patch)
2008-07-17 16:55 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
tellrob: review+
Details | Diff | Review

Description Andrew Smith 2007-04-20 13:24:30 PDT
A description is available here: http://www.w3.org/TR/css3-background/#the-border-image

I am starting to try and figure out how this might be done.
Comment 1 Andrew Smith 2007-05-31 17:59:49 PDT
Created attachment 266836 [details] [diff] [review]
what i have so far (not working)

I thought I should post what I've done so far. The patch is so horible I can't ask anyone to look at it but if anyone will, any comments (especially from an architecture standpoint) would be appreciated. Sorry it's taking so long, I've never done this kind of work before.

The parser part is mostly working, the renderer I barely touched. My plan is to get the border to show as quickly as I can and fix the ugly parts one by one after that. I don't think there's any way I can do it properly the first time.
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2007-06-03 22:21:58 PDT
Hey, this is awesome -- I'm glad you did the parser part first, because the renderer was very much up in the air until like.. last thursday.  Let's talk and figure out how to do the rendering; I think your best bet is to make sure the parser is solid and make sure you can get the right data down to nsCSSRendering::PaintBorder, and then let's talk about how to do the rendering (which I think will be much easier).
Comment 3 Andrew Smith 2007-06-12 10:45:33 PDT
Created attachment 268104 [details] [diff] [review]
partly working

This is a version with many small improvements. I think there may still be a hack or two in it, but overall i don't expect to change anything significant.

I posted (at least i think i did) a message on the www-style mailing list to clarify the specification, there are too many ways to interpret how the rendering is to be done. Until that's worked out I don't know what I can do - maybe figure out how to make the resized images look more smooth.
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-06-12 11:03:50 PDT
>+    imgIRequest *req = aPresContext->LoadImage(aBorderStyle.mBorderImage,
>+                                               aForFrame);

Won't this kill off any existing background image for the frame?  That doesn't seem like a good idea...  You may need to make that code deal with multiple images per frame...

The packing in nsStyleBorder could be better, no?

Does this implementation follow the "except table elements with border-collapse:collapse" thing?

You probably need to add tests for this stuff, including serialization and parsing tests.  See the tests in layout/style/test, in particular the property_database.js file there.  I suspect that once you add some values of this property to the property database and run those tests you'll find that it's not serialized correctly; I don't see any changes to nsCSSDeclaration in that patch, esp. to nsCSSDeclaration::AppendCSSValueToString.  The rendering tests should probably be done using reftest and such once we have answers on the rendering model.

I also don't see a computed style implementation in the patch...
Comment 5 Andrew Smith 2007-06-25 07:57:36 PDT
Created attachment 269676 [details] [diff] [review]
mostly working

In this version:
- added nsPresContext::LoadBorderImage() (comment #4)
- improved the packing in nsStyleBorder
- renders all 3 types of border-images (stretch, repeat, round) correctly if I understood what fantasai said on the www-style mailing list

Things that still need fixing:
- I added border-image to property_database.js and now 17 tests fail. I have no idea what they're trying to do in the first place, so it's very hard for me to fix. If someone's willing to help, I'll put the logs up on the web.
- I added nsComputedDOMStyle::GetBorderImage() but (not surprisingly) it's never called. Solving this may help with the tests, but again, I don't know when and how that stuff is supposed to be used.

Things that may need fixing:
- Should I use -moz-border-image instead of border-image? There are two CSS3 properties in nsIDOMNSCSS2Properties without the prefix.
- The border image isn't drawn when the table has border-collapse:collapse, but I couldn't figure out what that's supposed to do, so I can't say for sure it's not broken. Does someone know a page that explains border-collapse:collapse well?
- I use nsLayoutUtils::DrawImage() for drawing every tile. Which means each tile is resized separately. This may be good for 'round', which works magically, but it's not good for 'repeat' where each tile will end up being exactly the same size. Strangely I can't find a function in the tree that will take an nsRect and return it resized.
- Another problem with nsLayoutUtils::DrawImage() is that it doesn't do a smooth resize, so it looks very ugly in many cases. I don't know what other options there are.

This is what the rendering looks like: http://littlesvr.ca/border-image/firefox-border-image-01.png

Comments please?
Comment 6 fantasai 2007-06-25 09:18:18 PDT
You should use -moz-border-image, as the spec isn't quite stable and Safari is using -webkit-.

Our border-collapse code is a mess and needs rewriting (bug 203686). Don't worry about border-image not working there: it's not practical for you to try to fix it.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-06-27 16:33:19 PDT
The tests that use property_database.js test the following things:

1)  Serialization of the property (e.g. getting foo.style.propName) gives a
    string that parses to the same thing as the original value of the property.
2)  Setting the property to the computed value doesn't change the computed
    value.

If they're failing, that means that your changes to nsCSSDeclaration and/or nsComputedDOMStyle are wrong.

In particular, you need to make the former (see also comment 4) and add your property to the list of properties that get computed at the end of nsComputedDOMStyle.cpp.  And then you need to make GetBorderImage return whatever is the right thing; just the URI looks pretty wrong to me, no?
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-06-27 16:37:29 PDT
Oh, and the resize is not something you should worry about.  We should be using the same resizing algorithm throughout, and the layout code should not need to worry what that is.

You probably want to do what nsCSSRendering::PaintBackgroundWithSC does wrt DrawImage vs DrawTile.  Or something.  Except I guess DrawTile can't resize, eh?  File bugs on better thebes APIs here?
Comment 9 Vladimir Vukicevic [:vlad] [:vladv] 2007-07-04 04:46:02 PDT
Sorry it's taking me so long to take a look at the rendering portions... some comments:

- "horizontal", not "horisontal" :)

- for when you need to repeat the image, what you have is ok, but it would be much better (and potentially simpler) if you were to create a gfxPattern from the image, and set EXTEND_REPEAT on it.. and then just Fill() the region in a single shot.  EXTEND_REPEAT repeats in both directions though, so you'd have to make sure that your path was bound correctly on the one side.  You can resize as appropriate by setting a matrix on the gfxPattern before calling ctx->SetSource().

Other than that, it looks fine.
Comment 10 Andrew Smith 2007-07-20 05:12:37 PDT
Created attachment 273106 [details] [diff] [review]
patch ready for review

Changes that I remember doing to the patch since the last one:
- all mochitests now pass
- added a couple of reftests
- property name changed to -moz-border-image
- redid the 'repeat' rendering to not use DrawImage
- doesn't crash on windows any more

Tested on Linux and Windows, I don't have access to a Mac.

It's ready for review.
Comment 11 Andrew Smith 2007-07-20 05:14:32 PDT
Created attachment 273108 [details]
reftests

I didn't know how to put these in the patch. A new directory with a few files to go into layout/reftests/.
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-07-22 22:45:07 PDT
Andrew, I'd ask "vladimir@pobox" for review and "dbaron@moz" for super-review on the patch...
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-07-22 22:57:34 PDT
Though I can tell you right now that NS_FOR_CSS_SIDES is there for a reason.  You should be using it in new code, not removing it in old code.

Oh, and nsComputedDOMStyle::GetBorderImage leaks on some of the allocation failures...
Comment 14 Vladimir Vukicevic [:vlad] [:vladv] 2007-07-23 12:44:18 PDT
(In reply to comment #13)
> Though I can tell you right now that NS_FOR_CSS_SIDES is there for a reason. 
> You should be using it in new code, not removing it in old code.

This may well be my bad influence -- I didn't realize that macro existed until I'd already landed the new border chunks.  What's the reason for having it?  I assumed it was just a bit of extra readability...
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-07-23 13:52:31 PDT
Readability and making sure that everyone looks at the sides in the same order (e.g. no one's iterating backwards or whatnot).  But mostly readability.
Comment 16 Andrew Smith 2007-07-24 10:05:46 PDT
Created attachment 273606 [details] [diff] [review]
 patch ready for review

Fixed the potential memory leak in nsComputedDOMStyle::GetBorderImage().

I won't volunteer to use NS_FOR_CSS_SIDES. For beginners that macro is another nail to step on. Maybe for experts it adds to readability, I wouldn't know. If someone tells me to put it back, I will.
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-07-24 10:35:35 PDT
Maybe I wasn't clear.  I'm telling you to put it back in all the places where you removed it and to use it for the new code.  ;)
Comment 18 Andrew Smith 2007-07-24 17:01:47 PDT
Created attachment 273690 [details] [diff] [review]
patch ready for review

same patch, but using NS_FOR_CSS_SIDES. retested on linux just in case.

vlad, will you review the nsCSSRendering part (and the couple of cosmetic changes in gfx) please?
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-07-24 17:12:48 PDT
The change to reftest/README.txt is only relevant to non-DEBUG builds.  Since most developers should be using debug builds, I think it should be in parentheses, and should start with "If you're not using a DEBUG build,".
Comment 20 Andrew Smith 2007-07-24 19:27:50 PDT
Created attachment 273708 [details] [diff] [review]
patch ready for review

Updated the readme. Sorry about the bugmail spam Vlad.
Comment 21 Vladimir Vukicevic [:vlad] [:vladv] 2007-07-30 17:51:17 PDT
Comment on attachment 273708 [details] [diff] [review]
patch ready for review

Looks fine, with a few issues:

> void
>+nsCSSRendering::DrawBorderImageSide(nsPresContext* aPresContext,
>+                                    nsIRenderingContext* aRenderingContext,
>+                                    imgIContainer* aImage,
>+                                    nsRect& aDestRect,
>+                                    const nsRect* aSourceRect,
>+                                    PRUint8 aFillType,
>+                                    PRBool aHorizontal)
>+{
>+  nscoord tileSize; // width or height, depending on aHorizontal
>+  nscoord origDestCoord; // x or y, depending on aHorizontal
>+  nscoord origDestSize; // width or height, depending on aHorizontal
>+  
>+  if (aDestRect.width == 0 || aDestRect.height == 0 ||
>+      aSourceRect->width == 0 || aSourceRect->height == 0) {
>+    return;
>+  }
>+  
>+  if (aFillType == NS_STYLE_BORDER_IMAGE_STRETCH) {
>+    // XXX this resize doesn't look very smooth, needs to be replaced with something
>+    nsLayoutUtils::DrawImage(aRenderingContext, aImage, aDestRect, aDestRect, aSourceRect);
>+    return;
>+  }
>+  
>+  if(aFillType == NS_STYLE_BORDER_IMAGE_REPEAT) {
>+    nsresult rv;
>+    
>+    nsCOMPtr<gfxIImageFrame> frame;
>+    rv = aImage->GetCurrentFrame(getter_AddRefs(frame));
>+    if (NS_FAILED(rv))
>+      return;
>+    nsCOMPtr<nsIImage> image;
>+    image = do_GetInterface(frame);
>+    if (!image)
>+      return;
>+    
>+    // surface for the whole image
>+    gfxASurface* imageSurface = nsnull;
>+    rv = image->GetSurface(&imageSurface);
>+    if (NS_FAILED(rv))
>+      return;

This leaks -- Get methods always addref, so this should be:

  nsRefPtr<gfxASurface> imageSurface;
  rv = image->GetSurface(getter_AddRefs(imageSurface));
  if (NS_FAILED(rv) || !imageSurface)
    return;



>+    gfxIntSize gfxSourceSize(nsPresContext::AppUnitsToIntCSSPixels(aSourceRect->width),
>+                             nsPresContext::AppUnitsToIntCSSPixels(aSourceRect->height));
>+    
>+    // surface for just the tile i'm interested in drawing
>+    nsRefPtr<gfxASurface> tileSurface = gfxPlatform::GetPlatform()->CreateOffscreenSurface(
>+                                           gfxSourceSize, gfxASurface::ImageFormatARGB32);
>+    
>+    nsRefPtr<gfxContext> tileCtx = new gfxContext(tileSurface);
>+    tileCtx->SetSource(imageSurface, -gfxPoint(nsPresContext::AppUnitsToIntCSSPixels(aSourceRect->x),
>+                                               nsPresContext::AppUnitsToIntCSSPixels(aSourceRect->y)));
>+    tileCtx->SetOperator(gfxContext::OPERATOR_SOURCE);
>+    tileCtx->Paint();
>+    
>+    // to resize the tile to fit in the border exactly
>+    gfxFloat scaleRatio;
>+    if (aHorizontal) {
>+      scaleRatio = (gfxFloat)aSourceRect->height / aDestRect.height;
>+    } else {
>+      scaleRatio = (gfxFloat)aSourceRect->width / aDestRect.width;
>+    }
>+    
>+    // width and height are proportional, so using the same ratio is ok
>+    gfxMatrix scaleMatrix;
>+    scaleMatrix.Scale(scaleRatio, scaleRatio);
>+    
>+    // pattern to scale and repeat the tile
>+    nsRefPtr<gfxPattern> pattern = new gfxPattern(tileSurface);
>+    pattern->SetExtend(gfxPattern::EXTEND_REPEAT);
>+    pattern->SetMatrix(scaleMatrix);
>+    
>+    // where the actual border ends up being rendered
>+    gfxRect gfxDestRect;
>+    gfxDestRect.pos.x = nsPresContext::AppUnitsToFloatCSSPixels(aDestRect.x);
>+    gfxDestRect.pos.y = nsPresContext::AppUnitsToFloatCSSPixels(aDestRect.y);
>+    gfxDestRect.size.width = nsPresContext::AppUnitsToFloatCSSPixels(aDestRect.width);
>+    gfxDestRect.size.height = nsPresContext::AppUnitsToFloatCSSPixels(aDestRect.height);
>+    
>+    // offset to make the middle tile centered in the middle of the border
>+    gfxPoint renderOffset(0, 0);
>+    gfxFloat actualTileSize;
>+    int numWholeTiles;
>+    gfxFloat edgeTileSize;
>+    if (aHorizontal) {
>+      actualTileSize = gfxSourceSize.width / scaleRatio;
>+      
>+      numWholeTiles = (int)(gfxDestRect.size.width / actualTileSize);
>+      if (numWholeTiles % 2 == 0)
>+        numWholeTiles--;
>+      
>+      edgeTileSize = (gfxDestRect.size.width - numWholeTiles * actualTileSize) / 2;
>+      
>+      gfxDestRect.pos.x -= (actualTileSize - edgeTileSize);
>+      
>+      renderOffset.x += (actualTileSize - edgeTileSize);
>+    } else {
>+      actualTileSize = gfxSourceSize.height / scaleRatio;
>+      
>+      numWholeTiles = (int)(gfxDestRect.size.height / actualTileSize);
>+      if (numWholeTiles % 2 == 0)
>+        numWholeTiles--;
>+      
>+      edgeTileSize = (gfxDestRect.size.height - numWholeTiles * actualTileSize) / 2;
>+      
>+      gfxDestRect.pos.y -= (actualTileSize - edgeTileSize);
>+      
>+      renderOffset.y += (actualTileSize - edgeTileSize);
>+    }
>+    
>+    // render
>+    nsRefPtr<gfxContext> thebesCtx = (gfxContext*)
>+            aRenderingContext->GetNativeGraphicData(nsIRenderingContext::NATIVE_THEBES_CONTEXT);
>+    thebesCtx->Save();
>+    thebesCtx->Translate(gfxDestRect.pos);
>+    thebesCtx->SetPattern(pattern);
>+    thebesCtx->NewPath();
>+    thebesCtx->Rectangle(gfxRect(renderOffset, gfxDestRect.size));
>+    thebesCtx->Fill();
>+    thebesCtx->Translate(-gfxDestRect.pos);

Don't bother with this second Translate -- the transformation matrix will be restored when you call Restore()


>+    thebesCtx->Restore();
>+    
>+  } else if(aFillType == NS_STYLE_BORDER_IMAGE_ROUND) {
>+    if (aHorizontal) {
>+      int numNormalTiles; /* tiles that keep their proportions */
>+      
>+      tileSize = (aDestRect.height * aSourceRect->width) / aSourceRect->height;
>+      tileSize = (nscoord)(aDestRect.width / ceil((float)aDestRect.width / tileSize));
>+      
>+      numNormalTiles = aDestRect.width / tileSize;
>+      
>+      origDestCoord = aDestRect.x;
>+      
>+      origDestSize = aDestRect.width;
>+      aDestRect.width = tileSize;
>+      
>+      // draw the tiles
>+      while(aDestRect.x <= origDestCoord + origDestSize - tileSize) {
>+        nsLayoutUtils::DrawImage(aRenderingContext, aImage, aDestRect, aDestRect, aSourceRect);
>+        aDestRect.x += tileSize;
>+      }
>+      
>+      // restore original values
>+      aDestRect.x = origDestCoord;
>+      aDestRect.width = origDestSize;
>+    } else {
>+      int numNormalTiles; /* tiles that keep their proportions */
>+      
>+      tileSize = (aDestRect.width * aSourceRect->height) / aSourceRect->width;
>+      tileSize = (nscoord)(aDestRect.height / ceil((float)aDestRect.height / tileSize));
>+      
>+      numNormalTiles = aDestRect.height / tileSize;
>+      
>+      origDestCoord = aDestRect.y;
>+      
>+      origDestSize = aDestRect.height;
>+      aDestRect.height = tileSize;
>+      
>+      // draw the tiles
>+      while(aDestRect.y <= origDestCoord + origDestSize - tileSize) {
>+        nsLayoutUtils::DrawImage(aRenderingContext, aImage, aDestRect, aDestRect, aSourceRect);
>+        aDestRect.y += tileSize;
>+      }
>+      
>+      // restore original values
>+      aDestRect.y = origDestCoord;
>+      aDestRect.height = origDestSize;
>+    }
>+  }
>+}
>+
>+void

I read over the CSS changes, though I didn't look at them in too much detail -- I assume dbaron looked at that?

Otherwise, looks fine -- if you can attach a new patch with the above things fixed, I can get this checked in for you once the tree reopens (you don't have cvs access, right?  Can't rememeber.)
Comment 22 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-07-30 18:25:25 PDT
Hadn't really looked yet:


In nsCSSParser::ParseBorderImage, you should combine the initial ParseVariant calls -- it takes a mask.

In nsCSSParser::ParseBorderImage, you need to call ExpectEndProperty before returning PR_TRUE.  (Otherwise you will accept values with garbage at the end.)

In nsStyleStruct.h, the comment after mBorderImageSplit should say what types the nsStyleSides have in them, as most of the comments for nsStyleCoord and nsStyleSides values do.

I think mBorderHorizontalFill and mBorderVerticalFill should be named mBorderImage*.  You could abbreviate Horizontal and Vertical to H and V (or X and Y?) if that makes them too long.

Why did you make all the changes to nsStyleStructList.h ?

property_database.js should test some additional invalid_values possibilities, such as too many of the various types of repeated values, and a slash with no numbers (just keywords) following, and a slash with nothing following.

In nsRuleNode, the None and Initial cases should be combined with || rather than separated.

In nsRuleNode, where you call SetCoord, could you comment that passing an uninitialized parentCoord is ok because you're not passing SETCOORD_INHERIT?

+        if (coord.GetUnit() == eStyleUnit_Integer ||
+            coord.GetUnit() == eStyleUnit_Percent) {

You don't need this, since you passed in flags to guarantee it.


The stuff you're doing that messes with styles, transparent, and widths (in both the array and inherit cases) doesn't seem like it's going to work when stuff gets cached in the rule tree.  I think this bit needs to be redesigned, or things will break badly in cases when all properties in the struct are specified, particularly when across different rules in certain orders.  But I need to think about this some more...

+        // have vertcal value
vertical

I also haven't looked at the nsComputedDOMStyle code yet.
Comment 23 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-07-30 18:28:03 PDT
(In reply to comment #22)
> or things will break badly in cases when all properties in the struct are
> specified, particularly when across different rules in certain orders.  But I

Er, this is a reset struct, so it doesn't require all properties to be specified to cause problems.  It just requires other border properties vs. border-image to be specified in different cascading orders.
Comment 24 Vladimir Vukicevic [:vlad] [:vladv] 2007-07-30 19:14:15 PDT
(Whoops, my bad -- I thought you'd already looked this over.  Waiting until these issues get resolved.)
Comment 25 Andrew Smith 2007-07-31 08:01:09 PDT
Created attachment 274623 [details] [diff] [review]
patch with many small fixes

Fixed the problems Vlad mentioned in comment #21, and most of the problems mentioned by dbaron in comment #22.

> Why did you make all the changes to nsStyleStructList.h ?
I couldn't read it otherwise. Is it a problem?

> The stuff you're doing that messes with styles, transparent, and widths (in
> both the array and inherit cases) doesn't seem like it's going to work when
> stuff gets cached in the rule tree.  I think this bit needs to be redesigned,
> or things will break badly in cases when all properties in the struct are
> specified, particularly when across different rules in certain orders.  But I
> need to think about this some more...
Hm.. right. At first I didn't want to deal with the many changes that would be required if I kept the border-widths separate. Then i forgot about it cause it seemed to work. I'll see if I can think of a way. Do you have any suggestions?
Comment 26 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-07-31 09:31:29 PDT
(In reply to comment #25)
> > Why did you make all the changes to nsStyleStructList.h ?
> I couldn't read it otherwise. Is it a problem?

I don't think it's particularly readable either way, and I'd rather not mess up the cvs blame.

> > The stuff you're doing that messes with styles, transparent, and widths (in
> > both the array and inherit cases) doesn't seem like it's going to work when
> > stuff gets cached in the rule tree.  I think this bit needs to be redesigned,
> > or things will break badly in cases when all properties in the struct are
> > specified, particularly when across different rules in certain orders.  But I
> > need to think about this some more...
> Hm.. right. At first I didn't want to deal with the many changes that would be
> required if I kept the border-widths separate. Then i forgot about it cause it
> seemed to work. I'll see if I can think of a way. Do you have any suggestions?

There may be a way to make the approach you're using work with some additional changes to the code in nsRuleNode, but I need to look into that...
Comment 27 Andrew Smith 2007-08-01 08:36:33 PDT
Created attachment 274776 [details] [diff] [review]
patch using mBorderImageWidth

dbaron: will this work? I added mHaveBorderImgeWidth and mBorderImageWidth to nsStyleStruct. changed GetBorder() and GetBorderWidth() to return mBorderImageWidth when it's set.

I no longer touch the border style, transparent, or the 'other' border-width.

I expected to need GetActualBorder() and GetActualBorderWidth(), but everything seems to work without them. It should be reasonably easy to replace existing GetBorder() and GetBorderWidth() calls with these, where layout and rendering is not an issue.
Comment 28 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-08-01 14:28:14 PDT
The idea seems like it will work, but:
 * you should include the "a" in "Image"
 * you need to add code to set it to false when any of the other values are specified or when a border-image without / is specified, and inherit it when inherit is specified
 * you shouldn't leave the commented-out code
 * you need to audit all callers of GetBorder to figure out which one they want
 * I'd use the opposite names:  GetActualBorder() should include the border image width, GetComputedBorder() should not.  (And I'd replace GetBorder with two newly-named functions so you're sure to change all callers to whichever is right.)
 * I don't see why you're changing protected to public.
 * you need to copy the new member in the copy-constructor

(There might be some other problems, but that's what I saw at a quick skim.  Hopefully the additional computation in GetActualBorder won't make things too slow.)
Comment 29 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-08-01 14:50:38 PDT
Also:
 * you should add the new property to the end of nsIDOMNSCSS2Properties
 * The nsComputedDOMStyle code should check your new variable and output or not output the width section (and the code for what it outputs needs to be updated)
 * The nsComputedDOMStyle code should return none as a value, not as a value list.  (See GetContent or GetCounterIncrement for examples.)
 * In general, I'd suggest putting the allocation checks and appends right after the GetROCSSPrimitiveValue calls rather than putting other code in between.
Comment 30 Andrew Smith 2007-08-08 11:40:50 PDT
Created attachment 275814 [details] [diff] [review]
patch with more small fixes

Ok, this should do. I can't test it because the trunk is broken on linux, but I'm pretty sure it's right.

dbaron, will you reviw it please?
Comment 31 Andrew Smith 2007-08-08 11:46:40 PDT
Created attachment 275815 [details] [diff] [review]
patch with more small fixes

Urmph the previous attachment had some junk in it.
Comment 32 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-08-08 23:07:27 PDT
Did you run the style system mochitests?

If the trunk is too broken to test, then the tree needs to be closed.
Comment 33 Andrew Smith 2007-08-09 00:38:37 PDT
(In reply to comment #32)
> Did you run the style system mochitests?
> 
I did, and they all pass. It just won't render pages properly, so I can't do the manual testing I'm used to.

That's not caused by this patch because it's the same on a clean build and a nightly I tried. It could be that there's something wrong with my system though I don't recall changing anything before it broke.
Comment 34 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-08-20 17:45:40 PDT
Comment on attachment 275815 [details] [diff] [review]
patch with more small fixes

So I missed that there was already a GetComputedBorderWidth.  I think,
given that, the function you're adding called GetComputedBorder should
be called GetRoundedBorder, and there should be a comment explaining
that the process is
 computed ==[rounding]==> rounded ==[including border-image]==> actual
(But I agree that the change in the eCSSUnit_Inherit case in
nsRuleNode::ComputeBorderData is correct; the old code was actually
slightly wrong, and you should be using GetComputedBorderWidth.)

You shouldn't be inserting extra trailing whitespace in
nsCSSRendering::PaintBorder and PaintOutline.


I don't see why you can't just use nsPresContext::LoadImage -- I don't
see why any nsPresContext changes are needed at all.

In nsCSSDeclaration.cpp:

+        if (array->Item(i).GetUnit() == eCSSUnit_Null) {
+          continue;
+        }
+        else if (i == 5) {
+          aResult.AppendLiteral(" /");
+        }

No need for the "else ".

In nsCSSParser.cpp:

+  if (ParseVariant(aErrorCode, mTempData.mMargin.mBorderImage, 
+                   VARIANT_INHERIT | VARIANT_NONE, nsnull)) {
+    return PR_TRUE;
+  }

You need to SetPropertyBit before returning PR_TRUE.  (I'm surprised
this didn't break anything.  Did you not see any assertions (in
DoTransferTempData) or test failures?  If not, why not?)

+  mTempData.mMargin.mBorderImage.SetArrayValue(arr, eCSSUnit_Array);
+  
+  mTempData.SetPropertyBit(eCSSProperty_border_image);

Could you not put a blank line between those two?

nsRuleNode.cpp:

NEED TO DO WHOLE FILE
+      NS_FOR_CSS_SIDES(side) {
+        // an uninitialized parentCoord is ok because I'm not passing SETCOORD_INHERIT
+        if (SetCoord(arr->Item(5 + side), coord, parentCoord, SETCOORD_LENGTH, 
+                     aContext, mPresContext, inherited)) {
+          if (coord.GetUnit() == eStyleUnit_Coord) {
+            border->mBorderImageWidth.side(side) = coord.GetCoordValue();
+            border->mHaveBorderImageWidth = PR_TRUE;
+          }
+        }

It seems like you shouldn't have separate error handling for each side.
I think, instead:
 1. you should move the assignment to mHaveBorderImageWidth out of the
    loop over sides.
 2. assert (NS_ASSERTION) that SetCoord returns true rather than testing 
 3. handle eStyleUnit_Chars by setting the side to 0 (rather than
    letting the cases you don't handle be inconsistent depending on what
    other rules are specified and what other styles get computed first.)
    And you should probably also have an NS_WARNING.


+        // have vertcal value
vertical needs an "i"


There are some blank lines in nsStyleStruct.h where you added extra
trailing whitespace.

nsStyleStruct.cpp:

  You need to initialize the new member variables in the non-copy
  constructor too.  (How did this not cause test failures?)


Also, the first point in comment 29.

And your invalid_values tests in property_database.js all have an extra
"]" in them, and the last three tests need to have numbers in them too.

I think that should be it, though.
Comment 35 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-08-20 19:57:15 PDT
> I don't see why you can't just use nsPresContext::LoadImage

See comment 4.  If you have an animated background and animated border image, you want invalidates from both... LoadImage only keeps track of a single image, though.

Comment 36 Andrew Smith 2007-08-24 04:39:44 PDT
Created attachment 278043 [details] [diff] [review]
patch with yet more small fixes

Fixed everything mentioned in comment #34, except:

- am still not using nsPresContext::LoadImage()

- '2. assert (NS_ASSERTION) that SetCoord returns true rather than testing' Well, I copied the code from the beginning of the function where the actual border-image is dealt with, I don't see why this should be different.

- 'you should add the new property to the end of nsIDOMNSCSS2Properties' Can you clarify please? They are all in alphabetical order.

And some answers:

>You need to SetPropertyBit before returning PR_TRUE.  (I'm surprised
>this didn't break anything.  Did you not see any assertions (in
>DoTransferTempData) or test failures?  If not, why not?)
I couldn't make a debug build for the longest time, and didn't bother when I fixed it. Checked now though, to make sure there are no assertions. Tests always passed unless I said otherwise, I don't know how most of them work so I can't answer that.

>You need to initialize the new member variables in the non-copy
>  constructor too.  (How did this not cause test failures?)
All those values are ignored unless nsCOMPtr mBorderImage is true. nsCOMPtrs are initialised to 0 in the default constructor. But I now added them all to the nsStyleBorder constructor anyway.
Comment 37 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-08-24 09:10:06 PDT
(In reply to comment #36)
> - '2. assert (NS_ASSERTION) that SetCoord returns true rather than testing'
> Well, I copied the code from the beginning of the function where the actual
> border-image is dealt with, I don't see why this should be different.

You should fix it in both cases.  SetCoord will only return false if the value is not one of the cases that it's given in the bitfield.  You should know what the possible cases are, so if it's always going to return true you should be able to assert that rather than test it at runtime.  The assertion also helps catch if you're wrong, since it means that there's another case you need to handle.
 
> - 'you should add the new property to the end of nsIDOMNSCSS2Properties' Can
> you clarify please? They are all in alphabetical order.

The ones that were there when the interface was created were alphabetical; new members are added to the end because it makes binary compatibility easier (although still a pain, but it's good practice for interfaces that people might be using from outside Mozilla).  (Although maybe a few were slipped in to the middle.)

> >You need to initialize the new member variables in the non-copy
> >  constructor too.  (How did this not cause test failures?)
> All those values are ignored unless nsCOMPtr mBorderImage is true. nsCOMPtrs
> are initialised to 0 in the default constructor. But I now added them all to
> the nsStyleBorder constructor anyway.

Oops.  Right, you don't need this, then.  Sorry about that.
Comment 38 Andrew Smith 2007-08-24 10:16:35 PDT
Created attachment 278076 [details] [diff] [review]
patch with a couple more fixes

Fixed everything mentioned in comment #37
Comment 39 fantasai 2007-08-28 21:03:00 PDT
You probably also need to make some changes here
  http://mxr.mozilla.org/seamonkey/source/layout/base/nsFrameManager.cpp#1146
  // if old context had image and new context does not have the same image, 
  // stop the image load for the frame
Not sure--just happened to run across it and thought it might be relevent.
Comment 40 Mike Schroepfer 2007-11-23 10:36:59 PST
Andrew/David is this something we still want to try for?
Comment 41 Stuart Parmenter 2007-11-23 10:55:41 PST
I would really like to see this make it.
Comment 42 Mike Schroepfer 2007-11-23 11:08:04 PST
(In reply to comment #41)
> I would really like to see this make it.
> 

why?
Comment 43 Atle Smelvær 2008-03-06 01:57:22 PST
Will this CSS feature make it into Firefox 3? I've tried it on the current beta 3 release, but it does not work (it works in Safari). See temporary link:
http://www.datagrafikk.no/cssdemo/mybubble.htm
Comment 44 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-03-06 08:17:46 PST
No.
Comment 45 Atle Smelvær 2008-03-06 08:29:56 PST
Could you inform me of why? It looks like it's almost ready for release, and this is an important CSS feature that would help a lot when designing webpages with different types of border areas (that means a lot of pages). Should FireFox goal not be to handle most of the CSS standards? Right now Safari is way ahead on this area.
Comment 46 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-03-06 08:40:22 PST
It missed the deadline.  See http://dbaron.org/log/2007-10#e20071031b .  We want to ship what we have, then add more.
Comment 47 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-05-14 15:28:42 PDT
Comment on attachment 278076 [details] [diff] [review]
patch with a couple more fixes

I'm working on merging this to trunk.

>+        NS_ASSERTION(SetCoord(value, coord, parentCoord, SETCOORD_LENGTH, aContext,
>+                              mPresContext, inherited),
>+                     "SetCoord for border-width failed");

This (a change from the previous patch) isn't going to work in non-debug builds.
Comment 48 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-05-14 16:39:17 PDT
Created attachment 320999 [details] [diff] [review]
patch merged to trunk, with some comments addressed

This merges attachment 278076 [details] [diff] [review] and attachment 273108 [details] to trunk and fixes a bunch of problems I noticed.

It passes mochitest, but the second of the two reftests it adds fails.  I still need to figure out why that reftest is failing, and review the rest of the patch.
Comment 49 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-05-19 17:03:25 PDT
Created attachment 321677 [details] [diff] [review]
patch with more reftests

This adds a bunch of additional reftests in addition to the ones in attachment 273108 [details].  The patch fails most of the ones I just added; WebKit passes most of them (although not quite all).  So I think there's a good bit more work to be done.  (Though I admit there are probably a few mistakes in my tests as well.)

This still also needs reftests for the image scaling rules and for the stretch/round/repeat keywords.
Comment 50 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-05-24 18:57:24 PDT
Comment on attachment 321677 [details] [diff] [review]
patch with more reftests

Two things I've noticed recently:

 1) I forgot to add my additional reftests to the reftest.list

 2) The following line

>diff --git a/gfx/thebes/public/gfxImageSurface.h b/gfx/thebes/public/gfxImageSurface.h

>+    nsRefPtr<gfxContext> thebesCtx = (gfxContext*)
>+            aRenderingContext->GetNativeGraphicData(nsIRenderingContext::NATIVE_THEBES_CONTEXT);

should just be:

gfxContext *thebesCtx = aRenderingContext->ThebesContext();

(i.e., don't use nsRefPtr, and use ThebesContext rather than GetNativeGraphicData).
Comment 51 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-05-28 13:18:06 PDT
Some other known issues:
 * we need to make the border-drawing code draw the center image
   + we need to fix nsDisplayBorder::OptimizeVisibility to not bail for the
     dirty rect inside the padding rect case if mBorderImage is non-null
Comment 52 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-05-28 13:48:14 PDT
And (per debugging I just did with Rob Arnold) we need to fix the various setters in nsStyleBorder that check IsVisibleStyle to also check for the presence of a border-image, since a border-image overrides border-style.
Comment 53 Rob Arnold [:robarnold] 2008-05-28 13:58:13 PDT
Created attachment 322849 [details] [diff] [review]
Added drawing of center image
Comment 54 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-05-28 15:00:42 PDT
Created attachment 322861 [details] [diff] [review]
patch

Attachment 322849 [details] [diff] fixed the second half of 50 and the first half of comment 51.

This fixes:
 * the first half of comment 50
 * the second half of comment 51
 * comment 52
 * fixes the parsing code to match a recent change to the spec allowing 1-4 values for the <number>|<percentage> values giving the offsets into the image (and repeating them per usual CSS rules) rather than requiring all 4.

This gets all the reftests passing except for the bug where tests are showing a gap at the end of the tiling.
Comment 55 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-05-28 15:14:29 PDT
So the solid-image-2 reftest fails for me on Linux both looking at it at multiple DPI values (96, 120, 144) and using the reftest harness, but it doesn't fail for Rob on Windows...
Comment 56 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-05-28 15:22:15 PDT
Comment on attachment 322861 [details] [diff] [review]
patch

>+      if(rectToDrawSource.height != 0 && rectToDrawSource.width != 0 && rectToDraw.width != 0 && rectToDraw.height != 0) {

The rectToDrawSource checks actually need to be for > 0, since it could end up less than 0.  I think the same problem exists for the sides; not sure why there's no code at all to check for that case.

Checking for > 0 does agree with the spec, which says:

# If the sum of the right and left widths is equal to or
# greater than the width of the image, the images for the
# top and bottom edge and the middle part are empty, which
# has the same effect as if a nonempty transparent image
# had been specified for those parts. Analogously for the
# top and bottom values.
Comment 57 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-05-28 15:24:54 PDT
Also, the code for drawing the middle needs to use DrawBorderImageSide (potentially with modifications) so that you get the right tiling / stretching / rounding behavior in both directions.
Comment 58 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-05-28 22:49:27 PDT
I'm working on some additional tests.

Additional bugs I've noticed:
 * the parser doesn't accept non-integer values for the pixel offsets in the image, but the spec says it should.  (I'm not sure I agree with the spec here.  We should check what WebKit does and maybe propose a change to the spec.)

 * We don't round the border-width overrides that come from the -moz-border-image property the same way we round the border-width property (always down).  I think we should.  I have a test for this.
Comment 59 Rob Arnold [:robarnold] 2008-05-29 17:34:41 PDT
According to the spec, the border-width override should only happen if the image can be displayed. The current patch does not do this.
Comment 60 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-05-30 16:51:12 PDT
Created attachment 323172 [details] [diff] [review]
patch

This should fix the build errors on Windows with the previous patch; it also adds a few more tests, although there are still a bunch more I'd like to write.
Comment 61 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-04 14:16:13 PDT
(In reply to comment #59)
> According to the spec, the border-width override should only happen if the
> image can be displayed. The current patch does not do this.

I was just discussing this with Rob.  To fix this, we need something roughly like:
 * when we construct the nsImageLoader in LoadBorderImage, we need to pass the image loader a boolean saying whether the frame requires relayout when the image loads
 * inside nsImageLoader, we need a boolean passed to RedrawDirtyFrame that says this is a redraw for the image loading (rather than for animation)
 * when both booleans are true inside RedrawDirtyFrame, we need to call frame->GetPresContext()->PresShell()->FrameNeedsReflow(STYLE_CHANGE)
 * to avoid a race condition where the image load completes between reflow and repaint, we need to ensure that LoadBorderImage is called the first time we reflow an element with a border image

We can probably have automated tests for this (including the race condition case) using the HTTP server (with js-driven CGI) in mochitest, and we can probably even test the race condition by using things that force flushing of layout.

This may be complex enough that we want to postpone it until after this patch lands.  (I'm curious whether WebKit gets all of this correct.)
Comment 62 Damon Sicore (:damons) 2008-06-04 16:52:36 PDT
marking wanted1.9.1? to get this in the triage queue.  Also set to P2.
Comment 63 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-05 15:55:41 PDT
Oh, and we also need to update property_database.js to move one of the invalid values (the one with a single image offset rather than four) to the list of valid ones so that test_property_syntax_errors.html doesn't fail.
Comment 64 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-06 19:50:28 PDT
And I got some linker errors on Mac because I didn't include nsStyleStructInlines in nsHTMLStyleSheet, nsImageFrame, nsTablePainter, nsTableFrame, and nsTableCellFrame.  (gcc also gives a "used but never defined" warning.)

I'm thinking we might want to set up a shared hg repo for this rather than passing patches back and forth...
Comment 65 Rob Arnold [:robarnold] 2008-06-11 16:23:25 PDT
Created attachment 324704 [details] [diff] [review]
patch (mostly gfx fixes)

I had to rewrite all the drawing code to draw the middle image and I updated it to follow the spec. It currently passes all the reftests on windows (save 1 which is due to a race condition w/reflow and image loading).

There are some pixel alignment/rounding issues still at some zoom sizes, but the integral ones seem ok.

Other notable changes:
* modified nsImageLoader to optionally reflow instead of repaint.
* Border-image loading is started in nsHTMLReflowState::Init because it's called for every reflow
* Added a reftest for repeat
Comment 66 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-06-11 20:59:59 PDT
Why are there race conditions?  I wouldn't think there should be any... Or at least onload shouldn't fire until after all the loading is done.
Comment 67 Rob Arnold [:robarnold] 2008-06-30 17:48:09 PDT
I noticed on the border-image spec page (http://dev.w3.org/csswg/css3-background/#the-border-image) that the image for Figure 6 does not always load when the page is loaded; I have to refresh the page in order for it to show up (other people have seen this behavior on this page too). I suspect that the underlying cause may be the same as the cause for the 1 failing reftest.

Also, my last diff did not properly include the reftests for some unknown reason.
Comment 68 Rob Arnold [:robarnold] 2008-07-03 10:58:59 PDT
Created attachment 328008 [details] [diff] [review]
border-image patch

I've cleaned out some unrelated changes that made it into the patch. I'm also moving all the tests into their own patch.
Comment 69 Rob Arnold [:robarnold] 2008-07-03 11:00:24 PDT
Created attachment 328009 [details] [diff] [review]
border-image tests

These are the reftests and property tests. Known failures are the second two pixel-rounding tests and solid-image-1 test (which fixes itself when the page is reloaded).
Comment 70 Rob Arnold [:robarnold] 2008-07-03 14:32:11 PDT
Created attachment 328046 [details] [diff] [review]
border-image patch

DrawBorderImageSide no longer creates an intermediate surface for each part of the tile when drawing. It uses a matrix now to scale the image. Also, I disabled the nearest neighbor filtering which I accidentally left on in the last patch version.
Comment 71 Rob Arnold [:robarnold] 2008-07-03 14:32:57 PDT
Created attachment 328047 [details] [diff] [review]
border-image tests

Disabling the filter in the last patch now causes the repeat-image-1 reftest to fail (as is expected).
Comment 72 Rob Arnold [:robarnold] 2008-07-09 17:40:20 PDT
Created attachment 328782 [details] [diff] [review]
border-image patch

Cleaned up some redundant computations in the drawing code and also finally fixed border width overrides. Passes all the reftests in the border-image patch.
Comment 73 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-09 17:56:05 PDT
Just noticed an error introduced recently:  the IID bump in nsIDOMCSS2Properties.idl is on the wrong interface (the frozen nsIDOMCSS2Properties, rather than the unfrozen and actually modified nsIDOMNSCSS2Properties).
Comment 74 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-09 17:58:52 PDT
Comment on attachment 328782 [details] [diff] [review]
border-image patch

Vlad, could you review the code in nsCSSRendering?
Comment 75 Vladimir Vukicevic [:vlad] [:vladv] 2008-07-09 18:36:35 PDT
Comment on attachment 328782 [details] [diff] [review]
border-image patch


>+          if (side == 0 || side == 2)

Use the symbolic constants here (NS_SIDE_TOP, NS_SIDE_BOTTOM)

Here and elsewhere actually; there are a bunch of places where [0] [2] etc. are hardcoded.

One of the things that I don't like is that in DrawBorderImageSide, a temporary surface is created every time for the source rect.  This is kinda crappy, since the same source rect will always be used.  Can we (as a followup patch) reuse the segments, since they're going to remain constant sized?

Other than that, the CSSRendering parts look fine.
Comment 76 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-10 12:06:41 PDT
The diff is missing nsStyleStructInlines.h .  Could you rediff with that included?
Comment 77 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-10 12:07:26 PDT
And the one thing I noticed so far:

+  nscoord GetActualBorderWidth(PRUint8 aSide) const
   {
-    return mActualBorder.side(aSide);
+    if (IsBorderImageLoaded())
+      if (mHaveBorderImageWidth)
+        return mBorderImageWidth.side(aSide);
+      else
+        return mBorder.side(aSide);
+    else
+      return mActualBorder.side(aSide);
   }

I think this should just be "return GetActualBorder().side(aSide);"
Comment 78 Rob Arnold [:robarnold] 2008-07-10 14:11:13 PDT
Created attachment 328976 [details] [diff] [review]
border-image patch

Addressed comments from vlad (filed bug 444658) and comments from dbaron
Comment 79 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-15 16:18:42 PDT
Created attachment 329752 [details] [diff] [review]
review in the form of a diff

I have this in my tree preparing to land.  Here's a review in the form of a diff, although I haven't gone over everything yet -- mainly just looked over the changes.  I'd like to go over the whole thing again before landing.

I also intend to do two renames of members of nsStyleBorder in addition to this:
  GetRoundedBorder() -> GetComputedBorder()
  mActualBorder -> mComputedBorder
Comment 80 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-15 16:27:34 PDT
(And there was also quite a bit of merging to do -- which I've done -- mostly with bug 212633, although also with bug 363706.  Your tree is pretty out of date.)
Comment 81 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-15 19:04:12 PDT
So, when I run full reftests, I crash here:

#4  <signal handler called>
#5  gfxASurface::AddRef (this=0x0)
    at /home/dbaron/builds/mozilla-central/mozilla/gfx/thebes/src/gfxASurface.cpp:68
#6  0x00007f35f979daec in nsThebesImage::GetSurface (
    this=<value optimized out>, aSurface=0x7fff13c6e3f0)
    at /home/dbaron/builds/mozilla-central/mozilla/gfx/src/thebes/nsThebesImage.h:98
#7  0x00007f35fa0db113 in nsCSSRendering::DrawBorderImageSide (
    aThebesContext=0xe9fd00, aDeviceContext=<value optimized out>, 
    aImage=0x2567740, aDestRect=@0x7fff13c6e5a0, aInterSize=@0x7fff13c6e5b0, 
    aSourceRect=@0x7fff13c6e580, aHFillType=0 '\0', aVFillType=0 '\0')
    at /home/dbaron/builds/mozilla-central/mozilla/layout/base/nsCSSRendering.cpp:4385

loading mozilla/layout/reftests/border-image/transparent-image-1.html
Comment 82 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-15 19:10:37 PDT
... in particular, mSinglePixel for that image is true, so it doesn't have a surface.  (Can you do what you want using GetPattern?)
Comment 83 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-15 20:28:28 PDT
Created attachment 329772 [details] [diff] [review]
merged patch and tests

This incorporates attachment 328047 [details] [diff] [review] and attachment 328976 [details] [diff] [review], plus my comments above, and also:
 * Put GetActualBorder in nsStyleStruct.cpp, since it depends on nsStyleStructInlines
 * additional merging with box-shadow
Comment 84 Rob Arnold [:robarnold] 2008-07-16 10:31:29 PDT
I don't see the crash when I run the border-image reftests. I should be able to fix the code to use GetPattern which will hopefully fix it.
Comment 85 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-16 10:40:36 PDT
Created attachment 329859 [details] [diff] [review]
remainder of review comments in diff form

These are the rest of my review comments (I went through the whole thing).  Once I make sure this compiles and runs, I'll attach the merged patch.

Then all that remains is:
 * Rob reviewing the changes I made in this diff and the previous review comments diff
 * somebody fixing the crash on the reftest with the solid image.  (I'm not sure if there are any performance implications of pattern vs. surface -- whether we should try to use a pattern all the time (can we), or fork at the caller.)
Comment 86 Vladimir Vukicevic [:vlad] [:vladv] 2008-07-16 11:12:00 PDT
(In reply to comment #85)
>  * somebody fixing the crash on the reftest with the solid image.  (I'm not
> sure if there are any performance implications of pattern vs. surface --
> whether we should try to use a pattern all the time (can we), or fork at the
> caller.)

Using a surface as a source will result in a pattern internally anyway, so should be no perf hit with using a pattern directly (and that would be ideal).
Comment 87 Rob Arnold [:robarnold] 2008-07-16 11:16:47 PDT
Created attachment 329863 [details] [diff] [review]
uses patterns instead of the image directly

This is just a diff against your last merged patch. It hopefully fixes the crash you were seeing
Comment 88 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-16 12:03:25 PDT
Created attachment 329877 [details] [diff] [review]
merged patch and tests

Has attachment 329859 [details] [diff] [review] and attachment 329863 [details] [diff] [review] merged in.

I'll land this after lunch if the tree is green...
Comment 89 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-17 00:58:04 PDT
Created attachment 329964 [details]
reftest failures on mac

These two reftests were failing on mac (paste this into reftest-analyzer).  I marked them as such, but need to do more analysis tomorrow.
Comment 90 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-17 01:03:57 PDT
That said, I landed it:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/9b0b2391485c
backed it out:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/12d07c032ffc
and landed it again:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/2bf42512916d
and then marked the reftests failing on mac:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/a8f655e642fb

Still really want more reftests, and need to look into the failing Mac reftests.
Comment 91 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-17 16:55:48 PDT
Created attachment 330134 [details] [diff] [review]
the changes I made to fix the build

These are the changes I made to fix the tree last night -- a bunch of methods didn't need to be inlined anymore now that the presence of a border image no longer affects the values in mActualBorder (and we don't need to call RebuildActualBodrer in SetBorderImage).

This is just hg diff -r9b0b2391485c:a8f655e642fb.
Comment 92 Rob Arnold [:robarnold] 2008-07-17 17:44:28 PDT
Comment on attachment 330134 [details] [diff] [review]
the changes I made to fix the build

It's after the fact, but oh well it looks good.
Comment 93 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-17 19:27:49 PDT
(In reply to comment #89)
> Created an attachment (id=329964) [details]
> reftest failures on mac
> 
> These two reftests were failing on mac (paste this into reftest-analyzer).  I
> marked them as such, but need to do more analysis tomorrow.

I looked at these in GIMP, and it is the test that's showing colors that are off (not the reference).  So I filed bug 445911.

Comment 94 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-17 19:28:57 PDT
And marking fixed, since this landed yesterday.

(But we could really use some additional reftests.)
Comment 95 Robert Kaiser (not working on stability any more) 2008-07-25 04:59:43 PDT
We see a slight color variation and reftest failure with this on the SeaMonkey MacOSX (10.4) box now that we switched it to 1.9.1/hg.

The failure is in the div body of multicolor-image-2.html div.one (NOT in the actual borders), which is solid #4a298e on the reference rendering but has 6 of its 9 pixels at #4b2a8e (*+**+**+* where * is off and + is correct).

I fail to find where the test even defines that color for the div content area itself, but I just might not be an expert on that.
Comment 96 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-25 09:39:36 PDT
(In reply to comment #95)
> I fail to find where the test even defines that color for the div content area
> itself, but I just might not be an expert on that.

The border-image fills the content area too (if it's not transparent in that part of the image).
Comment 97 Robert Kaiser (not working on stability any more) 2008-07-25 11:14:40 PDT
(In reply to comment #96)
> The border-image fills the content area too (if it's not transparent in that
> part of the image).

Ah, ok, so that's where we get this color from. In that case, this looks to me like at least this box, perhaps all Tiger boxes (as FF has none to test it), fails the test with very similar symptoms to bug 445911, actually. Can we do something about this?
Comment 98 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-25 11:35:58 PDT
Um, the tests are already marked as known to fail on Mac for bug 445911, so that shouldn't be giving you errors that turn your tinderbox orange.
Comment 99 Robert Kaiser (not working on stability any more) 2008-07-25 13:47:29 PDT
I don't think _that_ test has been marked failing.
Comment 100 Serge Gautherie (:sgautherie) 2008-07-26 09:07:42 PDT
(In reply to comment #99)
> I don't think _that_ test has been marked failing.

Right, I filed bug 448121 ;-)

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