Closed
Bug 386640
Opened 18 years ago
Closed 17 years ago
ClearStyleDataAndReflow is fundamentally broken
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
People
(Reporter: dbaron, Assigned: sharparrow1)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
ClearStyleDataAndReflow and the ClearStyleData methods on nsStyleContext and nsRuleNode are fundamentally broken. They have the following problems:
* they can cause style changes without calling DidSetStyleContext (see bug 382422)
* they can cause the PeekStyleData optimizations in CalcStyleDifference to optimize away necessary notifications because:
+ the following reflow and potential repaint doesn't get structs that may have been gotten only
+ people make dynamic changes between the ClearStyleData and the reflow/repaint that is now asynchronous
We should instead do what I describe in bug 382422 comment 4.
Assignee | ||
Comment 1•18 years ago
|
||
I need to add comments and stuff, but is there anything wrong with this approach?
Assignee: nobody → sharparrow1
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•18 years ago
|
||
Seems like the right idea. You might need to do something with the result of ComputeStyleChangeFor. And you should probably add a comment that we could optimize with a faster version of ComputeStyleChangeFor that doesn't rerun rule matching. (The existing ReParentStyleContext codepath might even work, if we trust it.)
Assignee | ||
Comment 3•18 years ago
|
||
Okay, here's the completed version; probably better to get another set of eyes on it.
Attachment #270650 -
Attachment is obsolete: true
Attachment #270679 -
Flags: review?(bzbarsky)
Comment 4•18 years ago
|
||
Comment on attachment 270679 [details] [diff] [review]
Patch v1
>Index: base/nsPresContext.cpp
>Index: base/nsPresShell.cpp
>- if (aForceReflow){
>- mPresContext->ClearStyleDataAndReflow();
>- }
So the aForceReflow arg can go away, right? Do that or file a followup?
>Index: style/nsStyleSet.cpp
>+ // XXX What should I do for OOM?
Make this method a no-op on OOM. That is, first allocate the new ruletree and rulewalker, then if that succeeded go ahead and tear down the old stuff. In the failure case we'll render wrong, but not crash, right?
>+nsStyleSet::EndReconstruct() {
>+ NS_ASSERTION(mOldRuleTree, "Unmatched begin/end?");
And then here mOldRuleTree might be null if BeginReconstruct() failed. But we could document that if BeginReconstruct() returns an error the caller must not call EndReconstruct().
>+ for (PRInt32 i = mRoots.Length() - 1; i >= 0; --i)
This doesn't cause warnings from assigning a PRUint32 into a PRInt32?
If it does, you could really iterate this forwards, I think... I assume it was done backwards to avoid calling Length() more than once, and you could store the length in a local. Check, though?
>Index: style/nsStyleSet.h
>+#include "nsCOMArray.h"
I assume this change is not really for this bug?
>Index: style/nsStyleStruct.h
And same for the changes here? I'm fine with this landing, but perhaps a separate checkin with a comment like "removing unused code, rs=bzbarsky, no bug" or something?
r=bzbarsky on the patch itself, modulo above nits.
Attachment #270679 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 5•18 years ago
|
||
(In reply to comment #4)
> (From update of attachment 270679 [details] [diff] [review])
> >Index: base/nsPresContext.cpp
> >Index: base/nsPresShell.cpp
> >- if (aForceReflow){
> >- mPresContext->ClearStyleDataAndReflow();
> >- }
>
> So the aForceReflow arg can go away, right? Do that or file a followup?
Followup filed.
> >Index: style/nsStyleSet.cpp
> >+ // XXX What should I do for OOM?
>
> Make this method a no-op on OOM. That is, first allocate the new ruletree and
> rulewalker, then if that succeeded go ahead and tear down the old stuff. In
> the failure case we'll render wrong, but not crash, right?
Right; done.
> >+nsStyleSet::EndReconstruct() {
> >+ NS_ASSERTION(mOldRuleTree, "Unmatched begin/end?");
>
> And then here mOldRuleTree might be null if BeginReconstruct() failed. But we
> could document that if BeginReconstruct() returns an error the caller must not
> call EndReconstruct().
Done.
> >+ for (PRInt32 i = mRoots.Length() - 1; i >= 0; --i)
>
> This doesn't cause warnings from assigning a PRUint32 into a PRInt32?
No, at least on Windows.
> >Index: style/nsStyleSet.h
> >+#include "nsCOMArray.h"
>
> I assume this change is not really for this bug?
Not really, but it's correct in any case.
> >Index: style/nsStyleStruct.h
>
> And same for the changes here? I'm fine with this landing, but perhaps a
> separate checkin with a comment like "removing unused code, rs=bzbarsky, no
> bug" or something?
Okay, done.
No longer blocks: 388607
Assignee | ||
Comment 6•18 years ago
|
||
Attachment #272821 -
Flags: superreview?(roc)
Assignee | ||
Comment 7•18 years ago
|
||
Comment on attachment 272821 [details] [diff] [review]
Patch v2
Cancelling superreview; there's a couple of typos in this patch, and I found an issue in nsTreeStyleCache.
Attachment #272821 -
Flags: superreview?(roc)
Assignee | ||
Comment 8•18 years ago
|
||
This patch has a couple of additional changes to nsTreeBodyFrame to fix some issues there (primarily a crash from style contexts with invalid rule node pointers).
Attachment #272821 -
Attachment is obsolete: true
Attachment #272997 -
Flags: superreview?(roc)
Attachment #272997 -
Flags: review?(roc)
Reporter | ||
Comment 9•18 years ago
|
||
Do the optimizations in the tree style cache still work with those changes? I suspect they may not -- I remember writing a patch much like that once before. (I think there may have been issues with things like hovering over a tree causing a border on the tree forcing everything inside the tree to be restyled.)
Assignee | ||
Comment 10•18 years ago
|
||
No serious issues with the optimization; we trigger a DidSetStyleContext which appears unnecessary in response to user actions sometimes (maybe in response to focus changes?), but it never triggers during painting, which I think is the most important part of the optimization.
I'm not a style system peer and I probably shouldn't review this.
Assignee | ||
Updated•18 years ago
|
Attachment #272997 -
Flags: superreview?(roc)
Attachment #272997 -
Flags: superreview?(bzbarsky)
Attachment #272997 -
Flags: review?(roc)
Attachment #272997 -
Flags: review?(bzbarsky)
Comment 12•18 years ago
|
||
Comment on attachment 272997 [details] [diff] [review]
Patch v3
>Index: style/nsCSSValue.cpp
The changes to this file don't seem to be related to this patch.
>Index: style/nsStyleSet.cpp
>+ mOldRuleTree(nsnull)
This doesn't match the member order in the header; that will cause compile warnings. Put mOldRuleTree before mInShutdown here, please.
>Index: xul/base/src/tree/src/nsTreeBodyFrame.cpp
>+ // XXX The following is hacky, but it's not incorrect,
>+ // and appears to fix some bugs
Say which bugs please, so people who try to clean it up later know what to test, ok?
r+sr=bzbarsky with those nits.
Attachment #272997 -
Flags: superreview?(bzbarsky)
Attachment #272997 -
Flags: superreview+
Attachment #272997 -
Flags: review?(bzbarsky)
Attachment #272997 -
Flags: review+
Assignee | ||
Comment 13•17 years ago
|
||
Assignee | ||
Comment 14•17 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•