Closed Bug 1239276 Opened 4 years ago Closed 4 years ago

Makes DebuggerClient.connect return a promise

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 6 obsolete files)

In the continuation of bug 1227474, DebuggerClient.connect is one of the last, if not the last one? to not return a promise and requires creating temporary promise when called from a Task.
Attached patch patch v1 (obsolete) — Splinter Review
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8712158 - Attachment is obsolete: true
Attached patch patch v3 (obsolete) — Splinter Review
I replaced all the obvious occurances of client.connect
to use promises. So that we stop using the callback paradigm,
even when copy pasting existing code.

Also, in complex code, instead of having code with a lot of indentation like
method().then(() => {
  promise().then(() => {

  });
});
We will do:
method()
  .then(() => promise())
  .then(() => {

  });
It will make it easier to switch to Task if needed.
Attachment #8712225 - Flags: review?(jryans)
Attachment #8712173 - Attachment is obsolete: true
Comment on attachment 8712225 [details] [diff] [review]
patch v3

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

Great, thanks!

Please also clean up:

1. https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/connect/connect.js#83
2. https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox-process-window.js#36
3. All cases of "gClient.connect": https://dxr.mozilla.org/mozilla-central/search?q=gClient.connect+path%3Adevtools&redirect=false&case=true

::: devtools/shared/client/main.js
@@ +334,5 @@
>     *
>     * @param aOnConnected function
>     *        If specified, will be called when the greeting packet is
>     *        received from the debugging server.
>     */

Document the returned promise above.
Attachment #8712225 - Flags: review?(jryans) → feedback+
Attached patch document connect (obsolete) — Splinter Review
I'll merge this patch with the previous one.
Here is just the doc for DebuggerClient.connect.
Could you r+ patch v3 with this on top of it?
Attachment #8712759 - Flags: review?(jryans)
Attached patch Convert gClient.connect (obsolete) — Splinter Review
I'll also merge this patch once r+.
Waiting for green try before r?.
Attached patch convert gClient.connect - v2 (obsolete) — Splinter Review
Fixed the webapps test failure.
debugger and server tests would benefit some other refactoring,
as they have a lot of initialization code duplicated.
But I'm not sure it is a good idea to refactor this in this bug?
I think it would be great to first convert them to add_task.
Attachment #8712811 - Flags: review?(jryans)
Attachment #8712760 - Attachment is obsolete: true
Comment on attachment 8712811 [details] [diff] [review]
convert gClient.connect - v2

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

Great, thanks again for even more cleanup!

::: devtools/client/framework/connect/connect.js
@@ +77,5 @@
>    let response = yield clientConnect();
>    yield onConnectionReady(...response);
>  });
>  
>  function clientConnect() {

Inline this function where it's called.
Attachment #8712811 - Flags: review?(jryans) → review+
Comment on attachment 8712225 [details] [diff] [review]
patch v3

r+ given the additional patches.
Attachment #8712225 - Flags: feedback+ → review+
Attached patch patch v4Splinter Review
Squashed and rebased.
Attachment #8713256 - Flags: review+
Attachment #8712225 - Attachment is obsolete: true
Attachment #8712759 - Attachment is obsolete: true
Attachment #8712811 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/5cb797a911407cbddecd4f0c347ca0da251cff7a
Bug 1239276 - Add missing parenthese in test_protocol_async.js r=me. CLOSED TREE
https://hg.mozilla.org/mozilla-central/rev/f83987c2da14
https://hg.mozilla.org/mozilla-central/rev/5cb797a91140
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.