Bug 143681 - Provide SPI to allow changing whether JSContexts are remote debuggable by default
Summary: Provide SPI to allow changing whether JSContexts are remote debuggable by def...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-04-13 15:21 PDT by Joseph Pecoraro
Modified: 2015-04-16 09:34 PDT (History)
7 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (6.55 KB, patch)
2015-04-13 15:25 PDT, Joseph Pecoraro
timothy: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Improved Fix (3.95 KB, patch)
2015-04-14 18:03 PDT, Joseph Pecoraro
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-04-13 15:21:45 PDT
* 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.
Comment 1 Joseph Pecoraro 2015-04-13 15:22:36 PDT
<rdar://problem/20192167>
Comment 2 Joseph Pecoraro 2015-04-13 15:25:28 PDT
Created attachment 250678 [details]
[PATCH] Proposed Fix
Comment 3 Radar WebKit Bug Importer 2015-04-13 15:33:36 PDT
<rdar://problem/20525859>
Comment 4 Geoffrey Garen 2015-04-13 16:17:13 PDT
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.
Comment 5 Joseph Pecoraro 2015-04-13 16:34:58 PDT
(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
Comment 6 Geoffrey Garen 2015-04-14 13:50:51 PDT
> 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.
Comment 7 Joseph Pecoraro 2015-04-14 18:03:41 PDT
Created attachment 250769 [details]
[PATCH] Improved Fix

Better fix. This uses JSRemoteInspector.h and names the APIs JSRemoteInspectorGetInspectionEnabledByDefault and Set.
Comment 8 Joseph Pecoraro 2015-04-14 18:06:13 PDT
(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.
Comment 9 Darin Adler 2015-04-15 10:04:12 PDT
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.
Comment 10 mitz 2015-04-15 11:43:10 PDT
(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.
Comment 11 Joseph Pecoraro 2015-04-15 12:04:45 PDT
http://trac.webkit.org/changeset/182849
Comment 12 Darin Adler 2015-04-16 09:34:15 PDT
(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.