450652 - Style system changes to support CSS3 border-radius

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Zack Weinberg (:zwol)]

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.

Comment 1

[Zack Weinberg (:zwol)]

Attachment: part 1 (rev 1): bring property names in line with current draft spec

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.)

Comment 2

[Zack Weinberg (:zwol)]

Attachment: part 1 (rev 2): property names (with test suite corrections)

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.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.)

Comment 6

[fantasai]

No, I agree.

Comment 7

[Zack Weinberg (:zwol)]

Attachment: new part 1 (rev 0): data structures -- needs debugging

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.

Comment 8

[Zack Weinberg (:zwol)]

Attachment: new part 1 (rev 0.5) lots of bugs fixed, some remain

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  static PRBool HasNonZeroSide(const nsStyleCorners& aCorners);

I'd call this HasNonZeroCorner.

Comment 10

[Zack Weinberg (:zwol)]

Attachment: sample HTML broken by "new part 1 (rev 0.5)" - simplified version of reftests/bugs/421069.html

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.

Comment 11

[Zack Weinberg (:zwol)]

Attachment: corrected sample HTML broken by "new part 1 (rev 0.5)"

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

Comment 12

[Zack Weinberg (:zwol)]

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

Changed.

Comment 13

[Zack Weinberg (:zwol)]

Attachment: new part 1 (rev 1): data structures

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.

Comment 14

[Zack Weinberg (:zwol)]

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.

Comment 15

[Zack Weinberg (:zwol)]

Attachment: part 2 (rev 1): parser changes

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 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 17

[Zack Weinberg (:zwol)]

(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.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 19

[Zack Weinberg (:zwol)]

(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 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)

Comment 22

[Zack Weinberg (:zwol)]

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?

Comment 23

[Zack Weinberg (:zwol)]

(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.

Comment 24

[Zack Weinberg (:zwol)]

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?

Comment 25

[Zack Weinberg (:zwol)]

Attachment: part 1 (rev 2): data structures, rediffed + many of dbaron's requested changes

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.

Comment 26

[Zack Weinberg (:zwol)]

Attachment: part 2 (rev 2): parser rediffed

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 27

[Zack Weinberg (:zwol)]

Comment on attachment 334986 [details] [diff] [review]
part 2 (rev 1): parser changes

forgot to mark this obsolete.

Comment 28

[Zack Weinberg (:zwol)]

Attachment: part 3 (rev 0): beginnings of test cases

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

Comment 29

[Zack Weinberg (:zwol)]

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 30

[Zack Weinberg (:zwol)]

Comment on attachment 341058 [details] [diff] [review]
part 2 (rev 2): parser rediffed

carrying prior review of this patch forward

Comment 31

[Zack Weinberg (:zwol)]

Attachment: part 3 (rev 1): round-trip and basic "this does something" test cases

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.

Comment 32

[Zack Weinberg (:zwol)]

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

Comment 33

[Zack Weinberg (:zwol)]

Attachment: part 1 (rev 3): data structures; now with correct serialization of shorthand props

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.

Comment 34

[Zack Weinberg (:zwol)]

Attachment: part 2 (rev 3): parser; r+sr=dbaron

parser again; no change.

Comment 35

[Zack Weinberg (:zwol)]

Attachment: part 3 (rev 3): basic test cases

test case update.

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 /.

Comment 39

[Zack Weinberg (:zwol)]

(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.

Comment 40

[Zack Weinberg (:zwol)]

(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.

Comment 41

[Zack Weinberg (:zwol)]

> 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?

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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?)

Comment 43

[Zack Weinberg (:zwol)]

(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.

Comment 44

[Zack Weinberg (:zwol)]

Attachment: part 1 (rev 4): more fixes; r+sr=dbaron

revisions addressing previous review comments.

Comment 45

[Zack Weinberg (:zwol)]

Attachment: part 2 (rev 4): parser; r+sr=dbaron

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

Comment 46

[Zack Weinberg (:zwol)]

Attachment: part 3 (rev 4): basic test cases

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 47

[Zack Weinberg (:zwol)]

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

carrying flags forward

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 49

[Zack Weinberg (:zwol)]

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

Comment 50

[Reed Loden [:reed]]

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

Comment 51

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 52

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.