WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
161785
Add WebKit support for an option in Safari's debug menu similar to "Get Bytecode Profile" but for the Sampling Profiler's data
https://bugs.webkit.org/show_bug.cgi?id=161785
Summary
Add WebKit support for an option in Safari's debug menu similar to "Get Bytec...
Saam Barati
Reported
2016-09-08 20:19:41 PDT
This would be really helpful. Get Bytecode Profile lets you save out a file. We should do the same for the sampling profiler output.
Attachments
patch
(8.39 KB, patch)
2016-09-09 12:00 PDT
,
Saam Barati
beidson
: review-
Details
Formatted Diff
Diff
patch
(10.65 KB, patch)
2016-09-09 19:25 PDT
,
Saam Barati
thorton
: review+
Details
Formatted Diff
Diff
patch for landing
(10.84 KB, patch)
2016-09-12 10:41 PDT
,
Saam Barati
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Saam Barati
Comment 1
2016-09-09 12:00:43 PDT
Created
attachment 288423
[details]
patch
WebKit Commit Bot
Comment 2
2016-09-09 12:02:03 PDT
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 3
2016-09-09 12:34:57 PDT
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 4
2016-09-09 12:51:53 PDT
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
Comment 5
2016-09-09 19:25:49 PDT
Created
attachment 288476
[details]
patch
Saam Barati
Comment 6
2016-09-09 19:28:07 PDT
I'm going to add API tests in a follow up:
https://bugs.webkit.org/show_bug.cgi?id=161832
WebKit Commit Bot
Comment 7
2016-09-09 19:28:14 PDT
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 8
2016-09-09 19:34:21 PDT
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 9
2016-09-09 19:46:48 PDT
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
Comment 10
2016-09-09 19:47:10 PDT
Anders, any comments/suggestions on Tim's inquiry?
Saam Barati
Comment 11
2016-09-12 10:33:49 PDT
(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
Comment 12
2016-09-12 10:41:16 PDT
Created
attachment 288581
[details]
patch for landing I'll give some time before landing to allow Anders (or anybody else) comment.
WebKit Commit Bot
Comment 13
2016-09-12 10:43:15 PDT
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 14
2016-09-12 14:15:14 PDT
Comment on
attachment 288581
[details]
patch for landing Clearing flags on attachment: 288581 Committed
r205822
: <
http://trac.webkit.org/changeset/205822
>
WebKit Commit Bot
Comment 15
2016-09-12 14:15:20 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug