Closed
Bug 1387123
Opened 7 years ago
Closed 7 years ago
Replace all usages of require(promise).defer by require(devtools/shared/defer).defer
Categories
(DevTools :: General, enhancement)
DevTools
General
Tracking
(firefox57 fixed)
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
Attachments
(3 files)
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•7 years ago
|
||
(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.
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
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.
Attachment #8893494 -
Flags: review+
Comment 8•7 years ago
|
||
mozreview-review |
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.
Attachment #8893495 -
Flags: review+
Comment 9•7 years ago
|
||
mozreview-review |
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?
Attachment #8893496 -
Flags: review+
Assignee | ||
Comment 10•7 years ago
|
||
(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!
Assignee | ||
Comment 11•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
Just pushed a eslint fix to make that last try green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb5e2d5da8ded3fcb918a3576e14232231ed878f&selectedJob=121660017
Comment 18•7 years ago
|
||
Pushed by apoirot@mozilla.com:
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/40601ae660c6
https://hg.mozilla.org/mozilla-central/rev/a820e391d900
https://hg.mozilla.org/mozilla-central/rev/84755985ba13
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•