The generated InspectorBackendDispatcher interfaces should allow asynchronous implementations. Currently the interfaces require that the method must provide command response on return (i.e. synchronously). This could be excessive: some operations imply long work time and the protocol framework allows late response alright. There should be a way to specify that the particular command should allow asynchronous implementation and the generated interface should provide a callback object instead of output parameters.
New implementation allows "asynch": <boolean> property in command declaration JSON.
Created attachment 161834 [details] Patch
Comment on attachment 161834 [details] Patch Attachment 161834 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13732299 New failing tests: http/tests/inspector/appcache/appcache-iframe-manifests.html http/tests/inspector/injected-script-for-origin.html http/tests/inspector/extensions-network-redirect.html http/tests/inspector/change-iframe-src.html http/tests/inspector/modify-cross-domain-rule.html http/tests/inspector/resource-har-conversion.html http/tests/inspector/console-resource-errors.html http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html http/tests/inspector/extensions-headers.html http/tests/inspector-enabled/injected-script-discard.html http/tests/inspector/console-cross-origin-iframe-logging.html http/tests/inspector-enabled/contentSecurityPolicy-blocks-setTimeout.html http/tests/inspector/extensions-useragent.html http/tests/inspector/console-cd-completions.html http/tests/inspector/compiler-script-mapping.html http/tests/inspector-enabled/dom-storage-open.html http/tests/inspector/compiler-source-mapping-debug.html http/tests/inspector/web-socket-frame-error.html http/tests/inspector/console-xhr-logging.html http/tests/inspector/console-xhr-logging-async.html http/tests/inspector/console-cd.html http/tests/inspector/resource-har-headers.html http/tests/inspector/network-preflight-options.html http/tests/inspector/inspect-iframe-from-different-domain.html http/tests/inspector-enabled/console-log-before-frame-navigation.html http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval.html http/tests/inspector/resource-parameters.html http/tests/inspector-enabled/database-open.html
Comment on attachment 161834 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161834&action=review Please fix the failing tests and attach an example diff of the generated code. > Source/WebCore/inspector/CodeGeneratorInspector.py:1871 > + void sendFailure(ErrorString); const ErrorString& > Source/WebCore/inspector/CodeGeneratorInspector.py:1874 > + virtual ~CallbackBase(); style: destructor should go right after the constructor > Source/WebCore/inspector/CodeGeneratorInspector.py:1876 > + void sendIfActive(PassRefPtr<InspectorObject> partialMessage, ErrorString invocationError); const ErrorString& also partialMessage can be just InspectorObject* > Source/WebCore/inspector/CodeGeneratorInspector.py:1953 > + void sendResponse(long callId, PassRefPtr<InspectorObject> result, ErrorString invocationError); |result| type should be InspectorObject* instead of PassRefPtr<InspectorObject>. > Source/WebCore/inspector/CodeGeneratorInspector.py:2203 > + sendIfActive(PassRefPtr<InspectorObject>(), error); sendIfActive(0, error) > Source/WebCore/inspector/CodeGeneratorInspector.py:2208 > + return !m_alreadySent && m_backendImpl->isActive(); In theory we may end up sending response for a long-running command to a different frontend. Consider the following sequence: FE1 attaches and sends request1, then FE1 is detached and FE2 is attached, now reponse for the request1 is sent. > Source/WebCore/inspector/CodeGeneratorInspector.py:2903 > + None, decl_parameter_list, style nit: the line should be aligned by ( above. > Source/WebCore/inspector/CodeGeneratorInspector.py:2949 > + setter_argument) nit: weir alignment
Created attachment 162051 [details] Patch
Created attachment 162054 [details] Patch
Created attachment 162058 [details] Patch
Created attachment 162062 [details] Patch
Created attachment 162064 [details] Diff for sample
> Please fix the failing tests and attach an example diff of the generated code. Done and done. > > Source/WebCore/inspector/CodeGeneratorInspector.py:1871 > > + void sendFailure(ErrorString); > const ErrorString& Done > > Source/WebCore/inspector/CodeGeneratorInspector.py:1874 > > + virtual ~CallbackBase(); > style: destructor should go right after the constructor Done > > Source/WebCore/inspector/CodeGeneratorInspector.py:1876 > > + void sendIfActive(PassRefPtr<InspectorObject> partialMessage, ErrorString invocationError); > const ErrorString& also partialMessage can be just InspectorObject* Done. As to pointer, the farther API requires RefPtr style, thus making it RefPtr here. > > Source/WebCore/inspector/CodeGeneratorInspector.py:1953 > > + void sendResponse(long callId, PassRefPtr<InspectorObject> result, ErrorString invocationError); > |result| type should be InspectorObject* instead of PassRefPtr<InspectorObject>. The farther API requires RefPtr style, thus making it RefPtr here. > > Source/WebCore/inspector/CodeGeneratorInspector.py:2203 > > + sendIfActive(PassRefPtr<InspectorObject>(), error); > sendIfActive(0, error) Done > > Source/WebCore/inspector/CodeGeneratorInspector.py:2208 > > + return !m_alreadySent && m_backendImpl->isActive(); > In theory we may end up sending response for a long-running command to a different frontend. Consider the following sequence: FE1 attaches and sends request1, then FE1 is detached and FE2 is attached, now reponse for the request1 is sent. I believe the instance of InspectorBackendDispatcher (AKA backendImpl) is never reused in the next sessions. After FE1 is detached, isActive returns false, so the late response will be ignored and dropped and FE2 will never see it. > > Source/WebCore/inspector/CodeGeneratorInspector.py:2903 > > + None, decl_parameter_list, > style nit: the line should be aligned by ( above. Done > > Source/WebCore/inspector/CodeGeneratorInspector.py:2949 setter_argument) > nit: weir alignment Done
Comment on attachment 162062 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162062&action=review > Source/WebCore/inspector/CodeGeneratorInspector.py:1873 > + bool isActive(); bool isActive() const; > Source/WebCore/inspector/CodeGeneratorInspector.py:1956 > + bool isActive() { return m_inspectorFrontendChannel; } bool isActive() const {...}
> > Source/WebCore/inspector/CodeGeneratorInspector.py:1873 > > + bool isActive(); > bool isActive() const; > > Source/WebCore/inspector/CodeGeneratorInspector.py:1956 > > + bool isActive() { return m_inspectorFrontendChannel; } > bool isActive() const {...} Can you justify this with a positive reason? I don't see a lot of const/mutable semantics for InspectorBackendDispatcher class.
(In reply to comment #10) > > Please fix the failing tests and attach an example diff of the generated code. > > > Source/WebCore/inspector/CodeGeneratorInspector.py:1953 > > > + void sendResponse(long callId, PassRefPtr<InspectorObject> result, ErrorString invocationError); > > |result| type should be InspectorObject* instead of PassRefPtr<InspectorObject>. > The farther API requires RefPtr style, thus making it RefPtr here. > I would argue that they should be changed as well:) > > > Source/WebCore/inspector/CodeGeneratorInspector.py:2208 > > > + return !m_alreadySent && m_backendImpl->isActive(); > > In theory we may end up sending response for a long-running command to a different frontend. Consider the following sequence: FE1 attaches and sends request1, then FE1 is detached and FE2 is attached, now reponse for the request1 is sent. > You are right. I missed that we create new instance of the dispatcher when FE connects.
Comment on attachment 162062 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162062&action=review > Source/WebCore/inspector/CodeGeneratorInspector.py:1955 > + void sendResponse(long callId, PassRefPtr<InspectorObject> result, ErrorString invocationError); const ErrorString& > Source/WebCore/inspector/CodeGeneratorInspector.py:2058 > +void InspectorBackendDispatcherImpl::sendResponse(long callId, PassRefPtr<InspectorObject> result, ErrorString invocationError) const ErrorString&
(In reply to comment #12) > > > Source/WebCore/inspector/CodeGeneratorInspector.py:1873 > > > + bool isActive(); > > bool isActive() const; > > > Source/WebCore/inspector/CodeGeneratorInspector.py:1956 > > > + bool isActive() { return m_inspectorFrontendChannel; } > > bool isActive() const {...} > > Can you justify this with a positive reason? I don't see a lot of const/mutable semantics for InspectorBackendDispatcher class. Because it's a constant getter by it's nature that will not modify the class's internal state? :) Why not say about it to the compiler and a reader of the code? In fact, IMO, more logical question would be "Can you justify NOT using const here with a positive reason?" :) Can you? Anyway, I'm not insisting.
(In reply to comment #14) > (From update of attachment 162062 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162062&action=review > > > Source/WebCore/inspector/CodeGeneratorInspector.py:1955 > > + void sendResponse(long callId, PassRefPtr<InspectorObject> result, ErrorString invocationError); > const ErrorString& Done > > Source/WebCore/inspector/CodeGeneratorInspector.py:2058 > > +void InspectorBackendDispatcherImpl::sendResponse(long callId, PassRefPtr<InspectorObject> result, ErrorString invocationError) > const ErrorString& Done
Created attachment 162232 [details] Patch
Created attachment 162238 [details] Patch
Committed r127603: <http://trac.webkit.org/changeset/127603>
Comment on attachment 162238 [details] Patch Clearing r? since this was landed.