Closed Bug 1243452 Opened 4 years ago Closed 3 years ago

Make DebuggerClient.close return a promise

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 2 obsolete files)

Sibling of bug 1239276, but instead of connect, targets close function.
I think it will be the last one to convert.
Assignee: nobody → ajkerrigan
Status: NEW → ASSIGNED
Since this was a sibling of Bug 1243452, I've tried to follow the same approach
from that bug.

The primary update is in devtools/shared/client/main.js:

- Return a promise from DebuggerClient.close()
- Document the promise return and mark the callback parameter as deprecated

From there, I did some searches to find and update all calls to the close()
method which passed a callback parameter.
Attachment #8716737 - Flags: review?(jryans)
Comment on attachment 8716737 [details] [diff] [review]
Make DebuggerClient.close return a promise

Review of attachment 8716737 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks for working on this!  I believe you found all the right cases to clean up.

::: devtools/shared/client/main.js
@@ +368,5 @@
>     * @param aOnClosed function
>     *        If specified, will be called when the debugging connection
> +   *        has been closed. This parameter is deprecated - please use
> +   *        the returned Promise.
> +   *

Nit: We typically don't use a blank line between @param and @return

@@ +395,5 @@
>      // there is no need to detach client
>      // as we won't be able to send any message.
>      if (this._closed) {
>        cleanup();
> +      resolve();

We should `return deferred.promise;` after this, so the rest of the method is skipped as before.
Attachment #8716737 - Flags: review?(jryans) → review+
Good call, thanks for the review. Those changes should be in now.
Attachment #8716937 - Flags: review?(jryans)
Comment on attachment 8716937 [details] [diff] [review]
Make DebuggerClient.close return a promise

Submitted this for re-review unnecessarily.
Attachment #8716937 - Flags: review?(jryans)
Attachment #8716737 - Attachment is obsolete: true
Comment on attachment 8716937 [details] [diff] [review]
Make DebuggerClient.close return a promise

Fixed items from :jryans conditional review+
Attachment #8716937 - Flags: review+
My changes passed review a while back, but try runs kept showing too many instances of intermittent failures. I suspect the problem(s) are related to promise handling in tests that use DebuggerClient.close, but despite a bunch of test refactoring I wasn't able to get try runs to be green at a level I was comfortable with.

I don't have much time available lately to work on issues like this, and I think that's going to be the way for the foreseeable future. I'm leaving this update and unassigning in case someone else wants to pick it up before I get back to it.
Assignee: ajkerrigan → nobody
Status: ASSIGNED → NEW
Assignee: nobody → poirot.alex
Blocks: 1299503
A bunch of code already expect it to return a Promise :/
https://dxr.mozilla.org/mozilla-central/search?q=yield+client.close&redirect=false

Also, in my patch, I'm also converting all callsites to use the promise form,
while keeping the callback argument. I did that while thinking about addons, but may be that's useless?
Comment on attachment 8786970 [details]
Bug 1243452 - Make DebuggerClient.close return a Promise.

https://reviewboard.mozilla.org/r/75812/#review73784

Looks good, thanks for doing this cleanup!

::: devtools/server/tests/browser/head.js:90
(Diff revision 1)
>  /**
>   * Close a debugger client's connection.
>   * @param {DebuggerClient}
>   * @return {Promise} Resolves when the connection is closed.
>   */
>  function closeDebuggerClient(client) {

Seems like this can be inlined.

::: devtools/shared/client/main.js:353
(Diff revision 1)
>      this._transport.ready();
>      return deferred.promise;
>    },
>  
>    /**
>     * Shut down communication with the debugging server.

Update comment to note it returns a promise.
Attachment #8786970 - Flags: review?(jryans) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #11)
> A bunch of code already expect it to return a Promise :/
> https://dxr.mozilla.org/mozilla-central/search?q=yield+client.
> close&redirect=false
> 
> Also, in my patch, I'm also converting all callsites to use the promise form,
> while keeping the callback argument. I did that while thinking about addons,
> but may be that's useless?

Yes, I think you should preserve the callback argument for add-ons.  That seems best to me.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/52851295b34f
Make DebuggerClient.close return a Promise. r=jryans
https://hg.mozilla.org/mozilla-central/rev/52851295b34f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.