569719 - 17 miscellaneous code cleanup patches

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Zack Weinberg (:zwol)]

These are 17 miscellaneous cleanups that were originally prep work for bug 553456. The future of that bug is in doubt, but these are worthwhile in their own right and performance-neutral.  I'll explain each one as I file them.

Comment 1

[Zack Weinberg (:zwol)]

Attachment: 1/17: rename nsCSSDeclaration -> mozilla::css::Declaration

This just changes the type name; the header is still nsCSSDeclaration.h.

Comment 2

[Zack Weinberg (:zwol)]

Attachment: 2/17: rename some headers and source files

This renames nsCSSDeclaration.{h,cpp} to Declaration.{h,cpp} and arranges for Declaration.h to be exported as mozilla/css/Declaration.h.  It also does the same for nsCSSLoader.{h,cpp}, which were already defining a type in namespace mozilla::css::.

Comment 3

[Zack Weinberg (:zwol)]

Attachment: 3/17: Kill Declaration::InitializeEmpty and Declaration::RuleAbort

This makes allocating and deallocating Declaration objects more normal - they are empty on creation, and they can be destroyed with 'delete' instead of a special method call.  This allows one to use nsAutoPtr with them, for instance.

Comment 4

[Zack Weinberg (:zwol)]

Attachment: 4/17: nsDOMCSSDeclaration::SetCSSText code path cleanup

This streamlines the nsDOMCSSDeclaration::SetCSSText code path a bit by folding one method into its sole caller, removing boolean flag arguments to another method that were always called with the same value, and renaming one of the parser entry points to be more logical.

Comment 5

[Zack Weinberg (:zwol)]

Attachment: 5/17: default values for last two arguments to nsCSSParser::ParseProperty

Almost all callers of nsCSSParser::ParseProperty pass a dummy and PR_FALSE as the last two arguments.  This makes it so they don't have to bother.

Comment 6

[Zack Weinberg (:zwol)]

Attachment: 6/17: Move RuleDataPropertyAt from nsCSSExpandedDataBlock to nsRuleData

What it says on the tin.  (Also it's now called StorageFor.)

Comment 7

[Zack Weinberg (:zwol)]

Attachment: 7/17: use operator== for value lists, streamline Clone

nsCSSValueList and nsCSSValuePairList had 'Equal()' predicates when all other value types had operator==.  This makes them consistent.  Also, removes the 'aDeep' argument to their Clone() methods, which was invariably PR_TRUE for external callers.

Comment 8

[Zack Weinberg (:zwol)]

Attachment: 8/17: move Declaration::AppendCSSValueToString into nsCSSValue

Declaration::AppendCSSValueToString doesn't really have anything to do with Declaration objects, so this makes it an nsCSSValue method.  I called it AppendAsSpecified, but I could be persuaded to change that.

Comment 9

[Zack Weinberg (:zwol)]

Attachment: 9/17: Declaration::AppendStorageToString into nsCSSRect, nsCSSValuePair, etc

Same idea as the previous patch, but for Declaration::AppendStorageToString, which becomes AppendAsSpecified methods for nsCSSRect, nsCSSValuePair, nsCSSValueList, and nsCSSValuePairList (i.e. the other four storage types).

Comment 10

[Zack Weinberg (:zwol)]

Attachment: 10/17: have nsXULElement::GetStyle call nsStyledElement::GetStyle

This is a little far afield for 'style system' but it helps with problems I was having down the road in bug 553456, and fixes an XXX comment.  The tail of nsXULElement::GetStyle is almost but not quite the same as nsStyledElement::GetStyle; it appears to me that they *should* be the same; and there's no reason the former can't just call the latter, as far as I see.

Comment 11

[Zack Weinberg (:zwol)]

Attachment: 11/17: make css::Declaration methods return nothing when they always return NS_OK

Several css::Declaration methods always return NS_OK; this makes them return nothing.  Also, when GetCSSDeclaration is not being asked to allocate memory, it never fails, so its return value may be ignored (perhaps infallible-malloc means it never fails even then, but I wasn't sure about that.)  And in some places I reorganized code so that methods could tail-call DeclarationChanged.

Comment 12

[Zack Weinberg (:zwol)]

Attachment: 12/17: replace DeclarationChanged with PutCSSDeclaration

The next three patches need to be understood together; their net effect is to hide data blocks from style rules.  The original motivation was that (in bug 553456) I meant to eliminate data blocks altogether; css::Declaration would directly contain its data (in the form of several TArrays).  I think this change is still worthwhile even if we don't end up doing that, but I can see an argument otherwise.  It doesn't hurt anything, though.

This piece replaces DeclarationChanged() with a method called PutCSSDeclaration that takes the replacement Declaration object as an argument.

Comment 13

[Zack Weinberg (:zwol)]

Attachment: 13/17: Copy the entire declaration object before changing it in any way

EnsureMutable now copies the entire Declaration and returns the copy, rather than just copying the data blocks.  And we call EnsureMutable in more places.

Comment 14

[Zack Weinberg (:zwol)]

Attachment: 14/17: rules hold declarations, not data blocks

CSSImportantRule and CSSStyleRuleImpl hold pointers to Declaration objects, not direct pointers to nsCSSCompressedDataBlock objects.

Reference counting of nsCSSCompressedDataBlock is probably unnecessary at this point but I did not notice that until much later in the patch series for bug 553456, and I haven't untangled it from earlier patches that are not yet desirable.

Comment 15

[Zack Weinberg (:zwol)]

Attachment: 15/17: Refactor !important handling in the CSS parser.

This factors out the parsing of !important to its own method (called ParsePropertyTrailer, 'cos there might be other !tags someday), which removes a lot of repetition in ParseDeclaration.  The patch also enables fast-path value updates in ParseProperty, when an !important property changes but it does not lose the !important.

Comment 16

[Zack Weinberg (:zwol)]

Attachment: 16/17: Move CSSParserImpl::ClearTempData into nsCSSExpandedDataBlock

The file name says "remove cleartempdata" but what this really does is merge that method into nsCSSExpandedDataBlock::ClearProperty, which did exist but could not handle shorthands.  Now it can.

Comment 17

[Zack Weinberg (:zwol)]

Attachment: 17/17: move CopyValue and (Do)TransferTempData into nsCSSExpandedDataBlock

Same idea as the previous, but for the other parser methods that directly manipulate expanded data blocks.

Comment 18

[Zack Weinberg (:zwol)]

I will, of course, go back and fix up all the commit messages (which currently say "bug 553456 (x/30)".

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Seeing "... a CSS declaration block (which we misname a |nsCSSDeclaration|)...", 
I wonder if there is a particular reason not to rename nsCSSDeclaration to DeclarationBlock rather than Declaration?

Comment 20

[Zack Weinberg (:zwol)]

(In reply to comment #19)
> Seeing "... a CSS declaration block (which we misname a
> |nsCSSDeclaration|)...", 
> I wonder if there is a particular reason not to rename nsCSSDeclaration to
> DeclarationBlock rather than Declaration?

That comment is just wrong, and should go away (and might in fact go away in a later patch).  The CSSOM object for a declaration block is called CSSStyleDeclaration, so calling the internal object mozilla::css::Declaration is consistent.  I discovered this some time after I wrote that sentence.

Comment 21

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

It's still inconsistent with CSS, though.

Comment 22

[Zack Weinberg (:zwol)]

My inclination is to match CSSOM rather than CSS2 on this, because that way the internal C++ object has a name consistent with the C++ and IDL for the exposed CSSOM interfaces (nsDOMCSSDeclaration etc).

I don't care very much, though, I could have this patch rename it to mozilla::css::DeclarationBlock if you'd rather.

Comment 23

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

Comment on attachment 448842 [details] [diff] [review]
1/17: rename nsCSSDeclaration -> mozilla::css::Declaration

Maybe use |namespace css = mozilla::css;| in nsAttrValue.cpp too?
(avoids needing to rewrap)

>+    // It's worth a comment here that the main Declaration is refcounted,

How about s/Declaration/css::Declaration/

>-  ~nsCSSDeclaration(void);
>-····
>+  ~Declaration(void);
>+

If you're touching this, change (void) to ().  (And in the .cpp too.)

In nsICSSStyleRule.h, how about "due to a change in the declaration" 
instead of "... |Declaration|".

r=dbaron with that

Comment 24

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

Comment on attachment 448843 [details] [diff] [review]
2/17: rename some headers and source files

This changes the Declaration header from being private to public, which I think is not something we want.  So review- because of that.

Comment 25

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

Comment on attachment 448846 [details] [diff] [review]
3/17: Kill Declaration::InitializeEmpty and Declaration::RuleAbort

I think most of the simplification here (including RuleAbort) comes from infalliable malloc; I'm not sure about the removal of InitializeEmpty, since it's what allows us to not litter the code with null-checks (how confident are you that you got all the ones needed?)

Comment 26

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

Comment on attachment 448850 [details] [diff] [review]
4/17: nsDOMCSSDeclaration::SetCSSText code path cleanup

I don't like the name ParseDeclarationBody; it introduces a new term
("body") for no good reason.  How about ParseDeclarations instead?

nsCSSParser.h should clearly document that ParseDeclarations clears out
the css::Declaration before parsing into it.

r=dbaron with that

Comment 27

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

Comment on attachment 448851 [details] [diff] [review]
5/17: default values for last two arguments to nsCSSParser::ParseProperty

I'm ok with making aChanged have a default value.  However, I'd rather
not make aIsImportant have a default value, given that in some cases
it's information that callers have to propagate from elsewhere.  So I'd
rather make sure callers think about that.  (I'm also not a huge fan of
default params in general.)

So I'm ok with revising this in one of two ways:
 * swap the order of the last 2 parameters, and make aChanged default
   to null
 * drop the whole patch

Comment 28

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

Comment on attachment 448852 [details] [diff] [review]
6/17: Move RuleDataPropertyAt from nsCSSExpandedDataBlock to nsRuleData

>+  return reinterpret_cast<void*>
>+    (cssstruct + offsets.member_offset);

No need for a line break.

(And do you need to add the null-check of cssstruct?  Can it be an
assertion instead?  There was no null-check before.)

I'm not particularly happy about dragging more headers into
nsRuleData.h, which is widely included, but oh well...

Please leave mLevel and mIsImportantRule uninitialized.  We want to
catch failure to initialize them with valgrind.

Shouldn't the typesafe helper functions (ValueFor through
ValuePairListFor) return references rather than pointers?

r=dbaron with that

Comment 29

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

Comment on attachment 448853 [details] [diff] [review]
7/17: use operator== for value lists, streamline Clone

>+  bool operator==(nsCSSValueList const& aOther) const;
>+  bool operator!=(const nsCSSValueList& aOther) const
>+  { return !(*this == aOther); }

Please comment that these do deep comparison.

(same for ValuePairList)

r=dbaron with that

(Hopefully this won't make people make the mistake of doing pointer
comparison when the want object comparison...)

Comment 30

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

Comment on attachment 448854 [details] [diff] [review]
8/17: move Declaration::AppendCSSValueToString into nsCSSValue

AppendAsSpecified seems misnamed; it suggests that we know exactly how
it was specified, when in fact we've done a bit of normalization
(although much less than we've done for computed style).

Instead, could you call it SerializeTo or AppendToString?  (Or ask me
if you have another option.)

(This has bugged me for a while; I'm glad you're fixing it.)

r=dbaron with that rename

Comment 31

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

Comment on attachment 448855 [details] [diff] [review]
9/17: Declaration::AppendStorageToString into nsCSSRect, nsCSSValuePair, etc

Again, search-replace the name to match whatever you do in patch 8.

+  // This starts at eCSSProperty_UNKNOWN instead of 0 because _UNKNOWN
+  // gets used for some recursive calls below.
+  NS_ABORT_IF_FALSE(eCSSProperty_UNKNOWN <= aProperty &&
+                    aProperty < eCSSProperty_COUNT_no_shorthands,
+                    "property ID out of range");

Seems better not to assume the position of UNKNOWN relative to the
rest -- could you instead assert that it's either ==UNKNOWN or between
0 and COUNT_no_shorthands?

In nsStyleAnimation, please indent the case statements 2 lines in
from the switch (and their bodies another 2 lines).

Why did you remove AppendStorageToString when removing it just forces
you to duplicate it at both callers?

r=dbaron with that fixed

Comment 32

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

Comment on attachment 448858 [details] [diff] [review]
11/17: make css::Declaration methods return nothing when they always return NS_OK

Could you add an assertion at the GetCSSDeclaration callers documenting
that it can't fail when passed PR_FALSE.  (Then again, with infalliable
malloc, we can probably make it never fail, period, and just return
void, but that's a little more work.)

Or if it's too much work to fix all the callers, at least document it
in the header file as a requirement of implementations of the pure
virtual function.  Actually, document it in the header either way.

(I'd note that you were inconsistent in not removing |nsresult rv| in
nsDOMCSSDeclaration::RemoveProperty.)

r=dbaron with that

Comment 33

[Zack Weinberg (:zwol)]

(In reply to comment #23)
> (From update of attachment 448842 [details] [diff] [review])
> Maybe use |namespace css = mozilla::css;| in nsAttrValue.cpp too?
> (avoids needing to rewrap)
>
> >+    // It's worth a comment here that the main Declaration is refcounted,
> 
> How about s/Declaration/css::Declaration/
> 
> >-  ~nsCSSDeclaration(void);
> >-····
> >+  ~Declaration(void);
> >+
> 
> If you're touching this, change (void) to ().  (And in the .cpp too.)
> 
> In nsICSSStyleRule.h, how about "due to a change in the declaration" 
> instead of "... |Declaration|".
> 
> r=dbaron with that

I'll make all of these changes.

(In reply to comment #24)
> (From update of attachment 448843 [details] [diff] [review])
> This changes the Declaration header from being private to public, 
> which I think is not something we want.  So review- because of that.

I would argue that that ship has already sailed -- content/ has three
places that genuinely need it (plus several more that don't but include it anyway).  Yeah, they're all doing -I$(topsrcdir)/layout/style, but maybe they shouldn't have to (they need it for other headers than this one, though).

If I do take it out of EXPORTS, then the content/ users are going to be referring to it as "Declaration.h" which is suboptimal.  I'm open to other ideas.

(In reply to comment #25)
> (From update of attachment 448846 [details] [diff] [review])
> I think most of the simplification here (including RuleAbort) comes from
> infalliable malloc;

I see the real advantage as making it possible to use nsAutoPtr<css::Declaration> - that didn't happen in this patch but did later.

> I'm not sure about the removal of InitializeEmpty, since
> it's what allows us to not litter the code with null-checks (how confident are
> you that you got all the ones needed?)

Very confident but only over the course of the complete patch series; they probably aren't all in here.  Also, if we ever get back to bug 553456, that will make the null checks unnecessary again, in the end.

(In reply to comment #26)
> (From update of attachment 448850 [details] [diff] [review])
> I don't like the name ParseDeclarationBody; it introduces a new term
> ("body") for no good reason.  How about ParseDeclarations instead?
> 
> nsCSSParser.h should clearly document that ParseDeclarations clears out
> the css::Declaration before parsing into it.
> 
> r=dbaron with that

Will make these changes.

(In reply to comment #27)
> (From update of attachment 448851 [details] [diff] [review])
> I'm ok with making aChanged have a default value.  However, I'd rather
> not make aIsImportant have a default value, given that in some cases
> it's information that callers have to propagate from elsewhere.  So I'd
> rather make sure callers think about that.  (I'm also not a huge fan of
> default params in general.)
> 
> So I'm ok with revising this in one of two ways:
>  * swap the order of the last 2 parameters, and make aChanged default
>    to null
>  * drop the whole patch

It seems that there are only three calls to this API, I remembered there being more.  So I'd be okay with dropping the patch.  If we keep it, though, I do want to keep both default parameters, because two of the three uses pass a dummy and PR_FALSE, and the third passes non-dummies for both.

Comment 34

[Zack Weinberg (:zwol)]

(In reply to comment #28)
> (From update of attachment 448852 [details] [diff] [review])
> >+  return reinterpret_cast<void*>
> >+    (cssstruct + offsets.member_offset);
> 
> No need for a line break.
> 
> (And do you need to add the null-check of cssstruct?  Can it be an
> assertion instead?  There was no null-check before.)

I have a dim memory of its being necessary, but I'll take it out and retest.

> I'm not particularly happy about dragging more headers into
> nsRuleData.h, which is widely included, but oh well...

Yah.  I wish you could forward declare enums.

> Please leave mLevel and mIsImportantRule uninitialized.  We want to
> catch failure to initialize them with valgrind.

I don't agree with this.  The floods of "may be used uninitialized" warnings in layout/style mean that I wind up ignoring *all* warnings, and more than once this has meant that I wasted a bunch of time debugging something that the compiler would have caught if I had been paying attention.

> Shouldn't the typesafe helper functions (ValueFor through
> ValuePairListFor) return references rather than pointers?

Unfortunately, they can return null.

(In reply to comment #29)
> (From update of attachment 448853 [details] [diff] [review])
> >+  bool operator==(nsCSSValueList const& aOther) const;
> >+  bool operator!=(const nsCSSValueList& aOther) const
> >+  { return !(*this == aOther); }
> 
> Please comment that these do deep comparison.
> (same for ValuePairList)

Will do.

(In reply to comment #30)
> (From update of attachment 448854 [details] [diff] [review])
> AppendAsSpecified seems misnamed; it suggests that we know exactly how
> it was specified, when in fact we've done a bit of normalization
> (although much less than we've done for computed style).
> 
> Instead, could you call it SerializeTo or AppendToString?  (Or ask me
> if you have another option.)

I'll go with AppendToString, since that's consistent with other stuff in this area.  I don't remember why I picked AppendAsSpecified.

(In reply to comment #31)
> (From update of attachment 448855 [details] [diff] [review])
> Again, search-replace the name to match whatever you do in patch 8.
> 
> +  // This starts at eCSSProperty_UNKNOWN instead of 0 because _UNKNOWN
> +  // gets used for some recursive calls below.
> +  NS_ABORT_IF_FALSE(eCSSProperty_UNKNOWN <= aProperty &&
> +                    aProperty < eCSSProperty_COUNT_no_shorthands,
> +                    "property ID out of range");
> 
> Seems better not to assume the position of UNKNOWN relative to the
> rest -- could you instead assert that it's either ==UNKNOWN or between
> 0 and COUNT_no_shorthands?

Ok.

> In nsStyleAnimation, please indent the case statements 2 lines in
> from the switch (and their bodies another 2 lines).

Really? Usage in this directory seems to be that the case statements line up with 'switch'.

> Why did you remove AppendStorageToString when removing it just forces
> you to duplicate it at both callers?

Because I wanted it out of Declaration.cpp, to which it does not belong.  I suppose it could go in nsCSSDataBlock.cpp, but the duplicated code goes away in bug 576044 so I don't see much point in rearranging here.

(In reply to comment #32)
> (From update of attachment 448858 [details] [diff] [review])
> Could you add an assertion at the GetCSSDeclaration callers documenting
> that it can't fail when passed PR_FALSE.  (Then again, with infalliable
> malloc, we can probably make it never fail, period, and just return
> void, but that's a little more work.)
>
> Or if it's too much work to fix all the callers, at least document it
> in the header file as a requirement of implementations of the pure
> virtual function.  Actually, document it in the header either way.

I'll have another look at this and see if I can't make it return void.  There was a place (when passed PR_TRUE) where it seemed like it could fail even with infallible malloc, but I might have been wrong.  Or I'll do as you suggest.
 
> (I'd note that you were inconsistent in not removing |nsresult rv| in
> nsDOMCSSDeclaration::RemoveProperty.)

Yeah - see above about not noticing warnings.  I'll remove it.

Comment 35

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

(In reply to comment #33)
> I would argue that that ship has already sailed -- content/ has three
> places that genuinely need it (plus several more that don't but include it
> anyway).  Yeah, they're all doing -I$(topsrcdir)/layout/style, but maybe they
> shouldn't have to (they need it for other headers than this one, though).
> 
> If I do take it out of EXPORTS, then the content/ users are going to be
> referring to it as "Declaration.h" which is suboptimal.  I'm open to other
> ideas.

I'll accept it if you put, at the top of the file:

#ifndef _IMPL_NS_LAYOUT
#error "This file should only be included within the layout library"
#endif

> (In reply to comment #25)
> > (From update of attachment 448846 [details] [diff] [review] [details])
> > I think most of the simplification here (including RuleAbort) comes from
> > infalliable malloc;
> 
> I see the real advantage as making it possible to use
> nsAutoPtr<css::Declaration> - that didn't happen in this patch but did later.

Using nsAutoPtr on a reference-counted class seems like a bad idea.  I think I need to discuss patches 12-14 with you further, though.

> (In reply to comment #27)
> It seems that there are only three calls to this API, I remembered there being
> more.  So I'd be okay with dropping the patch.  If we keep it, though, I do
> want to keep both default parameters, because two of the three uses pass a
> dummy and PR_FALSE, and the third passes non-dummies for both.

I think it's best to drop the whole patch, then.

(In reply to comment #34)
> (In reply to comment #28)
> I have a dim memory of its being necessary, but I'll take it out and retest.

It shouldn't be.

> > Please leave mLevel and mIsImportantRule uninitialized.  We want to
> > catch failure to initialize them with valgrind.
> 
> I don't agree with this.  The floods of "may be used uninitialized" warnings in
> layout/style mean that I wind up ignoring *all* warnings, and more than once
> this has meant that I wasted a bunch of time debugging something that the
> compiler would have caught if I had been paying attention.

If there were a sensible value to initialize them to, I might be ok with it, but there isn't.  This particular occurrence also should not be responsible for any warnings, so the warnings argument isn't relevant here.

> > Shouldn't the typesafe helper functions (ValueFor through
> > ValuePairListFor) return references rather than pointers?
> 
> Unfortunately, they can return null.

Why?  Callers shouldn't be calling this without an SID check that ensures the struct exists.

> (In reply to comment #31)
> > In nsStyleAnimation, please indent the case statements 2 lines in
> > from the switch (and their bodies another 2 lines).
> 
> Really? Usage in this directory seems to be that the case statements line up
> with 'switch'.

For the ancient code, yes, but not anything written since 2002, unless it slipped by me.

> > Why did you remove AppendStorageToString when removing it just forces
> > you to duplicate it at both callers?
> 
> Because I wanted it out of Declaration.cpp, to which it does not belong.  I
> suppose it could go in nsCSSDataBlock.cpp, but the duplicated code goes away in
> bug 576044 so I don't see much point in rearranging here.

ok, I guess.

> (In reply to comment #32)
> > (From update of attachment 448858 [details] [diff] [review] [details])
> > Could you add an assertion at the GetCSSDeclaration callers documenting
> > that it can't fail when passed PR_FALSE.  (Then again, with infalliable
> > malloc, we can probably make it never fail, period, and just return
> > void, but that's a little more work.)
> >
> > Or if it's too much work to fix all the callers, at least document it
> > in the header file as a requirement of implementations of the pure
> > virtual function.  Actually, document it in the header either way.
> 
> I'll have another look at this and see if I can't make it return void.  There
> was a place (when passed PR_TRUE) where it seemed like it could fail even with
> infallible malloc, but I might have been wrong.  Or I'll do as you suggest.

With infalliable malloc it can probably return void, but it requires a few additional patches to remove return values from things it calls.



Sorry for the delay, again... I posted those comments (which I wrote on the flight) right before the summit, and then got swamped with summit stuff.

Regarding patch 10:  I'm not sure that ReparseStyleAttribute is safe for XUL elements; I think the review request there should really be transferred to bzbarsky or maybe sicking.

We need to chat sometime about patches 12-14; I don't understand your motivation for those patches is.

Comment 36

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

Comment on attachment 448864 [details] [diff] [review]
15/17: Refactor !important handling in the CSS parser.

Instead of ParsePropertyTrailer, how about ParsePriority, since that's
an already-existing term for it (in the CSSOM)?  (And remove all comments
referring to "trailer" as well.)

In SlotForValue, could you move the declaration of other down, and use
the pattern:

#ifdef DEBUG
  {
    nsCSSCompressedDataBlock *other = ...
    NS_ASSERTION(...
  }
#endif

so that it can't accidentally be used outside the ifdef?



This has a bunch of different refactorings jumbled together -- the refactoring of the parsing of declarations seems unrelated to the performance improvements to the !important case in ParseProperty.

Could you explain the motivation for the refactoring of property parsing?  A single function for the optional !important, the ;, and maybe the } doesn't seem like a logical group of things to me.

Comment 37

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

Comment on attachment 448865 [details] [diff] [review]
16/17: Move CSSParserImpl::ClearTempData into nsCSSExpandedDataBlock

The existing callers of ClearProperty don't need the shorthand check, though, so given that you have a non-shorthands function, it seems better to use it:
 * call it ClearLonghandProperty instead of DoClearProperty
 * use ClearLonghandProperty in nsCSSExpandedDataBlock::Clear, nsCSSDeclaration::RemoveProperty, and CSSParserImpl::DoTransferTempData

Also, make ClearLonghandProperty assert about aPropID.

r=dbaron with that

Comment 38

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

Comment on attachment 448866 [details] [diff] [review]
17/17: move CopyValue and (Do)TransferTempData into nsCSSExpandedDataBlock

Why does this refactoring make sense?  The things these methods do are conceptually quite specific to the parser.  Do you actually have other callers that use them?

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 448857 [details] [diff] [review]
10/17: have nsXULElement::GetStyle call nsStyledElement::GetStyle

Yeah, I guess the reparse thing is ok.  r=bzbarsky

Comment 40

[Zack Weinberg (:zwol)]

Attachment: part 1 as landed

I'm landing parts 1 and 2 of this now, to reduce my merge load.  This has been changed as requested in comment 23.

Comment 41

[Zack Weinberg (:zwol)]

Attachment: part 2 as landed

part 2 initially r- in comment 24, then accepted with a change (which has been made) in comment 35.

Comment 42

[Zack Weinberg (:zwol)]

Attachment: part 3: nsDOMCSSDeclaration::SetCSSText code path cleanup

Formerly part 4; now uses ParseDeclarations instead of ParseDeclarationBody.  Carrying r=dbaron forward from comment 26.

Comment 43

[Zack Weinberg (:zwol)]

Attachment: part 4: Move RuleDataPropertyAt from nsCSSExpandedDataBlock to nsRuleData

Formerly part 6.  Changed as requested in comment 28, except that the "typesafe helper functions" I just deleted, because (to my surprise - this patchset has been in development far too long) nothing actually uses them, and they'd go away in bug 576044 anyway.  Indeed, the null check was unnecessary.  Carrying r=dbaron forward.

Comment 44

[Zack Weinberg (:zwol)]

Attachment: part 5: operator== for value lists, streamline Clone

Formerly part 7.  Comment added as requested in comment 29, carrying r=dbaron forward.

Comment 45

[Zack Weinberg (:zwol)]

Attachment: part 6: move Declaration::AppendCSSValueToString into nsCSSValue

Formerly part 8; method renamed AppendToString per comment 30; carrying r=dbaron forward.

Comment 46

[Zack Weinberg (:zwol)]

Attachment: part 7: Declaration::AppendStorageToString into nsCSSRect, nsCSSValuePair, etc

Formerly part 9.  Changed as requested in comment 31.  Carrying r=dbaron forward.

Comment 47

[Zack Weinberg (:zwol)]

Attachment: part 8: have nsXULElement::GetStyle call nsStyledElement::GetStyle

Formerly part 10; r=bzbarsky from comment 39.  No changes.

Comment 48

[Zack Weinberg (:zwol)]

Attachment: part 9: remove pointless nsresult return values

Formerly part 11, changing some methods that cannot fail to return void instead of an nsresult.  

This is substantively different from the old revision so I'm asking for a new review.  The interesting change is that GetCSSDeclaration now returns the declaration object instead of returning the nsresult and putting the declaration in an out-parameter.  It *can* return null, and this *is* a failure if it was asked to allocate the declaration.  (It may not actually ever *happen*, but I could not prove to my own satisfaction that it couldn't.)  All this is documented.

There are also a few more places made explicitly infallible by virtue of infallible malloc (notably Declaration::InitializeEmpty) which let me get rid of a few more checks.

Comment 49

[Zack Weinberg (:zwol)]

Attachment: part 10: refactor parsing of !important and the end of a property declaration.

Formerly half of part 15, rewritten as discussed with dbaron face-to-face.  This factors out the parsing of !important to its own function, which allows me to remove a bunch of repetition in ParseDeclaration.

Comment 50

[Zack Weinberg (:zwol)]

Attachment: part 11: take the fast path in ParseProperty for !important replacing !important, as well as normal replacing normal.

What it says on the tin.  Formerly the other half of part 15.

Comment 51

[Zack Weinberg (:zwol)]

Attachment: part 12: replace nsCSSParser::ClearTempData with explicit use of mTempData.ClearProperty (and make that method handle shorthands)

Formerly part 16, revised as requested in comment 37; carrying r=dbaron forward.

Comment 52

[Zack Weinberg (:zwol)]

Attachment: part 13: move nsCSSParser methods that manipulate data blocks into the appropriate nsCSS*DataBlock classes.

Formerly part 17.  Yes, these methods are highly parser-specific, but with them moved, the parser doesn't have nearly as much intimate knowledge of the innards of an expanded data block, which is good when my ultimate goal here is to change those innards.

Comment 53

[Zack Weinberg (:zwol)]

Attachment: part 14: replace DeclarationChanged() with PutCSSDeclaration(), which takes the new declaration as an argument.

Formerly part 12, and not much changed since then.  The point of this exercise (old patches 3+12-14, new patches 14-18) is to arrange for style rules not to have to know anything about data blocks; they deal only in declarations.

Comment 54

[Zack Weinberg (:zwol)]

Attachment: part 15: Always copy the entire css::Declaration before changing it in any way.

A number of places that relied on the copying done internal to ExpandFrom now copy the entire declaration themselves.

Comment 55

[Zack Weinberg (:zwol)]

Attachment: part 16: Style rules don't refer directly to data blocks anymore.

What it says.

Comment 56

[Zack Weinberg (:zwol)]

Attachment: part 17: remove refcounting of declarations and data blocks.

These weren't *really* refcounted to begin with - they were owned by exactly one style rule or declaration - but we were using the refcounting to track mutability.  So now we do that explicitly instead.

Comment 57

[Zack Weinberg (:zwol)]

Attachment: part 18: kill css::Declaration::InitializeEmpty - declarations are empty when created

This gets rid of InitializeEmpty and CreateEmptyBlock.  Declarations are empty when first created.  Trades a few more null-checks in lookup paths against not creating objects on the heap that are going to be thrown away immediately.  (This was formerly part of patch 3; the rest of it is now superseded by the new part 17.)

Comment 58

[Zack Weinberg (:zwol)]

Attachment: part 18: kill css::Declaration::InitializeEmpty - declarations are empty when created

Tiny revision to remove a now-incorrect assertion.

Comment 59

[Zack Weinberg (:zwol)]

Part 17 (attachment 459672 [details] [diff] [review]) somehow fixes bug 338679 as a side effect.  I do not understand why - it would sort of make sense if part 16 fixed that bug, but not part 17 - but the todo in the mochitests consistently gets an UNEXPECTED-PASS with that patch applied, and not without.  bz, smaug, sayrer: any thoughts would be appreciated.

For the nonce I'm going to assume that it's a real fix and carry on with testing.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Doesn't part 17 make it so we treat the inline style decl as immutable so that when we modify the inline style we end up with two separate declarations for the two rules, etc?  So the test ends up passing because when we serialize the old rule we get the correct old value.

I would bet the test would fail if we then went ahead and changed style again without waiting for an intervening style reresolve, because that would mutate the declaration, right?  I wonder whether it's worth just marking declarations immutable if ToString is used on them to give us a completely correct fix for bug 338679....

Comment 61

[Zack Weinberg (:zwol)]

(In reply to comment #60)
> Doesn't part 17 make it so we treat the inline style decl as immutable so that
> when we modify the inline style we end up with two separate declarations for
> the two rules, etc?  So the test ends up passing because when we serialize the
> old rule we get the correct old value.

This seems plausible, except that part 16 is what should have made the old rule be preserved as you describe.  Part 17 _should have_ just made this explicit rather than implicit in the reference counting.

> I would bet the test would fail if we then went ahead and changed style again
> without waiting for an intervening style reresolve, because that would mutate
> the declaration, right?

I'll poke at that a little and see what happens.

> I wonder whether it's worth just marking declarations
> immutable if ToString is used on them to give us a completely correct fix for
> bug 338679....

Would we also need to do the same for partial serializations (Declaration::GetValue)?  And do you think that would have negative performance consequences when there are no mutation listeners?  That is what nixed sayrer's original patch for 338679, after all.

Comment 62

[Boris Zbarsky [:bzbarsky]]

> Would we also need to do the same for partial serializations

Imo, no.

> And do you think that would have negative performance consequences when there
> are no mutation listeners?

Hmm....  I don't think so, no (since I suspect we almost never call ToString in that situation).  Would need checking, though.  Probably a separate patch in bug 338679.

Comment 63

[Zack Weinberg (:zwol)]

>> I would bet the test would fail if we then went ahead and changed style again
>> without waiting for an intervening style reresolve, because that would mutate
>> the declaration, right?
>
> I'll poke at that a little and see what happens.

Sho'nuff, and making ToString call SetImmutable fixed it.  Patch posted in bug 336789 - applies on top of the stack in here.

Comment 64

[Zack Weinberg (:zwol)]

argh.  That's the fourth time I've mistyped "bug 338679" today.

Comment 65

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

Comment on attachment 459660 [details] [diff] [review]
part 9: remove pointless nsresult return values

>+  // This interface can return null regardless of the value of
>+  // aAllocate; however, a null return should only be considered a
>+  // failure if aAllocate is true.
>+  virtual mozilla::css::Declaration* GetCSSDeclaration(PRBool aAllocate) = 0;

s/This interface/This method/ (or if you want, interface method).

r=dbaron with that

Comment 66

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

Comment on attachment 459661 [details] [diff] [review]
part 10: refactor parsing of !important and the end of a property declaration.

r=dbaron, though I noticed bug 581579 while reviewing it.

Please don't try to fix bug 581579 until after this lands.

Comment 67

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

Comment on attachment 459662 [details] [diff] [review]
part 11: take the fast path in ParseProperty for !important replacing !important, as well as normal replacing normal.

r=dbaron

Comment 68

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

Comment on attachment 459664 [details] [diff] [review]
part 13: move nsCSSParser methods that manipulate data blocks into the appropriate nsCSS*DataBlock classes.


Please rename CopyValue to MoveValue while you're moving it.

>-private:
>+
>+    // Used to do a fast copy of a property value from source location to
>+    // destination location.  It's the caller's responsibility to make
>+    // sure that the source and destination locations point to the
>+    // right kind of objects for the property id.  This can only be
>+    // used for non-shorthand properties.
>+    static void CopyValue(void *aSource, void *aDest, nsCSSProperty aPropID,
>+                         PRBool* aChanged);
>+
>+ private:

Also:

 * don't reindent "private:"
 * fix the comment to say that it's doing a move rather than a copy,
   and that it runs correct destructors on aDest but replaces source with
   "unset" values


In nsCSSExpandedDataBlock::TransferFromBlock, aBlock would probably be
better named aFromBlock.

>+    void TransferFromBlock(nsCSSExpandedDataBlock& aBlock,
>+                          nsCSSProperty aPropID,
>+                          PRBool aIsImportant,
>+                          PRBool aOverrideImportant,
>+                          PRBool aMustCallValueAppended,
>+                          mozilla::css::Declaration* aDeclaration,
>+                          PRBool* aChanged);

One more space is needed for the handing indent.

r=dbaron with that

Comment 69

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

Comment on attachment 459698 [details] [diff] [review]
part 18: kill css::Declaration::InitializeEmpty - declarations are empty when created

Why do we want this?  InitializeEmpty is an edge case for object model manipulation; I'd rather the normal case (parsed CSS) not have extra null checks.

Comment 70

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

Comment on attachment 459667 [details] [diff] [review]
part 14: replace DeclarationChanged() with PutCSSDeclaration(), which takes the new declaration as an argument.

Use SetCSSDeclaration instead of PutCSSDeclaration, unless there's
a good reason for departing from standard terminology.

I think I prefer leaving the method on the rule called
DeclarationChanged than renaming it to PutDeclaration.

r=dbaron with that

Comment 71

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

Comment on attachment 459668 [details] [diff] [review]
part 15: Always copy the entire css::Declaration before changing it in any way.

Could you put the logic that's repeated in EnsureMutable and
AssertMutable into an IsMutable method (inline, const)?

Where you're removing the EnsureMutable call in ParseProperty,
it seems like it might be good to call AssertMutable (which could
easily be public).

r=dbaron with that

Comment 72

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

Comment on attachment 459672 [details] [diff] [review]
part 17: remove refcounting of declarations and data blocks.

patch 16:

Declaration::MapRuleInfoInto should be two methods:
MapNormalRuleInfoInto, and MapImportantRuleInfoInto.  The latter should
assert that mImportantData is non-null, so null-checks should not be
needed.

patch 16 and 17:

Why do we still have mFrozenDeclaration after patch 17?  Isn't
mDeclaration->mImmutable sufficient?

Could you leave InitializeEmpty and EnsureMutable where they are?
It just makes things harder to review and harder to |hg annotate|.

And why does patch 17 have code to handle the case where 
mFrozenDeclaration != mDeclaration.  That shouldn't be possible.  Is it?

It's fine if you merge these two patches together -- I'd rather not
review the mFrozenDeclaration management code.

Comment 73

[Zack Weinberg (:zwol)]

Attachment: part 9: remove pointless nsresult return values

(In reply to comment #65)
> Comment on attachment 459660 [details] [diff] [review]
> part 9: remove pointless nsresult return values
> 
> >+  // This interface can return null regardless of the value of
> >+  // aAllocate; however, a null return should only be considered a
> >+  // failure if aAllocate is true.
> >+  virtual mozilla::css::Declaration* GetCSSDeclaration(PRBool aAllocate) = 0;
> 
> s/This interface/This method/ (or if you want, interface method).
> 
> r=dbaron with that

done.

Comment 74

[Zack Weinberg (:zwol)]

Attachment: part 13: move nsCSSParser methods that manipulate data blocks into the appropriate nsCSS*DataBlock classes.

(In reply to comment #68)
> Comment on attachment 459664 [details] [diff] [review]
> part 13: move nsCSSParser methods that manipulate data blocks into the
> appropriate nsCSS*DataBlock classes.
> 
> Please rename CopyValue to MoveValue while you're moving it.

Done.

> >-private:
> >+
> >+    // Used to do a fast copy of a property value from source location to
> >+    // destination location.  It's the caller's responsibility to make
> >+    // sure that the source and destination locations point to the
> >+    // right kind of objects for the property id.  This can only be
> >+    // used for non-shorthand properties.
> >+    static void CopyValue(void *aSource, void *aDest, nsCSSProperty aPropID,
> >+                         PRBool* aChanged);
> >+
> >+ private:
> 
> Also:
> 
>  * don't reindent "private:"
>  * fix the comment to say that it's doing a move rather than a copy,
>    and that it runs correct destructors on aDest but replaces source with
>    "unset" values

Done.

> In nsCSSExpandedDataBlock::TransferFromBlock, aBlock would probably be
> better named aFromBlock.

Done.

> >+    void TransferFromBlock(nsCSSExpandedDataBlock& aBlock,
> >+                          nsCSSProperty aPropID,
> >+                          PRBool aIsImportant,
> >+                          PRBool aOverrideImportant,
> >+                          PRBool aMustCallValueAppended,
> >+                          mozilla::css::Declaration* aDeclaration,
> >+                          PRBool* aChanged);
> 
> One more space is needed for the handing indent.

Fixed (this was actually a case of hard tabs creeping into the file).

Comment 75

[Zack Weinberg (:zwol)]

(In reply to comment #69)
> Comment on attachment 459698 [details] [diff] [review]
> part 18: kill css::Declaration::InitializeEmpty - declarations are empty when
> created
> 
> Why do we want this?  InitializeEmpty is an edge case for object model
> manipulation; I'd rather the normal case (parsed CSS) not have extra null
> checks.

We want something like this because most existing uses of InitializeEmpty immediately follow up with a call to the parser, so we're allocating memory only to throw it away immediately.  But that might be better solved with a parser API change anyway.  I will drop it for now and revisit in bug 524223.

Comment 76

[Zack Weinberg (:zwol)]

Attachment: part 14: replace DeclarationChanged() with PutCSSDeclaration(), which takes the new declaration as an argument.

(In reply to comment #70)
> Comment on attachment 459667 [details] [diff] [review]
> part 14: replace DeclarationChanged() with PutCSSDeclaration(), which takes the
> new declaration as an argument.
> 
> Use SetCSSDeclaration instead of PutCSSDeclaration, unless there's
> a good reason for departing from standard terminology.
>
> I think I prefer leaving the method on the rule called
> DeclarationChanged than renaming it to PutDeclaration.

Made both these changes.  I preferred a present-tense verb for the method on the rule, but I don't care that much.

Comment 77

[Zack Weinberg (:zwol)]

Attachment: part 15: Always copy the entire css::Declaration before changing it in any way.

(In reply to comment #71)
> Comment on attachment 459668 [details] [diff] [review]
> part 15: Always copy the entire css::Declaration before changing it in any way.
> 
> Could you put the logic that's repeated in EnsureMutable and
> AssertMutable into an IsMutable method (inline, const)?
> 
> Where you're removing the EnsureMutable call in ParseProperty,
> it seems like it might be good to call AssertMutable (which could
> easily be public).
> 
> r=dbaron with that

Done.

Comment 78

[Zack Weinberg (:zwol)]

Attachment: part 16: remove refcounting of data blocks and declarations, style rules don't refer directly to data blocks

(In reply to comment #72)
> patch 16:
> 
> Declaration::MapRuleInfoInto should be two methods:
> MapNormalRuleInfoInto, and MapImportantRuleInfoInto.  The latter should
> assert that mImportantData is non-null, so null-checks should not be
> needed.

Done.

> patch 16 and 17:
> 
> Could you leave InitializeEmpty and EnsureMutable where they are?
> It just makes things harder to review and harder to |hg annotate|.

Sure.  Although I was thinking of (not in this bug) totally rearranging the methods in Declaration.h/Declaration.cpp into a more logical order.  Would you rather I didn't?

> Why do we still have mFrozenDeclaration after patch 17?  Isn't
> mDeclaration->mImmutable sufficient?
>
> And why does patch 17 have code to handle the case where 
> mFrozenDeclaration != mDeclaration.  That shouldn't be possible.  Is it?

You know, I think both of these were a vestige of using ref counts to indicate immutability.  I've taken mFrozenDeclaration completely out, and it passes smoke tests.  This new attachment is a merged patch 16/17.  I've pushed my whole queue to try again, along with the follow-ups for bug 338679 and bug 581579.

Comment 79

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

Comment on attachment 462227 [details] [diff] [review]
part 16: remove refcounting of data blocks and declarations, style rules don't refer directly to data blocks

> Declaration::Declaration(const Declaration& aCopy)
>   : mOrder(aCopy.mOrder),
>-    mData(aCopy.mData ? aCopy.mData->Clone()
>-                      : already_AddRefed<nsCSSCompressedDataBlock>(nsnull)),
>+    mData(aCopy.mData ? aCopy.mData->Clone() : nsnull),
>     mImportantData(aCopy.mImportantData
>-                      ? aCopy.mImportantData->Clone()
>-                      : already_AddRefed<nsCSSCompressedDataBlock>(nsnull))
>+                   ? aCopy.mImportantData->Clone() : nsnull),

I think this little cleanup will just result in a bug being filed
that we no longer compile on some odd compiler (on Solaris).  Skip it.

>+  // never null, except while expanded
>+  nsAutoPtr<nsCSSCompressedDataBlock> mData;

... or before the declaration is initialized for use by InitializeEmpty
or CompressFrom.

>   void ExpandTo(nsCSSExpandedDataBlock *aExpandedData) {
>     AssertMutable();
>     aExpandedData->AssertInitialState();
> 
>     NS_ASSERTION(mData, "oops");
>-    aExpandedData->Expand(&mData, &mImportantData);
>+    aExpandedData->Expand(mData.forget(), mImportantData.forget());
>     NS_ASSERTION(!mData && !mImportantData,
>                  "Expand didn't null things out");
>   }

The NS_ASSERTION text here doesn't particularly make sense anymore.  Either
remove the assertion or fix the text.

Seems like you could leave the null check out of
nsCSSExpandedDataBlock::DoExpand and put the aImportantBlock null-check
back in Expand.


You need to remove mRefCnt from nsCSSCompressedDataBlock.  I think it's
unused with this patch, though.

>-     * The compressed block passed in IS RELEASED by this method and
>-     * set to null, and thus cannot be used again.  (This is necessary
>-     * because ownership of sub-objects is transferred to the expanded
>-     * block in many cases.)
>+     * This method DELETES both of the compressed data blocks it is
>+     * passed.

Could you put the parenthetical back?

>+    void Expand(nsCSSCompressedDataBlock *aNormalBlock,
>+               nsCSSCompressedDataBlock *aImportantBlock);

bad indent

>+  // Not an owning reference; the CSSStyleRuleImpl that owns this
>+  // CSSImportantRule also owns the mDeclaration.
>+  css::Declaration* mDeclaration;

Need to add:  and any rule node pointing to this rule keeps that
CSSStyleRuleImple alive as well.

>+  // We are probably replacing the old declaration with |aDeclaration|
>+  // instead of taking ownership of the old declaration; only null out
>+  // aCopy.mDeclaration if we are taking ownership.
>+  if (mDeclaration == aCopy.mDeclaration) {

I don't see how this condition could ever be false, given the code
above.  (But I also agree that it needs to be false when
aCopy.mDeclaration is not mutable.)

Could you explain?

CSSStyleRuleImpl::RuleMatched should also assert at the end of the
function that mDeclaration->HasImportantData() == !!mImportantRule.
(This is important since if we ever decide to mark rules immutable
somewhere else it could break your code.)

Could you rename nsCSSDeclaration::MapRuleInfoInto into
MapNormalRuleInfoInto (and fix the caller in
 CSSStyleRuleImpl::MapRuleInfoInto)?



Pending granting review until I see the response to the question about
the CSSStyleRuleImpl copy constructor for DeclarationChanged.

Comment 80

[Zack Weinberg (:zwol)]

Attachment: part 16: remove refcounting of data blocks and declarations, style rules don't refer directly to data blocks

(In reply to comment #79)
> Comment on attachment 462227 [details] [diff] [review]
> >-    mData(aCopy.mData ? aCopy.mData->Clone()
> >-                      : already_AddRefed<nsCSSCompressedDataBlock>(nsnull)),
> >+    mData(aCopy.mData ? aCopy.mData->Clone() : nsnull),
...
> I think this little cleanup will just result in a bug being filed
> that we no longer compile on some odd compiler (on Solaris).  Skip it.

They're not refcounted anymore.  ->Clone() returns a bare pointer.
 
> >+  // never null, except while expanded
> >+  nsAutoPtr<nsCSSCompressedDataBlock> mData;
> 
> ... or before the declaration is initialized for use by InitializeEmpty
> or CompressFrom.

Fixed.

 
> >   void ExpandTo(nsCSSExpandedDataBlock *aExpandedData) {
> >     AssertMutable();
> >     aExpandedData->AssertInitialState();
> > 
> >     NS_ASSERTION(mData, "oops");
> >-    aExpandedData->Expand(&mData, &mImportantData);
> >+    aExpandedData->Expand(mData.forget(), mImportantData.forget());
> >     NS_ASSERTION(!mData && !mImportantData,
> >                  "Expand didn't null things out");
> >   }
> 
> The NS_ASSERTION text here doesn't particularly make sense anymore.  Either
> remove the assertion or fix the text.

I dropped the assertion - if forget() doesn't do its job we have bigger problems.


> Seems like you could leave the null check out of
> nsCSSExpandedDataBlock::DoExpand and put the aImportantBlock null-check
> back in Expand.

Done.

> You need to remove mRefCnt from nsCSSCompressedDataBlock.  I think it's
> unused with this patch, though.

Done.  I think I noticed that several patches later and didn't pop back out before fixing it.  *shrug*

> 
> >-     * The compressed block passed in IS RELEASED by this method and
> >-     * set to null, and thus cannot be used again.  (This is necessary
> >-     * because ownership of sub-objects is transferred to the expanded
> >-     * block in many cases.)
> >+     * This method DELETES both of the compressed data blocks it is
> >+     * passed.
> 
> Could you put the parenthetical back?

Sure, except I took out "in many cases" since it really is a wholesale deal.

> >+    void Expand(nsCSSCompressedDataBlock *aNormalBlock,
> >+               nsCSSCompressedDataBlock *aImportantBlock);
> 
> bad indent

Gah, I keep getting hard tabs in this file.  It seemed pointless to add an emacs modeline to a file I intended ultimately to delete, but I think I'll do it anyway.

> >+  // Not an owning reference; the CSSStyleRuleImpl that owns this
> >+  // CSSImportantRule also owns the mDeclaration.
> >+  css::Declaration* mDeclaration;
> 
> Need to add:  and any rule node pointing to this rule keeps that
> CSSStyleRuleImple alive as well.

Ok.  (I copied this text from elsewhere, but whatever.)

> >+  // We are probably replacing the old declaration with |aDeclaration|
> >+  // instead of taking ownership of the old declaration; only null out
> >+  // aCopy.mDeclaration if we are taking ownership.
> >+  if (mDeclaration == aCopy.mDeclaration) {
> 
> I don't see how this condition could ever be false, given the code
> above.  (But I also agree that it needs to be false when
> aCopy.mDeclaration is not mutable.)
> 
> Could you explain?

There is sneakiness hiding in the initializer list.  mSelector and mDOMRule are initialized from the corresponding members in aCopy, but mDeclaration is initialized from the aDeclaration argument.  That comes from the aDecl argument to DeclarationChanged, which will be the same as aCopy.mDecl if and only if EnsureMutable() didn't make a copy.  That invariant is not as statically enforced as I'd like it to be, but I'm running out of clever.  This is also why we have the ugly "if (decl != olddecl) delete decl;" in some failure cases in nsDOMCSSDeclaration.cpp.

I'm pretty sure this is correct as is.

> CSSStyleRuleImpl::RuleMatched should also assert at the end of the
> function that mDeclaration->HasImportantData() == !!mImportantRule.
> (This is important since if we ever decide to mark rules immutable
> somewhere else it could break your code.)

Heh, in fact I had to change this in the very next patch (finishing up the fix for bug 338679) because that needed to mark declarations immutable in another place.  Could you please look at the code now posted in that bug and tell me if you still want a change here, given that I plan to land this entire series + that patch + the patch for bug 581579 all at once?

> Could you rename nsCSSDeclaration::MapRuleInfoInto into
> MapNormalRuleInfoInto (and fix the caller in
>  CSSStyleRuleImpl::MapRuleInfoInto)?

Done.

> Pending granting review until I see the response to the question about
> the CSSStyleRuleImpl copy constructor for DeclarationChanged.

Comment 81

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

Comment on attachment 462639 [details] [diff] [review]
part 16: remove refcounting of data blocks and declarations, style rules don't refer directly to data blocks

OK, r=dbaron based on your comments.

I haven't looked at the revised patch, though, largely because you changed your diff settings (for the worse) since the previous patch, and therefore diffing the patches produces a ridiculous amount of noise due to the diff showing the repeated reduction in context.

Comment 82

[Zack Weinberg (:zwol)]

Attachment: interdiff of part 16 from previous revision

(In reply to comment #81)
> I haven't looked at the revised patch, though, largely because you changed your
> diff settings (for the worse) since the previous patch, and therefore diffing
> the patches produces a ridiculous amount of noise due to the diff showing the
> repeated reduction in context.

Sorry about that, I must not have hg set up correctly on my laptop.  Here's an interdiff.

Comment 83

[Zack Weinberg (:zwol)]

Attachment: roll-up patch for approval

All the pieces of this are reviewed, so here's a roll-up patch for approval.  I will land the pieces as individual commits, plus the patches for bug 338679 (not yet approved) and bug 581579 (already approved), in one push.

Comment 84

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/9c9fba3c49a6 et seq.