Closed
Bug 288574
Opened 19 years ago
Closed 19 years ago
[FIXr]Don't call ValueAppended for DOM changes that don't create the value
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.8beta2
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Depends on 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
22.86 KB,
patch
|
Details | Diff | Splinter Review | |
21.37 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
asa
:
approval1.8b2+
|
Details | Diff | Splinter Review |
This is spun off of bug 208727 comment 1. On the testcase at http://www.speich.net/computer/3d.php the patch I'll post to implement this cuts our time in TransferTempData by a factor of 5 (from about 1% of total time, to about 0.2%). Given that this also gives better behavior with editor in general (per bug 208727 comment 2), I think it's worth the bit of extra code.
Assignee | ||
Updated•19 years ago
|
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta2
Assignee | ||
Comment 1•19 years ago
|
||
This would be simpler if we could depend on the bit on mData, but since we can't...
Attachment #179246 -
Flags: superreview?(dbaron)
Attachment #179246 -
Flags: review?(dbaron)
Comment on attachment 179246 [details] [diff] [review] Like so >@@ -752,17 +758,18 @@ CSSParserImpl::ParseAndAppendDeclaration >+ if (!ParseDeclaration(errorCode, aDeclaration, PR_FALSE, >+ PR_FALSE, aChanged)) { I think this should be passing PR_TRUE. Otherwise duplicate declarations in CSSStyleDeclaration::cssText setter won't be serialized consistently with other duplicate declarations. That will mess up the case: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/style/nsDOMCSSDeclar ation.cpp&rev=3.60&mark=215-221#194 but that code should probably be fixed anyway. >+ if (aMustCallValueAppended || !PropertySetInData(aPropID)) { Why can't you just check the property bit in mData instead of writing this new PropertySetInData function?
Attachment #179246 -
Flags: superreview?(dbaron)
Attachment #179246 -
Flags: superreview-
Attachment #179246 -
Flags: review?(dbaron)
Attachment #179246 -
Flags: review-
Was it because of this? /* * mPropertiesSet stores a bit for every property that may be * present, to optimize compression of blocks with small numbers of * properties (the norm). The code does not rely on it to be exact; * it is allowable, although slower, if a bit is erroneously set * even though the property is not present. */ If so, perhaps you should make it exact. IIRC, it shouldn't be hard, if it requires any changes at all.
(See bug 208732.)
Assignee | ||
Comment 5•19 years ago
|
||
Yes, the extra code was because the bit is not exact. You're right that we should just fix bug 208732 first. I'll look into it.
Depends on: 208732
Assignee | ||
Comment 6•19 years ago
|
||
This makes the bit exact, except for mTempData in the parser where a null value with the bit set has a "special" meaning.... I've tested that this doesn't change our behavior for system fonts.
Attachment #179246 -
Attachment is obsolete: true
Assignee | ||
Comment 7•19 years ago
|
||
Attachment #180304 -
Flags: superreview?(dbaron)
Attachment #180304 -
Flags: review?(dbaron)
Assignee | ||
Comment 8•19 years ago
|
||
Comment on attachment 180304 [details] [diff] [review] Same as diff -w This actually gives even more improvement on DHTML, because we do less work for ComputeSize and Compress.
Attachment #180304 -
Flags: superreview?(dbaron)
Attachment #180304 -
Flags: superreview+
Attachment #180304 -
Flags: review?(dbaron)
Attachment #180304 -
Flags: review+
Assignee | ||
Comment 9•19 years ago
|
||
Comment on attachment 180304 [details] [diff] [review] Same as diff -w Requesting 1.8b approval. This helps out with DHTML a little bit, makes our serialization of inline style better, and is pretty safe....
Attachment #180304 -
Flags: approval1.8b2?
Assignee | ||
Updated•19 years ago
|
Summary: [FIX]Don't call ValueAppended for DOM changes that don't create the value → [FIXr]Don't call ValueAppended for DOM changes that don't create the value
Comment 10•19 years ago
|
||
Comment on attachment 180304 [details] [diff] [review] Same as diff -w a=asa
Attachment #180304 -
Flags: approval1.8b2? → approval1.8b2+
Assignee | ||
Comment 11•19 years ago
|
||
Fixed
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•