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

Status: RESOLVED FIXED

(DevTools :: DOM, enhancement)

Comment 1

[Michael Kohler [:mkohler]]

Attachment: Bug 1366529 - convert uses of 'defer' to 'new Promise' in client/dom

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

Comment 2

[Michael Kohler [:mkohler]]

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

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

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

[Tim Nguyen :ntim]

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 5

[Michael Kohler [:mkohler]]

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

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

Comment 6

[Michael Kohler [:mkohler]]

Thanks for the review. Pushed to reviewboard again.

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

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

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(() => {})`

Comment 8

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

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 9

[Michael Kohler [:mkohler]]

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 10

[Michael Kohler [:mkohler]]

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.

Comment 11

[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 12

[Nicolas Chevobbe [:nchevobbe]]

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/)

Comment 13

[Anushi Maheshwari[:Anushi1998]]

:nchevobbe I would like to work on this bug :)

Comment 14

[Nicolas Chevobbe [: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/ .

Comment 15

[Joshua]

Trying to get involved on my second bug fix. First with firefox. Would anyone mind pointing me in the correct start spot.

Comment 16

[Anushi Maheshwari[:Anushi1998]]

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

Comment 17

[Sean Reeise [:sreeise]]

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.

Comment 18

[Nicolas Chevobbe [: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.

Comment 19

[Sean Reeise [:sreeise]]

Attachment: Bug 1366529 Converted uses of defer to new Promise in devtools/client/dom r?nchevobbe

Converted uses of defer to new Promise/async await in devtools dom directory

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8b97f68a7fa
Converted uses of defer to new Promise in devtools/client/dom r=nchevobbe

Comment 21

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/e8b97f68a7fa