Last Comment Bug 682264 - Make EventListenerManager participate to the DOM Memory Reporter
: Make EventListenerManager participate to the DOM Memory Reporter
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug] (reviewing overload)
:
:
Mentors:
Depends on:
Blocks: 663271
  Show dependency treegraph
 
Reported: 2011-08-26 05:51 PDT by Olli Pettay [:smaug] (reviewing overload)
Modified: 2011-08-26 16:15 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (7.00 KB, patch)
2011-08-26 07:53 PDT, Olli Pettay [:smaug] (reviewing overload)
mounir: review+
Details | Diff | Splinter Review
patch (7.43 KB, patch)
2011-08-26 11:04 PDT, Olli Pettay [:smaug] (reviewing overload)
bzbarsky: review+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (reviewing overload) 2011-08-26 05:51:31 PDT

    
Comment 1 Olli Pettay [:smaug] (reviewing overload) 2011-08-26 07:53:42 PDT
Created attachment 556029 [details] [diff] [review]
patch
Comment 2 Mounir Lamouri (:mounir) 2011-08-26 08:11:28 PDT
Comment on attachment 556029 [details] [diff] [review]
patch

Review of attachment 556029 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/public/nsINode.h
@@ +281,5 @@
>  
>  // IID for the nsINode interface
>  #define NS_INODE_IID \
> +{ 0x5572c8a9, 0xbda9, 0x4b78, \
> +  { 0xb4, 0x1a, 0xdb, 0x1a, 0x83, 0xef, 0x53, 0x7e } }

Do you really need that?

@@ +294,5 @@
>  {
>  public:
>    NS_DECLARE_STATIC_IID_ACCESSOR(NS_INODE_IID)
>  
> +  virtual PRInt64 SizeOf() const;

Please use NS_DECL_DOM_MEMORY_REPORTER_SIZEOF

::: content/base/src/nsGenericElement.cpp
@@ +5409,5 @@
> +  nsEventListenerManager* elm =
> +    const_cast<nsINode*>(this)->GetListenerManager(PR_FALSE);
> +  if (elm) {
> +    size += elm->SizeOf();
> +  }

nit: I would have put a blank line before nsEventListenerManager* elm and juste here too (after the }).

::: content/events/src/nsEventListenerManager.cpp
@@ +981,5 @@
> +PRInt64
> +nsEventListenerManager::SizeOf() const
> +{
> +  PRInt64 size = sizeof(*this);
> +  const nsListenerStruct* ls;

Why a pointer? I think a reference would be fine here.

@@ +991,5 @@
> +    if (jsl) {
> +      size += jsl->SizeOf();
> +    }
> +  }
> +  return size;

I don't think I'm the best suited to review that method. That seems correct but I wouldn't be able to guarantee nothing is missing.

::: dom/base/nsIJSEventListener.h
@@ +89,5 @@
>    // there is already a handler!  The handler must already be bound to
>    // the right target.
>    virtual void SetHandler(void *aHandler) = 0;
>  
> +  virtual PRInt64 SizeOf() const = 0;

None of the members are owned by this class?
Comment 3 Olli Pettay [:smaug] (reviewing overload) 2011-08-26 11:04:22 PDT
Created attachment 556076 [details] [diff] [review]
patch
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-08-26 14:48:32 PDT
Comment on attachment 556076 [details] [diff] [review]
patch

r=me
Comment 5 Olli Pettay [:smaug] (reviewing overload) 2011-08-26 16:15:20 PDT
http://hg.mozilla.org/mozilla-central/rev/906413a7ae6b

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