Bug 138665 - Web Inspector: Agent commands do not actually return a promise when expected
Summary: Web Inspector: Agent commands do not actually return a promise when expected
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks: InspectorProtocol InspectorTest
  Show dependency treegraph
 
Reported: 2014-11-12 13:29 PST by Brian Burg
Modified: 2015-08-11 13:15 PDT (History)
8 users (show)

See Also:


Attachments
WIP (14.07 KB, patch)
2015-08-11 07:36 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (31.54 KB, patch)
2015-08-11 11:08 PDT, Brian Burg
timothy: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (631.62 KB, application/zip)
2015-08-11 11:45 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-mavericks (542.48 KB, application/zip)
2015-08-11 11:58 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2014-11-12 13:29:28 PST
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.
Comment 1 Radar WebKit Bug Importer 2014-11-12 13:29:42 PST
<rdar://problem/18960020>
Comment 2 BJ Burg 2015-08-11 07:36:16 PDT
Created attachment 258716 [details]
WIP
Comment 3 Timothy Hatcher 2015-08-11 11:06:22 PDT
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.
Comment 4 Brian Burg 2015-08-11 11:08:21 PDT
Created attachment 258727 [details]
Proposed Fix
Comment 5 Joseph Pecoraro 2015-08-11 11:13:34 PDT
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
> +2015-08-11  Brian Burg  <bburg@apple.com>
> +
> +        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.
Comment 6 Timothy Hatcher 2015-08-11 11:15:22 PDT
Comment on attachment 258727 [details]
Proposed Fix

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

> Source/WebInspectorUI/ChangeLog:65
> +2015-08-11  Brian Burg  <bburg@apple.com>
> +
> +        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.
Comment 7 BJ Burg 2015-08-11 11:31:32 PDT
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
Comment 8 Build Bot 2015-08-11 11:45:41 PDT
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
Comment 9 Build Bot 2015-08-11 11:45:43 PDT
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
Comment 10 Build Bot 2015-08-11 11:58:53 PDT
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
Comment 11 Build Bot 2015-08-11 11:58:55 PDT
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
Comment 12 BJ Burg 2015-08-11 12:52:22 PDT
(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.
Comment 13 BJ Burg 2015-08-11 13:15:40 PDT
Committed r188283: <http://trac.webkit.org/changeset/188283>