Summary: | Add WebKit support for an option in Safari's debug menu similar to "Get Bytecode Profile" but for the Sampling Profiler's data | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||
Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, bdakin, beidson, benjamin, commit-queue, darin, fpizlo, ggaren, gskachkov, jfbastien, joepeck, keith_miller, mark.lam, msaboff, oliver, sam, simon.fraser, sukolsak, thorton, ticaiolima, ysuzuki | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Saam Barati
2016-09-08 20:19:41 PDT
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. |