[Webkit-unassigned] [Bug 110779] WebKit API for enabling DOM logging for certain worlds

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=110779





--- Comment #16 from Ankur Taly <ataly at google.com>  2013-02-26 18:17:26 PST ---
@abarth: I made the changes that you suggested and uploaded a new patch. Please see the comments below.

(In reply to comment #4)
> (From update of attachment 190180 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190180&action=review
> 
> > Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:234
> > +    // HashMap doesn't support key 0, so we store the log at worldID + 1
> > +    coverWrappingLogs().set(worldID+1, log);
> 
> We should use an inline function to convert between the worldID and the key for this map.
I used the HashTraits approach suggested by James Robinson.
> 
> > Source/WebKit/chromium/public/WebCoverWrapping.h:39
> > +#if WEBKIT_USING_V8
> > +namespace v8 {
> > +class Value;
> > +template <class T> class Handle;
> > +}
> > +#endif
> 
> I think you can just #include v8.h in the API these days.
> 
Done.
> > Source/WebKit/chromium/public/WebCoverWrapping.h:49
> > +        virtual void logMessage(const char*, int, const v8::Handle<v8::Value>*, const char*) { }
> 
> Should we have names for these arguments.  It's not clear what these arguments mean.
> 
Done.

> > Source/WebKit/chromium/src/WebCoverWrapping.cpp:57
> > +    OwnPtr<WebCoverWrapping::Log*> m_coverWrappingLog;
> 
> OwnPtr<WebCoverWrapping::Log*>  <-- Do you have an extra * here?
> 
Yes, fixed that.
> > Source/WebKit/chromium/src/WebCoverWrapping.cpp:62
> > +    return DOMWrapperWorld::getCoverWrappingLog(worldID) ? true : false; 
> 
> There's no reason to use the "? :" operator here.  The compiler will convert it to bool for you.
> 
Done.
> > Source/WebKit/chromium/src/WebCoverWrapping.cpp:67
> > +    DOMWrapperWorld::setCoverWrappingLog(worldID, new CoverWrappingLogContainer(log));
> 
> This is a "naked new".  All calls to "new" should be wrapped in either adoptPtr or adoptRef, depending on whether CoverWrappingLogContainer is RefCounted.
The HashMap in DOMWrapperWorld now holds OwnPtrs, and the set call invokes adoptPtr on the raw pointer argument and then stores it in the HashMap.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.