Last Comment Bug 668013 - Add nsHTMLStyleSheet::SizeOf()
: Add nsHTMLStyleSheet::SizeOf()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on:
Blocks: 663271 667887
  Show dependency treegraph
 
Reported: 2011-06-28 13:40 PDT by Mounir Lamouri (:mounir)
Modified: 2011-07-25 05:57 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (4.09 KB, patch)
2011-06-28 13:40 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Review
Patch v2 (4.70 KB, patch)
2011-06-28 15:47 PDT, Mounir Lamouri (:mounir)
bzbarsky: review+
Details | Diff | Review
Patch v3 (5.93 KB, patch)
2011-07-05 08:30 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Review
Patch v3 (5.93 KB, patch)
2011-07-05 08:32 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Review

Description Mounir Lamouri (:mounir) 2011-06-28 13:40:43 PDT
Created attachment 542573 [details] [diff] [review]
Patch v1
Comment 1 Mounir Lamouri (:mounir) 2011-06-28 15:47:02 PDT
Created attachment 542633 [details] [diff] [review]
Patch v2

Trying something...
Comment 2 Boris Zbarsky [:bz] 2011-06-28 19:53:40 PDT
Comment on attachment 542633 [details] [diff] [review]
Patch v2

I wonder whether the |size += mFoo ? sizeof(*mFoo.get()) : 0| pattern could just use a macro...

>+  if (entry->mAttributes) {

mAttributes is never null.

The sizeof(nsIURI) thing there is of limited utility, for what it's worth.  It'll just claim to be one word, I bet.

>+    size += sizeof(MappedAttrTable_Ops);

Why?

>+    size += sizeof(*entry->mAttributes);

And I guess a followup to get nsMappedAttributes::SizeOf() ?

r=me with the nits fixed (in particular, take out or really explain the _Ops thing).
Comment 3 Mounir Lamouri (:mounir) 2011-07-05 08:30:11 PDT
(In reply to comment #2)
> I wonder whether the |size += mFoo ? sizeof(*mFoo.get()) : 0| pattern could
> just use a macro...

I agree. Though, I will keep that for a follow-up. There are a few situations that could use a macro or a helper method.

> >+  if (entry->mAttributes) {
> 
> mAttributes is never null.

I've put an NS_ASSERTION.

> The sizeof(nsIURI) thing there is of limited utility, for what it's worth. 
> It'll just claim to be one word, I bet.

Do you mean:
> size += mURL ? sizeof(*mURL.get()) : 0;

> >+    size += sizeof(MappedAttrTable_Ops);
> 
> Why?

Removed. Sorry, that was stupid.

> >+    size += sizeof(*entry->mAttributes);
> 
> And I guess a followup to get nsMappedAttributes::SizeOf() ?

bug 667887
Comment 4 Mounir Lamouri (:mounir) 2011-07-05 08:30:50 PDT
Created attachment 543955 [details] [diff] [review]
Patch v3

r=bz
Comment 5 Mounir Lamouri (:mounir) 2011-07-05 08:32:20 PDT
Created attachment 543956 [details] [diff] [review]
Patch v3
Comment 6 Mounir Lamouri (:mounir) 2011-07-18 17:21:25 PDT
Oups, comment 3 was expecting a reply from you Boris but you were not in CC ;)
Comment 7 Boris Zbarsky [:bz] 2011-07-18 20:32:40 PDT
>Do you mean:
>> size += mURL ? sizeof(*mURL.get()) : 0;

Yes.  *mURL.get() is of type nsIURI, which is a class with nothing but a vtable and only one vtable pointer.

And come to think of it, the mURL of an nsHTMLStyleSheet is always the URI of the document.  So we should just not count it here at all (apart from the space the mURL COMPtr takes in nsHTMLStyleSheet, which sizeof(*this) covered.
Comment 8 Marco Bonardo [::mak] 2011-07-20 06:55:58 PDT
this has been backed out by ehsan due to bustage with all the other changesets in the same push
Comment 9 Marco Bonardo [::mak] 2011-07-25 05:57:42 PDT
http://hg.mozilla.org/mozilla-central/rev/1ec9c2051c07

Note You need to log in before you can comment on or make changes to this bug.