I'll attach a patch tomorrow if my floppy drive cooperates and I find my floppies. The basic idea of the patch is as follows: 1) Add AttributeWillChange to nsIDocumentObserver 2) Cache the old style data in nsCSSFrameConstructor::AttributeWillChange 3) Compare the old to the new in nsCSSFrameConstructor::AttributeChanged The current setup passes the cached data out as an nsISupports, then gets it passed back in. I talked to jst, and he does not like that very much.. I'll be attaching the patch with that setup as-is (because it pretty much works) and I'll also attach the conversation he and I had. I'll work on posting some testcases for testing as well.

This is pretty much complete, imo, pending resolution of jst's concern. There are some testcases to test with (and more are welcome) at: http://web.mit.edu/bzbarsky/www/testcases/dynamic-inline-style-correctness/ http://web.mit.edu/bzbarsky/www/testcases/dynamic-inline-style-performance/

Comment on attachment 101301 [details] [diff] [review] Work-in-progress Good news -- this seems to fix a few random bugs. Bad news -- this regresses bug 138120. The testcase in that bug triggers the NS_ASSERTION(!parentFrame, "parent frame but no child frame or undisplayed entry"); assertion.... Which seems bad.

Comment on attachment 102414 [details] [diff] [review] This fixes the assertion and the regression (apply in addition to, not instead of) Why bother with asserts if they get ignored? I've moved this patch to its proper place -- bug 151883.

Comment on attachment 101301 [details] [diff] [review] Work-in-progress As suspected, this regresses bug 118415. I'm still not sure whether the patch for that _really_ fixed things in the old world or just papered over the problem... in any case, I need to come up with a fix in the context of the new code.

so i think i like jsts suggestion with a servergenerated key better since it less work in the common case (that we're not passing data from WillChange to HasChanged). One way to take care of the memorymanagement is to have the observer hold a hash of nsISupports, that way you should never leak anything. You still might hold on to an object much longer then you need though, if someone aborts, which admittedly is a bad thing.

hmm.. though we could add an AttributeDidntChange notification as well, that is called if someone aborts.

Hoping to get that regression addressed sometime this week. We need to come to a decision here on the exact way to pass the context around....

This resolves all known regressions. Remaining issues: 1) How should the notifications work? 2) What datatype should be used for storing the style data (nsCachedStyleData feels like a hack when all is said and done)... (I'd do a stub impl of nsIStyleContext, but we want to remove nsIStyleContext....) 3) How to avoid code duplication in the CalcDifference functions? 4) Disentangling from some other patches. Point #4 means there's stuff in this patch that's pretty irrelevant to this bug... I'm sorry about that; I'll try to disentangle as soon as I can... in the meantime, some general feedback on the approach and such would be much appreciated.

Yeah, there's got to be a way to eliminate the duplication of this CalcStyleDifference function. Maybe we could make them both small by using a loop to iterate through the different style structs, instead of writing them all out explicitly?

Yeah... Another possibility is that we have an nsIStyleContextBase and nsIStyleContext, with the latter inheriting from the former.... nsIStyleContextBase could be what CalcDifference() takes and instead of using nsCachedStyleData to move this info around I could use an nsIStyleContextBase (which would have the minimal number of methods needed to actually function in CalcDifference). Of course this represents even _more_ virtualization of nsIStyleContext, which may not be a good idea.... :(

Can't you just make a 'fake' nsStyleContext with your nsCachedStyleData in it, but decoupled from rules and other style contexts?

I considered doing that too... I'll give it a shot tonight or tomorrow and see how it affects perf.

Roc, that works beautifully. ;) This patch has all the irrelevant stuff stripped out and is basically reviewable.... (modulo the specific changes to nsIDocumentObserver) In case people are interested in the perf impact, here're the results on http://web.mit.edu/bzbarsky/www/testcases/dynamic-inline-style-performance/ (3 or 5 trials for each test): WITHOUT PATCH: inlineStyleOnBody: 1224 1205 1224 1208 1225 inlineStyleOnBodyWithSelectorOnStyleAttr: 1440 1378 1379 1441 1379 uselessInlineStyleOnBody: 1601 1606 1626 1627 1626 movingEmptyDivs: 12963 12879 12969 movingFullDivs: 57013 56354 56344 notMovingFullDivs: 46770 46268 46241 WITH PATCH: inlineStyleOnBody: 879 892 875 893 881 inlineStyleOnBodyWithSelectorOnStyleAttr: 1573 1533 1530 1535 1535 uselessInlineStyleOnBody: 431 415 436 419 436 movingEmptyDivs: 14035 13958 13923 movingFullDivs: 51825 51367 51483 notMovingFullDivs: 8693 8344 8217 And testing with http://alladyn.art.pl/gandalf/MozillaTests/dynl.html : WITHOUT PATCH: Creating 200 layers - time - 1174 1153 1195 Moving 200 layers 100 px to left - time - 8326 8627 8446 Cliping 200 layers - time - 310 310 311 Setting Opacity 10% - time - 8938* 8847* 8816* Reverting z-index for 200 layers - time - 307 319 337 Moving with changing opacity and clip - time - 21116* 21031* 21022* Inputing text into 200 layers - time - 512 509 509 Resizing 200 layers - time - 562 587 565 WITH PATCH: Creating 200 layers - time - 1142 1143 1137 Moving 200 layers 100 px to left - time - 8635 8583 8652 Cliping 200 layers - time - 321 318 320 Setting Opacity 10% - time - 24867** 22199** 22133** Reverting z-index for 200 layers - time - 325 297 292 Moving with changing opacity and clip - time - 42236** 40992** 41705** Inputing text into 200 layers - time - 525 517 495 Resizing 200 layers - time - 575 565 567 *No repaints or reflows happen between opacity=100% and opacity=10% due to continuous reframing **Repaints and reflows happen as opacity/position/clip change

I considered subclassing nsStyleContext for the snapshot (to override PeekStyleData not to look in the rulenode) , but wasn't sure whether making PeekStyleData virtual was ok.... if someone has strong feelings one way or the other, let me know.

I'm not getting to this till January, so this is not making 1.3.

> The current setup passes the cached data out as an nsISupports, then gets it > passed back in. I talked to jst, and he does not like that very much.. [...] +/* Strategy for dealing with inline style: + * + * 1) During AttributeWillChange, see if we already have an inline style + * rule. If we do, get the style context and cache the style data and the + * pointer (weak ref; we just care about the value) to the style rule. + * Pass this data out as aContext. + * 2) During AttributeChanged, see if we have a style rule and if so whether + * it's the same as the rule we remembered in AttributeWillChange. If the + * rules are the same, the style context will not be changing! + * 3) If the inline style rules are different, just do a normal re-resolve + * and get the hint from that. This will create new style contexts as + * needed. + */ NS_IMETHODIMP +nsCSSFrameConstructor::AttributeWillChange(nsIPresContext* aPresContext, + nsIContent* aContent, + PRInt32 aNameSpaceID, + nsIAtom* aAttribute, + nsISupports** aContext) +{ [...] + *aContext = + NS_STATIC_CAST(nsISupports*, + new StyleAttrChangeContext(snapshot, rule, importantRule, + aPresContext)); [...] +} Possible suggestion/simplication to avoid this issue. What about using the frame manager instead of dragging the context around? That is, rather than returning the context as you are doing (just to get it back later on), just stash it as a property in the frame manager. Then, when AttributeChanged() is called later on, you can fetch it back from there. + //XXXbz We need to decide what the contract is here. Is it OK to call + //AttributeWillChange but not AttributeChanged (eg if an error occurs while + //changing the attribute)? If so, we should say so explicitly, I think. It will also avoid this issue. In the case of success (i.e., perfect match of "willchange" and "changed" pairs), the property is removed. In case of failure ("willchange" without "changed"), there will be at most a dangling entry but it will overriden if "willchange" is called again, or it will be destroyed when the manager goes away, leaving no leak behind.

What about the case when style is being changed on a display:none element (which has a style context in the undisplayed map but has no frame)? I'm not sure I can use the frame manager there (though I _could_ maybe use the undisplayed map itself....)

> I _could_ maybe use the undisplayed map itself Seems plausible since the map refers to the old style context data -- until completion of the maintainance [set->ClearStyleData()] that AttributeChanged() does. This actually raises one question with your |GetStyleContextFor|... Isn't there a loophole in your patch in that you are getting the same old context in the situation of display=none? i.e., are you not testing the same old context against itself?

That's the general problem with the inline style changes -- the fact that the style context pointer does _not_ change in the process. That's why I save the old style data, then clear the style context's cache, then compare, forcing the style context to recompute style data in the process... There is an issue I'm looking into with this whole approach (and this may apply to the current code too) where style data may get cleared and this may cause later changes to not trigger the right change hint...

I'm a _lot_ happier with this one... It still fixes all the problems the old one fixed, but mostly _removes_ code. Perf testing shows that it's within 1-2% of current trunk on all the DHTML tests I've run (except the ones I rigged to show off the benefits of this patch -- we're 10-60% faster than trunk on those ;) ). This patch depends on bug 117316 being fixed so we don't grow the ruletree like mad. Once that happens, the perf here should be retested, but I suspect that it's not going to change much.

Comment on attachment 113089 [details] [diff] [review] Alternate (cleaner) approach r=dbaron (although maybe we shouldn't be using the literal 0x7fffffff as the weight without a macro to hide it)

Comment on attachment 113089 [details] [diff] [review] Alternate (cleaner) approach isn't there a PR_INT32_MAX macro for that?

Yeah, good point. I'll change that to PR_INT32_MAX

Comment on attachment 113089 [details] [diff] [review] Alternate (cleaner) approach yeah baby! I hope that we can hunt down and kill the other users of the hint from the parser...

*** Bug 194928 has been marked as a duplicate of this bug. ***

Request changing SEVERITY to CRITICAL because this blocks 195073 which is SEVERITY = CRITICAL. Also, comment 22 states this patch depends on 117316.

Let me know when you're ready to land.

Pretty much any time (as in the patch is ready to go). The problem is finding a time to watch the tree... Can you do Tuesday morning? Should we ask for a carpool?

I could watch the tree for you if needed (find me on IRC to make sure I'm around), and I don't think there's a need for a carpool.

This should apply to a current trunk.....

I already checked it in (based on the merging I did a week or so ago), actually.

Ah, cool. Thank you!

Fix checked in to trunk, 2003-03-06 11:06/07 PST with additional unused variable removal at 13:01 PST.

+ if (!decl) { + return result; + } + nsCOMPtr<nsICSSLoader> cssLoader; + nsCOMPtr<nsICSSParser> cssParser; + nsCOMPtr<nsIURI> baseURI; + + result = GetCSSParsingEnvironment(mContent, + getter_AddRefs(baseURI), + getter_AddRefs(cssLoader), + getter_AddRefs(cssParser)); + if (NS_FAILED(result)) { + return result; + } Possible leak of |decl|. (2 places). Also missing the check for success in the call of setDecl() and the ruleAbort business in case of failure.

> Possible leak of |decl|. (2 places). Actually, no. |decl| is not addrefed by GetCSSDeclaration. > and the ruleAbort business in case of failure Nom the only case in which RuleAbort() is needed is if the decl was never set in a rule (and the decl we get out of GetCSSDeclaration is guaranteed to be set in a rule). RuleAbort() is a fancy name for "delete" in the case of CSSDeclarations... and a decl owned by a rule will be released by the rule when the rule dies. Now we _could_ end up with a double-delete of the decl if SetHTMLAttribute fails in SetCSSDeclaration when called from GetCSSDeclaration... Bug 196271 filed. Also filed bug 196273 on general nsCSSDeclaration memory management.

Hmm... the not clearing of display data seems to have fixed some {ib} bugs (eg bug 175686). It'd be good to retest the other {ib} bugs.

*** Bug 156206 has been marked as a duplicate of this bug. ***

Note that this caused regression bug 196603

*** Bug 81678 has been marked as a duplicate of this bug. ***