1346009 - convert uses of "defer" to "new Promise" - client/webide directory

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect, P3)

Description

[hanbin.chang]

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

Comment 1

[Matt R [:stylizit] (slow, away until Sep)]

Attachment: Bug 1346009 - Convert 'defer' to 'new Promise' in client/webide;

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

Comment 2

[Tom Tromey :tromey]

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.

Comment 3

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8869779 [details]
Bug 1346009 - Convert 'defer' to 'new Promise' in client/webide;

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

Comment 4

[Matt R [:stylizit] (slow, away until Sep)]

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.

Comment 5

[Matt R [:stylizit] (slow, away until Sep)]

(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.

Comment 6

[Tom Tromey :tromey]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d250422890a
Convert 'defer' to 'new Promise' in client/webide; r=tromey

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d250422890a