Closed Bug 1366529 Opened 4 years ago Closed 2 years ago

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

Categories

(DevTools :: DOM, enhancement)

enhancement
Not set
normal

Tracking

(firefox65 fixed)

RESOLVED FIXED
Firefox 65
Tracking Status
firefox65 --- fixed

People

(Reporter: mkohler, Assigned: sreeise)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

No description provided.
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 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 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 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?
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.
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.
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
Product: Firefox → DevTools
Assignee: nikhilkumar.c16 → nobody
Status: ASSIGNED → NEW
:nchevobbe I would like to work on this bug :)
Flags: needinfo?(nchevobbe)
(In reply to Anushi Maheshwari[:Anushi1998] from comment #13)
> :nchevobbe I would like to work on this bug :)

Hello Anushi ! 
I just assigned it to you. 
I don't know if you already set up the work environment, but if not, you can look at https://docs.firefox-dev.tools/getting-started/ .
Assignee: nobody → anushimaheshwari95
Status: NEW → ASSIGNED
Flags: needinfo?(nchevobbe)
Trying to get involved on my second bug fix. First with firefox. Would anyone mind pointing me in the correct start spot.
Hi Joshua, The first step would be to build firefox on your system. You can refer this [1]. If you are interested in contribution to Mozilla [2] might be helpful to find good-first-bug.

Moreover, please do ask such questions on #newbies and #introduction IRC [3], as comments on any bug should be relevant to that bug to avoid things being messy :)

[1] https://docs.firefox-dev.tools/getting-started/build.html
[2] https://codetribute.mozilla.org
[3] https://wiki.mozilla.org/IRC
Hey Nicolas, do you mind if I take over this bug? 


If its ok to take over, I do have a question if you don't mind answering. Regarding the destroy function in panel.js, it is set to async and at the same time returning a promise. Is there any reason for this to still have the async keyword? I believe async ensures the result will be wrapped in a promise, so maybe that is why it is there? I am wondering after changing the function to return a new Promise constructor, would it really make sense to keep keep the 'async' part?

Thanks.
Flags: needinfo?(nchevobbe)
Hello Sean, I assigned you to the bug since they were no activity for a couple of months, thanks for helping us :)

So for reference, here's the destroy function https://searchfox.org/mozilla-central/rev/efc0d9172cb6a5849c6c4fc0f19d7fd5a2da9643/devtools/client/dom/panel.js#68-83 

Since it does not have any `await` expression, I think we can safely remove the `async` part on the function declaration indeed.
Assignee: anushimaheshwari95 → reeisesean
Flags: needinfo?(nchevobbe)
Converted uses of defer to new Promise/async await in devtools dom directory
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8b97f68a7fa
Converted uses of defer to new Promise in devtools/client/dom r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/e8b97f68a7fa
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
You need to log in before you can comment on or make changes to this bug.