Closed
Bug 1346009
Opened 7 years ago
Closed 7 years ago
convert uses of "defer" to "new Promise" - client/webide directory
Categories
(DevTools Graveyard :: WebIDE, defect, P3)
DevTools Graveyard
WebIDE
Tracking
(firefox55 fixed)
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: hanbin.chang, Assigned: stylizit)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8869779 [details] Bug 1346009 - Convert 'defer' to 'new Promise' in client/webide; https://reviewboard.mozilla.org/r/141298/#review145586 Thank you for the patch. I found a number of minor issues with it that I think need correcting. ::: devtools/client/webide/modules/addons.js:54 (Diff revision 1) > AddonManager.addAddonListener(addonsListener); > > var GetAvailableAddons_promise = null; > var GetAvailableAddons = exports.GetAvailableAddons = function () { > if (!GetAvailableAddons_promise) { > - let deferred = promise.defer(); > + GetAvailableAddons_promise = new Promise((resolve, reject) => { I think this removed the last use of `promise` here, so the `require` could be removed as well. ::: devtools/client/webide/modules/app-manager.js:678 (Diff revision 1) > - let deferred = promise.defer(); > + yield app.launch(); > + return new Promise(resolve => { This doesn't look correct to me. Instead I think the ordering has to be preserved -- create the new promise, then `yield app.launch()`, then yield the promise. ::: devtools/client/webide/modules/app-projects.js:134 (Diff revision 1) > -var loadDeferred = promise.defer(); > - > +var loadDeferred = new Promise(resolve => { > + resolve(IDB.open().then(function (projects) { > -loadDeferred.resolve(IDB.open().then(function (projects) { > - AppProjects.projects = projects; > + AppProjects.projects = projects; > - AppProjects.emit("ready", projects); > + AppProjects.emit("ready", projects); > -})); > + })); > +}); This looks weird -- but not your fault, the original was also pretty weird. It seems to me that there's no need to introduce a new promise here at all, something like ``` var loadDeferred = IDB.open().then(...); ``` would seem to be just as correct, only clearer. ::: devtools/client/webide/modules/app-validator.js:198 (Diff revision 1) > return Services.io.newURI(this.location).prePath; > } > }; > > AppValidator.prototype.validateLaunchPath = function (manifest) { > - let deferred = promise.defer(); > + return new Promise((resolve, reject) => { I couldn't find a use of `reject`; if there really isn't one, just leave it out. ::: devtools/client/webide/modules/app-validator.js:216 (Diff revision 1) > - deferred.resolve(); > + resolve(); > - return deferred.promise; > - } > + } Removing the `return` here changes flow control. ::: devtools/client/webide/modules/app-validator.js:226 (Diff revision 1) > - deferred.resolve(); > + resolve(); > - return deferred.promise; > - } > + } Likewise. ::: devtools/client/webide/modules/build.js:151 (Diff revision 1) > } > > // Subprocess changes CWD, we have to save and restore it. > let originalCwd = yield OS.File.getCurrentDirectory(); > try { > - let defer = promise.defer(); > + return new Promise((resolve, reject) => { I think this should `yield` rather than `return`, as that's what the old code did. ::: devtools/client/webide/modules/simulator-process.js:106 (Diff revision 1) > }); > }, > > // Request a B2G instance kill. > kill() { > - let deferred = promise.defer(); > + return new Promise((resolve, reject) => { I didn't see a use of `reject` here ::: devtools/client/webide/test/browser_tabs.js:70 (Diff revision 1) > > function connectToLocal(win, docRuntime) { > - let deferred = promise.defer(); > + return new Promise(resolve => { > - win.AppManager.connection.once( > + win.AppManager.connection.once( > win.Connection.Events.CONNECTED, > - () => deferred.resolve()); > + () => resolve()); This could just be replaced with plain `resolve` ::: devtools/client/webide/test/head.js:121 (Diff revision 1) > - setTimeout(() => { > + setTimeout(() => { > - deferred.resolve(); > + resolve(); > - }, time); > + }, time); The first argument to `setTimeout` could just be `resolve` ::: devtools/client/webide/test/test_simulators.html:75 (Diff revision 1) > > // Hack SimulatorProcesses to spy on simulation parameters. > > let runPromise; > function fakeRun() { > - runPromise.resolve({ > + Promise.resolve({ This change doesn't seem correct. Previously `runPromise` was being resolved, but now this is constructing and throwing away a new resolved promise. ::: devtools/client/webide/test/test_simulators.html:88 (Diff revision 1) > - runPromise = promise.defer(); > + return new Promise(() => { > - findAll(".runtime-panel-item-simulator")[i].click(); > + findAll(".runtime-panel-item-simulator")[i].click(); > - return runPromise.promise; > + }); > } ... continuing from above, here, previously, the returned promise could be resolved at some point, but now it cannot.
Attachment #8869779 -
Flags: review?(ttromey)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8869779 [details] Bug 1346009 - Convert 'defer' to 'new Promise' in client/webide; https://reviewboard.mozilla.org/r/141296/#review145942 ::: devtools/client/webide/modules/app-manager.js:494 (Diff revision 2) > - } > + } > - }, deferred.reject); > + }, reject); > + }); > > // Record connection result in telemetry > let logResult = result => { Moved logResult outside of the Promise so the next call to that function would work. I'm pretty sure it should have been undefined previously. ::: devtools/client/webide/modules/app-validator.js:202 (Diff revision 2) > AppValidator.prototype.validateLaunchPath = function (manifest) { > - let deferred = promise.defer(); > + return new Promise(resolve => { > - // The launch_path field has to start with a `/` > + // The launch_path field has to start with a `/` > - if (manifest.launch_path && manifest.launch_path[0] !== "/") { > + if (manifest.launch_path && manifest.launch_path[0] !== "/") { > - this.error(strings.formatStringFromName("validator.nonAbsoluteLaunchPath", [manifest.launch_path], 1)); > + this.error(strings.formatStringFromName("validator.nonAbsoluteLaunchPath", [manifest.launch_path], 1)); > - deferred.resolve(); > + resolve(); Just noticed that we should probably have `return resolve()` as well here. ::: devtools/client/webide/test/test_simulators.html:88 (Diff revision 2) > AddonSimulatorProcess.prototype.run = fakeRun; > OldAddonSimulatorProcess.prototype.run = fakeRun; > CustomSimulatorProcess.prototype.run = fakeRun; > > function runSimulator(i) { > - runPromise = promise.defer(); > + return new Promise(resolve => { I hope I got this one correctly: we save a reference to the resolver, return a pending Promise, that will be resolved in due time once fakeRun will be called. This should be the same as the old behavior? ::: devtools/client/webide/test/test_toolbox.html:48 (Diff revision 2) > win.AppManager.update("runtime-list"); > > - let deferred = promise.defer(); > + let deferred = new Promise(resolve => { > - win.AppManager.connection.once( > + win.AppManager.connection.once( > - win.Connection.Events.CONNECTED, > + win.Connection.Events.CONNECTED, > - () => deferred.resolve()); > + resolve); deferred.resolve() didn't exist.
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Tom Tromey :tromey from comment #2) > Comment on attachment 8869779 [details] > Bug 1346009 - Convert 'defer' to 'new Promise' in client/webide; > > https://reviewboard.mozilla.org/r/141298/#review145586 > > Thank you for the patch. I found a number of minor issues with it that I > think need correcting. > Thanks so much for the review! Good thing we have those, I made a lot of silly mistakes there... I addressed your issues in the updated patch and made some other improvements that I've commented. I left one issue hanging because I'm not quite sure I got this one right -- you can see it in the comment related.
Assignee | ||
Updated•7 years ago
|
Assignee: hanbin.chang → matthieu.rigolot
Status: NEW → ASSIGNED
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8869779 [details] Bug 1346009 - Convert 'defer' to 'new Promise' in client/webide; https://reviewboard.mozilla.org/r/141298/#review148756 Thanks for updating the patch. This looks good to me.
Attachment #8869779 -
Flags: review?(ttromey) → review+
Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1d250422890a Convert 'defer' to 'new Promise' in client/webide; r=tromey
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1d250422890a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•