[webkit-reviews] review denied: [Bug 88759] Remove the function ScriptDebugServer::supportsNativeBreakpoints() : [Attachment 147481] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Peter Wang
<peter.wang at torchmobile.com.cn>'s request for review:
Bug 88759: Remove the function ScriptDebugServer::supportsNativeBreakpoints()
https://bugs.webkit.org/show_bug.cgi?id=88759

Attachment 147481: Patch
https://bugs.webkit.org/attachment.cgi?id=147481&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.