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.
<rdar://problem/18960020>
Created attachment 258716 [details] WIP
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.
Created attachment 258727 [details] Proposed Fix
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 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 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 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
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 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
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
(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.
Committed r188283: <http://trac.webkit.org/changeset/188283>