Bug 78507 – Chrome V8 tracing is incomplete

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Rick Byers]

V8Proxy::evaluate has instrumentation to add a 'v8.run' entry in chrome://tracing.  If we're going to indicate when time is being spent inside V8, we should try to be more thorough.  I've found V8Proxy::callFunction to be another significant source in practice, and presumably V8Proxy::newInstance can also be an important entrypoint.

[Rick Byers]

Created attachment 126794 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 126794 [details]
Patch

All Changes require a ChangeLog entry.  But otherwise I doubt this is a controvesial change. :)

[Adam Barth]

Comment on attachment 126794 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Proxy.cpp:431
> +#if PLATFORM_CHROMIUM

This should be #if PLATFORM(CHROMIUM)

[Rick Byers]

Created attachment 126842 [details]
Patch

[Rick Byers]

Thanks for the feedback - didn't realize people would look at my patch as soon as I uploaded it (as opposed to the Chromium process where we have to explicitly ask someone to review).  Please see updated patch.

[Eric Seidel (no email)]

Comment on attachment 126842 [details]
Patch

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

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

The commit-queue will barf on this line.  You'll need to replace this line with either an explanation of why testing is impossible/impractical (as is likely the case here) or list tests which cover this change.

[Rick Byers]

Created attachment 126848 [details]
Patch

[Rick Byers]

Comment on attachment 126842 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> The commit-queue will barf on this line.  You'll need to replace this line with either an explanation of why testing is impossible/impractical (as is likely the case here) or list tests which cover this change.

Thanks, updated.

[Eric Seidel (no email)]

Comment on attachment 126848 [details]
Patch

Thanks.

[WebKit Review Bot]

Comment on attachment 126848 [details]
Patch

Clearing flags on attachment: 126848

Committed r107685: <http://trac.webkit.org/changeset/107685>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Moving all JavaScriptGlue bugs to JavaScriptCore. The JavaScriptGlue framework itself is long gone. And most of the more recent bugs put in this component were put there by people who thought this was for some other aspect of “JavaScript glue” and have nothing to do with the actual original reason for the existence of this component, which was an OS-X-only framework named JavaScriptGlue.