Closed Bug 1227978 Opened 4 years ago Closed 4 years ago

Cleanup codebase now that many DebuggerClient methods returns promises

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox46 fixed)

VERIFIED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Attached patch patch v1 (obsolete) — Splinter Review
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...
Attached patch debugger client - v1 (obsolete) — Splinter Review
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.
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.
Attachment #8702567 - Flags: review?(jlong)
Attachment #8698462 - Attachment is obsolete: true
Attachment #8698463 - Attachment is obsolete: true
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?)
Attachment #8702569 - Flags: review?(jlong)
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.
Attachment #8702570 - Flags: review?(jlong)
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 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`.
Attachment #8702567 - Flags: review?(jlong) → review+
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?
Attachment #8702569 - Flags: review?(jlong) → review+
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 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.
Attachment #8702570 - Flags: review?(jlong) → review+
(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.
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
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}]"
Flags: needinfo?(poirot.alex)
Had to rebase a new test using rdpInvoke.
Attachment #8706822 - Flags: review+
Attachment #8702570 - Attachment is obsolete: true
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
Flags: needinfo?(poirot.alex)
[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
Depends on: 1337516
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.