1387123 - Replace all usages of require(promise).defer by require(devtools/shared/defer).defer

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

[Alexandre Poirot [:ochameau]]

In order to help removing usages of Promise.jsm, we could automatically convert all usages of require("promise").defer as that's the only one API from Promise.jsm that doesn't work with DOM Promise.

We already have an helper module: devtools/shared/defer which exposes just and only that.
Ideally, we would followup to ensure that we can't replace some usages by plain DOM Promise, with the constructor pattern like: "new Promise(done => {})".

Comment 1

[Tom Tromey :tromey]

(In reply to Alexandre Poirot [:ochameau] from comment #0)

> Ideally, we would followup to ensure that we can't replace some usages by
> plain DOM Promise, with the constructor pattern like: "new Promise(done =>
> {})".

This followup is bug 1283869.

Comment 2

[Alexandre Poirot [:ochameau]]

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

Comment 3

[Alexandre Poirot [:ochameau]]

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

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1387123 - Replace all usages of require(promise).defer by require(devtools/shared/defer).defer.

In prevision of Promise.jsm removal, use defer helper module instead of Promise.jsm
as that's the only one feature that DOM Promise don't support.

PART1: Import devtools/shared/defer
$ sed -i '/require("promise")/a const defer = require("devtools/shared/defer");' $(egrep -lir "promise.defer\(\)" $(egrep -rl "require\(\"promise\"\)" devtools))

Review commit: https://reviewboard.mozilla.org/r/164574/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164574/

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1387123 - Replace all usages of require(promise).defer by require(devtools/shared/defer).defer.

In prevision of Promise.jsm removal, use defer helper module instead of Promise.jsm
as that's the only one feature that DOM Promise don't support.

PART2: Substitutes promise.defer usages with defer
$ sed -i 's/promise.defer/defer/gI' $(egrep -lir "promise.defer\(\)" devtools)
  Reset modification to the following files as they are using deprecated syncable promises as we don't want to touch them.
  http://searchfox.org/mozilla-central/search?q=deprecated-sync-thenables&case=true&regexp=false&path=
$ git checkout devtools/client/debugger/test/mochitest/
$ git checkout devtools/shared/client/main.js
$ git checkout devtools/client/debugger/debugger-controller.js
$ git checkout devtools/server/main.js

Review commit: https://reviewboard.mozilla.org/r/164576/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164576/

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1387123 - Replace all usages of require(promise).defer by require(devtools/shared/defer).defer.

In prevision of Promise.jsm removal, use defer helper module instead of Promise.jsm
as that's the only one feature that DOM Promise don't support.

PART3: Manual fixes
- Go manually fix: http://searchfox.org/mozilla-central/source/devtools/client/dom/dom-panel.js
  which has duplicated import of defer symbol (it already used to import it)
- execute eslint, see all the places where we import of 'promise' is no longer use it
  (there may be leftovers not covered by eslint that will be covered in bug 1387128)
- fix devtools/client/performance/panel.js and devtools/client/webconsole/console-output.js
  which use loader.lazyRequireGetter(this, "promise") and miss the defer import

Review commit: https://reviewboard.mozilla.org/r/164578/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164578/

Comment 7

[Tom Tromey :tromey]

Comment on attachment 8893494 [details]
Bug 1387123 - Replace all usages of require(promise).defer by require(devtools/shared/defer).defer.

https://reviewboard.mozilla.org/r/164574/#review170016

Thanks for the patch.  I like this approach.

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8893495 [details]
Bug 1387123 - Replace all usages of require(promise).defer by require(devtools/shared/defer).defer.

https://reviewboard.mozilla.org/r/164576/#review170020

Thanks.  Because this is large and auto-generated, I didn't read every hunk.
I do agree with the approach you took.  In the unlikely event that there's some issue, I imagine try will find it.

Comment 9

[Tom Tromey :tromey]

Comment on attachment 8893496 [details]
Bug 1387123 - Replace all usages of require(promise).defer by require(devtools/shared/defer).defer.

https://reviewboard.mozilla.org/r/164578/#review170026

Thanks again.  This looks good, though there's one question that I think needs to be answered.

::: devtools/server/main.js:736
(Diff revision 1)
>  
>      return this._onConnection(transport, prefix, true);
>    },
>  
>    connectToContent(connection, mm, onDestroy) {
> -    let deferred = Syncdefer();
> +    let deferred = SyncPromise.defer();

I guess that regexp could have been a bit tighter.  But no worries.

::: devtools/server/worker.js:31
(Diff revision 1)
>  };
>  
>  loadSubScript("resource://devtools/shared/worker/loader.js");
>  
> -var Promise = worker.require("promise");
>  const defer = require("devtools/shared/defer");

Should this be worker.require, like the other requires here?

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to Tom Tromey :tromey from comment #9)
> Comment on attachment 8893496 [details]
> ::: devtools/server/worker.js:31
> (Diff revision 1)
> >  };
> >  
> >  loadSubScript("resource://devtools/shared/worker/loader.js");
> >  
> > -var Promise = worker.require("promise");
> >  const defer = require("devtools/shared/defer");
> 
> Should this be worker.require, like the other requires here?

Good catch!

Comment 11

[Alexandre Poirot [:ochameau]]

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

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 8893494 [details]
Bug 1387123 - Replace all usages of require(promise).defer by require(devtools/shared/defer).defer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164574/diff/1-2/

Comment 13

[Alexandre Poirot [:ochameau]]

Comment on attachment 8893495 [details]
Bug 1387123 - Replace all usages of require(promise).defer by require(devtools/shared/defer).defer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164576/diff/1-2/

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8893496 [details]
Bug 1387123 - Replace all usages of require(promise).defer by require(devtools/shared/defer).defer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164578/diff/1-2/

Comment 15

[Alexandre Poirot [:ochameau]]

Quick update on last orange try:

* Debugger frontend still uses depreated-sync promises and it doesn't play well with these scripts.
So I opened bug 1388364 to handle Promise.jsm removal for devtools/client/debugger manually.

* devtools/server/tests/mochitest/inspector-helpers.js still need to import promise as it is a shared test script exposing promise symbol to various tests. I made a one line eslint exception. Hopefully we would get rid of this require(promise) in bug 1387128.

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8893496 [details]
Bug 1387123 - Replace all usages of require(promise).defer by require(devtools/shared/defer).defer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164578/diff/2-3/

Comment 17

[Alexandre Poirot [:ochameau]]

Just pushed a eslint fix to make that last try green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb5e2d5da8ded3fcb918a3576e14232231ed878f&selectedJob=121660017

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40601ae660c6
Replace all usages of require(promise).defer by require(devtools/shared/defer).defer. r=tromey
https://hg.mozilla.org/integration/autoland/rev/a820e391d900
Replace all usages of require(promise).defer by require(devtools/shared/defer).defer. r=tromey
https://hg.mozilla.org/integration/autoland/rev/84755985ba13
Replace all usages of require(promise).defer by require(devtools/shared/defer).defer. r=tromey

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40601ae660c6
https://hg.mozilla.org/mozilla-central/rev/a820e391d900
https://hg.mozilla.org/mozilla-central/rev/84755985ba13