| Summary: | Web Inspector: Provide a way to have alternate inspector agents | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||
| Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | benjamin, burg, cmarcelo, commit-queue, graouts, joepeck, timothy, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 240163 [details]
[PATCH] Proposed Fix
Everything is up for naming suggestions. Feature guard, new file names, etc. This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`) Attachment 240163 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_alternate_backend_dispatcher_header.py:38: expected 2 blank lines, found 1 [pep8/E302] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_alternate_backend_dispatcher_header.py:87: whitespace before '}' [pep8/E202] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_alternate_backend_dispatcher_header.py:56: [AlternateBackendDispatcherHeaderGenerator.generate_output] Instance of 'AlternateBackendDispatcherHeaderGenerator' has no 'domains_to_generate' member [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_alternate_backend_dispatcher_header.py:58: [AlternateBackendDispatcherHeaderGenerator.generate_output] Instance of 'AlternateBackendDispatcherHeaderGenerator' has no 'generate_license' member [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:192: trailing whitespace [pep8/W291] [5]
Total errors found: 5 in 36 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 240215 [details]
[PATCH] Proposed Fix
Some minor fixes.
Attachment 240215 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_alternate_backend_dispatcher_header.py:57: [AlternateBackendDispatcherHeaderGenerator.generate_output] Instance of 'AlternateBackendDispatcherHeaderGenerator' has no 'domains_to_generate' member [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_alternate_backend_dispatcher_header.py:59: [AlternateBackendDispatcherHeaderGenerator.generate_output] Instance of 'AlternateBackendDispatcherHeaderGenerator' has no 'generate_license' member [pylint/E1101] [5]
Total errors found: 2 in 36 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 240217 [details]
[PATCH] Proposed Fix
Missed a minor change to a template and had to rebaseline tests with it.
Attachment 240217 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_alternate_backend_dispatcher_header.py:57: [AlternateBackendDispatcherHeaderGenerator.generate_output] Instance of 'AlternateBackendDispatcherHeaderGenerator' has no 'domains_to_generate' member [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_alternate_backend_dispatcher_header.py:59: [AlternateBackendDispatcherHeaderGenerator.generate_output] Instance of 'AlternateBackendDispatcherHeaderGenerator' has no 'generate_license' member [pylint/E1101] [5]
Total errors found: 2 in 36 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 240217 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=240217&action=review Looks okay to me. I'd like Brian to review this. > Source/JavaScriptCore/inspector/augmentable/InspectorAugmentingAgent.h:60 > + m_backendDispatcher.clear(); = nullptr? > Source/JavaScriptCore/inspector/scripts/tests/expected/type-declaration-aliased-primitive-type.json-result:53 > +}; > + > + > + > + > +} // namespace Inspector Why the huge number of empty lines? I know it is generated but still... Comment on attachment 240217 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=240217&action=review I am not keen on Augmentable as the new adjective here, since using a different backend dispatcher doesn't augment the protocol with new messages. As far as I can tell, it just allows a client to redirect callbacks. > Source/JavaScriptCore/ChangeLog:34 > + Provide the private APIs a client could use to add alternate agents. nit (here and in bug title): alternate backend dispatchers? > Source/JavaScriptCore/inspector/augmentable/InspectorAugmentingAgent.h:40 > +class InspectorAugmentingAgent : public InspectorAgentBase { AugmentableAgent? > Source/JavaScriptCore/inspector/augmentable/InspectorAugmentingAgent.h:45 > + , m_alternateDispatcher(WTF::move(alternateDispatcher)) This is a weird difference from existing frontend and backend dispatchers, which only attach/detach the dispatchers in the callbacks below (didCreate/willDestroyFrontendAndBackend). How is this AugmentingAgent used? Is it a hardcoded new base class for some of the JSC agents, or..? More generally, why can't the alternate dispatcher be passed as the InspectorBackendDispatcher instance to didCreateFrontendAndBackend? With the current setup, it is hardcoded per Agent instance. If it is actually hardcoded, then maybe this subclass can have virtual frontendDispatcher() and backendDispatcher() methods and setters instead of overriding didCreateFrontendAndBackend and having special code paths for the alternate dispatcher. > Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:242 > } AFAICT, the generated alternate dispatchers only take the callId. Is there any reason we shouldn't use the same parameters as the backend dispatcher methods and mark them as UNUSED_PARAM? I'm confused why alternate and backend dispatchers can't be combined, since there's no ObjC-specific types or anything that would prevent that. Comment on attachment 240217 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=240217&action=review >> Source/JavaScriptCore/inspector/augmentable/InspectorAugmentingAgent.h:45 >> + , m_alternateDispatcher(WTF::move(alternateDispatcher)) > > This is a weird difference from existing frontend and backend dispatchers, which only attach/detach the dispatchers in the callbacks below (didCreate/willDestroyFrontendAndBackend). > > How is this AugmentingAgent used? Is it a hardcoded new base class for some of the JSC agents, or..? > > More generally, why can't the alternate dispatcher be passed as the InspectorBackendDispatcher instance to didCreateFrontendAndBackend? With the current setup, it is hardcoded per Agent instance. If it is actually hardcoded, then maybe this subclass can have virtual frontendDispatcher() and backendDispatcher() methods and setters instead of overriding didCreateFrontendAndBackend and having special code paths for the alternate dispatcher. This template agent here just holds onto the AlternateDispatcher. It doesn't use it until a frontend and backend are created. When didCreateFrontendAndBackend happens, it creates the strongly typed frontend/backend objects for the domain, then adds the typed AlternateDispatcher to the backend. This seemed the simplest way to create a strongly typed InspectorFooBackendDispatcher with a strongly typed AlternateFooDispatcher. - "why can't the alternate dispatcher be passed as the InspectorBackendDispatcher instance to didCreateFrontendAndBackend?" By reusing the current InspectorFooBackendDispatchers we get to share two useful blocks. (1) The Foo::dispatch which routes the incoming message to the different Foo::commandName methods. (2) The protocol type checking of incoming arguments in each Foo::commandName. Nit: Now that I see this again, I think we can just remove m_frontendDispatcher entirely from this template. It only cares about backend dispatch on the domain. >> Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:242 >> } > > AFAICT, the generated alternate dispatchers only take the callId. Is there any reason we shouldn't use the same parameters as the backend dispatcher methods and mark them as UNUSED_PARAM? I'm confused why alternate and backend dispatchers can't be combined, since there's no ObjC-specific types or anything that would prevent that. The generated alternate dispatchers takes the callId and all the in params. They are very similar to the abstract interfaces our own agents have to implement. Our current BackendDispatcher abstract interface which we implement in WebKit: class JS_EXPORT_PRIVATE InspectorApplicationCacheBackendDispatcherHandler { public: virtual void getFramesWithManifests(ErrorString&, RefPtr<Inspector::Protocol::Array<Inspector::Protocol::ApplicationCache::FrameWithManifest>>& out_frameIds) = 0; virtual void enable(ErrorString&) = 0; virtual void getManifestForFrame(ErrorString&, const String& in_frameId, String* out_manifestURL) = 0; virtual void getApplicationCacheForFrame(ErrorString&, const String& in_frameId, RefPtr<Inspector::Protocol::ApplicationCache::ApplicationCache>& out_applicationCache) = 0; protected: virtual ~InspectorApplicationCacheBackendDispatcherHandler(); }; The new AlternateBackendDispatcher abstract interface: class AlternateInspectorApplicationCacheBackendDispatcher : public AlternateInspectorBackendDispatcher { public: virtual ~AlternateInspectorApplicationCacheBackendDispatcher() { } virtual void getFramesWithManifests(long callId) = 0; virtual void enable(long callId) = 0; virtual void getManifestForFrame(long callId, const String& in_frameId) = 0; virtual void getApplicationCacheForFrame(long callId, const String& in_frameId) = 0; }; The next phase in these patches is generating an ObjC++ concrete implementation of an AlternateBackendDispatcher which exposes an ObjC @protocol for these commands. The out parameters are not specified here since I did not want to enforce synchronous message handling. Although that is the case today, I don't think we rely on it. >> Source/JavaScriptCore/inspector/scripts/tests/expected/type-declaration-aliased-primitive-type.json-result:53 >> +} // namespace Inspector > > Why the huge number of empty lines? I know it is generated but still... In the actual case, there are always domains with commands so there won't be blank lines. I didn't want to both with handling these special cases that don't exist. > How is this AugmentingAgent used?
It is only used as a shim to stick an agent in the registry for a domain name with a concrete implementation of an alternate backend dispatcher.
Ultimately in the patches I have we will have an ObjC protocol for say a DOM handler:
@protocol DOMDomainHandler
// ObjC set of commands to implement to handle DOM Commands
@end
We will have a concrete ObjC++ implementation of the AlternateBackendDispatchers which call out to the ObjC protocol:
class ObjCInspectorDOMBackendDispatcher final : public AlternateInspectorDOMBackendDispatcher {
public:
ObjCInspectorDOMBackendDispatcher(id<DOMDomainHandler> handler) { m_delegate = handler; }
virtual void getDocument(long callId) override;
virtual void requestChildNodes(long callId, int in_nodeId, const int* in_depth) override;
...
private:
RetainPtr<id<RWIProtocolDOMDomainHandler>> m_delegate;
};
Then when a client supplies their ObjC implementation we just slot in an AugmentingAgent for the "DOM" domain.
- (void)setDOMHandler:(id<DOMDomainHandler>)handler
{
...
auto alternateDispatcher = std::make_unique<ObjCInspectorDOMBackendDispatcher>(handler);
auto alternateAgent = std::make_unique<InspectorAugmentingAgent<InspectorDOMFrontendDispatcher, InspectorDOMBackendDispatcher, AlternateInspectorDOMBackendDispatcher>>(ASCIILiteral("DOM"), WTF::move(alternateDispatcher));
_controller->agentRegistry().append(WTF::move(alternateAgent));
}
Comment on attachment 240217 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=240217&action=review > Source/JavaScriptCore/inspector/scripts/codegen/generate_alternate_backend_dispatcher_header.py:84 > + parameters.append('%s in_%s' % (Generator.type_string_for_unchecked_formal_in_parameter(_parameter), _parameter.parameter_name)) Does this use the same signature as the same methods with the "async" flag? I don't see any reason why it should differ, if all alternate dispatchers use async message handling. >>> Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:242 >>> } >> >> AFAICT, the generated alternate dispatchers only take the callId. Is there any reason we shouldn't use the same parameters as the backend dispatcher methods and mark them as UNUSED_PARAM? I'm confused why alternate and backend dispatchers can't be combined, since there's no ObjC-specific types or anything that would prevent that. > > The generated alternate dispatchers takes the callId and all the in params. They are very similar to the abstract interfaces our own agents have to implement. > > Our current BackendDispatcher abstract interface which we implement in WebKit: > > class JS_EXPORT_PRIVATE InspectorApplicationCacheBackendDispatcherHandler { > public: > virtual void getFramesWithManifests(ErrorString&, RefPtr<Inspector::Protocol::Array<Inspector::Protocol::ApplicationCache::FrameWithManifest>>& out_frameIds) = 0; > virtual void enable(ErrorString&) = 0; > virtual void getManifestForFrame(ErrorString&, const String& in_frameId, String* out_manifestURL) = 0; > virtual void getApplicationCacheForFrame(ErrorString&, const String& in_frameId, RefPtr<Inspector::Protocol::ApplicationCache::ApplicationCache>& out_applicationCache) = 0; > protected: > virtual ~InspectorApplicationCacheBackendDispatcherHandler(); > }; > > The new AlternateBackendDispatcher abstract interface: > > class AlternateInspectorApplicationCacheBackendDispatcher : public AlternateInspectorBackendDispatcher { > public: > virtual ~AlternateInspectorApplicationCacheBackendDispatcher() { } > virtual void getFramesWithManifests(long callId) = 0; > virtual void enable(long callId) = 0; > virtual void getManifestForFrame(long callId, const String& in_frameId) = 0; > virtual void getApplicationCacheForFrame(long callId, const String& in_frameId) = 0; > }; > > The next phase in these patches is generating an ObjC++ concrete implementation of an AlternateBackendDispatcher which exposes an ObjC @protocol for these commands. The out parameters are not specified here since I did not want to enforce synchronous message handling. Although that is the case today, I don't think we rely on it. Oh, that clears up a lot. I guess this is the best way to do that right now. Do you think we should make all message handling asynchronous (in the long term)? That would allow us to unify dispatchers. Re: "we probably don't rely on sync handling", until the separate inspector process I would have guessed "no". But now I would wager "yes" since messages are synchronously handled through async IPC. So, it probably works but we may have subtle races already in trunk. Comment on attachment 240217 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=240217&action=review >> Source/JavaScriptCore/inspector/augmentable/InspectorAugmentingAgent.h:40 >> +class InspectorAugmentingAgent : public InspectorAgentBase { > > AugmentableAgent? Going with AlternateDispatchableAgent. >> Source/JavaScriptCore/inspector/scripts/codegen/generate_alternate_backend_dispatcher_header.py:84 >> + parameters.append('%s in_%s' % (Generator.type_string_for_unchecked_formal_in_parameter(_parameter), _parameter.parameter_name)) > > Does this use the same signature as the same methods with the "async" flag? I don't see any reason why it should differ, if all alternate dispatchers use async message handling. They are close. Async interfaces still take an error string (probably should be removed) and a callback param. The callback allows for success/failure and holds the callId. >>>> Source/JavaScriptCore/inspector/scripts/codegen/generate_backend_dispatcher_implementation.py:242 >>>> } >>> >>> AFAICT, the generated alternate dispatchers only take the callId. Is there any reason we shouldn't use the same parameters as the backend dispatcher methods and mark them as UNUSED_PARAM? I'm confused why alternate and backend dispatchers can't be combined, since there's no ObjC-specific types or anything that would prevent that. >> >> The generated alternate dispatchers takes the callId and all the in params. They are very similar to the abstract interfaces our own agents have to implement. >> >> Our current BackendDispatcher abstract interface which we implement in WebKit: >> >> class JS_EXPORT_PRIVATE InspectorApplicationCacheBackendDispatcherHandler { >> public: >> virtual void getFramesWithManifests(ErrorString&, RefPtr<Inspector::Protocol::Array<Inspector::Protocol::ApplicationCache::FrameWithManifest>>& out_frameIds) = 0; >> virtual void enable(ErrorString&) = 0; >> virtual void getManifestForFrame(ErrorString&, const String& in_frameId, String* out_manifestURL) = 0; >> virtual void getApplicationCacheForFrame(ErrorString&, const String& in_frameId, RefPtr<Inspector::Protocol::ApplicationCache::ApplicationCache>& out_applicationCache) = 0; >> protected: >> virtual ~InspectorApplicationCacheBackendDispatcherHandler(); >> }; >> >> The new AlternateBackendDispatcher abstract interface: >> >> class AlternateInspectorApplicationCacheBackendDispatcher : public AlternateInspectorBackendDispatcher { >> public: >> virtual ~AlternateInspectorApplicationCacheBackendDispatcher() { } >> virtual void getFramesWithManifests(long callId) = 0; >> virtual void enable(long callId) = 0; >> virtual void getManifestForFrame(long callId, const String& in_frameId) = 0; >> virtual void getApplicationCacheForFrame(long callId, const String& in_frameId) = 0; >> }; >> >> The next phase in these patches is generating an ObjC++ concrete implementation of an AlternateBackendDispatcher which exposes an ObjC @protocol for these commands. The out parameters are not specified here since I did not want to enforce synchronous message handling. Although that is the case today, I don't think we rely on it. > > Oh, that clears up a lot. I guess this is the best way to do that right now. > > Do you think we should make all message handling asynchronous (in the long term)? That would allow us to unify dispatchers. > > Re: "we probably don't rely on sync handling", until the separate inspector process I would have guessed "no". But now I would wager "yes" since messages are synchronously handled through async IPC. So, it probably works but we may have subtle races already in trunk. I like the simplicity of our synchronous handling of most messages. It allows for the in and out parameters at the call site and guarantees request/response semantics by not "forgetting" to call a callback function. |
Provide a way to have an alternate inspector agents for a debuggable. The ultimate goal for this would be to let a JSContext owner provide its own set of domain agents, like a DOMAgent / CSSAgent. We should never allow replacing an existing agent, e.g. Runtime / Debugger, they should only be additional/augmentative. That would mean nothing needs to be done for a WebCore::Page, only JSContext inspection would be affected. The approach I'm thinking of taking is: - Expose an interface that an external client could use to - know when an inspector connects/disconnects (would be useful for someone with their own agents) - add an Agent with an alternate backend dispatcher (to provide new agents) - frontend channel (useful for events, or arbitrary backend -> frontend messages) - Give each typed InspectorFooBackendDispatcher an AlternateInspectorFooBackendDispatcher - During backend dispatch, if there is an alternate, use the alternate, otherwise do normal dispatch - this lets the generic backend dispatcher continue to do the per-method dispatching / input message type validation This will be behind a feature guard so there is no impact at all on those that do not have it enabled. As mentioned above only JSContext debugging is affected and such debugging is only currently available on Mac/iOS.