Alexandre Poirot [:ochameau] Assignee
	Description • 9 years ago

Sibling of bug 1239276, but instead of connect, targets close function.
I think it will be the last one to convert.

	AJ Kerrigan [:ajkerrigan]
	Comment 1 • 9 years ago

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 2 • 9 years ago

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.

	AJ Kerrigan [:ajkerrigan]
	Comment 3 • 9 years ago

Good call, thanks for the review. Those changes should be in now.

	AJ Kerrigan [:ajkerrigan]
	Comment 4 • 9 years ago

Comment on attachment 8716937 [details] [diff] [review]
Make DebuggerClient.close return a promise

Submitted this for re-review unnecessarily.

	AJ Kerrigan [:ajkerrigan]
	Comment 5 • 9 years ago

Comment on attachment 8716937 [details] [diff] [review]
Make DebuggerClient.close return a promise

Fixed items from :jryans conditional review+

	AJ Kerrigan [:ajkerrigan]
	Comment 6 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6960b4ecbba9

	AJ Kerrigan [:ajkerrigan]
	Comment 7 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9519d1665725

	AJ Kerrigan [:ajkerrigan]
	Comment 8 • 8 years ago

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.

	Alexandre Poirot [:ochameau] Assignee
	Comment 9 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c60bf0e58c04

	Alexandre Poirot [:ochameau] Assignee
	Comment 11 • 8 years ago

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?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 12 • 8 years ago
mozreview-review

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.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 14 • 8 years ago

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

	Pulsebot
	Comment 15 • 8 years ago

Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/52851295b34f
Make DebuggerClient.close return a Promise. r=jryans

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/52851295b34f