Bug 88759 – Remove the function ScriptDebugServer::supportsNativeBreakpoints()

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Peter Wang]

We should remove the function ScriptDebugServer::supportsNativeBreakpoints(), since both V8 and JSC support it now. And the common code of Inspector should also be removed.

[Peter Wang]

Created attachment 147481 [details]
Patch

[Pavel Feldman]

Comment on attachment 147481 [details]
Patch

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

> Source/WebCore/inspector/Inspector-1.0.json:-2026
> -                "name": "supportsNativeBreakpoints",

Please don't change this, shipped versions are retained as is. You are removing the hidden method from the protocol, so versioning check will pass.

> Source/WebCore/inspector/front-end/NetworkPanel.js:-141
> -        if (Capabilities.nativeInstrumentationEnabled)

Oh, I missed this one. It sounds like we imply both:

1) ability to stop from within native code and
2) ability to collect stack traces from native code

by native instrumentation. I don't think (2) has been addressed for JSC. So there will be no stack traces for initiator below.


Before stacks are there, I'd suggest to introduce a preference Preferences.displayInitiator = false in JSC and override it to true in the WebKit/chromium/src/js/DevTools.js. It will be used in this file.

> Source/WebKit/blackberry/WebCoreSupport/inspectorBB.js:-26
> -    Preferences.nativeInstrumentationEnabled = true;

Nit: I don't think you should override preferences for your port at all. We do it in chromium for browser-specific features (see the list in the DevTools.js). Most of the items in your list are already capabilities, so these values are already not taken into account.

[Peter Wang]

(In reply to comment #2)
> (From update of attachment 147481 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147481&action=review
> 
> > Source/WebCore/inspector/Inspector-1.0.json:-2026
> > -                "name": "supportsNativeBreakpoints",
> 
> Please don't change this, shipped versions are retained as is. You are removing the hidden method from the protocol, so versioning check will pass.
I see, ok.
> > Source/WebCore/inspector/front-end/NetworkPanel.js:-141
> > -        if (Capabilities.nativeInstrumentationEnabled)
> 
> Oh, I missed this one. It sounds like we imply both:
> 
> 1) ability to stop from within native code and
> 2) ability to collect stack traces from native code
> 
> by native instrumentation. I don't think (2) has been addressed for JSC. So there will be no stack traces for initiator below.
> 
> 
> Before stacks are there, I'd suggest to introduce a preference Preferences.displayInitiator = false in JSC and override it to true in the WebKit/chromium/src/js/DevTools.js. It will be used in this file.

I did some investigation of it. Seems JSC already has the code for stack traces (JSC::createScriptCallStack), but works incorrectly now. Now the "Initiator" items on my Inspector just show a default value: "Other". 
So could I just remove the "nativeInstrumentationEnabled" here, and then try to make JSC work correctly? Maybe that way seems more clear. And also, if I introduce a preference item, maybe, I'll face challenges from another people.  

> > Source/WebKit/blackberry/WebCoreSupport/inspectorBB.js:-26
> > -    Preferences.nativeInstrumentationEnabled = true;
> 
> Nit: I don't think you should override preferences for your port at all. We do it in chromium for browser-specific features (see the list in the DevTools.js). Most of the items in your list are already capabilities, so these values are already not taken into account.

[Peter Wang]

(In reply to comment #2)
> (From update of attachment 147481 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147481&action=review
> Nit: I don't think you should override preferences for your port at all. We do it in chromium for browser-specific features (see the list in the DevTools.js). Most of the items in your list are already capabilities, so these values are already not taken into account.
ok.

[Pavel Feldman]

> I did some investigation of it. Seems JSC already has the code for stack traces (JSC::createScriptCallStack), but works incorrectly now. Now the "Initiator" items on my Inspector just show a default value: "Other". 
> So could I just remove the "nativeInstrumentationEnabled" here, and then try to make JSC work correctly? Maybe that way seems more clear. And also, if I introduce a preference item, maybe, I'll face challenges from another people.  

See https://bugs.webkit.org/show_bug.cgi?id=40118 for the bug on the stack trace collecting. So someone is already making it work for JSC.

I think property is fine (better than "Other" all over the place). Could you elaborate on the "I'll face challenges from another people."?

[Peter Wang]

(In reply to comment #5)
> > I did some investigation of it. Seems JSC already has the code for stack traces (JSC::createScriptCallStack), but works incorrectly now. Now the "Initiator" items on my Inspector just show a default value: "Other". 
> > So could I just remove the "nativeInstrumentationEnabled" here, and then try to make JSC work correctly? Maybe that way seems more clear. And also, if I introduce a preference item, maybe, I'll face challenges from another people.  
> 
> See https://bugs.webkit.org/show_bug.cgi?id=40118 for the bug on the stack trace collecting. So someone is already making it work for JSC.
> 
> I think property is fine (better than "Other" all over the place). Could you elaborate on the "I'll face challenges from another people."?

Never mind. I'm just not sure if someone will be wondering why there is a new preference item appeared. The way to resolve it is ok for me.

[Peter Wang]

Created attachment 147563 [details]
Patch

[WebKit Review Bot]

Comment on attachment 147563 [details]
Patch

Clearing flags on attachment: 147563

Committed r120711: <http://trac.webkit.org/changeset/120711>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.