Bug 169089 - Add WebKit2 hooks to notify the VM that the user has requested a debugger break.
Summary: Add WebKit2 hooks to notify the VM that the user has requested a debugger break.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-02 12:19 PST by Mark Lam
Modified: 2017-03-02 18:41 PST (History)
8 users (show)

See Also:

Attachments
proposed patch. (20.83 KB, patch)
2017-03-02 14:35 PST, Mark Lam 		no flags Details | Formatted Diff | Diff
proposed patch. (21.49 KB, patch)
2017-03-02 15:47 PST, Mark Lam 		thorton: review+
Details | Formatted Diff | Diff
patch for landing. (21.80 KB, patch)
2017-03-02 16:14 PST, Mark Lam 		no flags Details | Formatted Diff | Diff
patch for landing: applied one missing change. (21.78 KB, patch)
2017-03-02 16:24 PST, Mark Lam 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2017-03-02 12:19:02 PST 
Patch coming.
Comment 1 Mark Lam 2017-03-02 14:35:44 PST 
Created attachment 303243 [details]
proposed patch.

Let's try this on the EWS.
Comment 2 Mark Lam 2017-03-02 15:47:31 PST 
Created attachment 303261 [details]
proposed patch.
Comment 3 Joseph Pecoraro 2017-03-02 16:00:40 PST 
Comment on attachment 303261 [details]
proposed patch.

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

Patch looks fine to me. It should get a WebKit2 owner approval.

> Source/WebKit2/CMakeLists.txt:622
> +    UIProcess/WebInspectorInterruptDispatcher.messages.in

This should be down by: WebProcess/WebPage/WebInspector.messages.in it is not UIProcess. (Should fix GTK)

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:6460
> +				FEE43FD21E67AFC60077D6D1 /* WebInspectorInterruptDispatcher.messages.in */,
> +				FEE43FD01E67AFC60077D6D1 /* WebInspectorInterruptDispatcher.cpp */,
> +				FEE43FD11E67AFC60077D6D1 /* WebInspectorInterruptDispatcher.h */,

Nit: Sort order, the messages file should be after the .h in the Xcode sidebar.

> Source/WebKit2/WebProcess/WebPage/WebInspectorInterruptDispatcher.messages.in:24
> +    NotifyNeedDebuggerBreak(uint64_t pageID)

pageID is not used and doesn't seem like it will be used anytime soon, so lets drop it.
Comment 4 Tim Horton 2017-03-02 16:04:02 PST 
Comment on attachment 303261 [details]
proposed patch.

r+ if you fix joe's things
Comment 5 Joseph Pecoraro 2017-03-02 16:08:58 PST 
Comment on attachment 303261 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/VM.cpp:960
> +#if OS(DARWIN)

Is there something special about this that needs the OS(DARWIN) guard?
Comment 6 Joseph Pecoraro 2017-03-02 16:10:46 PST 
Comment on attachment 303261 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/VM.cpp:960
>> +#if OS(DARWIN)
> 
> Is there something special about this that needs the OS(DARWIN) guard?

Oh, probably for the getpid(). WTF has getCurrentProcessID() which should work for all ports.
Comment 7 Mark Lam 2017-03-02 16:14:09 PST 
Created attachment 303263 [details]
patch for landing.

Thanks for the reviews.  I've applied all the requested changes.  Will wait for the EWS bots to be happy before landing.
Comment 8 Mark Lam 2017-03-02 16:24:57 PST 
Created attachment 303267 [details]
patch for landing: applied one missing change.
Comment 9 WebKit Commit Bot 2017-03-02 18:41:48 PST 
Comment on attachment 303267 [details]
patch for landing: applied one missing change.

Clearing flags on attachment: 303267

Committed r213338: <http://trac.webkit.org/changeset/213338>
Comment 10 WebKit Commit Bot 2017-03-02 18:41:53 PST 
All reviewed patches have been landed.  Closing bug.