Web Inspector: Agent commands do not actually return a promise when expected

• Component: Web Inspector
• Status: RESOLVED FIXED

[Brian Burg]

It seems that LayoutTests/inspector/protocol-return-promise.html is buggy, masking the error. It's only found now because Jono tried to .then on an agent command without .promise().

We don't actually return a promise from InspectorBackend.Command.create.(fn callable). That's because invokeWithArguments doesn't return a promise either- we need to convert the manual callback to a promise in (fn callable) like we do in InspectorBackend.Command.promise.

[Radar WebKit Bug Importer]

<rdar://problem/18960020>

[BJ Burg]

Created attachment 258716 [details]
WIP

[Timothy Hatcher]

Comment on attachment 258716 [details]
WIP

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

Looks good.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:154
> +        ++this._pendingResponsesCount;
> +        let sequenceId = this._lastSequenceId++;

Newline.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:196
> +        console.assert(this._callbackData.has(sequenceId), sequenceId, this._callbackData);
> +        let responseData = this._callbackData.take(sequenceId);

I like to put newlines after asserts like this. Or move the other let statements up.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:199
> +        var processingStartTime;

let

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:208
> +        var processingDuration = Date.now() - processingStartTime;

let

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:224
> +        for (var parameterName of command.replySignature)

let

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:448
> +        if (typeof callback === "function")
> +            return this._instance._backend._sendCommandToBackendWithCallback(instance, commandArguments, callback);
> +        else
> +            return this._instance._backend._sendCommandToBackendExpectingPromise(instance, commandArguments);

No need for the else.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:504
> +        if (callback)
> +            return instance._backend._sendCommandToBackendWithCallback(instance, parameters, callback);
> +        else
> +            return instance._backend._sendCommandToBackendExpectingPromise(instance, parameters);

No need for the else.

[Brian Burg]

Created attachment 258727 [details]
Proposed Fix

[Joseph Pecoraro]

Comment on attachment 258727 [details]
Proposed Fix

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

I haven't looked at this in detail yet, so leaving r?

> Source/WebInspectorUI/ChangeLog:63
> [phone]  Brian Burg  <[e-mail]>
> +
> +        Web Inspector: Agent commands do not actually return a promise when expected
> +        https://bugs.webkit.org/show_bug.cgi?id=138665

Double ChangeLog.

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:113
> +        return !this.element.querySelector("." + "indeterminate-progress-spinner");

This seems like an ideal place to actually have had the variable.

But, if we are dropping the variable, then no need for string concatenation here, just do ".foo" as one string.

[Timothy Hatcher]

Comment on attachment 258727 [details]
Proposed Fix

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

> Source/WebInspectorUI/ChangeLog:65
> [phone]  Brian Burg  <[e-mail]>
> +
> +        Web Inspector: Agent commands do not actually return a promise when expected
> +        https://bugs.webkit.org/show_bug.cgi?id=138665
> +
> +        Reviewed by NOBODY (OOPS!).

Double ChangeLog.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:95
> +        if (this._pendingResponses.size === 0)

! here.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:159
> +            responseData.sendRequestTime = Date.now();

Should we use performance.now()?

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:448
> +            return instance._backend._sendCommandToBackendWithCallback(instance, commandArguments, callback);
> +        else
> +            return instance._backend._sendCommandToBackendExpectingPromise(instance, commandArguments);

No else.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:504
> +            return instance._backend._sendCommandToBackendWithCallback(instance, parameters, callback);
> +        else
> +            return instance._backend._sendCommandToBackendExpectingPromise(instance, parameters);

No else.

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:113
> -        return !this.element.querySelector("." + WebInspector.IndeterminateProgressSpinner.StyleClassName);
> +        return !this.element.querySelector("." + "indeterminate-progress-spinner");

Land separate.

[BJ Burg]

Comment on attachment 258727 [details]
Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:159
>> +            responseData.sendRequestTime = Date.now();
> 
> Should we use performance.now()?

AFAIK, it's not enabled for Mavericks, so we'd want to polyfill it. A bug exists for this already: https://bugs.webkit.org/show_bug.cgi?id=135467

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:448
>> +            return instance._backend._sendCommandToBackendExpectingPromise(instance, commandArguments);
> 
> No else.

Er, the if branch should not return anything.

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:504
>> +            return instance._backend._sendCommandToBackendExpectingPromise(instance, parameters);
> 
> No else.

As above.

>>> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:113
>>> +        return !this.element.querySelector("." + "indeterminate-progress-spinner");
>> 
>> This seems like an ideal place to actually have had the variable.
>> 
>> But, if we are dropping the variable, then no need for string concatenation here, just do ".foo" as one string.
> 
> Land separate.

Filed: https://bugs.webkit.org/show_bug.cgi?id=147886

[Build Bot]

Comment on attachment 258727 [details]
Proposed Fix

Attachment 258727 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/45314

New failing tests:
inspector/protocol/inspector-backend-invocation-return-value.html

[Build Bot]

Created attachment 258730 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 258727 [details]
Proposed Fix

Attachment 258727 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/45345

New failing tests:
inspector/protocol/inspector-backend-invocation-return-value.html

[Build Bot]

Created attachment 258733 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[BJ Burg]

(In reply to comment #11)
> Created attachment 258733 [details]
> Archive of layout-test-results from ews101 for mac-mavericks
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-ews.
> Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

Oops, forgot to include changes to the test in the <body>. Will update before landing.

[BJ Burg]

Committed r188283: <http://trac.webkit.org/changeset/188283>