Closed
Bug 667883
Opened 13 years ago
Closed 13 years ago
Implement nsAttrAndChildArray::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, 3 obsolete files)
4.68 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #542439 -
Flags: review?(jst)
Comment 1•13 years ago
|
||
The handling of mapped attrs here looks wrong because they're shared. Also, the attr slots are in mBuffer, so aren't you double-counting them?
Assignee | ||
Comment 2•13 years ago
|
||
Is that better?
Attachment #542439 -
Attachment is obsolete: true
Attachment #542501 -
Flags: review?(bzbarsky)
Attachment #542439 -
Flags: review?(jst)
Assignee | ||
Comment 3•13 years ago
|
||
Updated patch based on our IRC conversation.
Attachment #542501 -
Attachment is obsolete: true
Attachment #542503 -
Flags: review?(bzbarsky)
Attachment #542501 -
Flags: review?(bzbarsky)
Comment 4•13 years ago
|
||
Comment on attachment 542503 [details] [diff] [review] Patch v3 >+ size += sizeof(*mImpl); Take that out; that's what NS_IMPL_EXTRA_SIZE handles for you. r=me with that.
Attachment #542503 -
Flags: review?(bzbarsky) → review+
Comment 5•13 years ago
|
||
Comment on attachment 542503 [details] [diff] [review] Patch v3 +nsGenericElement::SizeOf() const +{ + PRInt64 size = Element::SizeOf(); + + size -= sizeof(mAttrsAndChildren); + size += mAttrsAndChildren.SizeOf(); + + return size; What here takes into account the size of the other members of nsGenericElement? Like the refcount, etc? Or is that dealt with elsewhere?
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to comment #5) > Comment on attachment 542503 [details] [diff] [review] [review] > Patch v3 > > +nsGenericElement::SizeOf() const > +{ > + PRInt64 size = Element::SizeOf(); > + > + size -= sizeof(mAttrsAndChildren); > + size += mAttrsAndChildren.SizeOf(); > + > + return size; > > What here takes into account the size of the other members of > nsGenericElement? Like the refcount, etc? Or is that dealt with elsewhere? I will deal with that later.
Assignee | ||
Comment 7•13 years ago
|
||
Comment on attachment 542503 [details] [diff] [review] Patch v3 In nsIContent.h: >- PRInt64 SizeOf() const { >+ virtual PRInt64 SizeOf() const { > return sizeof(*this); > } In nsGenericElement.cpp: >+PRInt64 >+nsGenericElement::SizeOf() const >+{ >+ PRInt64 size = Element::SizeOf(); I realize this is wrong... :( nsIContent::SizeOf() is going to return sizeof(nsIContent) even if |this| is nsGenericElement... This is really annoying because what we are doing is already hardly maintainable but if we have to explicitly add SizeOf() on all the inheritance chain, this is going to be even worse... Actually, here, the correct call I think should be: PRInt64 size = Element::SizeOf() + sizeof(*this) - sizeof(Element); In addition, we will have to add: Element::SizeOf() { return sizeof(*this); }
Assignee | ||
Comment 8•13 years ago
|
||
I wrote a helper method that could help us make this issue less annoying:
template <class TypeCurrent, class TypeParent>
PRInt64 GetInheritedSize(TypeCurrent* obj) {
return obj->TypeParent::SizeOf() - sizeof(TypeParent) + sizeof(TypeCurrent);
}
For exemple, nsGenericElement::SizeOf() could be rewritten like this:
>+PRInt64
>+nsGenericElement::SizeOf() const
>+{
>+ PRInt64 size = nsContentUtils::GetInheritedSize(nsGenericElement, Element);
[...]
There are two issue with this:
1. We require devs to call GetInheritedSize
2. We need to have ::SizeOf() implemented in all the heritage chain
I think we can try to solve these two issues by creating a macro that would create a basic SizeOf() method for all supported elements.
A more complex solution would be to make SizeOf pure virtual for non-leaf classes but that might have bigger consequences than we would like.
Assignee | ||
Comment 9•13 years ago
|
||
Use helpers introduced in bug 669308.
Attachment #542503 -
Attachment is obsolete: true
Attachment #543946 -
Flags: review?(bzbarsky)
Comment 10•13 years ago
|
||
Comment on attachment 543946 [details] [diff] [review] Patch v4 r=me
Attachment #543946 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite-
Whiteboard: [needs review] → [inbound]
Comment 11•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/b5daeed15a48
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
You need to log in
before you can comment on or make changes to this bug.
Description
•