This would be really helpful. Get Bytecode Profile lets you save out a file. We should do the same for the sampling profiler output.
Created attachment 288423 [details] patch
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.
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.
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.
Created attachment 288476 [details] patch
I'm going to add API tests in a follow up: https://bugs.webkit.org/show_bug.cgi?id=161832
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.
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).
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.
Anders, any comments/suggestions on Tim's inquiry?
(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.
Created attachment 288581 [details] patch for landing I'll give some time before landing to allow Anders (or anybody else) comment.
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.
Comment on attachment 288581 [details] patch for landing Clearing flags on attachment: 288581 Committed r205822: <http://trac.webkit.org/changeset/205822>
All reviewed patches have been landed. Closing bug.