Bug 111483 - Modify log method in DOMActivityLogger
Summary: Modify log method in DOMActivityLogger
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Nobody
Depends on:
Reported: 2013-03-05 14:39 PST by Ankur Taly
Modified: 2013-03-06 14:58 PST (History)
8 users (show)

See Also:

Patch (4.66 KB, patch)
2013-03-05 15:24 PST, Ankur Taly
no flags Details | Formatted Diff | Diff
Patch (4.54 KB, patch)
2013-03-05 18:06 PST, Ankur Taly
no flags Details | Formatted Diff | Diff
Patch (4.86 KB, patch)
2013-03-06 10:59 PST, Ankur Taly
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ankur Taly 2013-03-05 14:39:03 PST
This patch modifies the "log" method in WebDOMActivityLogger (and V8DOMActivityLogger)
so that it takes  const WebString& (const String&) arguments instead of const char*,
and also takes an additional context handle.
Comment 1 Ankur Taly 2013-03-05 15:24:08 PST
Created attachment 191581 [details]
Comment 2 WebKit Review Bot 2013-03-05 15:27:54 PST
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 3 Adam Barth 2013-03-05 15:31:00 PST
Comment on attachment 191581 [details]

This patch does not explain why you think we should make this change.  It only explains what change you're making.

I'm skeptical about needing to include the context in the log message.  The current context is available in a static from V8 via v8::Context::GetCurrent().
Comment 4 Ankur Taly 2013-03-05 16:52:03 PST
The WebDOMActivityLogger class is overridden on the Chromium side with a log method that marshalls all its arguments and then sends them over to the browser process. This marshaling requires a handle to the current context.

The chromium reviewers suggested avoiding using the static v8::context::GetCurent() and instead have the context explicitly passed to the log method.

FYI, the link to the chromium patch is: 
Comment 5 Ankur Taly 2013-03-05 18:06:31 PST
Created attachment 191625 [details]
Comment 6 Ankur Taly 2013-03-05 18:07:35 PST
I checked again with Matt Perry (OWNER on chrome/renderer/extensions) and it turns out that it is fine to invoke the static getCurrent() in order access the context.

I have updated the patch so that it only makes the following change: move const char* arguments of the log methods in WebDOMActivityLogger and V8DOMActvityLogger to const WebString& and const String& respectively.
Comment 7 Adam Barth 2013-03-05 22:53:18 PST
Comment on attachment 191625 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=191625&action=review

> Source/WebCore/ChangeLog:9
> +        https://bugs.webkit.org/show_bug.cgi?id=111483
> +
> +        Reviewed by NOBODY (OOPS!).
> +

Please explain why you're making this change in the ChangeLog.

> Source/WebCore/ChangeLog:10
> +        No new tests (OOPS!).

This line will prevent your patch from being landed.  You should explain that there are no tests because there is no behavior change.
Comment 8 Adam Barth 2013-03-05 22:53:33 PST
(The patch looks fine otherwise.)
Comment 9 Ankur Taly 2013-03-06 10:59:56 PST
Created attachment 191795 [details]
Comment 10 Adam Barth 2013-03-06 14:34:21 PST
Comment on attachment 191795 [details]

Comment 11 WebKit Review Bot 2013-03-06 14:58:39 PST
Comment on attachment 191795 [details]

Clearing flags on attachment: 191795

Committed r144988: <http://trac.webkit.org/changeset/144988>
Comment 12 WebKit Review Bot 2013-03-06 14:58:43 PST
All reviewed patches have been landed.  Closing bug.