Many of the ways we use the hints in nsCSSPropList.h are probably worse than just doing reresolution and using the appropriate change list. We should evaluate these callers (perhaps one at a time), and remove their uses if they are inappropriate. See bug 130620 comment 25 and bug 130620 comment 26 for what is probably one such case.

This removes hints from the CSS parser and replaces them with a boolean "something changed" out parameter that's computed in a much simpler way, and uses that boolean flag, as Boris suggested to me.

Comment on attachment 126604 [details] [diff] [review] remove hints from nsCSSParser Hmm.. With this patch, it looks like "changed" will be true if I do something like: foo.style.top = foo.style.top; Any way to detect when the value has actually changed? We'd probably need to do that check in TransferTempData or something.... (return "true" if at least one of the values we transferred was different from the value already in mData or at least one important bit was different....)

As a further note, once this patch goes in the only user of that long hint list will be nsCSSDeclaration::GetStyleImpact, which is only called from one place; I suspect that the caller doesn't nned to call it at all, but that should be a separate patch that gets some testing....

Make the optimization exact, per bzbarsky's comment.

Comment on attachment 126715 [details] [diff] [review] remove hints from nsCSSParser >Index: src/nsCSSParser.cpp > // After a successful parse of |aPropID|, transfer data from > // |mTempData| to |mData|. > void TransferTempData(nsCSSDeclaration* aDeclaration, >- nsCSSProperty aPropID, PRBool aIsImportant); >+ nsCSSProperty aPropID, PRBool aIsImportant, >+ PRBool* aChanged); Add a comment that says that this method will set aChanged to PR_TRUE if something changes but will NOT affect it otherwise (so calling it multiple times with the same pointer is a boolear OR of the individual calls). > CSSParserImpl::ParseAndAppendDeclaration(const nsAString& aBuffer, > if (aClearOldDecl) { > mData.AssertInitialState(); > aDeclaration->ClearData(); > } else { We want to set *aChanged to PR_TRUE when we clear the decl, I think.... (so that doing |foo.style.cssText = "";| will do the right thing). r+sr=bzbarsky with those two changes. If you get a chance, please take a minute and run through the tests at http://web.mit.edu/bzbarsky/www/testcases/dynamic-inline-style-correctness/ -- some of the "styleAttrRemoval" tests would fail if the second comment were not addressed....

attachment 126715 [details] [diff] [review] checked in to trunk, 2003-06-30 14:39 -0700. I was failing those tests, and the change you suggested fixed it.

This removes the one remaining user -- nsGenericHTMLElement::UnsetAttr was using it for style attribute removal. With this change we should just go through normal reresolution.

Comment on attachment 126885 [details] [diff] [review] remove one remaining user of hints Most excellent. roc will be very happy. ;)

Comment on attachment 126947 [details] [diff] [review] remove hints from property table This was pretty much done by search-replace. There isn't anything here you wouldn't expect, so no need to look too closely...

attachment 126685 [details] [diff] [review] checked in to trunk, 2003-07-02 14:05 -0700. attachment 126947 [details] [diff] [review] checked in to trunk, 2003-07-02 15:17 -0700. I still want to do further work related to hints (see bug 211308, esp. bug 211308 comment 7, and bug 211371).