Bug 161785 – Add WebKit support for an option in Safari's debug menu similar to "Get Bytecode Profile" but for the Sampling Profiler's data

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

[Saam Barati]

This would be really helpful. Get Bytecode Profile lets you save out a file. We should do the same for the sampling profiler output.

[Saam Barati]

Created attachment 288423 [details]
patch

[WebKit Commit Bot]

Attachment 288423 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebPageProxy.cpp:2880:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebPageProxy.h:762:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 288423 [details]
patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:5387
> +        send(Messages::WebPageProxy::StringCallback(String("Sampling profiler is not enabled."), callbackID));

Nit: I think you could use ASCIILiteral() instead of String().

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:5396
> +    send(Messages::WebPageProxy::StringCallback(String("Sampling Profiler is not enabled on this platform."), callbackID));

Ditto.

[Brady Eidson]

Comment on attachment 288423 [details]
patch

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

This needs API tests.

> Source/WebCore/ChangeLog:3
> +        Add webkit support for an option in Safari's debug menu similar to "Get Bytecode Profile" but for the sampling profiler's data

WebKit

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:5387
>> +        send(Messages::WebPageProxy::StringCallback(String("Sampling profiler is not enabled."), callbackID));
> 
> Nit: I think you could use ASCIILiteral() instead of String().

Additionally, this sends back a seemingly valid string back for an error case.

The callback passed in to the SPI allows for error cases, so we should invoke that.

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:5396
>> +    send(Messages::WebPageProxy::StringCallback(String("Sampling Profiler is not enabled on this platform."), callbackID));
> 
> Ditto.

And ditto.

[Saam Barati]

Created attachment 288476 [details]
patch

[Saam Barati]

I'm going to add API tests in a follow up:
https://bugs.webkit.org/show_bug.cgi?id=161832

[WebKit Commit Bot]

Attachment 288476 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebPageProxy.cpp:2880:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebPageProxy.h:762:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 288476 [details]
patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:5001
> +    auto callback = m_callbacks.take<StringCallback>(callbackID);

I think that this should be <CallbackBase>, right? Unless you make this specifically invalidateStringCallback.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:5008
> +    m_loadDependentStringCallbackIDs.remove(callbackID);

Not 100% sure about this in the generic case, too. Anders is probably the best person to advise, you should check with him (or see if he has a better way to do this, since, as discussed, it's super bizarre that this hasn't existed before).

[Saam Barati]

Comment on attachment 288476 [details]
patch

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

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:5001
>> +    auto callback = m_callbacks.take<StringCallback>(callbackID);
> 
> I think that this should be <CallbackBase>, right? Unless you make this specifically invalidateStringCallback.

Yeah, this is totally a copy-paste bug.

[Saam Barati]

Anders, any comments/suggestions on Tim's inquiry?

[Saam Barati]

(In reply to comment #8)
> Comment on attachment 288476 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288476&action=review
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:5001
> > +    auto callback = m_callbacks.take<StringCallback>(callbackID);
> 
> I think that this should be <CallbackBase>, right? Unless you make this
> specifically invalidateStringCallback.
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:5008
> > +    m_loadDependentStringCallbackIDs.remove(callbackID);
> 
> Not 100% sure about this in the generic case, too. Anders is probably the
> best person to advise, you should check with him (or see if he has a better
> way to do this, since, as discussed, it's super bizarre that this hasn't
> existed before).

Actually, it appears that things that expect a StringCallback will add to
the m_loadDependentStringCallbackIDs Set. Therefore, I think it's safest
to change invalidateCallback into invalidateStringCallback as you suggested
and then invalidateStringCallback can remove the callback from the set.

[Saam Barati]

Created attachment 288581 [details]
patch for landing

I'll give some time before landing to allow Anders (or anybody else) comment.

[WebKit Commit Bot]

Attachment 288581 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebPageProxy.cpp:2880:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebPageProxy.h:762:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 288581 [details]
patch for landing

Clearing flags on attachment: 288581

Committed r205822: <http://trac.webkit.org/changeset/205822>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.