Bug 95649 – Web Inspector: CodeGeneratorInspector.py: support asynchronous command implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Peter Rybin]

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.

[Peter Rybin]

New implementation allows "asynch": <boolean> property in command declaration JSON.

[Peter Rybin]

Created attachment 161834 [details]
Patch

[WebKit Review Bot]

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

[Yury Semikhatsky]

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

[Peter Rybin]

Created attachment 162051 [details]
Patch

[Peter Rybin]

Created attachment 162054 [details]
Patch

[Peter Rybin]

Created attachment 162058 [details]
Patch

[Peter Rybin]

Created attachment 162062 [details]
Patch

[Peter Rybin]

Created attachment 162064 [details]
Diff for sample

[Peter Rybin]

> 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

[Andrey Adaikin]

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 {...}

[Peter Rybin]

> > 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.

[Yury Semikhatsky]

(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.

[Yury Semikhatsky]

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&

[Andrey Adaikin]

(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.

[Peter Rybin]

(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

[Peter Rybin]

Created attachment 162232 [details]
Patch

[Peter Rybin]

Created attachment 162238 [details]
Patch

[Vsevolod Vlasov]

Committed r127603: <http://trac.webkit.org/changeset/127603>

[Vsevolod Vlasov]

Comment on attachment 162238 [details]
Patch

Clearing r? since this was landed.