1366531 - convert uses of "defer" to "new Promise" in client/jsonview

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, enhancement)

Comment 1

[Michael Kohler [:mkohler]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26c98a7d6b8054e31167a1d62f5253d2b3c3477c

Comment 2

[Michael Kohler [:mkohler]]

Attachment: Bug 1366531 - convert uses of defer to 'new Promise' in client/jsonview

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

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8869789 [details]
Bug 1366531 - convert uses of defer to 'new Promise' in client/jsonview

https://reviewboard.mozilla.org/r/141342/#review145112

::: devtools/client/jsonview/test/head.js:30
(Diff revision 1)
>  function addJsonViewTab(url) {
>    info("Adding a new JSON tab with URL: '" + url + "'");
>  
> -  let deferred = promise.defer();
> +  return addTab(url).then(tab => {
> -  addTab(url).then(tab => {
>      let browser = tab.linkedBrowser;
>  
>      // Load devtools/shared/frame-script-utils.js
>      getFrameScript();
>  
>      // Load frame script with helpers for JSON View tests.
>      let rootDir = getRootDirectory(gTestPath);
>      let frameScriptUrl = rootDir + "doc_frame_script.js";
>      browser.messageManager.loadFrameScript(frameScriptUrl, false);
>  
>      // Resolve if the JSONView is fully loaded or wait
>      // for an initialization event.
>      if (content.window.wrappedJSObject.jsonViewInitialized) {
> -      deferred.resolve(tab);
> +      return tab;
> -    } else {
> -      waitForContentMessage("Test:JsonView:JSONViewInitialized").then(() => {
> -        deferred.resolve(tab);
> -      });
>      }
> -  });
>  
> -  return deferred.promise;
> +    return waitForContentMessage("Test:JsonView:JSONViewInitialized").then(() => {
> +      return tab;
> +    });
> +  });

I think we could turn this function into an async one.
Do you mind to try Michael ?

Comment 4

[Michael Kohler [:mkohler]]

Comment on attachment 8869789 [details]
Bug 1366531 - convert uses of defer to 'new Promise' in client/jsonview

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

Comment 5

[Michael Kohler [:mkohler]]

Thanks. Pushed to reviewboard and try again.

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

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8869789 [details]
Bug 1366531 - convert uses of defer to 'new Promise' in client/jsonview

https://reviewboard.mozilla.org/r/141342/#review147430

All good, thanks !
I pushed to TRY and will land this if it's green

Comment 7

[Michael Kohler [:mkohler]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #6)
> I pushed to TRY and will land this if it's green

Was it green? Can't find it somehow :(

Comment 8

[Oriol Brufau [:Oriol]]

Michael, I'm afraid you will need to redo your patch, because in bug 1368605 I added a parameter to addJsonViewTab.

Comment 9

[Michael Kohler [:mkohler]]

(In reply to Oriol [:Oriol] from comment #8)
> Michael, I'm afraid you will need to redo your patch, because in bug 1368605
> I added a parameter to addJsonViewTab.

No worries. If everything goes well with my tasks before the All Hands, I will be able to do this on Saturday. Otherwise it will need to wait until after the All Hands (or I might be able to do it in the plane, we'll see).

Comment 10

[Michael Kohler [:mkohler]]

Unassigning myself as there has been no recent action from my side and I don't see myself doing any action on this for the next few weeks.

Sorry about this, if somebody wants to take this, go for it! :)

Comment 11

[Oriol Brufau [:Oriol]]

Attachment: Bug 1366531 - convert uses of defer to 'new Promise' in client/jsonview

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

Comment 12

[Oriol Brufau [:Oriol]]

Continuing Michael's patch.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8926603 [details]
Bug 1366531 - convert uses of defer to 'new Promise' in client/jsonview

https://reviewboard.mozilla.org/r/197834/#review203232

Thanks for taking it Oriol.
I have a nit and a comment, what do you think ?

::: devtools/client/jsonview/test/head.js:59
(Diff revision 1)
> -    // Add a timeout.
> +  // Otherwise wait for an initialization event, but with a time limit.
> +  return new Promise(async function (resolve, reject) {
> +    waitForContentMessage("Test:JsonView:JSONViewInitialized").then(() => {
> +      resolve(tab);
> +    });
>      if (timeout >= 0) {
> -      new Promise(resolve => {
> -        if (content.window.document.readyState === "complete") {
> +      if (content.window.document.readyState !== "complete") {
> +        await waitForContentMessage("Test:JsonView:load");
> -          resolve();
> -        } else {
> -          waitForContentMessage("Test:JsonView:load").then(resolve);
> -        }
> +      }
> -      }).then(() => {
> -        setTimeout(() => {
> +      await waitForTime(timeout);
> +      reject(new Error("JSON Viewer did not load."));
> -          deferred.reject("JSON Viewer did not load.");
> -        }, timeout);
> -      });
>      }

This is a bit hard to follow, maybe we could make this easier. What do you think of :


```
const onJSONViewInitialized = waitForContentMessage("Test:JsonView:JSONViewInitialized");
if (timeout <= 0) {
  return onJSONViewInitialized;
}

if (content.window.document.readyState !== "complete") {
  await waitForContentMessage("Test:JsonView:load");
}

let timeoutId = setTimeout(() => throw new Error("JSON Viewer did not load."), timeout);
const tab = await onJSONViewInitialized;
// Cancel throwing, or we get an uncaught exception warning.
clearTimeout(timeoutId);

return tab;
```

::: devtools/client/jsonview/test/head.js:157
(Diff revision 1)
>  
>    return executeInContent("Test:JsonView:SendString", data);
>  }
>  
>  function waitForTime(delay) {
> -  let deferred = defer();
> +  return new Promise((resolve, reject) => {

eslint might complain of unused  `reject`, I think we can remove it. 

(In case you don't know, In TRY push we now have to use  `-j eslint` to trigger eslint job. they are not added automatically like they used to be)

Comment 14

[Oriol Brufau [:Oriol]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13)
> let timeoutId = setTimeout(() => throw new Error("JSON Viewer did not load."), timeout);

But throwing in a timer won't reject the promise, right?

If you run this code in the console

   var p = (async function() { 
      setTimeout(() => {throw new Error()}, 0);
      await new Promise(resolve => setTimeout(resolve, 1e3));
   })()

then you first see the error but p is not rejected. Instead, after 1 second it resolves to undefined.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Oriol Brufau [:Oriol] from comment #14)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #13)
> > let timeoutId = setTimeout(() => throw new Error("JSON Viewer did not load."), timeout);
> 
> But throwing in a timer won't reject the promise, right?
> 
> If you run this code in the console
> 
>    var p = (async function() { 
>       setTimeout(() => {throw new Error()}, 0);
>       await new Promise(resolve => setTimeout(resolve, 1e3));
>    })()
> 
> then you first see the error but p is not rejected. Instead, after 1 second
> it resolves to undefined.

True, something like that would work : 
```
   var p = async function(wait) { 
     let onTimeout = new Promise((_, rej) => setTimeout(rej, 1000))
     let onExpected = new Promise(resolve => setTimeout(resolve, wait));
     await Promise.race([onExpected, onTimeout]);
     return await onExpected;
   };
   // prints 10 yay
   p(10).then(() => console.log(10, "yay")).catch((e)=> console.warn(10, "nah", e))
   // prints 2000 nah
   p(2000).then(() => console.log(2000, "yay")).catch((e)=> console.warn(2000, "nah", e))
```

which could be translated to : 


```
const onJSONViewInitialized = waitForContentMessage("Test:JsonView:JSONViewInitialized");
if (timeout <= 0) {
  return onJSONViewInitialized;
}

if (content.window.document.readyState !== "complete") {
  await waitForContentMessage("Test:JsonView:load");
}

let onTimeout = new Promise((_, rej) => 
  setTimeout(() => reject(new Error("JSON Viewer did not load.")), timeout));

await Promise.race([onJSONViewInitialized, onTimeout]);
return await onJSONViewInitialized;
```


Do you think that would work ?

Comment 16

[Oriol Brufau [:Oriol]]

Comment on attachment 8926603 [details]
Bug 1366531 - convert uses of defer to 'new Promise' in client/jsonview

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

Comment 17

[Oriol Brufau [:Oriol]]

Comment on attachment 8926603 [details]
Bug 1366531 - convert uses of defer to 'new Promise' in client/jsonview

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

Comment 18

[Oriol Brufau [:Oriol]]

I only added .then(() => tab) to onJSONViewInitialized, and checked !(timeout >= 0) because to handle undefined properly.

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8926603 [details]
Bug 1366531 - convert uses of defer to 'new Promise' in client/jsonview

https://reviewboard.mozilla.org/r/197834/#review203360

Looks good, thanks Oriol. r+ if TRY is green

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c25a8edd5db
convert uses of defer to 'new Promise' in client/jsonview r=nchevobbe

Comment 21

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/4c25a8edd5db