Bug 143681 – Provide SPI to allow changing whether JSContexts are remote debuggable by default

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
Provide SPI to allow changing whether JSContexts are remote debuggable by default

Some clients may have many JSContexts and wish to make them non-debuggable by default and enable debugging per-context. This should work well with auto-attach.

[Joseph Pecoraro]

<rdar://problem/20192167>

[Joseph Pecoraro]

Created attachment 250678 [details]
[PATCH] Proposed Fix

[Radar WebKit Bug Importer]

<rdar://problem/20525859>

[Geoffrey Garen]

Comment on attachment 250678 [details]
[PATCH] Proposed Fix

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

What make this API useful? If I want to disable the inspector (or enable it), can't I just call the setter when I create a context? What does "default" mean in this context?

> Source/JavaScriptCore/API/JSContextRefPrivate.h:103
> +JS_EXPORT bool JSGlobalContextGetRemoteInspectionEnabledByDefault() CF_AVAILABLE(10_11, 9_0);

It seems inappropriate for these to be JSGlobalContext functions when they do not take a JSGlobalContextRef as an argument and they do not pertain to a particular JSGlobalContextRef.

[Joseph Pecoraro]

(In reply to comment #4)
> Comment on attachment 250678 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250678&action=review
> 
> What make this API useful? If I want to disable the inspector (or enable
> it), can't I just call the setter when I create a context? What does
> "default" mean in this context?

Currently all JSContexts are debuggable by default. Toggling this state on/off is a private API. This is so that developers "just get" JSContext debugging when they build and run their application, without toggling something in code.

Also related, with auto-attach, when the JSContext is created it may be attached, offering no time to opt-out. We discussed alternatives to address the auto-attach case, like only run auto-attach the first time something is evaluated in the context, but having a setting for the default case would handle the original concern.


> > Source/JavaScriptCore/API/JSContextRefPrivate.h:103
> > +JS_EXPORT bool JSGlobalContextGetRemoteInspectionEnabledByDefault() CF_AVAILABLE(10_11, 9_0);
> 
> It seems inappropriate for these to be JSGlobalContext functions when they
> do not take a JSGlobalContextRef as an argument and they do not pertain to a
> particular JSGlobalContextRef.

That sounds reasonable. How do these names sound?

    JSGetRemoteInspectionEnabledByDefault
    JSSetRemoteInspectionEnabledByDefault

[Geoffrey Garen]

> Currently all JSContexts are debuggable by default. Toggling this state
> on/off is a private API. This is so that developers "just get" JSContext
> debugging when they build and run their application, without toggling
> something in code.

Who does the toggling?

> That sounds reasonable. How do these names sound?
> 
>     JSGetRemoteInspectionEnabledByDefault
>     JSSetRemoteInspectionEnabledByDefault

Maybe 

JSRemoteInspectionGetEnabledByDefault
JSRemoteInspectionSetEnabledByDefault

for slightly better namespacing.

[Joseph Pecoraro]

Created attachment 250769 [details]
[PATCH] Improved Fix

Better fix. This uses JSRemoteInspector.h and names the APIs JSRemoteInspectorGetInspectionEnabledByDefault and Set.

[Joseph Pecoraro]

(In reply to comment #6)
> > Currently all JSContexts are debuggable by default. Toggling this state
> > on/off is a private API. This is so that developers "just get" JSContext
> > debugging when they build and run their application, without toggling
> > something in code.
> 
> Who does the toggling?

Currently there is no public API to disable inspection of a JSContext or WebView. Everything is enabled by default. The mention to toggling in the comment above is that there is no toggle.

That said, there are client who want to be able to toggle, per-Context / per-WebView which are debuggable. Those are the situations these SPIs improve.

[Darin Adler]

Comment on attachment 250769 [details]
[PATCH] Improved Fix

Really confusing that an SPI header is in a directory called API. The only way to figure out this is API, not SPI, would be to look at the Xcode project. Would be nicer if we had followed the original WebKit convention and have SPI headers include the suffix Private and corresponding internal headers include the suffix Internal.

(In reply to comment #9)
> Comment on attachment 250769 [details]
> [PATCH] Improved Fix
> 
> Really confusing that an SPI header is in a directory called API. The only
> way to figure out this is API, not SPI, would be to look at the Xcode
> project. Would be nicer if we had followed the original WebKit convention
> and have SPI headers include the suffix Private and corresponding internal
> headers include the suffix Internal.

Was this ever the WebKit convention? See, for example, WebCache.h, dating back to 2006, or WebTextIterator.h, dating back to 2008.

I think the convention was, and should continue to be, that we only use the suffix Private if there is a corresponding public header, and we only use the suffix Internal if there is a corresponding public or private header.

[Joseph Pecoraro]

http://trac.webkit.org/changeset/182849

[Darin Adler]

(In reply to comment #10)
> I think the convention was, and should continue to be, that we only use the
> suffix Private if there is a corresponding public header, and we only use
> the suffix Internal if there is a corresponding public or private header.

Sounds sensible.

I suppose the real issue is that WebKit elsewhere does not have directories named API. I suppose that’s my real complaint. Imagine that we had a directory named “Public” that had a mix of private and public things in it. Since API and Public mean roughly the same thing, that’s what’s going on here.