Closed Bug 455364 Opened 12 years ago Closed 12 years ago

border-image artifacts

Categories

(Core :: Web Painting, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: vlad, Assigned: zwol)

References

()

Details

(Keywords: fixed1.9.1, Whiteboard: [fixed1.9.1b3])

Attachments

(8 files, 11 obsolete files)

1.99 KB, image/png
Details
554 bytes, image/png
Details
1.90 KB, text/html
Details
2.38 KB, text/html
Details
8.60 KB, patch
vlad
: review+
Details | Diff | Splinter Review
16.06 KB, patch
zwol
: review+
Details | Diff | Splinter Review
46.90 KB, patch
zwol
: review+
Details | Diff | Splinter Review
46.96 KB, patch
Details | Diff | Splinter Review
In the css3.info examples, both show artifacts -- I'm pretty sure they're both the same thing.  The 'round' has lots of little yellow dots inside the block, and the 'stretch' has a big smeared bit on the left and smaller on the right.  Screenshots can be seen on john's blog at http://ejohn.org/blog/border-image-in-firefox/
Flags: wanted1.9.1?
Component: Style System (CSS) → Layout: View Rendering
QA Contact: style-system → layout.view-rendering
The problems also happen (with today's mozilla-central nightly) on Windows and Linux.  So -> All.
OS: Mac OS X → All
Hardware: PC → All
Has somebody measured the image to make sure this behavior isn't correct?
Flags: wanted1.9.1? → blocking1.9.1+
Priority: -- → P3
One potential problem is that border-image is doing (or not doing) its own pixel-snapping and probably isn't getting things right in all situations. See bug 458487, it should probably use the new nsLayoutUtils::DrawImage.
The image has problems; *all* of the diamonds overflow the boxes that the
styling assigns, and it's not possible to adjust the styling to make all nine
subregions simultaneously correct.

I have made a corrected image and will see if the problems persist.
Attached image corrected border image
So I've just attached four things:

 -- the original border image from css3.info
 -- my corrected version of the same image
    (the diamonds aren't exactly the same but that's ok)
 -- a simple, self-contained (uses data: urls) test case
    matching css3.info's design, using my corrected image
 -- the same, using a palette-compressed version of the original

One may clearly see that the corrected-image test case shows no smears but the original-image test case does show smears.

That doesn't mean there's no bug here!  This test case is not doing anything especially evil to the pixel snapping.
I've emailed the folks at css3.info about the problem image.

Also, there is a similar image linked from http://www.w3.org/TR/css3-background/#the-border-image ( http://www.w3.org/TR/css3-background/border.png ) -- it is not exactly the same as my corrected image, but its diamonds are within their 27x27 regions, and a test case that uses it doesn't show artifacts either.
Assignee: nobody → zweinberg
I've written a series of three patches which, collectively, arrange to use roc's new nsLayoutUtils::Draw*Image methods to draw border images.  They don't work, and I can't figure out why, so I'm posting them here and begging for help.

This patch adds new variants of the Draw*Image methods that take the image as a gfxIImageFrame* or an nsIImage* instead of an imgIContainer*.  This makes it possible to give Draw*Image an image that didn't come directly from the network - for instance, an nsIImage* that is a small piece of another nsIImage*, which the second patch will make easy to create.

It is possible that I am not handling the down-conversion from imgIContainer* correctly; I don't understand what the rectangle returned by gfxIImageFrame::GetRect() is for, and the only thing I can think of that makes any sense at all would imply that the size of an imgIContainer* is not necessarily the size of the nsIImage* that's the current frame, but that seems ridiculous.
Attachment #349781 - Flags: review?(roc)
Part 2 adds an nsIImage::Extract() method which produces another nsIImage containing a rectangular sub-region of the original.  Here again I'm not sure I did all the image manipulation correctly, with particular concern about how the sub-image and related drawing context are created, written to, and finalized.

It also refactors nsMargin a bit so that nsIntMargin has all the same methods as nsMargin even when NSCOORD_IS_FLOAT is true.  Since NSCOORD_IS_FLOAT is not a functional mode right now, that part isn't actually necessary, but I didn't realize that until after I'd done it.
Attachment #349783 - Flags: review?(vladimir)
And here's the actual DrawBorderImage rewrite.  As mentioned above, it doesn't work, and there's a bunch of debugging code intentionally left in the patch.

The way in which it doesn't work is: only the very first call to nsLayoutUtils::Draw*Image from DrawBorderImage, in a given page, draws anything.  In other words, if you have

<div id="a">...</div>
<div id="b">...</div>
<div id="c">...</div>

where #a and #c are styled with -moz-border-image and #b is styled with background-image, then: only the upper left hand corner of the border-image for #a is drawn; the background image for #b is drawn; and nothing is drawn for #c.

The debugging code tells me that this is not a problem with the computation of source and destination rectangles (despite which, I would appreciate any suggestions anyone might have for reducing the amount of repetition involved) and so I am left with the hypothesis that I've done something horribly wrong (and yet invisible) with the image manipulation.
Attachment #349787 - Flags: review?(roc)
(In reply to comment #10)
> It is possible that I am not handling the down-conversion from imgIContainer*
> correctly; I don't understand what the rectangle returned by
> gfxIImageFrame::GetRect() is for, and the only thing I can think of that makes
> any sense at all would imply that the size of an imgIContainer* is not
> necessarily the size of the nsIImage* that's the current frame, but that seems
> ridiculous.

Yes, yes it is. And yet, true. Look at the computation of 'padding' in nsLayoutUtils::DrawImage.
You should probably run reftests on patch 1 alone to get rid of the bugs there. Do we really need the full set of DrawImage variants in that patch though? IMHO the DrawSingleImage(Unscaled) variants should not be needed.
This revised version of part 1 now passes reftests by itself.  Also I took out all the variant methods that weren't actually being used in part 3 - so we now have nsIImage* variants of DrawImage and DrawSingleImage, but nothing else.

The complete patch series still breaks the border-image tests.  On looking at the code again, I think it's just that in part 3, I didn't compute the destination rectangles for painting correctly, but I won't be able to verify that until later today.
Attachment #349781 - Attachment is obsolete: true
Attachment #349991 - Flags: review?(roc)
Attachment #349781 - Flags: review?(roc)
A slight correction to this part of the patch proved necessary - I had the coordinate transform backward, so ::Extract gave back nonsense for any rectangle not starting at the image origin.
Attachment #349783 - Attachment is obsolete: true
Attachment #350896 - Flags: review?(vladimir)
Attachment #349783 - Flags: review?(vladimir)
Here's the current state of the third part of the patch.  It is *not* ready to go as is, but if anyone wants to look at it, be my guest.  Still to do: make all coordinate calculations on the image take the innerRect into account, verify that 'repeat' and 'round' do the right thing in all cases, and fix a seaming bug which regresses reftest border-image/solid-image-2.html (note the addition of a -2a.html).

My current theory is that the seaming bug boils down to nearest-neighbor sampling not actually always sampling only the pixels in the image (as advertised) as it occurs only with 'stretch' mode and only for specific, large stretch factors.  The patch in bug 463938 does not fix it (unsurprisingly, as the image in question is being upscaled, not downscaled).
Attachment #349787 - Attachment is obsolete: true
Attachment #349787 - Flags: review?(roc)
The patch in bug 468496 fixes border-image/solid-image-2.html.  Adding dependency.
Depends on: 468496
Blocks: 470250
This works around bug 468496.  nsIImage objects can now be flagged as always to use image surfaces instead of whatever the platform would prefer, and ::Extract sets that flag on all images it creates.  Thus, border images take correctness over performance, which should be okay since they should be small.  Also, ::Extract calls ::Optimize before returning so that the solid-color optimization gets done.  With this revision, the existing border-image tests do not regress on Linux when all three patches are applied.  When I revise part 3 I shall also include a border-image variation of the 1x2 image testcase from bug 468496 to ensure that this isn't just due to the solid-color optimization now applying.

There is also some minor cleanup to make the #ifdeffage in nsThebesImage.cpp less heinous.  I'd *like* to shove all of this crud down into gfxPlatform, but not in this bug.
Attachment #350896 - Attachment is obsolete: true
Attachment #353877 - Flags: review?(vladimir)
Attachment #350896 - Flags: review?(vladimir)
Since we have a workaround, it's not necessary for this to depend on bug 468496 anymore.
No longer depends on: 468496
Comment on attachment 353877 [details] [diff] [review]
part 2, rev 3: nsIImage::Extract and nsMargin refactor (works around bug 468496)

Erm, what?  This patch seems to change all sorts of stuff, and I don't understand why.


>+  /**
>+   * Extract a rectangular region of the nsIImage and return it as a new
>+   * nsIImage.
>+   * @param aSubimage  the region to extract
>+   * @param aResult    the extracted image
>+   */
>+  virtual nsresult Extract(const nsIntRect& aSubimage,
>+                           nsIImage** aResult NS_OUTPARAM) = 0;
>+

Add this to the end of the interface (not that nsIImage is all that public, but still).

>--- a/gfx/public/nsMargin.h
>+++ b/gfx/public/nsMargin.h

Why are these nsMargin changes here at all?  It just seems like formatting etc. changes.  Are there any functional changes?

>diff --git a/gfx/src/thebes/nsThebesImage.cpp b/gfx/src/thebes/nsThebesImage.cpp
>--- a/gfx/src/thebes/nsThebesImage.cpp
>+++ b/gfx/src/thebes/nsThebesImage.cpp
>@@ -41,54 +41,48 @@
> 
> #include "gfxContext.h"
> #include "gfxPattern.h"
> 
> #include "gfxPlatform.h"
> 
> #include "prenv.h"
> 
>-static PRBool gDisableOptimize = PR_FALSE;

Why are you getting rid of this?  Oh, moving parts of it into Optimize?  I guess that's fine, but there's no need to change this.  Joe is busy collapsing a bunch of these image classes anyway, so any major cleanup here is just going to conflict for no gain.

> nsresult
>-nsThebesImage::Init(PRInt32 aWidth, PRInt32 aHeight, PRInt32 aDepth, nsMaskRequirements aMaskRequirements)
>+nsThebesImage::Init(PRInt32 aWidth, PRInt32 aHeight, PRInt32 aDepth,
>+                    nsMaskRequirements aMaskRequirements)
> {
>     mWidth = aWidth;
>     mHeight = aHeight;
> 
>     // Reject over-wide or over-tall images.
>     if (!AllowedImageSize(aWidth, aHeight))
>         return NS_ERROR_FAILURE;
> 
>@@ -117,43 +111,36 @@ nsThebesImage::Init(PRInt32 aWidth, PRIn
>         default:
>             format = gfxImageSurface::ImageFormatRGB24;
>             mAlphaDepth = 0;
>             break;
>     }
> 
>     mFormat = format;
> 
>-#ifdef XP_WIN
>-    if (!ShouldUseImageSurfaces()) {
>-        mWinSurface = new gfxWindowsSurface(gfxIntSize(mWidth, mHeight), format);
>-        if (mWinSurface && mWinSurface->CairoStatus() == 0) {
>-            // no error
>-            mImageSurface = mWinSurface->GetImageSurface();
>-        }
>-    }
>-
>-    if (!mImageSurface)
>-        mWinSurface = nsnull;
>-#endif

Again, why are you getting rid of this?  There's a specific reason why win32 surfaces have to be created in this way (so that we can extract an image surface that points to the same pixels in the internal win32 DIB).  Creating a gfxImageSurface instead removes that optimization.

The rest of the code seems ok, though I'm still wondering why we can't step back and figure out what we need to fix inside cairo to get the behaviour that we want, to avoid having to do all this stuff in layers above.  Ideally we would start with a pure cairo testcase that shows the problem on the multiple platforms, and then we'd figure out what's needed to change internally to fix the problem.
Attachment #353877 - Flags: review?(vladimir) → review-
(In reply to comment #21)
> (From update of attachment 353877 [details] [diff] [review])
> Erm, what?  This patch seems to change all sorts of stuff, and I don't
> understand why.

The critical part of this is the new method, nsIImage::Extract().  All else is either cleanups done in passing, or workarounds for cairo problems.

> >+  virtual nsresult Extract(const nsIntRect& aSubimage,
> >+                           nsIImage** aResult NS_OUTPARAM) = 0;
> >+
> 
> Add this to the end of the interface (not that nsIImage is all that
> public, but still).

Ok.

> Why are these nsMargin changes here at all?

To quote comment 11,

# [This piece] refactors nsMargin a bit so that nsIntMargin has
# all the same methods as nsMargin even when NSCOORD_IS_FLOAT is
# true. Since NSCOORD_IS_FLOAT is not a functional mode right now,
# that part isn't actually necessary, but I didn't realize that
# until after I'd done it.

I would be happy to pull that out to a separate bug entirely, if you prefer.

> >-static PRBool gDisableOptimize = PR_FALSE;
> 
> Why are you getting rid of this?  Oh, moving parts of it into Optimize?

Yeah, the thought was that as long as I was messing with the nsIImage constructor, there was no need for anything other than ::Optimize to be aware of the environment variable.

> Joe is busy collapsing a bunch of these image classes anyway, 

Bug number?  I'd be curious to see what it ends up looking like.  The ifdeffage in here makes my chain saw hand itchy.

> so any major cleanup here is just going to conflict for no gain.

I can take this back out too.

> >-#ifdef XP_WIN
> >-    if (!ShouldUseImageSurfaces()) {
> >-        mWinSurface = new gfxWindowsSurface(gfxIntSize(mWidth, mHeight), format);
> >-        if (mWinSurface && mWinSurface->CairoStatus() == 0) {
> >-            // no error
> >-            mImageSurface = mWinSurface->GetImageSurface();
> >-        }
> >-    }
> >-
> >-    if (!mImageSurface)
> >-        mWinSurface = nsnull;
> >-#endif
> 
> Again, why are you getting rid of this?  There's a specific reason why win32
> surfaces have to be created in this way (so that we can extract an image
> surface that points to the same pixels in the internal win32 DIB).  Creating a
> gfxImageSurface instead removes that optimization.

Isn't it functionally equivalent to create an image surface and then wrap it with a win32 surface, as is done below this point after the changes?  The point of the rearrangement is to control all of these optimizations with one if block.

> The rest of the code seems ok, though I'm still wondering why we can't step
> back and figure out what we need to fix inside cairo to get the behaviour that
> we want, to avoid having to do all this stuff in layers above.  Ideally we
> would start with a pure cairo testcase that shows the problem on the multiple
> platforms, and then we'd figure out what's needed to change internally to fix
> the problem.

The new ::Extract method is necessary regardless, unless and until cairo adds the ability for a pattern to draw from a subregion of a surface.

The Cairo/X11 bugs that I'm working around with the "never use device surface" flag are discussed in bug 468496.  I think it would be pretty easy to extend the "stretch single pixel" test case attached to that bug to test for Cairo/Mac bugs, too, but I don't have a Mac.
(In reply to comment #22)
> > Joe is busy collapsing a bunch of these image classes anyway, 
> 
> Bug number?  I'd be curious to see what it ends up looking like.  The ifdeffage
> in here makes my chain saw hand itchy.

Don't think there is one yet.

> > >-#ifdef XP_WIN
> > >-    if (!ShouldUseImageSurfaces()) {
> > >-        mWinSurface = new gfxWindowsSurface(gfxIntSize(mWidth, mHeight), format);
> > >-        if (mWinSurface && mWinSurface->CairoStatus() == 0) {
> > >-            // no error
> > >-            mImageSurface = mWinSurface->GetImageSurface();
> > >-        }
> > >-    }
> > >-
> > >-    if (!mImageSurface)
> > >-        mWinSurface = nsnull;
> > >-#endif
> > 
> > Again, why are you getting rid of this?  There's a specific reason why win32
> > surfaces have to be created in this way (so that we can extract an image
> > surface that points to the same pixels in the internal win32 DIB).  Creating a
> > gfxImageSurface instead removes that optimization.
> 
> Isn't it functionally equivalent to create an image surface and then wrap it
> with a win32 surface, as is done below this point after the changes?  The point
> of the rearrangement is to control all of these optimizations with one if
> block.

There's no way to wrap an image surface with a win32 surface.  The surface constructors that take a cairo_surface_t* really should all be protected; they're only used for initializing a new wrapper.  Doing that would require them to all be friends with gfxASurface, but that's probably cleaner in the long run.

> > The rest of the code seems ok, though I'm still wondering why we can't step
> > back and figure out what we need to fix inside cairo to get the behaviour that
> > we want, to avoid having to do all this stuff in layers above.  Ideally we
> > would start with a pure cairo testcase that shows the problem on the multiple
> > platforms, and then we'd figure out what's needed to change internally to fix
> > the problem.
> 
> The new ::Extract method is necessary regardless, unless and until cairo adds
> the ability for a pattern to draw from a subregion of a surface.

Yep, we should get that on the cairo table -- ideally it would just be a new pattern type that takes another pattern and a subregion.  But yeah, I'm ok with Extract, we should just attack this problem at a lower level sometime soon.

> The Cairo/X11 bugs that I'm working around with the "never use device surface"
> flag are discussed in bug 468496.  I think it would be pretty easy to extend
> the "stretch single pixel" test case attached to that bug to test for Cairo/Mac
> bugs, too, but I don't have a Mac.
revised as requested.  Should I bother filing a bug for the nsMargin/nsIntMargin changes, given how broken NSCOORD_IS_FLOAT is right now?
Attachment #353877 - Attachment is obsolete: true
Attachment #353912 - Flags: review?(vladimir)
Comment on attachment 353912 [details] [diff] [review]
part 2, rev 4: nsIImage::Extract (no more nsMargin changes, vlad's comments addressed)
[Checkin: See comment 39 & 49]

Yeah, that looks fine; thanks!  For the nsMargin stuff, sure, though I think there are a lot more NSCOORD_IS_FLOAT type things floating around out there.
Attachment #353912 - Flags: review?(vladimir) → review+
Attachment #353912 - Attachment description: part 3, rev 4: nsIImage::Extract (no more nsMargin changes, vlad's comments addressed) → part 2, rev 4: nsIImage::Extract (no more nsMargin changes, vlad's comments addressed)
Here's what I hope is the final iteration of the changes to DrawBorderImage itself.  This should properly handle the innerRect and repeat/round.  However, I'm not sure how much either of those are tested by the reftests we've got for border-image.  Also, the seaming regression is gone.

These three patches, together, pass reftests on Linux.  I have limited ability to test on Windows (i.e. I can do it but it takes a very long time due to virtual machine slowness) and no ability to test on Mac.
Attachment #350897 - Attachment is obsolete: true
Attachment #354358 - Flags: review?(roc)
+  // (For vector images the split points would be in CSS pixels.)

I'm not sure what this means. For SVG images, we would define one image pixel to be one CSS pixel *of the SVG document*, which could be different from CSS pixels in the document containing the image. So this comment could be misleading and I think you should just remove it.

+  NS_ASSERTION(innerRect.width == img->GetWidth(), "img inner width off");
+  NS_ASSERTION(innerRect.height == img->GetHeight(), "img inner height off");

Why are these valid?

+      case eStyleUnit_Percent:
+        if (s == NS_SIDE_TOP || s == NS_SIDE_BOTTOM)
+          split.side(s) = coord.GetPercentValue() * imageSize.height;
+        else
+          split.side(s) = coord.GetPercentValue() * imageSize.width;
+        break;
+      case eStyleUnit_Factor:
+        split.side(s) = coord.GetFactorValue();
+        break;

There's rounding going on here. I'd prefer it to be explicit, using NS_lround probably. And it's probably also worth using common subexpressions so you can write something like

   PRInt32 imgDimension = (s == NS_SIDE_TOP || s == NS_SIDE_BOTTOM) ? imageSize.height : imageSize.width;
   double dim;
   switch (...) {
   case eStyleUnit_Percent:
     dim = coord.GetPercentValue()*imgDimension;
     break;
   case eStyleUnit_Factor:
     dim = coord.GetFactorValue();
     break;
   }
   if (dim < 0) {
     dim = 0;
   } else if (dim > imgDimension) {
     dim = imgDimension;
   }
   split.side(s) = NS_lround(dim);

This table-driven code looks better than what was there, but I think it could be improved by making the main loop a nested i/j loop from 0 to 2 and 0 to 2. Then destAreas and subAreas would not be necessary, since those rects can be trivially computed at each iteration. fillStyleH and fillStyleV can then be replaced with a simple "j == 1 ? aBorderStyle.mBorderImageHFill : NS_STYLE_BORDER_IMAGE_STRETCH" etc. unitSize's components can each be written in the form "j != 1 ? borderWidth[j] : (i != 1 ? borderHeight[i] : splitWidth[i]*hFactor)". I suppose an enum would be better than 0,1,2 though.

+  for (int i = 0; i < 9; i++)
+    DrawBorderImageComponent(aRenderingContext, img, aDirtyRect,
+                             destAreas[i], subAreas[i],
+                             fillStyleH[i], fillStyleV[i], unitSize[i]);

Put the loop body in {}

+  nsRect aScale;

Don't start the name with 'a' since it's not a parameter. Plus the name is not very descriptive. I would call this "nsRect dest;" and rename the "aDest" parameter to "aFill".

+  switch (aHFill) {
+    case NS_STYLE_BORDER_IMAGE_STRETCH:
+      aScale.y = aDest.y;

Shouldn't that be "aVFill"? If this really passed tests, please add a test it wouldn't have passed...
(In reply to comment #27)
> +  NS_ASSERTION(innerRect.width == img->GetWidth(), "img inner width off");
> +  NS_ASSERTION(innerRect.height == img->GetHeight(), "img inner height off");
> 
> Why are these valid?

Is it not an invariant of the image classes that the rectangle produced by gfxIImageFrame::GetRect() precisely encloses the pixels of the nsIImage that you can GetInterface() from that gfxIImageFrame?  These assertions check that invariant.  I've added a comment to that effect.

All your other comments should be addressed in this new revision.
Attachment #354358 - Attachment is obsolete: true
Attachment #356275 - Flags: review?(roc)
Attachment #354358 - Flags: review?(roc)
(In reply to comment #28)
> Is it not an invariant of the image classes that the rectangle produced by
> gfxIImageFrame::GetRect() precisely encloses the pixels of the nsIImage that
> you can GetInterface() from that gfxIImageFrame?  These assertions check that
> invariant.  I've added a comment to that effect.

I don't think so.  I think imagelib currently exposes animated images where some subframes only cover parts of the image to the callers through its API.  I wish it didn't, though.
(In reply to comment #29)
> (In reply to comment #28)
> > Is it not an invariant of the image classes that the rectangle produced by
> > gfxIImageFrame::GetRect() precisely encloses the pixels of the nsIImage that
> > you can GetInterface() from that gfxIImageFrame?  These assertions check that
> > invariant.  I've added a comment to that effect.
> 
> I don't think so.  I think imagelib currently exposes animated images where
> some subframes only cover parts of the image to the callers through its API.  I
> wish it didn't, though.

Uh, as far as I can tell, that's exactly what GetRect() does.  The way I think it works is, if you've got an imgIContainer for an animated image, and some subframes only cover part of the image, then the gfxIImageFrame for each of those frames is going to report its own width and height equal to all the other frames, but if you convert it to an nsIImage, that will report a smaller width and height, and the rectangle returned by GetRect() tells you where to place the nsIImage within the larger image.  So that rectangle should always have width and height equal to the nsIImage's reported width and height.

I haven't tried to use an animated gif for a border image though (and I'm not sure how to create one with this particular property, anyway).
I think I was just confusing the imgIContainer/gfxImageFrame relationship with the gfxImageFrame/nsIImage relationship.
If I've got it wrong, it's not just a problem with the assertions - there's code to compensate for this imagelib wart that would be wrong too.
Attached patch part 3, rev 4a: rediffed (obsolete) — Splinter Review
I'm not sure what went into trunk to cause the patch to be inapplicable, but here it is rediffed.
Attachment #356275 - Attachment is obsolete: true
Attachment #356627 - Flags: review?(roc)
Attachment #356275 - Flags: review?(roc)
Status: NEW → ASSIGNED
+        unitSize.height = splitHeight[j]*hFactor;

Shouldn't this be vFactor? Currently vFactor is unused... Sounds like we need another test here to cover that issue!
part 1 needed rediffing after some churn in nsLayoutUtils.h.
Attachment #358240 - Flags: superreview+
Attachment #358240 - Flags: review+
Attachment #349991 - Attachment is obsolete: true
(In reply to comment #35)
> +        unitSize.height = splitHeight[j]*hFactor;
> 
> Shouldn't this be vFactor? Currently vFactor is unused... Sounds like we need
> another test here to cover that issue!

Yeah, that was supposed to be vFactor.  This revision fixes that and adds tests for the center image scaling.  Unfortunately I couldn't think of any way to do the reference other than a fixed PNG.
Attachment #356627 - Attachment is obsolete: true
Attachment #358242 - Flags: review?(roc)
Attachment #356627 - Flags: review?(roc)
Flags: wanted1.9.1+
Flags: blocking1.9.1-
Flags: blocking1.9.1+
Pushed part 1 to trunk: http://hg.mozilla.org/mozilla-central/rev/fed00e0efe5c. I plan to land the other parts over the next few days.
Pushed part 2: http://hg.mozilla.org/mozilla-central/rev/ecc20943472d

I would have pushed part 3 as well but I'm a bit afraid the reftests will fail and I already have some slightly risky stuff in this push.
Pushed part 3: http://hg.mozilla.org/mozilla-central/rev/a142df653655
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Keywords: checkin-needed
Whiteboard: [needs 191 landing]
Blocks: 445911
Bad news: the failures that appear on both Mac and Windows are down to the workarounds for the X video driver issues with scaling.  Specifically: if I change nsThebesImage::Draw() to always use nearest-neighbor scaling, irrespective of platform, then the failures of center-scaling-[23].html go away.

There is no workaround for this, unless you can think of a way to duplicate the visual effect that border-image produces without using border-image or a reference PNG.  I can't.

IMO we need to find a better workaround for those scaling problems.  I have an idea, which I will describe in bug 468496 either later tonight or tomorrow morning.
No longer blocks: 445911
You could hand-split the image into various subimages and then compose them together using a set of positioned <img> images, which will let you do the scaling. If that doesn't work, maybe CSS transforms to do the scaling, or even SVG.
This revision just changes the center-scaling* tests to use HTML references, with the border image manually cut up into its nine components and tiled with <img width=X height=Y> scaling.  All the border-image reftests now pass on Windows as well as Linux.  I also took out the XFAILs for the tests that are now passing on Mac.
Attachment #358242 - Attachment is obsolete: true
Attachment #362367 - Flags: review+
Committers: please try to re-land "part 3, rev 5a" only.
Keywords: checkin-needed
Whiteboard: [needs landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/749dfebefd83
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Whiteboard: [needs 191 landing]
Depends on: 477732
(In reply to comment #46)
> Pushed http://hg.mozilla.org/mozilla-central/rev/749dfebefd83

This has broken the display of tabs in chrome parts: add-on manager, error console, the main window tabs of the Grapple theme. See bug 477732.
No longer depends on: 477732
Depends on: 477732
Depends on: 479156
Comment on attachment 358240 [details] [diff] [review]
part 1, rev 2a: rediffed
[Checkin: Comment 38 & 48]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f6dee06e4fd7
Attachment #358240 - Attachment description: part 1, rev 2a: rediffed → part 1, rev 2a: rediffed [Checkin: Comment 38 & 48]
Comment on attachment 353912 [details] [diff] [review]
part 2, rev 4: nsIImage::Extract (no more nsMargin changes, vlad's comments addressed)
[Checkin: See comment 39 & 49]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bb6ce950bc11

after fixing context for
{
patching file gfx/src/thebes/nsThebesImage.h
Hunk #2 FAILED at 153
1 out of 2 hunks FAILED
}
Attachment #353912 - Attachment description: part 2, rev 4: nsIImage::Extract (no more nsMargin changes, vlad's comments addressed) → part 2, rev 4: nsIImage::Extract (no more nsMargin changes, vlad's comments addressed) [Checkin: See comment 39 & 49]
Attachment #362367 - Attachment description: part 3, rev 5a: hand-tiled image grids for reference (fixes Windows failures) → part 3, rev 5a: hand-tiled image grids for reference (fixes Windows failures) [Checkin: Comment 46]
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing] → [needs 191 landing: part 3] [fixed1.9.1b3]
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 362367 [details] [diff] [review]
part 3, rev 5a: hand-tiled image grids for reference (fixes Windows failures)
[Checkin: Comment 46]

Does not apply to 1.9.1:
{
patching file layout/base/nsCSSRendering.cpp
Hunk #1 FAILED at 260
Hunk #3 FAILED at 1713
2 out of 3 hunks FAILED
}
Here's part 3 rediffed to apply to the 1.9.1 branch; no functional changes were necessary.  I get bizarre errors ("REFTEST TEST-UNEXPECTED-FAIL | | EXCEPTION: ReferenceError: xulRuntime is not defined") when I try to run the reftests, so this has not actually been tested.
Comment on attachment 363805 [details] [diff] [review]
part 3 for 1.9.1 branch
[Checkin: Comment 52]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/70f75e6e8fcf
Attachment #363805 - Attachment description: part 3 for 1.9.1 branch → part 3 for 1.9.1 branch [Checkin: Comment 52]
Whiteboard: [needs 191 landing: part 3] [fixed1.9.1b3] → [fixed1.9.1b3]
dbaron clued me in on IRC as to what I was doing wrong.  The 1.9.1 branch reftests pass with the above patch applied.
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.