1227978 - Cleanup codebase now that many DebuggerClient methods returns promises

Status: VERIFIED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

Thanks to bug 1227474, most DebuggerClient method now returns a promise.
We should get rid of all temporary promises and various workarounds to simplify our codebase.

Comment 1

[Alexandre Poirot [:ochameau]]

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

Comment 2

[Alexandre Poirot [:ochameau]]

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

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

Global patch that refactor most client APIs.
We can get rid of the rdpInvoke junk we used to workaround the fact that Client APIs wasn't returning promises...

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: debugger client - v1

Another patch focused on debugger actors that uses request() method manually
and don't resolve any value whereas they pass one to callback function.

I still miss some modification somewhere as the debugger is broken with these two patches.

Comment 5

[Alexandre Poirot [:ochameau]]

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

Comment 6

[Alexandre Poirot [:ochameau]]

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

Comment 7

[Alexandre Poirot [:ochameau]]

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

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Simplify usages of Debuggerclient APIs now that all requests returns promises.

James, here is a set a patches to cleanup DebuggerClient side.
This is a refactoring, so it isn't really urgent patch,
but feel free to redirect to :jryans.
I thought you would be a good reviewer as there was a lot of modification around debugger clients.

This first patch simply refactor random code to benefit from bug 1227474.
Various codes were crafting a temporary promise to work around the fact that client APIs
weren't returning a promise. Thanks to bug 1227474, they mostly all do.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Convert all DebuggerClient request methods to return promises

This patch piles up on top of bug 1227474. I didn't realized many other client methods
weren't returning promises and instead only support callbacks.
This patch convert them all to also return a promise.
(We could followup to remove callback support and cleanup this code?)

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Remove unecessary usages of promiseInvoke, rdpInvoke

And finally, a patch to remove the various helpers introduced over time
that used to workaround the fact that client APIs weren't promise-proof.

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8702569 [details] [diff] [review]
Convert all DebuggerClient request methods to return promises

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

::: devtools/client/debugger/test/mochitest/browser_dbg_interrupts.js
@@ +79,5 @@
> +      })
> +      .then(() => {
> +        let p = ensureThreadClientState(gPanel, "resumed");
> +        gThreadClient.resume();
> +        return p;

I had to listen for the event first as resume() now only resolves once the request is fully processed.

::: devtools/client/debugger/test/mochitest/browser_dbg_scripts-switching-03.js
@@ +24,5 @@
>        return finished;
>      }
>  
>      function testDebuggerLoadingError() {
> +      ok(gEditor.getText().includes(gL10N.getFormatStr("errorLoadingText2", "No such actor for ID: fake.actor")),

This modification is related to debugger-view one.
I have hard time to understand existing code as it has been abstracted and it is hard to know where does the data comes from.
But it looks like textInfo.error isn't what we expected. It was a error message and not a URL.

Comment 12

[James Long (:jlongster)]

Comment on attachment 8702567 [details] [diff] [review]
Simplify usages of Debuggerclient APIs now that all requests returns promises.

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

sweet!

::: devtools/client/performance/legacy/front.js
@@ +356,5 @@
>      }
>  
>      let systemDeferred = promise.defer();
> +    let form = yield this._client.listTabs();
> +    let systemHost = yield getDeviceFront(this._client, form).getDescription();

Looking at the previous code, seems like `getDeviceFront` isn't async and doesn't need a `yield`.

Comment 13

[James Long (:jlongster)]

Comment on attachment 8702569 [details] [diff] [review]
Convert all DebuggerClient request methods to return promises

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

Looks sane. Are you planning on removing the callback parameter soon?

::: devtools/shared/client/main.js
@@ +453,5 @@
>        if (!aResponse.error) {
>          tabClient = new TabClient(this, aResponse);
>          this.registerClient(tabClient);
>        }
>        aOnResponse(aResponse, tabClient);

How come `aOnResponse` still exists here? Can't we just remove that now that it returns a promise, or are you wanting to slowly migrate usages of this and remove it later?

@@ +478,5 @@
>  
>        let workerClient = new WorkerClient(this, aResponse);
>        this.registerClient(workerClient);
>        aOnResponse(aResponse, workerClient);
> +      return [aResponse, workerClient];

Ok, seeing `aOnResponse` here confirms my above comment. You probably want to support both interfaces for now. Can you file a bug to remove the callback parameters as well at some point?

Comment 14

[James Long (:jlongster)]

Comment on attachment 8702569 [details] [diff] [review]
Convert all DebuggerClient request methods to return promises

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

Looks sane. Are you planning on removing the callback parameter soon?

EDIT: oops, forgot to review the rest of the files. Just a few minor nits.

::: devtools/client/debugger/test/mochitest/browser_dbg_interrupts.js
@@ +79,5 @@
> +      })
> +      .then(() => {
> +        let p = ensureThreadClientState(gPanel, "resumed");
> +        gThreadClient.resume();
> +        return p;

I'd rather you keep the previous code, but change it to `ensureThreadClientState(gPanel, "attached")`. This is super confusing, but that will make sure that the thread is fully resumed and has the state "attached". If you pass in "resumed" it will wait for that *event* called resumed.

@@ +111,5 @@
> +      })
> +      .then(() => {
> +        let p = ensureThreadClientState(gPanel, "resumed");
> +        gThreadClient.resume();
> +        return p;

Ditto here.

::: devtools/client/debugger/test/mochitest/browser_dbg_scripts-switching-03.js
@@ +24,5 @@
>        return finished;
>      }
>  
>      function testDebuggerLoadingError() {
> +      ok(gEditor.getText().includes(gL10N.getFormatStr("errorLoadingText2", "No such actor for ID: fake.actor")),

Can you look into what changed here? The test was passing before, so why did the error message change? Would be nice to know.

If you look in `content/actions/sources.js` you'll see that's where it gets the source in `loadSourceText`.

::: devtools/shared/client/main.js
@@ +453,5 @@
>        if (!aResponse.error) {
>          tabClient = new TabClient(this, aResponse);
>          this.registerClient(tabClient);
>        }
>        aOnResponse(aResponse, tabClient);

How come `aOnResponse` still exists here? Can't we just remove that now that it returns a promise, or are you wanting to slowly migrate usages of this and remove it later?

@@ +478,5 @@
>  
>        let workerClient = new WorkerClient(this, aResponse);
>        this.registerClient(workerClient);
>        aOnResponse(aResponse, workerClient);
> +      return [aResponse, workerClient];

Ok, seeing `aOnResponse` here confirms my above comment. You probably want to support both interfaces for now. Can you file a bug to remove the callback parameters as well at some point?

Comment 15

[James Long (:jlongster)]

Comment on attachment 8702570 [details] [diff] [review]
Remove unecessary usages of promiseInvoke, rdpInvoke

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

Awesome! No comments here, looks like a straight-forward cleanup.

Comment 16

[Alexandre Poirot [:ochameau]]

(In reply to James Long (:jlongster) from comment #13)
> Comment on attachment 8702569 [details] [diff] [review]
> Convert all DebuggerClient request methods to return promises
> 
> Review of attachment 8702569 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks sane. Are you planning on removing the callback parameter soon?

I don't know because of addons, but that would be great to do it at some point.

> Ok, seeing `aOnResponse` here confirms my above comment. You probably want
> to support both interfaces for now.

Yes

> Can you file a bug to remove the callback parameters as well at some point?

Bug 1237598.

Comment 17

[Alexandre Poirot [:ochameau]]

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

Comment 18

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/8b4ced39ad5858ae05a65998676e4c1ffea859da
Bug 1227978 - Simplify usages of Debuggerclient APIs now that all requests returns promises. r=jlongster

https://hg.mozilla.org/integration/fx-team/rev/f99079d057cc45414b8e894c3703ee0221fe3f99
Bug 1227978 - Convert all DebuggerClient request methods to return promises. r=jlongster

https://hg.mozilla.org/integration/fx-team/rev/d8a429b575e3359ceaa13d2ae2b7bcfc4e3d730b
Bug 1227978 - Remove unecessary usages of promiseInvoke, rdpInvoke r=jlongster

Comment 19

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/8badb4d61bc8

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for XPCshell bustage: TEST-UNEXPECTED-TIMEOUT | devtools/server/tests/unit/test_sourcemaps-03.js | Test timed out

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=6515593&repo=fx-team

09:07:44     INFO -  TEST-START | devtools/server/tests/unit/test_sourcemaps-03.js
09:12:44  WARNING -  TEST-UNEXPECTED-TIMEOUT | devtools/server/tests/unit/test_sourcemaps-03.js | Test timed out
09:12:44     INFO -  TEST-INFO took 300001ms
09:12:44     INFO -  >>>>>>>
09:12:44     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
09:12:44     INFO -  PROCESS | 5114 | DBG-SERVER: Packet 0 sent from "root"
09:12:44     INFO -  (xpcshell/head.js) | test pending (2)
09:12:44     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
09:12:44     INFO -  running event loop
09:12:44     INFO -  PROCESS | 5114 | DBG-TEST: resource://devtools/shared/Loader.jsm:462: strict error: ReferenceError: reference to undefined property Services.appinfo.name
09:12:44     INFO -  PROCESS | 5114 | DBG-TEST: head_dbg.js observed a console message: ReferenceError: reference to undefined property Services.appinfo.name
09:12:44     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property Services.appinfo.name" {file: "resource://devtools/shared/Loader.jsm" line: 462}]"

Comment 21

[Alexandre Poirot [:ochameau]]

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

Comment 22

[Alexandre Poirot [:ochameau]]

Attachment: Remove unecessary usages of promiseInvoke, rdpInvoke

Had to rebase a new test using rdpInvoke.

Comment 23

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/cc53a3e1223a0e38cae65a79ba54ef2563911ea2
Bug 1227978 - Simplify usages of Debuggerclient APIs now that all requests returns promises. r=jlongster

https://hg.mozilla.org/integration/fx-team/rev/39414bcdb43b278be8f22114c6408e6789bff151
Bug 1227978 - Convert all DebuggerClient request methods to return promises. r=jlongster

https://hg.mozilla.org/integration/fx-team/rev/fdbc63d77a52fa851174bdfaf081858450b759bb
Bug 1227978 - Remove unecessary usages of promiseInvoke, rdpInvoke r=jlongster

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cc53a3e1223a
https://hg.mozilla.org/mozilla-central/rev/39414bcdb43b
https://hg.mozilla.org/mozilla-central/rev/fdbc63d77a52

Comment 25

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS				Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected