Closed
Bug 667887
Opened 14 years ago
Closed 14 years ago
Implement nsMappedAttributes::SizeOf()
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: mounir, Assigned: mounir)
References
Details
Attachments
(1 file, 2 obsolete files)
1.54 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #542444 -
Flags: review?(jst)
![]() |
||
Comment 1•14 years ago
|
||
This is wrong. Mapped attributes are shared across elements, so adding them to the size of mAttrsAndChildren is wrong. You want to add them to the size of the document itself, by going through the HTML stylesheet.
Updated•14 years ago
|
Attachment #542444 -
Flags: review?(jst) → review+
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 2•14 years ago
|
||
This patch tries to take into account Boris comment.
Attachment #542444 -
Attachment is obsolete: true
Attachment #542636 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review]
![]() |
||
Comment 3•14 years ago
|
||
Comment on attachment 542636 [details] [diff] [review]
Patch v2
So this sets the size to:
sizeof(nsMappedAttributes) + mAttrCount * (sizeof(void*) + sizeof(InternalAttr))
This looks wrong. The correct size of an nsMappedAttributes, from |nsMappedAttributes::operator new| is:
sizeof(nsMappedAttributes) - sizeof(void*) + mBufferSize*sizeof(InternalAttr)
In particular, nsMappedAttributes allocates all its storage for InternalAttr structs inline. Now your first problem is that mBufferSize is debug-only....
I wonder whether using mAttrCount in practice is ok because we never have long-lived nsMappedAttributes for mAttrCount != mBufferSize.
Attachment #542636 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 4•14 years ago
|
||
Attachment #542636 -
Attachment is obsolete: true
Attachment #543969 -
Flags: review?(bzbarsky)
![]() |
||
Comment 5•14 years ago
|
||
Comment on attachment 543969 [details] [diff] [review]
Patch v3
Can you add an assert that mAttrCount == mBufferSize in SizeOf? If that ever fails to be true, we want to know.
r=me with that.
Attachment #543969 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•14 years ago
|
Flags: in-testsuite-
Whiteboard: [needs review] → [inbound]
Assignee | ||
Updated•14 years ago
|
Flags: in-testsuite-
Comment 6•14 years ago
|
||
this has been backed out by ehsan due to bustage with all the other changesets in the same push
Whiteboard: [inbound]
Comment 7•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in
before you can comment on or make changes to this bug.
Description
•