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.

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

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

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.

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.

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.

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

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.

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.

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

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.

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.

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.

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

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.

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.

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.

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

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

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?

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

It's still inconsistent with CSS, though.

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

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

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

(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 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 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 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 on attachment 448857 [details] [diff] [review] 10/17: have nsXULElement::GetStyle call nsStyledElement::GetStyle Yeah, I guess the reparse thing is ok. r=bzbarsky

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

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

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

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.

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

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

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

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

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.

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.

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

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

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.

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.

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

What it says.

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.

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

Tiny revision to remove a now-incorrect assertion.

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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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.

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