Bug 42328 – WebKitTestRunner needs to support layoutTestController.setJavaScriptProfilingEnabled

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Maciej Stachowiak]

WebKitTestRunner needs to support layoutTestController.setJavaScriptProfilingEnabled

[Maciej Stachowiak]

<rdar://problem/8193637>

[Ryosuke Niwa]

Added inspector/profiler/cpu-profiler-profiling-without-inspector.html to the skipped list in http://trac.webkit.org/changeset/88597.

[Jesus Sanchez-Palencia]

Created attachment 143883 [details]
WIP for ews testing

[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.

[Gustavo Noronha (kov)]

Comment on attachment 143883 [details]
WIP for ews testing

Attachment 143883 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12803245

[Build Bot]

Comment on attachment 143883 [details]
WIP for ews testing

Attachment 143883 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12816002

[Jesus Sanchez-Palencia]

Created attachment 143893 [details]
WIP for ews testing v2

[Jesus Sanchez-Palencia]

Created attachment 144076 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 144076 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Add setJavaScriptProfilingEnabled() to window.internals.settings. No new tests, but this

Given the last reviews related to window.internals, I think would be good to have some rationale why this function is a good fit for moving there.

> Source/WebCore/testing/InternalSettings.cpp:120
> +#if ENABLE(INSPECTOR)
> +    if (page() && page()->inspectorController())
> +        page()->inspectorController()->disableProfiler();
> +#endif

Why not save and use the original value like the others?

I'm also wondering whether we could have InspectorController::setProfilerEnabled(bool) instead of disableProfiler()/enableProfiler().

[Jesus Sanchez-Palencia]

(In reply to comment #9)

Thanks for having a look!

> > Source/WebCore/testing/InternalSettings.cpp:120
> > +#if ENABLE(INSPECTOR)
> > +    if (page() && page()->inspectorController())
> > +        page()->inspectorController()->disableProfiler();
> > +#endif
> 
> Why not save and use the original value like the others?
> 
> I'm also wondering whether we could have InspectorController::setProfilerEnabled(bool) instead of disableProfiler()/enableProfiler().

I like the idea. I will upload a new patch soon with this and also with the default value coming from InspectorController::profilerEnabled().

[Jesus Sanchez-Palencia]

Created attachment 144126 [details]
WIP for ews testing v3

[Gustavo Noronha (kov)]

Comment on attachment 144126 [details]
WIP for ews testing v3

Attachment 144126 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12803577

[Build Bot]

Comment on attachment 144126 [details]
WIP for ews testing v3

Attachment 144126 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12801549

[Build Bot]

Comment on attachment 144126 [details]
WIP for ews testing v3

Attachment 144126 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12802540

[Jesus Sanchez-Palencia]

Created attachment 144145 [details]
Patch

[Build Bot]

Comment on attachment 144145 [details]
Patch

Attachment 144145 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12805587

[Hajime Morrita]

It looks you forgot to remove now-invalidated symbols from WebCore.exp.in.

[Jesus Sanchez-Palencia]

Created attachment 144347 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 144347 [details]
Patch

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

Seems reasonable.

> Source/WebCore/testing/InternalSettings.cpp:105
> +    , m_originalJavaScriptProfilingEnabled((page() && page()->inspectorController()) ? page()->inspectorController()->profilerEnabled() : false)

This could be an && instead of a ternary.

[Jesus Sanchez-Palencia]

(In reply to comment #19)
> (From update of attachment 144347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144347&action=review
> 
> Seems reasonable.
> 
> > Source/WebCore/testing/InternalSettings.cpp:105
> > +    , m_originalJavaScriptProfilingEnabled((page() && page()->inspectorController()) ? page()->inspectorController()->profilerEnabled() : false)
> 
> This could be an && instead of a ternary.

Thanks! I will fix it before landing.

[Jesus Sanchez-Palencia]

Committed r118705: <http://trac.webkit.org/changeset/118705>