This is a piece of bug 431176, covering the style system changes required to parse the properties as specified in http://dev.w3.org/csswg/css3-background/#the-border-radius and make sure the values get to the border renderer. I'll be attaching a patch series shortly.

This large mechanical patch renames the existing -moz-border-radius* and -moz-outline-radius* CSS properties, and the corresponding DOM properties, in line with what is currently specified at http://dev.w3.org/csswg/css3-background/#the-border-radius: -moz-border-radius-topleft becomes -moz-border-top-left-radius and so on. (outline-* are not in the spec but it seems best to be consistent.)

I forgot to run hg qrefresh before uploading the original version of this patch. This version includes all the test suite changes that are necessary.

The patch looks fine. The nsCSSLoader.h change perhaps belongs in a different patch (at least in a separate changeset); I don't mind the change, though. However, I think renaming twice is a bad idea. Given that this draft should be expected to be in CR relatively soon, I'd rather not change from our current names to -moz-[draft names] and then almost immediately thereafter to [draft names] without -moz-. So I think I'd actually rather hold off on the property renaming here. There's also the question of whether we want to alias the old names to the new ones. I think we should avoid doing that unless we have a strong sense that Web content depends on it. However, in this case, we might be trying to avoid breaking themes, which would dictate that we do add aliases.

Comment on attachment 333983 [details] [diff] [review] part 1 (rev 2): property names (with test suite corrections) I'm going to mark review- simply because I think CR is close enough that we should wait for it to do the rename rather than rename twice, although fantasai might disagree.

(Note that we should also wait for correctly implementing what the CR says, but I think you have other patches floating around to get us closer to that.)

No, I agree.

OK, so I'm now leaving the property names alone -- I'll spin off another bug to change directly from the names we have now to the standard names, and make a note that it needs to wait both for this bug and for a CR. This patch rearranges data structures and the border-rendering interface so that the CSS parser *can* provide pairs of coordinates. However, the parser still *doesn't*. There were a bunch of changes needed to the parser but they should have no effect on the grammar. Unfortunately, there's a severe bug in here somewhere: on startup I get a bunch of these ###!!! ASSERTION: reading uninitialized value: 'mUnit != eStyleUnit_Null', file ../../dist/include/layout/nsStyleCoord.h, line 88 and then a crash inside gtk+. I'm currently quite stumped on the problem - the assertions are triggered by nsLayoutUtils::HasNonZeroSide() (which now has a variant that works on nsStyleCorners objects) and I don't see why that didn't cause exactly the same assertions when it was getting nsStyleSides objects. I haven't really looked into the crash but I would suspect it has the same ultimate cause... Anyway, any debugging help anyone has time to provide would be much appreciated.

This revision passes all the layout mochitests and most of the reftests. There are still a handful of reftest failures: box-properties/CSS21-t100303-simple.xhtml box-properties/CSS21-t100303.xhtml bugs/368020-1.html bugs/403181-1.xml bugs/421069.html bugs/421069-ref.html bugs/421885-1.xml pixel-rounding/background-image-tiling.html I think most of these are problems with background-position. It's most obvious with bugs/421069.html, where 'background-position: <whatever>' is clearly being treated as 'background-position: top left'. But I can't figure out why; some of this stuff comes close to the code for background-position, but it doesn't seem like it should be breaking it.

+ static PRBool HasNonZeroSide(const nsStyleCorners& aCorners); I'd call this HasNonZeroCorner.

Here's an example of what breaks. In FF3 this will render as a zigzag line, sort of a blocky backwards "C". With the patch, you get an "F" with four crossbars.

... same testcase but not dependent on a PNG file on my local hard disk this time.

(In reply to comment #9) > + static PRBool HasNonZeroSide(const nsStyleCorners& aCorners); > I'd call this HasNonZeroCorner. Changed.

Here's the final version of part 1 of the patch. It passes the reftests and the layout subtree of the mochitests. I'll run the full mochitests overnight. Thanks to roc for helping me debug the last set of reftest failures. This patch has suffered a bit of mission creep. roc's suggestion to change the new HasNonZeroSide variant to HasNonZeroCorner exposed that the old variant was now unused, so I took it out. Also, I had to revise PaintRoundedBackground pretty drastically to keep it working, and then I noticed that the 'padding' argument to that function was completely unused, and transitively the same for PaintBackground/PaintBackgroundWithSC. So a bunch of files are touched just to take that argument out.

Part 2, for the record, will be to make nsCSSParser recognize two arguments to the *radius* longhand properties and the a b c d / e f g h notation to the shorthand properties. And will include test cases for that.

Here's the parser changes. Test cases are still in development - automating tests for this is nontrivial - but I've put it through its paces a little and it seems to work.

Comment on attachment 334810 [details] [diff] [review] new part 1 (rev 1): data structures So I started looking at this on the airplane yesterday, but I'm probably going to come back to it after looking at Keith's transforms patch. Here are the comments I have so far: > This patch has suffered a bit of mission creep. roc's suggestion to change the > new HasNonZeroSide variant to HasNonZeroCorner exposed that the old variant was > now unused, so I took it out. Also, I had to revise PaintRoundedBackground > pretty drastically to keep it working, and then I noticed that the 'padding' > argument to that function was completely unused, and transitively the same for > PaintBackground/PaintBackgroundWithSC. So a bunch of files are touched just to > take that argument out. For what it's worth, when this happens, if you're using mq, it's often useful to let the mission creep into a series of stacked patches rather than a single patch; it's easier to review, and easier to take a piece out later if it turns out to be unwanted. Local style in nsHTMLHRElement is to use braces for one line blocks. Please follow it. Also, maybe add an nsCSSCornerSizes::halfCorners with all 8 members, and use it to do this in a loop? >+ PRUint32 c = side*2 + side%2; If you're doing this, please PR_STATIC_ASSERT your assumptions about the side constants and the corner constants right next to it. (At least assert that each is its current value, just to document that you're assuming that, if you don't want to document the exact assumptions more precisely.) >+ gfxFloat Li = side%2 ? maxHeight : maxWidth; >+ gfxFloat Si = radii[c % 8] + radii[(c+2) % 8]; Could you call them |length| and |sum| instead of |Li| and |Si|? >+ nsStyleCoord c = aBorderRadius.Get(i); Maybe use const? > default: >+ aTwipsRadii[i] = 0; Maybe an NS_NOTREACHED for this default case? > IsSolidBorder(const nsStyleBorder& aBorder) > { >- if (nsLayoutUtils::HasNonZeroSide(aBorder.mBorderRadius) || aBorder.mBorderColors) >+ if (nsLayoutUtils::HasNonZeroCorner(aBorder.mBorderRadius) >+ || aBorder.mBorderColors) Mozilla style is to put the || at the end of the line rather than the start. Exposing ComputeInnerRadii as a global function isn't ideal. Could it be a static method on a class? >+ // ??? if a corner is nonzero in only one dimension, we might get >+ // away with treating it as zero. unlikely to happen outside of >+ // contrived test cases. We don't really have a habit of using "???"; I'd say drop it. And could you make the second sentence a complete sentence, and capitalize both? And also document the current behavior in the header (in fact, perhaps move this comment there?). NEED TO GO BACK TO: PaintBackgroundColor and PaintRoundedBackground UP TO nsCSSParser.cpp

(In reply to comment #16) > (From update of attachment 334810 [details] [diff] [review]) > So I started looking at this on the airplane yesterday, but I'm probably going > to come back to it after looking at Keith's transforms patch. That's fine. I will respond to your comments now but I probably won't do the revision just yet. > For what it's worth, when this happens, if you're using mq, it's often > useful to let the mission creep into a series of stacked patches rather > than a single patch; it's easier to review, and easier to take a piece > out later if it turns out to be unwanted. I haven't yet figured out how to split patches with mq but I will. Would you like me to separate the removal-of-unused-padding-argument change from everything else? > Local style in nsHTMLHRElement is to use braces for one line blocks. > Please follow it. Also, maybe add an nsCSSCornerSizes::halfCorners with > all 8 members, and use it to do this in a loop? Ok. > >+ PRUint32 c = side*2 + side%2; > > If you're doing this, please PR_STATIC_ASSERT your assumptions about the > side constants and the corner constants right next to it. Ok. > >+ gfxFloat Li = side%2 ? maxHeight : maxWidth; > >+ gfxFloat Si = radii[c % 8] + radii[(c+2) % 8]; > > Could you call them |length| and |sum| instead of |Li| and |Si|? Sure, but I picked those names to match css3-background's description of the algorithm. > > >+ nsStyleCoord c = aBorderRadius.Get(i); > Maybe use const? sure. > > default: > >+ aTwipsRadii[i] = 0; > > Maybe an NS_NOTREACHED for this default case? It can get there with eCSSUnit_Null. Is default: NS_NOTREACHED("unexpected unit in GetBorderRadiusTwips"); case eCSSUnit_Null: aTwipsRadii[i] = 0; too clever? > Mozilla style is to put the || at the end of the line rather than the start. Oops. Will fix. > Exposing ComputeInnerRadii as a global function isn't ideal. Could it > be a static method on a class? I suppose it could be a static method on nsCSSBorderRenderer, would that do? > >+ // ??? if a corner is nonzero in only one dimension, we might get > >+ // away with treating it as zero. unlikely to happen outside of > >+ // contrived test cases. > > We don't really have a habit of using "???"; I'd say drop it. And could > you make the second sentence a complete sentence, and capitalize both? > And also document the current behavior in the header (in fact, perhaps > move this comment there?). Sure.

(In reply to comment #17) > I haven't yet figured out how to split patches with mq but I will. Would you > like me to separate the removal-of-unused-padding-argument change from > everything else? It's not worth the work to separate for this one; I've already gone through the mental work in this case, which wasn't too much for this one. > > Could you call them |length| and |sum| instead of |Li| and |Si|? > > Sure, but I picked those names to match css3-background's description of the > algorithm. But it's clear from the description that they stand for length and sum, which are slightly more informative names (although not much). > > > default: > > >+ aTwipsRadii[i] = 0; > > > > Maybe an NS_NOTREACHED for this default case? > > It can get there with eCSSUnit_Null. Is > > default: > NS_NOTREACHED("unexpected unit in GetBorderRadiusTwips"); > case eCSSUnit_Null: > aTwipsRadii[i] = 0; > > too clever? This takes eStyleUnit_*, not eCSSUnit_*, and I'm not sure why anything other than length or percent should be in the computed style data. (There is an nsStyleUnit_Null, but I'm not even sure it should exist. It really is rather like a null pointer in that it shouldn't happen... although maybe we've used it in strange ways in a few places.) > > Exposing ComputeInnerRadii as a global function isn't ideal. Could it > > be a static method on a class? > > I suppose it could be a static method on nsCSSBorderRenderer, would that do? Sure.

(In reply to comment #18) > This takes eStyleUnit_*, not eCSSUnit_*, and I'm not sure why anything other > than length or percent should be in the computed style data. I think I was just reasoning by analogy with the nsRuleNode code -- I have no evidence that eStyleUnit_Null shows up here. I'll make your originally suggested change and watch for assertions while developing test cases.

Comment on attachment 334810 [details] [diff] [review] new part 1 (rev 1): data structures I wonder (not really related to this patch, though) whether PaintBackgroundColor ever gets passed nonzero radii when it's drawing the background for the root element. If it's a case where we're expected to draw something to fill the widget, that might be a problem... Why did you rename the parameter to PaintRoundedBackground from aBGClipArea to aBorderArea when you didn't rename the same thing in PaintBackgroundColor. Which one is it? (It looks to me like it's actually the background-clip area, which means some of your new code in PaintRoundedBackground is wrong. I'm pretty sure there's something wrong here, although I'm not sure what the right way to fix it is. I'll need to look at this closely in the revised patch... nsCSSParser.cpp: >+ NS_FOR_CSS_SIDES(side) { >+ (aRadii.*nsCSSCornerSizes::corners[side]) >+ .SetBothValuesTo(temp.*nsCSSRect::sides[side]); >+ mTempData.SetPropertyBit(aPropIDs[side]); >+ } although I think it's going away in patch 2, you probably want some PR_STATIC_ASSERTs here about equivalence between side constants and corner constants. >+ { >+ nsCSSValue temp; >+ if (ParsePositiveVariant(aErrorCode, temp, VARIANT_HLP, nsnull) >+ == PR_FALSE) >+ return PR_FALSE; >+ NS_ASSERTION(nsCSSProps::kTypeTable[aPropID] == eCSSType_ValuePair, >+ nsPrintfCString(64, "type error (property='%s')", >+ nsCSSProps::GetStringValue(aPropID).get()).get()); >+ nsCSSValuePair& storage = >+ *static_cast<nsCSSValuePair*>(mTempData.PropertyAt(aPropID)); >+ storage.SetBothValuesTo(temp); >+ mTempData.SetPropertyBit(aPropID); >+ return PR_TRUE; >+ } this probably ought to be its own function. Also, use ! rather than "== PR_FALSE". nsCSSStruct.cpp: +/* static */ const nsCSSCornerSizes::corner_type +nsCSSCornerSizes::corners[4] = { This should be preceded by PR_STATIC_ASSERTS about the values of the corner constants. nsCSSStruct.h: + // argument is an NS_CORNER_* constant from nsStyleConsts.h + nsCSSValue const & GetSingleCoord(PRUint32 aCorner) const { + if (aCorner % 2 == 0) + return (this->*corners[aCorner/2]).mXValue; + else + return (this->*corners[aCorner/2]).mYValue; + } This would be simpler given the halfCorners array I proposed above, I think. nsComputedDOMStyle.cpp: I think things might be a bit cleaner if GetOutlineRadiusFor and GetBorderRadiusFor just took two corner constants instead of one side constant. Your handling of failure cases in GetEllipseRadii isn't quite right, in just one edge case -- if the valueList->AppendCSSValue(valY) hits allocation failure, you'll double-free valX. I think the best fix is to rewrite so that the error cases are early returns and the normal case runs to the end (which I think is more readable anyway). You can still rely on delete null being safe by doing: if (!valX || !valueList->AppendCSSValue(valX)) { delete valX; delete valueList; return NS_ERROR_OUT_OF_MEMORY; } nsStyleCoord.cpp: In COMPARE_INDEXED_COORD, could you put the backslashes in the 79th column? Could you make the ToString and AppendToString methods of nsStyleCorners #ifdef DEBUG ? (If not, why not?) And maybe even do the same for nsStyleCoord? nsStyleCoord.h: nsStyleCorners::operator!= does not need the PRBool cast (and please remove it from nsStyleSides too). nsStyleStruct.h: Could you keep the "//" comments lined up for the lines that you're changing? (But don't change other lines.)

Comment on attachment 334986 [details] [diff] [review] part 2 (rev 1): parser changes r+sr=dbaron (although I asked in my review of the other patch that the ParseProperty code you're changing move into its own function)

dbaron: You suggested a nsCSSCorners::halfCorners[] array of pointers-to-members to simplify the nsHTMLHRElement code and elsewhere. I tried to do this but I got stuck, because: - we have to use nsCSSValuePairs in nsCSSCornerSizes so that each CSS property corresponds to just one field of a CSS struct, because this is what nsCSSPropList.h expects; - the compiler will not let me take a pointer-to-member to e.g. mTopLeft.mXValue (where mTopLeft is a nsCSSValuePair) using the obvious notation, i.e. typedef nsCSSValue nsCSSCornerSizes::*half_corner_type; ... /* static */ const nsCSSCornerSizes::half_corner_type nsCSSCornerSizes::halfCorners[8] = { &nsCSSCornerSizes::mTopLeft.mXValue // ... }; produces an "invalilid use of non-static data member" error. Do you know any way to make this actually work? Also, regarding the various requests for PR_STATIC_ASSERTs: I propose to stick a macro NS_SIDE_TO_CORNER() in nsStyleConsts.h which takes three arguments: a side number and two booleans, together indicating which of the two coordinates for which of the two corners adjacent to that side you're interested in. This will work for all the places that currently use math on side numbers to get corner numbers, I think. I will then put an exhaustive set of PR_STATIC_ASSERTs for the inputs and outputs of that macro in one place - the bottom of nsStyleCoord.cpp makes sesne to me. How's that?

(In reply to comment #20) > I wonder (not really related to this patch, though) whether > PaintBackgroundColor ever gets passed nonzero radii when it's drawing > the background for the root element. If it's a case where we're > expected to draw something to fill the widget, that might be a > problem... I was able to make bad things happen with background-color:rgba(0,0,0,0) on the HTML element and a BODY element that doesn't fill the viewport - see bug 453566. However, I can get them with or without rounded corners on either BODY or HTML. > Why did you rename the parameter to PaintRoundedBackground from > aBGClipArea to aBorderArea when you didn't rename the same thing in > PaintBackgroundColor. Which one is it? (It looks to me like it's > actually the background-clip area, which means some of your new code in > PaintRoundedBackground is wrong. I'm pretty sure there's something > wrong here, although I'm not sure what the right way to fix it is. I'll > need to look at this closely in the revised patch... Well, I'm not altogether sure what the background-clip area is supposed to be, but as best I can tell by tracing through the code, the rectangle that ultimately gets passed to PaintRoundedBackground is -- usually, anyway; I'm not sure about some of the stuff in PaintBackgroundWithSC -- the border box. Hence having to shrink it down via ComputeInnerRadii.

I've managed to come up with a test case where the interaction between background-clip and border-radius (as it exists now, i.e. circular corners only) is currently buggy. This is what I was trying to fix by messing with ComputeInnerRadii. I've split it off as bug 456219 and propose to lay down the fix (which I'll code probably on Monday) for that first, thus getting that mess out of this patch. Sound good?

This is part 1 rediffed against latest trunk, plus many of dbaron's requested changes. I may have missed some of them; I'll do another sweep tomorrow.

part 2 rediffed against trunk. If there were any requested changes to this piece I have *not* made them, unless I did it several weeks ago and don't remember. Again, I'm going to go through the patches properly tomorrow.

Comment on attachment 334986 [details] [diff] [review] part 2 (rev 1): parser changes forgot to mark this obsolete.

incomplete set of test cases, putting them up so others can see them.

Comment on attachment 341057 [details] [diff] [review] part 1 (rev 2): data structures, rediffed + many of dbaron's requested changes I now believe I have addressed all previous review comments.

Comment on attachment 341058 [details] [diff] [review] part 2 (rev 2): parser rediffed carrying prior review of this patch forward

Here's some more sensible test cases; there is a basic set of "this feature does *something*" reftests, which pass, and a handful of additions to property_database.js in order to test CSSOM round-tripping, which exposes a bug in part 1: given this property -moz-border-radius: 1px / 4px; .getPropertyValue() returns "1px 4px 1px 4px 1px 4px 1px 4px" instead of "1px 1px 1px 1px / 4px 4px 4px 4px" and so the second cycle in test_value_storage fails. I am sure this is an easy fix ... but I can't actually find the code that implements .getPropertyValue() for shorthands.

Ignore previous cry for help, I found the code that needs changing. Revision to part 1 shortly.

Here's what I hope will be the final iteration on part 1; relative to the previous iteration, the only change is to augment the value-serialization logic in nsCSSDeclaration to handle elliptical shorthand borders correctly. As a side effect this fixed some todos, invalidating the previous part 3, so to avoid confusion I'm re-uploading all three patches in order.

parser again; no change.

test case update.

Comment on attachment 341202 [details] [diff] [review] part 1 (rev 3): data structures; now with correct serialization of shorthand props The indentation change in nsComputedDOMStyle::GetOutlineRadiusBottomLeft seems wrong. Is it correct that this code paints the wrong thing when -moz-background-clip: padding is combined with border-radius? Could you file a followup bug on that? (I seem to remember you discussing it.) > + NS_ASSERTION(*vals[0] == *vals[1], > + "Some but not all radii inherit or initial"); > + NS_ASSERTION(*vals[0] == *vals[2], > + "Some but not all radii inherit or initial"); > + NS_ASSERTION(*vals[0] == *vals[3], > + "Some but not all radii inherit or initial"); > + > + AppendCSSValueToString(aProperty, vals[0]->mXValue, aResult); You can't assert these, since there are separate properties. Instead, just mark the relevant tests as known to fail. We need to make a more general fix for this bug, up at the beginning of the function (see where it checks that all subproperties are set) -- but that should be a separate bug. Somehow your diff no longer has the function names (diff -p); it would be good to fix that for next time.

Comment on attachment 341204 [details] [diff] [review] part 3 (rev 3): basic test cases I fail to see how your previous nsCSSDeclaration.cpp changes would have even *compiled* without these changes. Could you explain that to me?

Comment on attachment 341204 [details] [diff] [review] part 3 (rev 3): basic test cases Please put the code changes in this patch into the changeset for patch 1, not here,e so that all the pieces compile on their own. Please remove the tab literals from property_database.js and then indent things correctly. And I think you'll reed to remove the gKnownFails removals per my previous comment. In property_database.js, for the shorthand properties, please also test lists of different lengths before and after the /.

(In reply to comment #36) > (From update of attachment 341202 [details] [diff] [review]) > The indentation change in nsComputedDOMStyle::GetOutlineRadiusBottomLeft > seems wrong. Fixed. > Is it correct that this code paints the wrong thing when > -moz-background-clip: padding is combined with border-radius? Could you > file a followup bug on that? (I seem to remember you discussing it.) That's correct, but it can be demonstrated with circular border radii, so it's already filed - bug 456219, which i have just made not block this bug. > > + NS_ASSERTION(*vals[0] == *vals[1], > > + "Some but not all radii inherit or initial"); > > + NS_ASSERTION(*vals[0] == *vals[2], > > + "Some but not all radii inherit or initial"); > > + NS_ASSERTION(*vals[0] == *vals[3], > > + "Some but not all radii inherit or initial"); > > + > > + AppendCSSValueToString(aProperty, vals[0]->mXValue, aResult); > > You can't assert these, since there are separate properties. Instead, > just mark the relevant tests as known to fail. We need to make a more > general fix for this bug, up at the beginning of the function (see where > it checks that all subproperties are set) -- but that should be a > separate bug. Ok. > Somehow your diff no longer has the function names (diff -p); it would > be good to fix that for next time. I have "diff = --git -p" in .hgrc; I have no idea why it stopped working.

(In reply to comment #37) > (From update of attachment 341204 [details] [diff] [review]) > I fail to see how your previous nsCSSDeclaration.cpp changes would have even > *compiled* without these changes. Could you explain that to me? I was testing with all three patches applied, and I think I clumsily refreshed the code changes into the testsuite patch instead of the code patch, is all.

> You can't assert these, since there are separate properties. Instead, > just mark the relevant tests as known to fail. We need to make a more > general fix for this bug, up at the beginning of the function (see where > it checks that all subproperties are set) -- but that should be a > separate bug. It occurs to me that this means I can't count on all four properties being set, either. Correct?

(In reply to comment #39) > I have "diff = --git -p" in .hgrc; I have no idea why it stopped working. That only applies to the "diff" command, not other commands that do diffing. You want [diff] git = 1 # allowed starting in Mercurial 1.0: showfunc = 1 # allowed starting in Mercurial 1.0.1: unified = 8 (In reply to comment #41) > It occurs to me that this means I can't count on all four properties being set, > either. Correct? No, you can, because of http://hg.mozilla.org/mozilla-central/annotate/b4afc0408353/layout/style/nsCSSDeclaration.cpp#l538 . I'm just saying we need more code there to also deal with inherit/initial, which can only be specified as a whole and not on subparts. (That's the function this stuff is in, right?)

(In reply to comment #42) > [diff] > git = 1 > # allowed starting in Mercurial 1.0: > showfunc = 1 > # allowed starting in Mercurial 1.0.1: > unified = 8 Thanks! > > It occurs to me that this means I can't count on all four properties being > > set, either. Correct? > > No, you can, because of > http://hg.mozilla.org/mozilla-central/annotate/b4afc0408353/layout/style/nsCSSDeclaration.cpp#l538 > . Aha. Missed the implications there. > (That's the function this stuff is in, right?) Yep. Revised patches shortly.

revisions addressing previous review comments.

rediffed mainly to pick up proper hg diff settings. no code changes.

Revised test cases. I'm pinging this for re-review because (a) I forgot to do "hg add" before "hg qrefresh" all this time, so the new reftests weren't actually in the patch; (b) the changes requested in comment 36 did not trigger any test failures that needed to be marked todo (once I took out the removals of todo status that were in rev 3), and I'd like confirmation that that's fine.

Comment on attachment 341217 [details] [diff] [review] part 2 (rev 4): parser; r+sr=dbaron carrying flags forward

Comment on attachment 341218 [details] [diff] [review] part 3 (rev 4): basic test cases Yep, the removal of the removal of the todos was all I expected.

tagging for checkin. (Can this still squeeze in before the feature freeze, given we seem to have a mess in the tree right now?)

http://hg.mozilla.org/mozilla-central/rev/996f4f70b3d7 http://hg.mozilla.org/mozilla-central/rev/5ada96f19485 http://hg.mozilla.org/mozilla-central/rev/f50462bd2599

(In reply to comment #39) > > Is it correct that this code paints the wrong thing when > > -moz-background-clip: padding is combined with border-radius? Could you > > file a followup bug on that? (I seem to remember you discussing it.) > > That's correct, but it can be demonstrated with circular border radii, so it's > already filed - bug 456219, which i have just made not block this bug. But that bug is a regression from this patch, which was the point. This patch removed the code from PaintRoundedBackground that fixed that (although it was always broken for images, I think). I just made it block this bug again. (In reply to comment #36) > You can't assert these, since there are separate properties. Instead, > just mark the relevant tests as known to fail. We need to make a more > general fix for this bug, up at the beginning of the function (see where > it checks that all subproperties are set) -- but that should be a > separate bug. This is bug 376075.

(In reply to comment #51) > (In reply to comment #36) > > You can't assert these, since there are separate properties. Instead, > > just mark the relevant tests as known to fail. We need to make a more > > general fix for this bug, up at the beginning of the function (see where > > it checks that all subproperties are set) -- but that should be a > > separate bug. > > This is bug 376075. Er, sorry, bug 160403.