Bug 111483 – Modify log method in DOMActivityLogger

• Component: WebKit API
• Status: RESOLVED FIXED

[Ankur Taly]

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.

[Ankur Taly]

Created attachment 191581 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 191581 [details]
Patch

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

[Ankur Taly]

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: 
https://codereview.chromium.org/12390076/

[Ankur Taly]

Created attachment 191625 [details]
Patch

[Ankur Taly]

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.

[Adam Barth]

Comment on attachment 191625 [details]
Patch

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.

[Adam Barth]

(The patch looks fine otherwise.)

[Ankur Taly]

Created attachment 191795 [details]
Patch

[Adam Barth]

Comment on attachment 191795 [details]
Patch

Thanks.

[WebKit Review Bot]

Comment on attachment 191795 [details]
Patch

Clearing flags on attachment: 191795

Committed r144988: <http://trac.webkit.org/changeset/144988>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.