576044 - Box complex longhand values (eliminate storage types)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Zack Weinberg (:zwol)]

At present, each property value in a CSS data block (compressed or expanded) can have one of five different "storage types": value, value pair, rectangle, value list, or value pair list.  This complicates code that needs to iterate over a data block, and code that constructs values for properties (nsCSSParser and nsStyleAnimation, mainly).  It also led to me introducing an extra indirection in bug 553456, which I think is what made that work not win performance-wise.

This patch series changes things so that every longhand value can be represented by an nsCSSValue object.  The other four storage types become value units.  You will see how much tidier the code becomes.  I have not yet run performance tests, but I will shortly.  With a little more work, nsStyleAnimation::Value might also be removable (it seems to have a great deal of overlap with nsCSSValue, especially after this change); there are also further CSS parser improvements that should be possible (specifically, I hope to get rid of mTempData).

nsCSSValue does not grow.  We do heap allocations when we weren't previously, but only for longhand properties that use a rectangle or value pair for their storage, which are rare.  (*Shorthand* properties mapped to an nsCSSRect or nsCSSValuePair are still stored that way.)

Comment 1

[Zack Weinberg (:zwol)]

Attachment: part 1: move storage classes out of nsCSSStruct.h/nsCSSStruct.cpp

Note: all of these patches have a structural dependency on the complete patch series in bug 569719.

This is prep work - it just moves the "storage classes" (nsCSSRect, nsCSSValuePair, nsCSSValueList, nsCSSValuePairList) from nsCSSStruct.h/.cpp to nsCSSValue.h/.cpp.

Comment 2

[Zack Weinberg (:zwol)]

Attachment: part 2: valuepair

This and the next three patches add nsCSSUnit codes for each of the "complex" values, and cease storing them unboxed.  I did them in ascending order of the number of properties that use them, so we start with ValuePair.

A note about lifetimes.  None of the storage types are presently refcounted, and we want to keep it that way because they're used directly in several places.  However, nsCSSValue refcounts everything else that it holds a pointer to (notably Image and Gradient objects) and it makes life easier if we are consistent with that.  So what I did was introduce a refcounted subclass of each, which is only used internally by nsCSSValue - all other code doesn't have to care.

(Yes, these types should maybe be nested classes of nsCSSValue, like Image etc, but that would be a large mostly-mechanical change tangential to the purpose here, so I haven't done it.)

dbaron: you suggested a ref-counting template, but that turns out not to go well with what NS_INLINE_DECL_REFCOUNTING does with its argument; I might have been able to work around that but, again, it seemed tangential to the purpose.

Comment 3

[Zack Weinberg (:zwol)]

Attachment: part 3: rect

Comment 4

[Zack Weinberg (:zwol)]

Attachment: part 4: valuepairlist

For this (and ValueList) there are actually two unit codes: one for a "normal" list refcounted by the nsCSSValue, and one for a "dependent" list that belongs to some other object.  This let me avoid teaching nsStyleAnimation::Value about the refcounting scheme, which was getting hairier than I wanted to deal with.  Thanks to dholbert for the idea.  For "normal" lists, note that only the first object in the linked list has a refcount field.

Comment 5

[Zack Weinberg (:zwol)]

Attachment: part 5: valuelist

Comment 6

[Zack Weinberg (:zwol)]

Attachment: part 6: remove remaining vestiges of nsCSSType

To keep part 5 focused, I left a bunch of switch statements and other uses of nsCSSType intact, even though there was only one possibility.  This prunes all of the detritus.

Comment 7

[Zack Weinberg (:zwol)]

Attachment: Benchmark of new patch series

I'm about to post a whole new patch series for this bug, but before I do, I thought I'd present some benchmarking results.  This image has nine stacked charts in it; each shows the total number of instructions executed by one of the CSS parser entry points for some chosen input, as a function of how many of the patches in this bug were applied on top of m-c.  So, if the blue line goes up as you go right, that's bad, and if it goes down, that's good.  (Instruction counts were taken with the instrumentation from bug 568863, and I checked that in this case they're a reliable but less noisy proxy for wall time.)  There are 100 data points in each column of each chart, but they mostly overprint on each other; the faint gray dots in the bottom chart show some kind of bimodal thing going on there.

The patch series is divided into four sets.  The farthest-left column (labeled "00-parser-profiling-hooks") is a baseline measurement, representing the performance of an unmodified browser.  The next patch over is some setup work which just moves code around; you can see that its performance is roughly equivalent to the unmodified measurement.  The next five patches, grouped at the bottom with a black bar, are the meat of this bug, in which I change the storage representation of various longhand properties until they're all the same.  The remaining five patches are opportunistic cleanup work made easier by this bug; they were not expected to have significant performance impact, although they do seem to have done to some extent.

On the very top is a 'control' measurement: ParseColor(), processing the single token #112233.  This should therefore be almost entirely the cost of CSS parser setup and teardown, plus the cost of NS_HexToRGB(), and should *not* be notably affected by changes to nsCSSValue and friends.  As you can see, there is a penalty of about 10 extra instructions; I think this is because nsCSSParser has two nsCSSExpandedDataBlock objects embedded in it, and the changes in here make those have more nsCSSValue fields and therefore take longer to initialize.  I think this is an acceptable cost for the significant gains you'll see going down the chart, and anyway, those embedded objects are on the chopping block once I get done with *this* patch.

The next five charts, moving downward, are the cost of parsing a style attribute containing exactly one longhand property, with the indicated storage type: top to bottom, nsCSSValue, nsCSSValuePair, nsCSSRect, nsCSSValuePairList, and nsCSSValueList.  This is the order in which I eliminated storage types (other than nsCSSValue).  I was expecting to see a performance hit for each of these in the column corresponding to the patch that changed its storage, and that is what we see for pair and rect, but interestingly, it is *not* what we see for pairlist and list, which actually get *faster*, all at once, in the patch that eliminates lists.  I'm not sure exactly why that happened but I think we are doing less work in the parser proper, which makes up for the extra allocations and bookkeeping in value storage.

The next two charts are again parsing a style attribute, but in these cases it contains one shorthand: moderately complex 'font' and 'background' values.  'font' is sped up across the board, particularly by the last patch in the marked group, which is the one that eliminates the kTypeTable.  This makes sense for shorthands, which are dominated by the cost of TransferTempData.  The background shorthand is unusually slow - more than twice as expensive as any of the others - and while it gains some in the middle, the last few patches give back some of the improvement.  I think this is because the background shorthand sets a whole lot of properties and they're all set to lists.  The last few changes make parser operations on nsCSSExpandedDataBlock a little more "hands off" and this is going to cost something.

Finally, the last chart is perhaps the most realistic: a style attribute with several properties in it (all longhand, as it happens).  This shows a steady improvement along the patch sequence.

These changes should have very little impact outside of the parser, but they may speed up MapRuleInfoInto a bit, mainly because of not having to dispatch on the storage type.  Also, I think someone who understands the animation code better than I do should have a hard look at nsStyleAnimation::Value and see if it could share more with nsCSSValue after these changes.

Comment 8

[Zack Weinberg (:zwol)]

Comment on attachment 463849 [details]
Benchmark of new patch series

oops, that's not itself a patch

Comment 9

[Zack Weinberg (:zwol)]

Attachment: part 1: move storage classes out of nsCSSStruct.h/nsCSSStruct.cpp

This moves nsCSSRect, nsCSSValuePair, nsCSSValueList, and nsCSSValuePairList from nsCSSStruct* to nsCSSValue*, and also cleans up a wart where there *should have been* a nsCSSValuePair in nsCSSValue::Gradient, but the structure declaration was inaccessible so it had to be two Values instead.

Comment 10

[Zack Weinberg (:zwol)]

Attachment: part 2: valuepair as value unit

The explanation in comment 2 still applies.

Comment 11

[Zack Weinberg (:zwol)]

Attachment: part 3: rect as value unit

Comment 12

[Zack Weinberg (:zwol)]

Attachment: part 4: valuepairlist as value unit

comment 4 is still relevant.

Comment 13

[Zack Weinberg (:zwol)]

Attachment: part 5: valuelist as value unit

Comment 14

[Zack Weinberg (:zwol)]

Attachment: part 6: remove remaining vestiges of nsCSSType

Comment 15

[Zack Weinberg (:zwol)]

Attachment: part 7: cleanup pass on Declaration and DataBlock

#include pruning, removal of out-of-memory checks, comment corrections, formatting, etc.

Comment 16

[Zack Weinberg (:zwol)]

Attachment: part 8: remove the last MoveValue call from the CSS parser

This continues a theme started in bug 569719 of moving accessor code out of the parser and into the data block classes.

Comment 17

[Zack Weinberg (:zwol)]

Attachment: part 9: AddProperty method for nsCSSExpandedDataBlock

Similar rationale as part 8.

Comment 18

[Zack Weinberg (:zwol)]

Attachment: part 10: remove all direct access to nsCSSExpandedDataBlock data members from parser

The parser now consistently adds all properties to mTempData by using AppendValue.

Comment 19

[Zack Weinberg (:zwol)]

Attachment: part 11: make a whole lot of assertions fatal

Finally, this patch does what it says: all assertions in the listed files are now fatal.

Comment 20

[Zack Weinberg (:zwol)]

Attachment: part 12: fix assertions in nsStyleAnimation.cpp

I discovered yesterday that part 3 causes nsStyleAnimation::AddWeighted and ::ComputeDistance to start throwing assertions on the content/smil mochitests.  The fix is contained to nsStyleAnimation so I'm posting it as a separate patch for ease of review.

AddWeighted and ComputeDistance don't bother explicitly declining to handle eUnit_Auto and eUnit_Normal in their big switches.  This is fine in present trunk because animation objects with those units never get to either function, but part 3 of this bug changes the specified-value representation of 'clip:auto' in a way that makes these functions start being called for eUnit_Auto, if you animate a clip.

The cure is simply to add a couple of entries to the list of units that we don't animate at the top of the big switch in each function.  I believe it is still impossible to get eUnit_Normal in here, but it seems to me that comprehensive coverage is the way to go.  Therefore, I also restructured these functions to remove the default case from the switch, so that we will in the future get warnings if we don't cover all units.

Comment 21

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

Could you explain how you've changed the representation of clip:auto?  (And, in particular, how does clip:auto differ from clip:rect(auto,auto,auto,auto)?)

Comment 22

[Zack Weinberg (:zwol)]

(In reply to comment #21)
> Could you explain how you've changed the representation of clip:auto?  (And, in
> particular, how does clip:auto differ from clip:rect(auto,auto,auto,auto)?)

Before part 3, clip:auto was represented as a nsCSSRect object with all four sides set to eCSSUnit_RectIsAuto, and clip:rect(auto,auto,auto,auto) was represented as a nsCSSRect object with all four sides set to eCSSUnit_Auto.

After part 3, clip:rect(auto,auto,auto,auto) is the same as it used to be (but boxed into an nsCSSValue with unit eCSSUnit_Rect); but clip:auto is just a bare nsCSSValue with unit eCSSUnit_Auto.  The _RectIsAuto unit is gone.  If you look at part 3 you'll see special cases for eCSSUnit_RectIsAuto deleted from nsStyleAnimation::ComputeDistance and ::AddWeighted, and this is what I should have replaced them with (but didn't, because I didn't notice the new assertions because I only ran the full mochitest suite on try server).

Comment 23

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

Comment on attachment 463852 [details] [diff] [review]
part 1: move storage classes out of nsCSSStruct.h/nsCSSStruct.cpp

Please leave nsCSSValueList::AppendToString as it was before;
it was better the old way.

Likewise, convert nsCSSValuePairList::AppendToString back to the old
control flow; leave the renaming from |val| to |item|, though.

>   nsCSSUnit GetUnit() const { return mUnit; }
>   PRBool    IsLengthUnit() const
>     { return eCSSUnit_Inch <= mUnit && mUnit <= eCSSUnit_Pixel; }
>-  PRBool    IsFixedLengthUnit() const  
>+  PRBool    IsFixedLengthUnit() const
>     { return eCSSUnit_Inch <= mUnit && mUnit <= eCSSUnit_Pica; }
>-  PRBool    IsRelativeLengthUnit() const  
>+  PRBool    IsRelativeLengthUnit() const
>     { return eCSSUnit_EM <= mUnit && mUnit <= eCSSUnit_Pixel; }
>-  PRBool    IsAngularUnit() const  
>+  PRBool    IsAngularUnit() const
>     { return eCSSUnit_Degree <= mUnit && mUnit <= eCSSUnit_Radian; }
>-  PRBool    IsFrequencyUnit() const  
>+  PRBool    IsFrequencyUnit() const
>     { return eCSSUnit_Hertz <= mUnit && mUnit <= eCSSUnit_Kilohertz; }
>-  PRBool    IsTimeUnit() const  
>+  PRBool    IsTimeUnit() const
>     { return eCSSUnit_Seconds <= mUnit && mUnit <= eCSSUnit_Milliseconds; }

Please drop this particular whitespace cleanup; it'll just give roc
merge conflicts.


This patch is completely missing changes to nsCSSStruct.h.  You have to
remove the class declarations from there as well (a big hunk at the top,
but don't remove nsCSSCornerSizes, which you didn't move, and which you
have changes to, or nsCSSValueListRect, which you didn't move).  That
doesn't cause compilation errors now, but it will the first time
somebody changes one of the classes.

Please don't switch nsCSSValue.h from including nsString.h to nsAString.h;
nsString.h is preferred.

r=dbaron with those changes

Comment 24

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

(In reply to comment #23)
> This patch is completely missing changes to nsCSSStruct.h.  You have to
> remove the class declarations from there as well (a big hunk at the top,
> but don't remove nsCSSCornerSizes, which you didn't move, and which you
> have changes to, or nsCSSValueListRect, which you didn't move).  That
> doesn't cause compilation errors now, but it will the first time
> somebody changes one of the classes.

Er, never mind this; I think I thought I was looking at patch 1 when I was actually looking at patch 2.

Comment 25

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

Comment on attachment 463854 [details] [diff] [review]
part 2: valuepair as value unit

The changes to Declaration::GetValue for border-radius aren't correct;
they'll crash given a declaration containing:
  -moz-border-radius: 2em;
  -moz-border-radius-topleft: 3em 4em;
or one containing:
  -moz-border-radius: 2em / 3em;
  -moz-border-radius-topleft: 4em;
since they assume that either all 4 corners are pairs or no corners
are pairs.  Please fix this and add a test.

>+  // Copy to heap.  Note: the value pair returned by ParseBoxPositionValues
>+  // must _not_ be collapsed to a single value if the x- and y-values are
>+  // equal, unlike many other uses of value pairs.  But we still want to
>+  // collapse inherit and initial.

The "Copy to heap." comment is a little odd.  Also, in general,
nsRuleNode has to match the structures that the parser produces, so the
big "must _not_" seems inconsistent with the rest of the code.  I'd
change the comment to just:
  // We want positions always stored as pairs, even if the values are
  // the same, so they always serialize as pairs, and to keep the
  // computation code simple.

You need to change nsCSSValue::operator== for pairs.

In nsCSSValue:

>+public:

no need; it is already

>+  nsCSSValuePair& GetPairValue() const; // body below

could you explicitly write |inline|?

However, I don't think I like the const-ness here.  I'd prefer
overloading:
  +  nsCSSValuePair& GetPairValue(); // body below
  +  const nsCSSValuePair& GetPairValue() const; // body below

>+  void SetPairValue(const nsCSSValuePair* aPair);

Why does this take a pointer rather than a reference?  I'd
think it should take a reference.  (The only user is in
nsStyleAnimation.)

>+                    SETCOORD_LPH|SETCOORD_INITIAL_HALF|SETCOORD_BOX_POSITION,

Could you put spaces around the |s (and then break the line)?

nsStyleAnimation::UncomputeValue, you should at least add a somewhat
scary comment saying that the redundant value elimination is a problem
for some properties, but not any animatable ones.


I'd note that this patch isn't ok on its own; the changes to
nsCSSValuePair::AppendToString will also require changes that I presume
are in the nsCSSValuePairList patch.

review- because I want to re-review the code for the first comment.

Comment 26

[Zack Weinberg (:zwol)]

I'm uploading a new patch series shortly - addressing these requests had knock-on effects throughout.

(In reply to comment #25)
> Comment on attachment 463854 [details] [diff] [review]
> part 2: valuepair as value unit
> 
> The changes to Declaration::GetValue for border-radius aren't correct;
> they'll crash given a declaration containing:
>   -moz-border-radius: 2em;
>   -moz-border-radius-topleft: 3em 4em;
> or one containing:
>   -moz-border-radius: 2em / 3em;
>   -moz-border-radius-topleft: 4em;
> since they assume that either all 4 corners are pairs or no corners
> are pairs.  Please fix this and add a test.

Done.

> >+  // Copy to heap.  Note: the value pair returned by ParseBoxPositionValues
> >+  // must _not_ be collapsed to a single value if the x- and y-values are
> >+  // equal, unlike many other uses of value pairs.  But we still want to
> >+  // collapse inherit and initial.
> 
> The "Copy to heap." comment is a little odd.  Also, in general,
> nsRuleNode has to match the structures that the parser produces, so the
> big "must _not_" seems inconsistent with the rest of the code.  I'd
> change the comment to just:
>   // We want positions always stored as pairs, even if the values are
>   // the same, so they always serialize as pairs, and to keep the
>   // computation code simple.

Ok.  (I used slightly different wording.)

> You need to change nsCSSValue::operator== for pairs.

It appears that I noticed this in the "valuelist" patch and fixed it at that point in one go.  I've now adjusted the patches so that each one does its own additions to operator==.

> In nsCSSValue:
> 
> >+public:
> 
> no need; it is already

Must've been a merge botch.  Gone.

> >+  nsCSSValuePair& GetPairValue() const; // body below
> 
> could you explicitly write |inline|?
> 
> However, I don't think I like the const-ness here.  I'd prefer
> overloading:
>   +  nsCSSValuePair& GetPairValue(); // body below
>   +  const nsCSSValuePair& GetPairValue() const; // body below

This is inconsistent with all the other getters, but whatever.  Changed, & subsequent patches changed to match.

> >+  void SetPairValue(const nsCSSValuePair* aPair);
> 
> Why does this take a pointer rather than a reference?  I'd
> think it should take a reference.  (The only user is in
> nsStyleAnimation.)

It takes a pointer because nsStyleAnimation::Value::GetCSSValuePairValue returns a pointer.  I didn't change this.


> >+                    SETCOORD_LPH|SETCOORD_INITIAL_HALF|SETCOORD_BOX_POSITION,
> 
> Could you put spaces around the |s (and then break the line)?

I left out the spaces so I didn't have to break the line, but whatever.  Changed.

> nsStyleAnimation::UncomputeValue, you should at least add a somewhat
> scary comment saying that the redundant value elimination is a problem
> for some properties, but not any animatable ones.

Done.

> I'd note that this patch isn't ok on its own; the changes to
> nsCSSValuePair::AppendToString will also require changes that I presume
> are in the nsCSSValuePairList patch.

Yah.  Each patch in this series *compiles* on its own, and passes smoke tests, but does not necessarily pass the full test suite.  I have only run the full test suite on the whole patch series, and I don't think it would be a good use of either my time or tryserver cycles to fix it so each patch is good in itself.

---

It would be more efficient, I think, if you reviewed the whole patch series at once rather than doing one chunk and then waiting for me to produce an update before going on to the next one  It takes me several hours to update this thing *independent* of how many change requests I have stacked up; it's almost all fixing merge conflicts and waiting for the compiler and/or the test suite, and that's time I can't spend finishing bug 524223, which I really would like to be done with before I leave.  In two days.

Comment 27

[Zack Weinberg (:zwol)]

I meant time I can't spend finishing bug 553456.

Comment 28

[Zack Weinberg (:zwol)]

Attachment: part 1: move storage classes out of nsCSSStruct.h/nsCSSStruct.cpp

revised per comment 23, carrying r+ forward.

Comment 29

[Zack Weinberg (:zwol)]

Attachment: part 2: valuepair as value unit

revised per comment 25, 26

Comment 30

[Zack Weinberg (:zwol)]

Attachment: part 3: rect as value unit

rediff + tweaks per comment 26

Comment 31

[Zack Weinberg (:zwol)]

Attachment: part 4: valuepairlist as value unit

rediff + tweaks per comment 26

Comment 32

[Zack Weinberg (:zwol)]

Attachment: part 5: valuelist as value unit

rediff + tweaks per comment 26

Comment 33

[Zack Weinberg (:zwol)]

Attachment: part 6: remove remaining vestiges of nsCSSType

rediff + tweaks per comment 26

Comment 34

[Zack Weinberg (:zwol)]

Attachment: part 7: cleanup pass on Declaration and DataBlock

rediff

Comment 35

[Zack Weinberg (:zwol)]

Attachment: part 8: remove the last MoveValue call from the CSS parser

rediff

Comment 36

[Zack Weinberg (:zwol)]

Attachment: part 9: AddProperty method for nsCSSExpandedDataBlock

rediff

Comment 37

[Zack Weinberg (:zwol)]

Attachment: part 10: remove all direct access to nsCSSExpandedDataBlock data members from parser

rediff

Comment 38

[Zack Weinberg (:zwol)]

Attachment: part 11: make a whole lot of assertions fatal

rediff

Comment 39

[Zack Weinberg (:zwol)]

Attachment: part 12: fix assertions in nsStyleAnimation.cpp

rediff

Comment 40

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

(In reply to comment #26)
> It would be more efficient, I think, if you reviewed the whole patch series at
> once rather than doing one chunk and then waiting for me to produce an update
> before going on to the next one  It takes me several hours to update this thing
> *independent* of how many change requests I have stacked up; it's almost all
> fixing merge conflicts and waiting for the compiler and/or the test suite, and
> that's time I can't spend finishing bug 524223, which I really would like to be
> done with before I leave.  In two days.

I wasn't waiting for you to update patches; if I had been I'd have said so.

But reviewing each one of these is a multi-hour task, so I'm not going to get through all 10 in one day.

Comment 41

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

(In reply to comment #40)
> I wasn't waiting for you to update patches; if I had been I'd have said so.

(or you could have asked whether I was)

Comment 42

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

Comment on attachment 465081 [details] [diff] [review]
part 2: valuepair as value unit

r=dbaron

Comment 43

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

Comment on attachment 465080 [details] [diff] [review]
part 1: move storage classes out of nsCSSStruct.h/nsCSSStruct.cpp

You introduced an indentation error in nsCSSValueList::AppendToString:

+    if (nsCSSProps::PropHasFlags(aProperty,
+                                 CSS_PROPERTY_VALUE_LIST_USES_COMMAS))
+        aResult.Append(PRUnichar(','));

You should fix the indent here (2 spaces, not 4).

Otherwise revisions look good.

Comment 44

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

Comment on attachment 465083 [details] [diff] [review]
part 3: rect as value unit

In nsCSSRect::AppendToString, in the assertion, rather than messing
with a variable and an #ifdef DEBUG block, just repeat |mTop.GetUnit()|
inside the NS_ABORT_IF_FALSE.  (It should compile to the same thing
anyway, under optimization.)

In nsRuleNode::ComputeDisplayData, could you leave the indentation of
the "- display->mClip.x" when computing display->mClip.width as-is.  (I
think it's better as it was, and better to match the corresponding code
for height, which you didn't change.)

In nsStyleAnimation, I don't see why you removed this:
-      NS_ABORT_IF_FALSE(nsCSSProps::kTypeTable[aProperty] ==
-                          eCSSType_Rect, "type mismatch");
from UncomputeValue.  Can you add it back?

This was a good bit smaller than patch 2. :-)

r=dbaron with that

Comment 45

[Zack Weinberg (:zwol)]

(In reply to comment #40)
> I wasn't waiting for you to update patches; if I had been I'd have said so.
> 
> But reviewing each one of these is a multi-hour task, so I'm not going to get
> through all 10 in one day.

Sorry for being cranky at you.  It just seemed like you were waiting on me because patch 2 was reviewed immediately after patch 1 and then there was nothing for a full day.

(In reply to comment #43)
> Comment on attachment 465080 [details] [diff] [review]
> part 1: move storage classes out of nsCSSStruct.h/nsCSSStruct.cpp
> 
> You introduced an indentation error in nsCSSValueList::AppendToString:

Fixed.  I'd blame the 4-space indent in nsCSSDataBlock but that didn't come from there so I dunno.

> Comment on attachment 465083 [details] [diff] [review]
> part 3: rect as value unit
> 
> In nsCSSRect::AppendToString, in the assertion, rather than messing
> with a variable and an #ifdef DEBUG block, just repeat |mTop.GetUnit()|
> inside the NS_ABORT_IF_FALSE.  (It should compile to the same thing
> anyway, under optimization.)

Done.

> In nsRuleNode::ComputeDisplayData, could you leave the indentation of
> the "- display->mClip.x" when computing display->mClip.width as-is.  (I
> think it's better as it was, and better to match the corresponding code
> for height, which you didn't change.)

Done.  (Not sure why I did that, I agree it looks better as it was.  I blame emacs' autoindenter.)

> In nsStyleAnimation, I don't see why you removed this:
> -      NS_ABORT_IF_FALSE(nsCSSProps::kTypeTable[aProperty] ==
> -                          eCSSType_Rect, "type mismatch");
> from UncomputeValue.  Can you add it back?

No, I can't.  The constant eCSSType_Rect is no longer defined, and the kTypeTable entries for these properties say eCSSType_Value.  In part 6,
kTypeTable ceases to exist too.

> This was a good bit smaller than patch 2. :-)

And those changes did not cause remotely as many merge conflicts as the previous set :-)   You're not going to like part 5, though.  (Part 6 shouldn't be as bad - it's big, but it's almost entirely deletions.)

  9K 576044-00-parser-profiling-hooks.diff
 36K 576044-01-css-storage-types-out-of-cssstruct-h.diff
 37K 576044-01-css-storage-types-out-of-cssstruct-h.diff~
 92K 576044-02-valuepair-as-value-unit.diff
 57K 576044-03-rect-as-value-unit.diff
 66K 576044-04-valuepairlist-as-value-unit.diff
186K 576044-05-valuelist-as-value-unit.diff
164K 576044-06-nscsstype-vestiges.diff
 12K 576044-07-post-cleanups.diff
 12K 576044-08-remove-movevalue-from-parser.diff
  3K 576044-09-datablock-addproperty-method.diff
 17K 576044-10-avoid-direct-datablock-access-in-parser.diff
 60K 576044-11-fatal-assertions.diff
 21K 576044-12-fix-style-animation-assertions.diff

Comment 46

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

Comment on attachment 465084 [details] [diff] [review]
part 4: valuepairlist as value unit

There are some things in ParseBackground I don't like, but it looks
like they're removed in patch 5, so that's fine.  I'll look through
the new code in patch 5 when I get there.

I like the cleanup in the other parsing functions that's in this patch;
there's a bunch of good simplification.

nsCSSValue::operator== doesn't work between Dependent and non-Dependent
pair lists, which seems like it could lead to serious potential
confusion.  Possible fixes are:
 * make error to call operator== with a *Dep value (I think this may
   well be ok, in which case it's my preference; use NS_ABORT_IF_FALSE)
 * fix operator== to actually work
 * drop Dep, and replace its uses with something that copies

In nsCSSValue.h, all values of the enum have the type as a comment;
please do that for PairListDep too.

It would be better for both GetPairListValue methods to do this:
 if (mUnit == eCSSUnit_PairList) {
   return mValue.mPairList;
 } else {
   NS_ABORT_IF_FALSE(mUnit == eCSSUnit_PairListDep, "...");
   return mValue.mPairListDependent;
 }
so that there doesn't have to be a dynamic type check (to see whether to
return null) at all once the compiler realizes the two branches of the
if compile to the same thing.

I'd been thinking that I wanted you to remerge SetBackgroundList and
SetBackgroundPairList either in patch 5 or a higher patch, but actually
it's probably not worth it.  (It looks doable by adding another member,
of type T* nsCSSValue::*mListGetterFunc(), to InitialInheritLocationFor,
and probably renaming it to BackgroundListTypeTraits or something like
that.  And also a getter for the units, which could be called only in an
assertion instead of explicitly checking PairList/PairListDep.)  So
don't bother, or if you do, use an additional patch on top of
everything.

The quotes computation in ComputeQuotesData ought to assert that
ourQuotes->mXValue.GetUnit() == eCSSUnit_String (and same for Y).
(It used to be in a check that mXValue was eCSSUnit_String.)

Again, I'd prefer if you didn't remove this assertion in
nsStyleAnimation::UncomputeValue:
-      NS_ABORT_IF_FALSE(nsCSSProps::kTypeTable[aProperty] ==
-                          eCSSType_ValuePairList, "type mismatch");

r=dbaron with those changes, but I'd like to look at what you've
done with operator==

Comment 47

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

One additional comment on patch 4:  in order to avoid regressing the serialization of -moz-background-size: cover and -moz-background-size: contain by adding an extra space at the end, you should change either patch 2 or patch 4 so that nsCSSValuePair::AppendToString doesn't append a space if the second value's unit is null.  (And if you're adding the test, may as well also stick the call to serialize the second value inside that test.)  You should probably also add a test for this.

Comment 48

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

Comment on attachment 465085 [details] [diff] [review]
part 5: valuelist as value unit

In CSSParserImpl::ParseBackground:

>+  // background-color can only be set once, so it's not in BackgroundItem.

There's no |BackgroundItem| anymore, so this should be reworded.

In the inherit/initial handling:
>+    AppendValue(eCSSProperty_background_image,      color);
>+    AppendValue(eCSSProperty_background_repeat,     color);
>+    AppendValue(eCSSProperty_background_attachment, color);
>+    AppendValue(eCSSProperty_background_clip,       color);
>+    AppendValue(eCSSProperty_background_origin,     color);
>+    AppendValue(eCSSProperty_background_position,   color);
>+    AppendValue(eCSSProperty_background_size,       color);
>+    AppendValue(eCSSProperty_background_color,      color);

please make this a loop instead:

  for (const nsCSSProperty* subprops =
         nsCSSProps::SubpropertyEntryFor(eCSSProperty_background);
       *subprops != eCSSProperty_UNKNOWN; ++subprops) {
    AppendValue(*subprops, color);
  }

>+  color.Reset(); // just to be sure

Assert if you want, but don't add real code that might not get
optimized away.

In ParseBackgroundItem, could you put the PR_STATIC_ASSERTS back
where they were?

And also put the |haveColor = PR_TRUE| back where it was; I don't
see a good reason to move it, and the move makes it inconsistent
with all the other functions.  (Twice.)

I believe the kContentKTable / kContentAltKTable split makes
-moz-alt-content get serialized as ... well, I'm not exactly sure.  In
any case, the code path that needs to work is
nsCSSValue::AppendToString's eCSSUnit_Enumerated clause, which calls
nsCSSProps::LookupPropertyValue which uses kKeywordTableTable, which
looks only at kContentKTable because that's what's in nsCSSPropList.h.
I can see two reasonable ways to unbreak this, off the top of my head:
 1) have *three* tables; make the full one be the official table for
    the property, and then have two subtables splitting the values
 2) go back to storing -moz-alt-content as a single item value list,
    which I think might be simpler in the end.
Please add a test.

In ParseTransitionProperty, you dropped this comment, which is
important (in that it says why we do what we do in a case where
the spec is vague):
>-    // Exclude 'none' and 'all' and 'inherit' and 'initial' according to
>-    // the same rules as for 'counter-reset' in CSS 2.1 (except
>-    // 'counter-reset' doesn't exclude 'all' since it doesn't support
>-    // 'all' as a special value).
Please add it back.

Also, you changed this to also check for '-moz-initial'.  Don't.
'-moz-initial' is a perfectly valid value of transition-property.

In ParseTransition:

>         if (multipleItems) {
>           // This is a syntax error.
>           return PR_FALSE;
>+        } else {
>+          // Unbox a solitary 'none' or 'all'.
>+          if (val.GetUnit() == eCSSUnit_None) {
>+            values[3].SetNoneValue();
>+          } else {
>+            values[3].SetAllValue();
>+          }
>+          break;
>         }
>         continue;

You've managed to have three different ways to exit when there are only
two possibilities.  How about removing both the |else| (not needed after
the return) and the |continue|?

(I think the ease with which it lets you write unreachable code is
the reason to dislike else-after-return.  While I see that it's nice
sometimes, it isn't here.)

In nsCSSProps.h:
> // A property that needs to have image loads started when a URL value
>-// for the property is used for an element.  Supported only for
>-// eCSSType_Value and eCSSType_ValueList.
>+// for the property is used for an element.
> #define CSS_PROPERTY_START_IMAGE_LOADS            (1<<5)

It's still worth saying that it's only supported for the image (or, when
CSS_PROPERTY_IMAGE_IS_IN_ARRAY_0 is set, array) being directly in the
value or or as an item in an eCSSUnit_List value.

nsCSSStruct.cpp:
>- * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * and other provisions required by the GPL or the LGPL. If you do not deleteu

oops!

In nsCSSValue.cpp, same issues for ListDep as for PairListDep
in the previous patch.

(Note that it wouldn't be that hard to make operator== work properly
using an AdjustUnit function; I think it's safe to assume that casting
a class with no vtable pointer to its only base class doesn't require
an adjustment.)

Do you recall why you added null-checks in SetDependentListValue
and SetDependentPairListValue?

nsCSSValue.h: same thing as previous about the comments for ListDep,
and also about the type checks in both GetListValue impls.

In GetShadowData in nsRuleNode.cpp, please add assertions that the
type of each item in the list is eCSSUnit_Array.  (You removed a
check for the array type in ComputeTextData.)  It could go right at 
the top of the for loop (for each value, this time!).

>+    // can get a _None in here from transform animation

Should be easily fixable with a small change to
nsStyleAnimation::UncomputeValue.  Maybe an additional patch on top; if
not, I'll try to do it in a followup.

Still in nsRuleNode.cpp:
>+  } break;
I'd prefer having the break inside the braces, i.e.,:
+    break;
+  }
(at least five times; I'll stop noting them now)

In nsRuleNode::HasAuthorSpecifiedRules, you made the
firstBackgroundImage stuff more complicated when you actually should
have made it simpler.  In particular, instead of what you did, you
should:
 * remove |firstBackImage|
 * make backgroundValues just point to &colorData.mBackImage instead
 * remove the chunk:
>        if ((ruleTypeMask & NS_AUTHOR_SPECIFIED_BACKGROUND) &&
>            colorData.mBackImage &&
>            firstBackgroundImage.GetUnit() == eCSSUnit_Null) {
>          // Handle background-image being a value list
>          firstBackgroundImage = colorData.mBackImage->mValue;
>        }
and everything should be good.

Again, in nsStyleAnimation::UncomputeValue, I'd like the assertion
to stay.


r=dbaron with those things fixed

Comment 49

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

Comment on attachment 465087 [details] [diff] [review]
part 6: remove remaining vestiges of nsCSSType

>+                    ShouldIgnoreColors(aRuleData)) {
>+                    if (iProp == eCSSProperty_background_color) {

Could you leave the opening brace after ShouldIgnoreColors on its
own line?  Quirk of 4-space indent, please.


> #ifdef DEBUG
>-            void *prop = PropertyAt(iProp);
>+            nsCSSValue* val = PropertyAt(iProp);
>+            NS_ASSERTION(val->GetUnit() != eCSSUnit_Null,
>+                         "null value while computing size");
> #endif

Drop the ifdefs, and just make this:
NS_ASSERTION(PropertyAt(iProp)->GetUnit != eCSSUnit_Null,
             "null value while computing size");


Er, ok, ignore my comments on the previous two patches about
keeping the assertions in nsStyleAnimation::UncomputeValue.
Not sure what I was thinking.

nsStyleAnimation.h:
>-   * The first form fills in one of the nsCSSType types into the void*;
>-   * for some types this means that the void* is pointing to memory
>-   * owned by the nsStyleAnimation::Value.  (For all complex types, the
>-   * nsStyleAnimation::Value owns the necessary objects so that the
>-   * caller does not need to do anything to free them.  However, this
>-   * means that callers using the void* variant must keep
>-   * |aComputedValue| alive longer than the structure into which they've
>-   * filled the value.)
>+   * The first form fills in an nsCSSValue object; the second form
>+   * produces a string.

Given the dependent list and pairlist types, you absolutely need
to keep this warning about lifetimes.  So put it back, reworded
appropriately.

r=dbaron with that

Comment 50

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

Comment on attachment 465088 [details] [diff] [review]
part 7: cleanup pass on Declaration and DataBlock

>-     * Allocate a new compressed block and transfer all of the state
>-     * from this expanded block to the new compressed block, clearing
>+     * Allocate a new pair of compressed blocks and transfer all of
>+     * the state from this expanded block to the new blocks, clearing

This makes it sound like two are always allocated.  It's usually one.
Could you update to reflect that aNormalBlock is always allocated, and
aImportantBlock sometimes is.


I could have done without the Declaration::GetValueIsImportant reorg or 
the initialization of cursor_important, but I guess they're ok.

r=dbaron with the comment fix

Comment 51

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

Comment on attachment 465089 [details] [diff] [review]
part 8: remove the last MoveValue call from the CSS parser

The tri-state return value is pretty bad, and I object to new uses of success nsresults other than NS_OK.

Please just make this return a boolean for whether the "trying" succeeded, and have a PRBool* aChanged for the rest.

Other than that it looks fine, though.

Comment 52

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

I'll continue tomorrow.

Comment 53

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

Comment on attachment 465090 [details] [diff] [review]
part 9: AddProperty method for nsCSSExpandedDataBlock

>Bug 576044 (9/11): Add an AddValue method to nsCSSExpandedDataBlock.  r=dbaron

Fix the commit message to match the name you're using.

>+nsCSSExpandedDataBlock::AddLonghandProperty(nsCSSProperty aProperty,
>+                                            const nsCSSValue& aValue)
>+{
>+    NS_ASSERTION(nsCSSProps::IsShorthand(aProperty), "property out of range");

This assertion condition needs a ! in front of it.  (Doesn't seem too well tested.)

r=dbaron with that fixed; good to run through try before landing, though.

(But if you tested it and the assertion didn't fire, then I retract my review; something is seriously wrong.)

Comment 54

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

Comment on attachment 465091 [details] [diff] [review]
part 10: remove all direct access to nsCSSExpandedDataBlock data members from parser

r=dbaron

Comment 55

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

Comment on attachment 465092 [details] [diff] [review]
part 11: make a whole lot of assertions fatal

> void
> nsCSSExpandedDataBlock::AddLonghandProperty(nsCSSProperty aProperty,
>                                             const nsCSSValue& aValue)
> {
>-    NS_ASSERTION(nsCSSProps::IsShorthand(aProperty), "property out of range");
>+    NS_ABORT_IF_FALSE(!nsCSSProps::IsShorthand(aProperty),
>+                      "property out of range");

Ah, you fixed the issue from two patches back in this patch.  It should
really be in the other one, though.  Trivial to fix by editing the mq
patch files (just edit the minus line in this patch too).

>diff --git a/layout/style/nsCSSProps.cpp b/layout/style/nsCSSProps.cpp
>--- a/layout/style/nsCSSProps.cpp
>+++ b/layout/style/nsCSSProps.cpp
>@@ -1,9 +1,9 @@
>-/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* -*- Mode: xC++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Oops.  Please fix.

r=dbaron with those fixed

Comment 56

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

Comment on attachment 465094 [details] [diff] [review]
part 12: fix assertions in nsStyleAnimation.cpp

r=dbaron

Comment 57

[Zack Weinberg (:zwol)]

Working through these; will post a new series when done.

(In reply to comment #46)
> Comment on attachment 465084 [details] [diff] [review]
> nsCSSValue::operator== doesn't work between Dependent and non-Dependent
> pair lists, which seems like it could lead to serious potential
> confusion.  Possible fixes are:
>  * make error to call operator== with a *Dep value (I think this may
>    well be ok, in which case it's my preference; use NS_ABORT_IF_FALSE)
>  * fix operator== to actually work
>  * drop Dep, and replace its uses with something that copies

I made it abort.  The assertion does not fire on the content/smil or layout/style mochitests.  I'll run the full test suite via the try server when I'm done updating the patch series.

> In nsCSSValue.h, all values of the enum have the type as a comment;
> please do that for PairListDep too.

Done.

> It would be better for both GetPairListValue methods to do this:
>  if (mUnit == eCSSUnit_PairList) {
>    return mValue.mPairList;
>  } else {
>    NS_ABORT_IF_FALSE(mUnit == eCSSUnit_PairListDep, "...");
>    return mValue.mPairListDependent;
>  }
> so that there doesn't have to be a dynamic type check (to see whether to
> return null) at all once the compiler realizes the two branches of the
> if compile to the same thing.

Done.

> I'd been thinking that I wanted you to remerge SetBackgroundList and
> SetBackgroundPairList either in patch 5 or a higher patch, but actually
> it's probably not worth it.  (It looks doable by adding another member,
> of type T* nsCSSValue::*mListGetterFunc(), to InitialInheritLocationFor,
> and probably renaming it to BackgroundListTypeTraits or something like
> that.  And also a getter for the units, which could be called only in an
> assertion instead of explicitly checking PairList/PairListDep.)  So
> don't bother, or if you do, use an additional patch on top of
> everything.

I'm not going to bother.

> The quotes computation in ComputeQuotesData ought to assert that
> ourQuotes->mXValue.GetUnit() == eCSSUnit_String (and same for Y).
> (It used to be in a check that mXValue was eCSSUnit_String.)

Done.

(In reply to comment #47)
> One additional comment on patch 4:  in order to avoid regressing the
> serialization of -moz-background-size: cover and -moz-background-size: contain
> by adding an extra space at the end, you should change either patch 2 or patch
> 4 so that nsCSSValuePair::AppendToString doesn't append a space if the second
> value's unit is null.  (And if you're adding the test, may as well also stick
> the call to serialize the second value inside that test.)  You should probably
> also add a test for this.

You missed a subtlety: both overloads of SetPairValue abort if *either* value is eCSSUnit_Null (or _Inherit or _Initial), so nsCSSValuePair::AppendToString never sees a half-null pair.  This is not the case for pairlists, but nsCSSValuePairList already has the check you ask for here, so (in particular) -moz-background-size: cover/contain does not regress.

I added a check for mYValue being null to nsCSSValuePair::AppendToString anyway (in part 2), because we might in the future want a half-null pair for some reason and then it would be easy to miss, but I can't add a test, because there's no way to trigger the potential problem in the current code.

Comment 58

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

Which patch changed how cover and contain are stored, then?  I didn't see any changes to ParseBackgroundSizeValues.

Comment 59

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

Ah, sorry, I missed that nsCSSValuePairList::AppendToString wasn't using nsCSSValuePair::AppendToString.  I guess it doesn't even use nsCSSValuePair.

Comment 60

[Zack Weinberg (:zwol)]

(In reply to comment #59)
> Ah, sorry, I missed that nsCSSValuePairList::AppendToString wasn't using
> nsCSSValuePair::AppendToString.  I guess it doesn't even use nsCSSValuePair.

Yeah, it never has.  I suppose that could change, but it doesn't seem necessary for this bug.

Moving on ...

(In reply to comment #48)
> Comment on attachment 465085 [details] [diff] [review]
> part 5: valuelist as value unit

Note: I've reorganized this to put the important bits up top.

> I believe the kContentKTable / kContentAltKTable split makes
> -moz-alt-content get serialized as ... well, I'm not exactly sure.  In
> any case, the code path that needs to work is
> nsCSSValue::AppendToString's eCSSUnit_Enumerated clause, which calls
> nsCSSProps::LookupPropertyValue which uses kKeywordTableTable, which
> looks only at kContentKTable because that's what's in nsCSSPropList.h.
> I can see two reasonable ways to unbreak this, off the top of my head:
>  1) have *three* tables; make the full one be the official table for
>     the property, and then have two subtables splitting the values
>  2) go back to storing -moz-alt-content as a single item value list,
>     which I think might be simpler in the end.
> Please add a test.

As is, -moz-alt-content serializes to the empty string.  I've fixed it
with your approach 1, with the subtables living in nsCSSParser.cpp
since that's the only place that needs them.  The existing
property_database.js tests can detect this, but -moz-alt-content did
not appear in that file, so I've added it.

> Also, you changed [ParseTransitionProperty] to also check for
> '-moz-initial'.  Don't.  '-moz-initial' is a perfectly valid value
> of transition-property.

You really want me to make '-moz-initial' behave differently than
'initial'?!  Especially considering that the leading ParseVariant()
recognizes '-moz-initial' but *not* 'initial'?  If anything it seems
like we should take *initial* out of that special case, not -moz-initial.

I left this unchanged for now.

> (Note that it wouldn't be that hard to make operator== work properly
> using an AdjustUnit function; I think it's safe to assume that casting
> a class with no vtable pointer to its only base class doesn't require
> an adjustment.)

True, but I don't think we need it.

> Do you recall why you added null-checks in SetDependentListValue
> and SetDependentPairListValue?

nsStyleAnimation::Value::GetCSSValueListValue and ::GetCSSValuePairListValue
can, under some circumstances, return null.  I don't know exactly what those
circumstances are, but I definitely get crashes in layout/style mochitests if
I replace the null checks with assertions.  Formerly, a null list/pairlist
pointer for list-type storage was treated the same way that eCSSUnit_Null is
for value-type storage, so I believe a null-check after Reset() is the correct
thing here.

> >+    // can get a _None in here from transform animation
>
> Should be easily fixable with a small change to
> nsStyleAnimation::UncomputeValue.  Maybe an additional patch on top; if
> not, I'll try to do it in a followup.

I'd rather it were done in a followup, I don't really understand
nsStyleAnimation.

[no questions below here]

> >+  // background-color can only be set once, so it's not in BackgroundItem.
>
> There's no |BackgroundItem| anymore, so this should be reworded.

Done.

> please make this a loop instead:
>
>   for (const nsCSSProperty* subprops =
>          nsCSSProps::SubpropertyEntryFor(eCSSProperty_background);
>        *subprops != eCSSProperty_UNKNOWN; ++subprops) {
>     AppendValue(*subprops, color);
>   }

Done.

> >+  color.Reset(); // just to be sure
>
> Assert if you want, but don't add real code that might not get
> optimized away.

Done (just removed it).

> In ParseBackgroundItem, could you put the PR_STATIC_ASSERTS back
> where they were?

Done.

> And also put the |haveColor = PR_TRUE| back where it was; I don't
> see a good reason to move it, and the move makes it inconsistent
> with all the other functions.  (Twice.)

Done.

> In ParseTransitionProperty, you dropped this comment, which is
> important (in that it says why we do what we do in a case where
> the spec is vague):
> >-    // Exclude 'none' and 'all' and 'inherit' and 'initial' according to
> >-    // the same rules as for 'counter-reset' in CSS 2.1 (except
> >-    // 'counter-reset' doesn't exclude 'all' since it doesn't support
> >-    // 'all' as a special value).
> Please add it back.

Done.

> In ParseTransition:
[...]
> You've managed to have three different ways to exit when there are only
> two possibilities.  How about removing both the |else| (not needed after
> the return) and the |continue|?

Done.

> In nsCSSProps.h:
> > // A property that needs to have image loads started when a URL value
> >-// for the property is used for an element.  Supported only for
> >-// eCSSType_Value and eCSSType_ValueList.
> >+// for the property is used for an element.
> > #define CSS_PROPERTY_START_IMAGE_LOADS            (1<<5)
>
> It's still worth saying that it's only supported for the image (or, when
> CSS_PROPERTY_IMAGE_IS_IN_ARRAY_0 is set, array) being directly in the
> value or or as an item in an eCSSUnit_List value.

Done.

> nsCSSStruct.cpp:
> >- * and other provisions required by the GPL or the LGPL. If you do not delete
> >+ * and other provisions required by the GPL or the LGPL. If you do not deleteu
>
> oops!

Fixed.

> In nsCSSValue.cpp, same issues for ListDep as for PairListDep
> in the previous patch.

Done.

> nsCSSValue.h: same thing as previous about the comments for ListDep,
> and also about the type checks in both GetListValue impls.

Done.

> In GetShadowData in nsRuleNode.cpp, please add assertions that the
> type of each item in the list is eCSSUnit_Array.  (You removed a
> check for the array type in ComputeTextData.)  It could go right at
> the top of the for loop (for each value, this time!).

Done.

> Still in nsRuleNode.cpp:
> >+  } break;
> I'd prefer having the break inside the braces, i.e.,:
> +    break;
> +  }
> (at least five times; I'll stop noting them now)

Not all of those were mine, but I just went through and changed them all.

> In nsRuleNode::HasAuthorSpecifiedRules, you made the
> firstBackgroundImage stuff more complicated when you actually should
> have made it simpler.  In particular, instead of what you did, you
> should:
>  * remove |firstBackImage|
>  * make backgroundValues just point to &colorData.mBackImage instead
>  * remove the chunk:
> >        if ((ruleTypeMask & NS_AUTHOR_SPECIFIED_BACKGROUND) &&
> >            colorData.mBackImage &&
> >            firstBackgroundImage.GetUnit() == eCSSUnit_Null) {
> >          // Handle background-image being a value list
> >          firstBackgroundImage = colorData.mBackImage->mValue;
> >        }
> and everything should be good.

Done.  (Teach me to read the entire function, I totally should have
caught that.)

Comment 61

[Zack Weinberg (:zwol)]

(In reply to comment #49)
> Comment on attachment 465087 [details] [diff] [review]
> part 6: remove remaining vestiges of nsCSSType
> 
> >+                    ShouldIgnoreColors(aRuleData)) {
> >+                    if (iProp == eCSSProperty_background_color) {
> 
> Could you leave the opening brace after ShouldIgnoreColors on its
> own line?  Quirk of 4-space indent, please.

Sure.

> > #ifdef DEBUG
> >-            void *prop = PropertyAt(iProp);
> >+            nsCSSValue* val = PropertyAt(iProp);
> >+            NS_ASSERTION(val->GetUnit() != eCSSUnit_Null,
> >+                         "null value while computing size");
> > #endif
> 
> Drop the ifdefs, and just make this:
> NS_ASSERTION(PropertyAt(iProp)->GetUnit != eCSSUnit_Null,
>              "null value while computing size");

Done.

> Er, ok, ignore my comments on the previous two patches about
> keeping the assertions in nsStyleAnimation::UncomputeValue.
> Not sure what I was thinking.

:-)

> Given the dependent list and pairlist types, you absolutely need
> to keep this warning about lifetimes.  So put it back, reworded
> appropriately.

Good point.  Here's the new phrasing:

   * The first overload fills in an nsCSSValue object; the second
   * produces a string.  The nsCSSValue result may depend on objects
   * owned by the |aComputedValue| object, so users of that variant
   * must keep |aComputedValue| alive longer than |aSpecifiedValue|.

(In reply to comment #50)
> Comment on attachment 465088 [details] [diff] [review]
> part 7: cleanup pass on Declaration and DataBlock
> 
> This makes it sound like two are always allocated.  It's usually one.
> Could you update to reflect that aNormalBlock is always allocated, and
> aImportantBlock sometimes is.

New wording:

     * Allocate new compressed blocks and transfer all of the state
     * from this expanded block to the new blocks, clearing this
     * expanded block.  A normal block will always be allocated, but
     * an important block will only be allocated if there are
     * !important properties in the expanded block; otherwise
     * |*aImportantBlock| will be set to null.

Good?

(In reply to comment #51)
> Comment on attachment 465089 [details] [diff] [review]
> part 8: remove the last MoveValue call from the CSS parser
> 
> The tri-state return value is pretty bad, and I object to new uses of success
> nsresults other than NS_OK.
> 
> Please just make this return a boolean for whether the "trying" succeeded, and
> have a PRBool* aChanged for the rest.

Done.  I was trying to avoid out-parameters, but honestly I don't like non-NS_OK success nsresults either, and the calling code winds up neater this way.

(In reply to comment #53)
> Comment on attachment 465090 [details] [diff] [review]
> part 9: AddProperty method for nsCSSExpandedDataBlock
> 
> Fix the commit message to match the name you're using.

Fixed.

> >+nsCSSExpandedDataBlock::AddLonghandProperty(nsCSSProperty aProperty,
> >+                                            const nsCSSValue& aValue)
> >+{
> >+    NS_ASSERTION(nsCSSProps::IsShorthand(aProperty), "property out of range");
> 
> This assertion condition needs a ! in front of it.  (Doesn't seem too well
> tested.)

Fixed.  This function wasn't actually *used* until the next patch, and the assertion wasn't fatal until patch 11, so I didn't notice until patch 11.

(In reply to comment #55)
> 
> Ah, you fixed the issue from two patches back in this patch.  It should
> really be in the other one, though.  Trivial to fix by editing the mq
> patch files (just edit the minus line in this patch too).

Yah, I moved it.

> >-/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> >+/* -*- Mode: xC++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Fixed this too.  And that appears to be all the review comments; revised patch series shortly.

Comment 62

[Zack Weinberg (:zwol)]

Attachment: part 1: move storage classes out of nsCSSStruct.h/nsCSSStruct.cpp

Here comes new patch series.  In addition to the revisions described above, these have all been merged to latest trunk (in particular, on top of bug 537890).  My commit access seems to have been turned off by mistake, so this will unfortunately not get pushed to try until tomorrow morning at the earliest.

Comment 63

[Zack Weinberg (:zwol)]

Attachment: part 2: valuepair as value unit

Comment 64

[Zack Weinberg (:zwol)]

Attachment: part 3: rect as value unit

Comment 65

[Zack Weinberg (:zwol)]

Attachment: part 4: valuepairlist as value unit

Not carrying r+ forward on this one because of the "I'd like to look at what you've done with operator==" in comment 46.

Comment 66

[Zack Weinberg (:zwol)]

Attachment: part 5: valuelist as value unit

Not carrying r+ forward on this one because of the [-moz-]initial thing discussed in comment 48 and comment 60.

Comment 67

[Zack Weinberg (:zwol)]

Attachment: part 6: remove remaining vestiges of nsCSSType

Comment 68

[Zack Weinberg (:zwol)]

Attachment: part 7: cleanup pass on Declaration and DataBlock

Comment 69

[Zack Weinberg (:zwol)]

Attachment: part 8: remove the last MoveValue call from the CSS parser

Comment 70

[Zack Weinberg (:zwol)]

Attachment: part 9: AddProperty method for nsCSSExpandedDataBlock

Comment 71

[Zack Weinberg (:zwol)]

Attachment: part 10: remove all direct access to nsCSSExpandedDataBlock data members from parser

Comment 72

[Zack Weinberg (:zwol)]

Attachment: part 11: make a whole lot of assertions fatal

Comment 73

[Zack Weinberg (:zwol)]

Attachment: part 12: fix assertions in nsStyleAnimation.cpp

Comment 74

[Zack Weinberg (:zwol)]

Attachment: roll-up patch for approval

Speculatively requesting a2.0 on the whole series.

Comment 75

[Benjamin Smedberg]

Comment on attachment 466962 [details] [diff] [review]
roll-up patch for approval

Please don't request approval until reviews are complete.

Comment 76

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

Comment on attachment 466948 [details] [diff] [review]
part 4: valuepairlist as value unit

r=dbaron

Comment 77

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

Comment on attachment 466950 [details] [diff] [review]
part 5: valuelist as value unit

Could you make kContentSolitaryKWs and kContentListKWs |static|?

And maybe add an assertion (NS_ABORT_IF_FALSE, since I don't think it can be a PR_STATIC_ASSERT, although I could be wrong) that
nsCSSProps::kContentKTable[NS_ARRAY_LENGTH(kContentListKWs) + NS_ARRAY_LENGTH(kContentSolitaryKWs) - 4] == eCSSKeyword_UNKNOWN
?  (I think it's -4.  The point is to check that the size of the lists adds to the size of kContentKTable, to warn anyone who might be changing one of them.)

In comment 48, I wrote:
> Also, you changed this to also check for '-moz-initial'.  Don't.
> '-moz-initial' is a perfectly valid value of transition-property.

In comment 60, you wrote:
> You really want me to make '-moz-initial' behave differently than
> 'initial'?!  Especially considering that the leading ParseVariant()
> recognizes '-moz-initial' but *not* 'initial'?  If anything it seems
> like we should take *initial* out of that special case, not -moz-initial.

Yes, I really want what I said.  We treat -moz-initial specially for the first value, but only for now.  But we should forbid 'initial' because the rule forbidding 'initial' (for counters) is in CSS 2.1 and has been through CR -- it's forbidden for future-compatibility.  It happens that we implement that future with a -moz- prefix, but that's distinct from the rule forbidding it.  (Also forbidding '-moz-initial' in addition to 'initial' might be a reasonable request, but it shouldn't be jammed into this patch... and I think will be a moot point relatively soon so isn't worth changing.  However, we definitely should be forbidding 'initial'.)

r=dbaron with that

Comment 78

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

Comment on attachment 466955 [details] [diff] [review]
part 8: remove the last MoveValue call from the CSS parser

>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
>+  nsresult result;
>+

>-  nsresult result = mScanner.GetLowLevelError();
>+  result = mScanner.GetLowLevelError();

Drop these changes; they made sense in the previous version but no longer do.

r=dbaron with that

Comment 79

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

Comment on attachment 466962 [details] [diff] [review]
roll-up patch for approval

a2.0=dbaron

Comment 80

[Zack Weinberg (:zwol)]

Attachment: part 5: valuelist as value unit

(In reply to comment #77)
> Could you make kContentSolitaryKWs and kContentListKWs |static|?

Done.

> And maybe add an assertion (NS_ABORT_IF_FALSE, since I don't think it can be a
> PR_STATIC_ASSERT, although I could be wrong) that
> nsCSSProps::kContentKTable[NS_ARRAY_LENGTH(kContentListKWs) +
> NS_ARRAY_LENGTH(kContentSolitaryKWs) - 4] == eCSSKeyword_UNKNOWN
> ?  (I think it's -4.  The point is to check that the size of the lists adds to
> the size of kContentKTable, to warn anyone who might be changing one of them.)

And done.  (-4 is correct.  I had it check [-4] == eCSSKeyword_UNKNOWN and [-3] = -1, because both are numerically -1.)

> Yes, I really want what I said.  We treat -moz-initial specially for the first
> value, but only for now.  But we should forbid 'initial' because the rule
> forbidding 'initial' (for counters) is in CSS 2.1 and has been through CR --
> it's forbidden for future-compatibility.  It happens that we implement that
> future with a -moz- prefix, but that's distinct from the rule forbidding it. 
> (Also forbidding '-moz-initial' in addition to 'initial' might be a reasonable
> request, but it shouldn't be jammed into this patch... and I think will be a
> moot point relatively soon so isn't worth changing.  However, we definitely
> should be forbidding 'initial'.)

Ok, I've made the change.  Thanks for the explanation.

Comment 81

[Zack Weinberg (:zwol)]

Attachment: part 8: remove the last MoveValue call from the CSS parser

(In reply to comment #78)
> >diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
> >+  nsresult result;
> >+
> 
> >-  nsresult result = mScanner.GetLowLevelError();
> >+  result = mScanner.GetLowLevelError();
> 
> Drop these changes; they made sense in the previous version but no longer do.

Done.

Comment 82

[Zack Weinberg (:zwol)]

Attachment: updated rollup patch

Comment 83

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

No need to bother updating the rollup patch.  And *please* land it as separate changesets.

Comment 84

[Zack Weinberg (:zwol)]

(In reply to comment #83)
> No need to bother updating the rollup patch.  And *please* land it as separate
> changesets.

I did that to make it easier for Taras to push it to try for me :)

The next time I'll have an opportunity to land things is probably next week, fyi.  IT still hasn't turned my commit access back on, and CMU orientation starts tomorrow.

Comment 85

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

http://hg.mozilla.org/mozilla-central/rev/301875d4f9b6
http://hg.mozilla.org/mozilla-central/rev/4fc85e572c38
http://hg.mozilla.org/mozilla-central/rev/ed89c9e297ab
http://hg.mozilla.org/mozilla-central/rev/a3e21759b570
http://hg.mozilla.org/mozilla-central/rev/b88472b0af90
http://hg.mozilla.org/mozilla-central/rev/f09c1638d3c1
http://hg.mozilla.org/mozilla-central/rev/980f0170d982
http://hg.mozilla.org/mozilla-central/rev/659a0864e035
http://hg.mozilla.org/mozilla-central/rev/4bb2e0074aeb
http://hg.mozilla.org/mozilla-central/rev/5a9bd15fd7a8
http://hg.mozilla.org/mozilla-central/rev/2f078585a0f6
http://hg.mozilla.org/mozilla-central/rev/992491c618de