[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 #21 from Ankur Taly <ataly at google.com>  2013-02-27 16:10:24 PST ---
(In reply to comment #19)
> (From update of attachment 190556 [details])
> 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?
No, we can avoid that. DOM loggers can be cached in perContextData during context initialization
and all DOM wrappers would pick them up from there.
> 
> 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 ?
Yes, thank you for that suggestion. I changed it to "Logger".
> 
> > 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.)
Done.
> 
> > Source/WebKit/chromium/public/WebCoverWrapping.h:40
> > +class WebCoverWrapping {
> 
> What is a WebCoverWrapping?
The class is now called "WebDOMActivityLogger"
> 
> > Source/WebKit/chromium/public/WebCoverWrapping.h:42
> > +    class Log {
> 
> Log -> Logger
Done.
> 
> > 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
Done.
> 
> > Source/WebKit/chromium/public/WebCoverWrapping.h:51
> > +    static bool hasLog(int worldID);
> 
> Why is this API needed?
This is needed by the Chromium side to figure out if a logger has already been set for an isolated world.
In such a case we can avoid constructing a new logger and overwriting the old one. For instance for scripts injected using the extension API tabs.executeCode, we may not want to set a fresh logger if there already exists a logger set for the world (from say a previous tabs.executeCode call by the same extension).
> 
> > 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?

Logging can be enabled for any world. The guarantee is that the DOM bindings for all
those worlds for which logging is not enabled would not be affected. We include the main world
to log DOM activity inside extension background pages.

> 
> > 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):
Done.

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

I changed the implementation of setDOMActivityLogger so that it does nothing when supplied with a Null argument. This way we can have the invariant that all DOMActivityLoggerContainers in DOMWrapperWorld
would have non-null WebDOMActivityLoggers embedded in them. 

The logger will be invoked on all DOM API calls that are covered by the policy. 
To begin with we will have a small policy that includes critical calls like document.write, document.createElement, document.appendChild, window.location and so on.

We do not have any performance measurements at the moment but we do plan to have them in the near future. 
> 
> > Source/WebKit/chromium/src/WebCoverWrapping.cpp:44
> > +    explicit CoverWrappingLogContainer(WebCoverWrapping::Log* log)
> 
> This class should take a PassOwnPtr rather than a raw pointer.
> 
Done.
> > Source/WebKit/chromium/src/WebCoverWrapping.cpp:50
> > +#if WEBKIT_USING_V8
> 
> This ifdef isn't needed.  We always use V8.
Done.
> 
> > Source/WebKit/chromium/src/WebCoverWrapping.cpp:67
> > +    DOMWrapperWorld::setCoverWrappingLog(worldID, adoptPtr(new CoverWrappingLogContainer(log)));
> 
> We should call adoptPtr(log) here.
Done.

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