[webkit-reviews] review denied: [Bug 110779] WebKit API for enabling DOM logging for certain worlds : [Attachment 190180] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Ankur Taly <ataly at google.com>'s
request for review:
Bug 110779: WebKit API for enabling DOM logging for certain worlds
https://bugs.webkit.org/show_bug.cgi?id=110779

Attachment 190180: Patch
https://bugs.webkit.org/attachment.cgi?id=190180&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.

> 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.

> 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.

> Source/WebKit/chromium/src/WebCoverWrapping.cpp:57
> +    OwnPtr<WebCoverWrapping::Log*> m_coverWrappingLog;

OwnPtr<WebCoverWrapping::Log*>	<-- Do you have an extra * here?

> 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.

> 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.