[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 #19 from Adam Barth <abarth at webkit.org>  2013-02-27 11:24:38 PST ---
(From update of attachment 190556)
View in context: https://bugs.webkit.org/attachment.cgi?id=190556&action=review

> Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:237
> +V8DOMCoverWrappingLog* DOMWrapperWorld::getCoverWrappingLog(int worldID)

Every DOM call will now have a hash lookup to see if it has a logger?

Maybe we should store the logger on the DOMWrapperWorld object?

> Source/WebCore/bindings/v8/V8DOMCoverWrappingLog.h:38
> +class V8DOMCoverWrappingLog {

What is a CoverWrapping?  Also, this isn't a log, it's a logger.  Perhaps we should call this a V8DOMLogger ?

> Source/WebCore/bindings/v8/V8DOMCoverWrappingLog.h:42
> +    virtual void logMessage(const char*, int, const v8::Handle<v8::Value>*, const char*) { }

logMessage -> log   (The word "message" doesn't mean anything here.)

> Source/WebKit/chromium/public/WebCoverWrapping.h:40
> +class WebCoverWrapping {

What is a WebCoverWrapping?

> Source/WebKit/chromium/public/WebCoverWrapping.h:42
> +    class Log {

Log -> Logger

> Source/WebKit/chromium/public/WebCoverWrapping.h:46
> +        virtual void logMessage(const char* apiCall, int argc, const v8::Handle<v8::Value>* args, const char* extra) { }

logMessage -> log

> Source/WebKit/chromium/public/WebCoverWrapping.h:51
> +    static bool hasLog(int worldID);

Why is this API needed?

> Source/WebKit/chromium/public/WebCoverWrapping.h:54
> +    // Sets the provided log object for the world identified 
> +    // by worldID (worldID may be 0 identifying the main world).

I thought we weren't going to support installing a logger for the main world?

> Source/WebKit/chromium/public/WebCoverWrapping.h:55
> +    static void setLog(int worldID, Log*);

I would just make this a function in the global scope (inside the WebKit namespace):

setDOMLogger(int worldID, WebDOMLogger*);

Can you clear a logger by supplying a null for the second argument?  Are we going to call the logger for every API, or are we going to filter?  Have you measured the performance overhead of (1) supporting this feature even without setting a logger and (2) when logging is turned on?

> Source/WebKit/chromium/src/WebCoverWrapping.cpp:44
> +    explicit CoverWrappingLogContainer(WebCoverWrapping::Log* log)

This class should take a PassOwnPtr rather than a raw pointer.

> Source/WebKit/chromium/src/WebCoverWrapping.cpp:50
> +#if WEBKIT_USING_V8

This ifdef isn't needed.  We always use V8.

> Source/WebKit/chromium/src/WebCoverWrapping.cpp:67
> +    DOMWrapperWorld::setCoverWrappingLog(worldID, adoptPtr(new CoverWrappingLogContainer(log)));

We should call adoptPtr(log) here.

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