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

ASSIGNED
Assigned to

Status

ASSIGNED
a year ago
2 months ago

People

(Reporter: mkohler, Assigned: nikhilkumar.c16)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
Comment hidden (mozreview-request)

Comment 3

a year ago
mozreview-review
Comment on attachment 8869832 [details]
Bug 1366529 - convert uses of 'defer' to 'new Promise' in client/dom

https://reviewboard.mozilla.org/r/141378/#review145118

::: devtools/client/dom/dom-panel.js:79
(Diff revision 1)
>      exportIntoContentScope(this.panelWin, provider, "DomProvider");
>  
>      this.shouldRefresh = true;
>    },
>  
>    destroy: Task.async(function* () {

I think we could remove the Task.async wrapper here, as you did for the `open` function.

::: devtools/client/dom/dom-panel.js:84
(Diff revision 1)
> -    let deferred = promise.defer();
> +    this._destroying = new Promise((resolve, reject) => {
> -    this._destroying = deferred.promise;
> -
> -    this.target.off("navigate", this.onTabNavigated);
> +      this.target.off("navigate", this.onTabNavigated);
> -    this._toolbox.off("select", this.onPanelVisibilityChange);
> +      this._toolbox.off("select", this.onPanelVisibilityChange);
>  
> -    this.emit("destroyed");
> +      this.emit("destroyed");
>  
> -    deferred.resolve();
> +      resolve();
> +    });

not the fault of your patch, but I don't really see the value of having this promise here since we don't wait for asynchronous events.

We could turn the `destroying` property into a boolean flag, to be careful, but I think the whole thing can be a simple function.

Do I miss anything ? What do you think Michael ?

::: devtools/client/dom/dom-panel.js:143
(Diff revision 1)
>    getPrototypeAndProperties: function (grip) {
> -    let deferred = defer();
> -
>      if (!grip.actor) {
>        console.error("No actor!", grip);
> -      deferred.reject(new Error("Failed to get actor from grip."));
> +      return Promise.reject(new Error("Failed to get actor from grip."));
> -      return deferred.promise;
>      }
>  
>      // Bail out if target doesn't exist (toolbox maybe closed already).
>      if (!this.target) {
> -      return deferred.promise;
> +      return new Promise(() => {});
>      }
>  
>      // If a request for the grips is already in progress
>      // use the same promise.
>      let request = this.pendingRequests.get(grip.actor);
>      if (request) {
>        return request;
>      }
>  
> +    const promise = new Promise((resolve, reject) => {
> -    let client = new ObjectClient(this.target.client, grip);
> +      let client = new ObjectClient(this.target.client, grip);
> -    client.getPrototypeAndProperties(response => {
> +      client.getPrototypeAndProperties(response => {
> -      this.pendingRequests.delete(grip.actor, deferred.promise);
> +        this.pendingRequests.delete(grip.actor);
> -      deferred.resolve(response);
>  
> -      // Fire an event about not having any pending requests.
> +        // Fire an event about not having any pending requests.
> -      if (!this.pendingRequests.size) {
> +        if (!this.pendingRequests.size) {
> -        this.emit("no-pending-requests");
> +          this.emit("no-pending-requests");
> -      }
> +        }
> +
> +        resolve(response);
> +      });
>      });
>  
> -    this.pendingRequests.set(grip.actor, deferred.promise);
> +    this.pendingRequests.set(grip.actor, promise);
>  
> -    return deferred.promise;
> +    return promise;
>    },

Could we turn this function into an async ?
We seems to do work to ensure we return a Promise, which would be done automatically if the function is async.

::: devtools/client/dom/dom-panel.js:146
(Diff revision 1)
>    getPrototypeAndProperties: function (grip) {
> -    let deferred = defer();
> -
>      if (!grip.actor) {
>        console.error("No actor!", grip);
> -      deferred.reject(new Error("Failed to get actor from grip."));
> +      return Promise.reject(new Error("Failed to get actor from grip."));

here if we havbe an async function. we could throw the Error

Comment 4

a year ago
mozreview-review
Comment on attachment 8869832 [details]
Bug 1366529 - convert uses of 'defer' to 'new Promise' in client/dom

https://reviewboard.mozilla.org/r/141378/#review147058

::: devtools/client/dom/dom-panel.js:151
(Diff revision 1)
> -      return deferred.promise;
>      }
>  
>      // Bail out if target doesn't exist (toolbox maybe closed already).
>      if (!this.target) {
> -      return deferred.promise;
> +      return new Promise(() => {});

return Promise.resolve()

::: devtools/client/dom/dom-panel.js:180
(Diff revision 1)
>    getRootGrip: function () {
> -    let deferred = defer();
> +    return new Promise((resolve, reject) => {
> -
> -    // Attach Console. It might involve RDP communication, so wait
> +      // Attach Console. It might involve RDP communication, so wait
> -    // asynchronously for the result
> +      // asynchronously for the result
> -    this.target.activeConsole.evaluateJSAsync("window", res => {
> +      this.target.activeConsole.evaluateJSAsync("window", res => {
> -      deferred.resolve(res.result);
> +        resolve(res.result);
> +      });
>      });

This could be an async function as evaluateJSAsync returns a Promise (https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/client.js#289) :

async getRootGrip() {
  // Attach Console. It might involve RDP communication, so wait
  // asynchronously for the result
  let response = await this.target.activeConsole.evaluateJSAsync();
  return response.result;
}
Comment hidden (mozreview-request)

Comment 7

a year ago
mozreview-review
Comment on attachment 8869832 [details]
Bug 1366529 - convert uses of 'defer' to 'new Promise' in client/dom

https://reviewboard.mozilla.org/r/141378/#review147428

One small thing and it will be good :)
Thanks Michael !

::: devtools/client/dom/dom-panel.js:140
(Diff revisions 1 - 2)
> -      return Promise.reject(new Error("Failed to get actor from grip."));
> +      return new Error("Failed to get actor from grip.");
>      }
>  
>      // Bail out if target doesn't exist (toolbox maybe closed already).
>      if (!this.target) {
> -      return new Promise(() => {});
> +      return true;

I think it would be more approriate to keep the same functionnality with `await new Promise(() => {})`
Attachment #8869832 - Flags: review?(nchevobbe) → review-

Comment 8

a year ago
mozreview-review
Comment on attachment 8869832 [details]
Bug 1366529 - convert uses of 'defer' to 'new Promise' in client/dom

https://reviewboard.mozilla.org/r/141378/#review147502

::: devtools/client/dom/dom-panel.js:151
(Diff revisions 1 - 2)
>        return request;
>      }
>  
> -    const promise = new Promise((resolve, reject) => {
> -      let client = new ObjectClient(this.target.client, grip);
> +    let client = new ObjectClient(this.target.client, grip);
> -      client.getPrototypeAndProperties(response => {
> +    const result = await client.getPrototypeAndProperties();

It doesn't seem we set this.pendingRequests anymore after the conversion to await, which might lead to duplicate requests?
(Reporter)

Comment 9

a year ago
mozreview-review-reply
Comment on attachment 8869832 [details]
Bug 1366529 - convert uses of 'defer' to 'new Promise' in client/dom

https://reviewboard.mozilla.org/r/141378/#review147428

> I think it would be more approriate to keep the same functionnality with `await new Promise(() => {})`

What would be the appropriate way to do this? In the current trunk version, this solely returns deferred.promise. What would be the equivalent in an async function?

I somehow haven't completely wrapped my mind around async/await yet it seems.
(Reporter)

Comment 10

a year ago
mozreview-review-reply
Comment on attachment 8869832 [details]
Bug 1366529 - convert uses of 'defer' to 'new Promise' in client/dom

https://reviewboard.mozilla.org/r/141378/#review147502

> It doesn't seem we set this.pendingRequests anymore after the conversion to await, which might lead to duplicate requests?

Agreed, will fix tonight.
(Reporter)

Comment 11

a year ago
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
Hello Nikhil,
I assigned this bug to you. There is already a patch attached, but since it was a long time ago, I suggest you start from scratch.
Here's all the defer calls there is in this folder: https://searchfox.org/mozilla-central/search?q=defer&case=false&regexp=false&path=devtools%2Fclient%2Fdom

I think it's your first bug, so I encourage you reading http://docs.firefox-dev.tools/getting-started/ , and more specifically http://docs.firefox-dev.tools/getting-started/build.html (make sure to use Artifact builds as there much faster to work with).

Don't hesitate to ask questions, either here, on IRC or Slack (https://devtools-html-slack.herokuapp.com/)
Assignee: nobody → nikhilkumar.c16
Status: NEW → ASSIGNED

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.