The overall goal is to enable logging of DOM accesses carried out by JavaScript code running in V8, on a per-world basis. One use-case for such a mechanism would be to allow the chromium side to obtain a log of DOM accesses carried by extension code running in certain isolated worlds. Our approach is to wrap specific DOM bindings for the world so that when invoked, they first log a message into a log object provided by chromium side. This patch adds initial plumbing for the above mechanism. In particular it adds the following methods to the WebKit chromium API: * void WebCoverWrapping::setLog(int worldID, WebCoverWrapping::Log* log) * bool WebCoverWrapping::hasLog(int worldID) The method setLog simply adds an entry for the worldID and log, in a HashMap present in DOMWrapperWorld (similar to the map for SecurityOrigin and ContentSecurityPolicy) Subsequent WebCore patches would add code to appropriately wrap the DOM bindings for a world.
Created attachment 190175 [details] Patch
Created attachment 190180 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 190180 [details] Patch 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.
This patch also lacks tests. How does this patch relate to the tracing code that haraken is adding to the bindings?
ataly: I've been trying to insert TRACE_EVENT_STATE() macros into all DOM getters/setters/methods (https://bugs.webkit.org/show_bug.cgi?id=110667). Given that each DOM getter/setter/method takes within ~20 nano seconds, we cannot log something at every call. So TRACE_EVENT_STATE() just updates a global state variable so that a sampling profiler can read the global state periodically. The traced results will be integrated to about://tracing.
@haraken: The difference might be that Ankur is looking for a complete log, not just a sampled profile.
(In reply to comment #7) > @haraken: The difference might be that Ankur is looking for a complete log, not just a sampled profile. Previously complete logging was implemented by INC_STAT() macros. However, no one had used them and thus abarth@ removed them:) ataly: What use cases do you have in mind?
@haraken: The main use-case is to have logging enabled for a given isolated world and for a set of DOM object-properties specified by a policy. Policy could be thought of as a set of pairs of the form <cName, pName> which mean that every access to the property pName of object of class cName must be logged. We intend to use this mechanism for having a DOM activity log for Chrome extensions. The key constraint is to do this *only* for the specified isolated world, without touching the DOM bindings for any of the other worlds. So the other worlds do not see a direct slow down as a result of the logging. It seems that your patch inserts TRACE_EVENT_STATE() in all DOM bindings regardless of the world. @abarth: I will address your comments and update the patch soon. Regarding tests: We have another patch lined up that actually implements the wrapping mechanism. In that patch we also have tests for the WebCoverWrapping API that associate a log object with a world, inject a script into the world and then check if all the expected DOM actions get logged. These tests would be placed in Source/WebKit/chromium/tests Do you think we need tests for just this patch as well?
> Do you think we need tests for just this patch as well? It's fine to defer the tests until you've landed enough of the code to test it.
ataly: I understood that TRACE_EVENT wouldn't be helpful for your use cases. > So the other worlds do not see a direct slow down as a result of the logging. Wouldn't this insert an if branch to hot DOM attribute getters/setters/methods to check whether we should log or not? (If you are just planning to insert the logging to a small part of DOM getters/setters/methods that are not hot, it would be OK.) > Policy could be thought of as a set of pairs of the form <cName, pName> which mean that every access to the property pName of object of class cName must be logged. We intend to use this mechanism for having a DOM activity log for Chrome extensions. I don't understand what a policy is. Is it something exposed to developers? If you just want to know real-world usage of DOM activities, you might be able to use FeatureObservers.
Comment on attachment 190180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190180&action=review > Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:233 > + // HashMap doesn't support key 0, so we store the log at worldID + 1 You can use key 0 with the appropriate trait - see HashTraits.h. Looks like you want something like UnsignedWithZeroKeyHashTraits
(In reply to comment #11) > ataly: I understood that TRACE_EVENT wouldn't be helpful for your use cases. > > > So the other worlds do not see a direct slow down as a result of the logging. > > Wouldn't this insert an if branch to hot DOM attribute getters/setters/methods to check whether we should log or not? (If you are just planning to insert the logging to a small part of DOM getters/setters/methods that are not hot, it would be OK.) > Our goal was to avoid such an if statement as much as possible. Therefore the approach we take is to configure a different set of DOM templates for worlds with logging enabled. We check whether logging is needed, once, in the "initializeIfNeeded" method of DOMWindowShell and then set a flag in the context. Then we modify methods in V8DOMConfiguration to configure a different set of templates if the logging flag is enabled in the context. These templates have a wrapper around all the relevant callbacks, getters and setters, that first send a message to the log object and then forward the invocation. (Ulfar E. has implemented this particular patch.). > > Policy could be thought of as a set of pairs of the form <cName, pName> which mean that every access to the property pName of object of class cName must be logged. We intend to use this mechanism for having a DOM activity log for Chrome extensions. > > I don't understand what a policy is. Is it something exposed to developers? If you just want to know real-world usage of DOM activities, you might be able to use FeatureObservers. A policy determines which DOM object-property accesses must be logged. For instance in the case of an extension we would be interested in logging all access to window.location, document.write, document.createElement etc., so we would add these to the policy. At the moment we plan to have a fixed policy for all worlds that is implemented as a hard-coded struct. Later we can add methods to WebCoverWrapping to also accept a per-world policy object from the chromium side.
Thanks for the clarification. I understood the situation. > Our goal was to avoid such an if statement as much as possible. Therefore the approach we take is to configure a different set of DOM templates for worlds with logging enabled. marja@ is implementing two ObjectTemplates; one for the main world and the other for isolated worlds, which will be helpful for you. Unless your change affects the main world, I don't have any concern about performance.
Created attachment 190408 [details] Patch
@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.
Comment on attachment 190408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190408&action=review > Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:234 > + coverWrappingLogs().set(worldID, adoptPtr(log)); That usage is not correct. This function should take a PassOwnPtr as an argument. Please call adoptPtr immediately after calling new.
Created attachment 190556 [details] Patch
Comment on attachment 190556 [details] Patch 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.
Created attachment 190625 [details] Patch
(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.
(In reply to comment #21) > (In reply to comment #19) > > (From update of attachment 190556 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=190556&action=review > > > > > 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. I see. Won't you need to do the hash lookup to see if there is a logger configured for a world? That seems like it will impact performance even if no logger is configured. > 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. Do you plan to teach WebKit about this policy, or does WebKit always just call the log function? Maybe the answer to this question is related to my previous question and the issue is just that I don't understand this part of the design. > We do not have any performance measurements at the moment but we do plan to have them in the near future. Would you be willing to do some performance measurements before landing this patch? The performance aspects are an important consideration here. I would be interested in knowing the overhead for the following cases: 1) No logging configured for world. 2) Logging configured for world, but the API is not logged according to the policy. 3) Logging is configured for world, and the API is actually logged. In particular, I'm curious how this affects Dromeo's dom-traverse benchmark in these cases. You can find dom-traverse in the PerformanceTests directory. You can find more information about PerformaceTests on this wiki page: http://trac.webkit.org/wiki/Performance%20Tests
(In reply to comment #22) > (In reply to comment #21) > > (In reply to comment #19) > > > (From update of attachment 190556 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=190556&action=review > > > > > > > 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. > > I see. Won't you need to do the hash lookup to see if there is a logger configured for a world? That seems like it will impact performance even if no logger is configured. Yes for all worlds there will be a one-time HashMap lookup while initializing the context. (we consider the existence of a logger as an indication from the chromium side that logging should be enabled for that world). > > > 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. > > Do you plan to teach WebKit about this policy, or does WebKit always just call the log function? Maybe the answer to this question is related to my previous question and the issue is just that I don't understand this part of the design. As mentioned earlier, think of the policy as a set of pairs of the form <cName,pName>. Each such pair means that all access to property pName of objects with className= cName must be logged. This set will be hard-coded into the source code, in some struct that lives in WebCore. If logging is enabled for the world then the methods batchConfigureCallbacks and configureAttribute in V8DOMConfiguration would consult this policy struct for every DOM attribute that they configure. If the attribute is found in the policy struct then the methods will take a different branch and install wrappers for the attribute, otherwise they will behave the way they are right now. The actual patch makes a few other changes as well, but the gist is what is described above. Note that in the current design the policy is the same for all worlds and it is hard-coded. Later, if needed, we can have the policy struct also come from the Chromium side by proving a route through the WebKit API. Furthermore, the policy struct could also be different for each world. May be the word policy here is confusing? > > > We do not have any performance measurements at the moment but we do plan to have them in the near future. > > Would you be willing to do some performance measurements before landing this patch? The performance aspects are an important consideration here. I would be interested in knowing the overhead for the following cases: > > 1) No logging configured for world. > 2) Logging configured for world, but the API is not logged according to the policy. > 3) Logging is configured for world, and the API is actually logged. > > In particular, I'm curious how this affects Dromeo's dom-traverse benchmark in these cases. You can find dom-traverse in the PerformanceTests directory. You can find more information about PerformaceTests on this wiki page: > > http://trac.webkit.org/wiki/Performance%20Tests
(In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > (In reply to comment #19) > > > > (From update of attachment 190556 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=190556&action=review > > > > > > > > > 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. > > > > I see. Won't you need to do the hash lookup to see if there is a logger configured for a world? That seems like it will impact performance even if no logger is configured. > > Yes for all worlds there will be a one-time HashMap lookup while initializing the context. (we consider the existence of a logger as an indication from the chromium side that logging should be enabled for that world). Ah, initialization-time lookup makes more sense. I remember your saying that you'd then configure the wrappers different when there's logging involved. > > > 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. > > > > Do you plan to teach WebKit about this policy, or does WebKit always just call the log function? Maybe the answer to this question is related to my previous question and the issue is just that I don't understand this part of the design. > > As mentioned earlier, think of the policy as a set of pairs of the form <cName,pName>. Each such pair means that all access to property pName of objects with className= cName must be logged. > > This set will be hard-coded into the source code, in some struct that lives in WebCore. Oh, I see. Why hard-coded? It seems like something that should be configurable by the embedder. > If logging is enabled for the world then the methods batchConfigureCallbacks and configureAttribute in V8DOMConfiguration would consult this policy struct for every DOM attribute that they configure. If the attribute is found in the policy struct then the methods will take a different branch and install wrappers for the attribute, otherwise they will behave the way they are right now. The actual patch makes a few other changes as well, but the gist is what is described above. > > Note that in the current design the policy is the same for all worlds and it is hard-coded. > > Later, if needed, we can have the policy struct also come from the Chromium side by proving a route > through the WebKit API. Furthermore, the policy struct could also be different for each world. I see. > May be the word policy here is confusing? I'd call it a logging filter, but maybe I've drunk the Python logging coolaid too long. Ok, this is making more sense now.
Comment on attachment 190625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190625&action=review Ok, this generally looks good. Below are a bunch of naming nits. > Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:225 > +typedef HashMap< int, OwnPtr<V8DOMActivityLogger>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int> > DOMActivityLoggerMap; "< int" <--- please remove space between < and int I bet the WTF:: prefixes aren't needed on this line. > Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:233 > +void DOMWrapperWorld::setDOMActivityLogger(int worldID, PassOwnPtr<V8DOMActivityLogger> logger) worldID -> worldId We're not super consistent about this in WebCore, but I think we're supposed to match the DOM style (e.g., getElementById) setDOMActivityLogger -> setActivityLogger (We already know we're talking about the DOM because this is a method of DOMWrapperWorld.) > Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:238 > +V8DOMActivityLogger* DOMWrapperWorld::getDOMActivityLogger(int worldID) Please remove the "get" prefix. getDOMActivityLogger -> activityLogger > Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:240 > + DOMActivityLoggerMap& loggers = domActivityLoggers(); This could be a const reverence. > Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:242 > + return it == loggers.end() ? 0 : (it->value).get(); (it->value) <--- the ( ) aren't needed. > Source/WebCore/bindings/v8/DOMWrapperWorld.h:95 > + // Associate a logger object with the world identified by worldID (worlID may be 0 object <-- this word is redundant and can be removed. > Source/WebCore/bindings/v8/V8DOMActivityLogger.h:42 > + virtual void log(const char* apiCall, int argc, const v8::Handle<v8::Value>* args, const char* extra) { } apiCall -> functionName ? methodName ? It's not a "call"... args -> argv What is "extra" ? > Source/WebKit/chromium/public/WebDOMActivityLogger.h:47 > +bool hasDOMActivityLogger(int worldID); You're going to need to mark this function and setDOMActivityLogger with WEBKIT_EXPORT so that the component build will link. > Source/WebKit/chromium/src/WebDOMActivityLogger.cpp:49 > + virtual ~DOMActivityLoggerContainer() { } You can skip this line. The compiler will generate it for you. > Source/WebKit/chromium/src/WebDOMActivityLogger.cpp:66 > + if (logger) It's probably better to make it an error to call this function with a null pointer. The way to do that is to ASSERT(logger).
Created attachment 190861 [details] Patch
Comment on attachment 190861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190861&action=review This looks good. There's just one nit about the include. You've probably left the office for today, so I can try fixing that for you. > Source/WebCore/bindings/v8/V8DOMActivityLogger.h:2 > + * Copyright (C) 2009 Google Inc. All rights reserved. 2009 -> 2013 > Source/WebKit/chromium/public/WebDOMActivityLogger.h:34 > +#include "../../../Platform/chromium/public/WebCanvas.h" Why do we need this? You should include WebCommon.h or something like that. > Source/WebKit/chromium/src/WebDOMActivityLogger.cpp:53 > +private: nit: We would normally put a blank line above this line.
Created attachment 190865 [details] Patch for landing
Comment on attachment 190865 [details] Patch for landing Clearing flags on attachment: 190865 Committed r144408: <http://trac.webkit.org/changeset/144408>
All reviewed patches have been landed. Closing bug.
Reopening to attach new patch.
Created attachment 191084 [details] Patch
Comment on attachment 191084 [details] Patch Why didn't the previous patch break the build without this change?
Comment on attachment 190865 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=190865&action=review > Source/WebKit/chromium/public/WebDOMActivityLogger.h:34 > +#include "../../../Platform/chromium/public/WebCommon.h" This should be "public/WebCommon.h" or something like that (please don't use a relative path.)
> Why didn't the previous patch break the build without this change? Because the API isn't called yet. > > Source/WebKit/chromium/public/WebDOMActivityLogger.h:34 > > +#include "../../../Platform/chromium/public/WebCommon.h" > > This should be "public/WebCommon.h" or something like that (please don't use a relative path.) Unfortunately, this is the style we're using currently. We tried a bunch of different options, and this was the least bad. I'm hoping we can revisit this in the future and use something more sensible.
@Ankur: Typically we use a new bug for new patches, even simple follow-up patches like this one. You can link bugs together using the "depends on" and "blocks" fields if you like.
Comment on attachment 191084 [details] Patch Rejecting attachment 191084 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 191084, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: fatal: read error: Connection reset by peer Died at Tools/Scripts/update-webkit line 151. Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2 Updating OpenSource fatal: read error: Connection reset by peer Died at Tools/Scripts/update-webkit line 151. Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2 Updating OpenSource fatal: read error: Connection reset by peer Died at Tools/Scripts/update-webkit line 151. Full output: http://webkit-commit-queue.appspot.com/results/16785808
Comment on attachment 191084 [details] Patch Clearing flags on attachment: 191084 Committed r144564: <http://trac.webkit.org/changeset/144564>