remove |aForceUnique| from ResolveStyleContext methods

RESOLVED FIXED in mozilla1.0

Status

()

RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({perf})

Trunk
mozilla1.0
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

We should remove the |aForceUnique| parameter from all the ResolveStyleContext
methods (3 on nsIStyleSet, 3 on nsIPresContext), since the only caller that
passes PR_TRUE for this parameter is nsFrameManager::ReResolveStyleContext.

This could give a bit of a performance win doing style data computation, but I
the biggest performance would be that it could cause much less data to be blown
away for a ReResolve, since it could mean that we Re-Resolve to all the same
style contexts.  (We should be careful to allow this to work for the root as
well if it matches the same rule node, which is the only place where we'd want a
same-rule-node check, I think.)
Keywords: perf
Actually, this won't help all that much in clearing less data right now, since
we currently rebuild the rule tree for stylesheet add/remove, and we hopefully
don't call ReResolve too often when there's nothing to ReResolve...

Comment 2

17 years ago
We only rebuild the rule tree on a stylesheet removal.  A stylesheet 
addition does not require a rebuild of the rule tree.

Comment 3

17 years ago
Stylesheet addition also gets hit reasonably often in the real world, since 
<style> is often encountered in the <body> of a Web page.
Oh, right.  I used to think that was true and somehow I looked at the code and
convinced myself otherwise.  Now that I look again, I see it is true.
Also, I also just noticed that whether a style context was forced to be unique
is in its mBits, so we don't need to remove the feature to get the perf win --
ReResolveStyleContext could pass aForceUnique based on the old context's bits. 
Do you think there's any reason to keep this API?

Comment 6

17 years ago
Nope.

Comment 8

17 years ago
Comment on attachment 76496 [details] [diff] [review]
patch

sr=hyatt.
Attachment #76496 - Flags: superreview+

Updated

17 years ago
Attachment #76496 - Flags: review+

Comment 9

17 years ago
Comment on attachment 76496 [details] [diff] [review]
patch

NS_ADDREF(newContext = oldContext);

This looks ugly to me, I'd do the assignment seperately from the ADDREF.

+	       // XXXldb |oldContext| is null by this point, so this will
+	       // never do anything.

So, why not remove it (or put in an assertion or warning if you are unsure)?


r=attinasi

Comment 10

17 years ago
Comment on attachment 76496 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #76496 - Flags: approval+
Target Milestone: --- → mozilla1.0
Fix checked in 2002-03-31 08:58/59 PST.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.