Closed Bug 1366531 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: JSON Viewer, enhancement)

Product:
DevTools
Component:
JSON Viewer
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: mkohler, Assigned: Oriol)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1366531 - convert uses of defer to 'new Promise' in client/jsonview
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
      No description provided.
Attachment #8869789 - Flags: review?(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 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
Attachment #8869789 - Flags: review?(nchevobbe) → review+
(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 :(
Michael, I'm afraid you will need to redo your patch, because in bug 1368605 I added a parameter to addJsonViewTab.
(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).
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! :)
Assignee: me → nobody
Status: ASSIGNED → NEW
Continuing Michael's patch.
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Attachment #8869789 - Attachment is obsolete: true
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)
(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.
(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 ?
I only added .then(() => tab) to onJSONViewInitialized, and checked !(timeout >= 0) because to handle undefined properly.
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
Attachment #8926603 - Flags: review?(nchevobbe) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4c25a8edd5db
convert uses of defer to 'new Promise' in client/jsonview r=nchevobbe
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4c25a8edd5db
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Depends on: 1421631
No longer depends on: 1421631
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: