Closed
Bug 1366529
Opened 7 years ago
Closed 6 years ago
convert uses of "defer" to "new Promise" in client/dom
Categories
(DevTools :: DOM, enhancement)
DevTools
DOM
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 hidden (mozreview-request) |
Reporter | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=851fc49f46b2e37d91060eece6a5a1f0e2bdfbb0
Comment 3•7 years 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•7 years 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) |
Reporter | ||
Comment 6•7 years ago
|
||
Thanks for the review. Pushed to reviewboard again. https://treeherder.mozilla.org/#/jobs?repo=try&revision=951d63fc514f65b9773634dad7f4565b409ab8dc
Comment 7•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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
Comment 12•6 years ago
|
||
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®exp=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•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
Assignee: nikhilkumar.c16 → nobody
Status: ASSIGNED → NEW
Comment 14•6 years ago
|
||
(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)
Comment 15•6 years ago
|
||
Trying to get involved on my second bug fix. First with firefox. Would anyone mind pointing me in the correct start spot.
Comment 16•6 years ago
|
||
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
Assignee | ||
Comment 17•6 years ago
|
||
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)
Comment 18•6 years ago
|
||
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)
Assignee | ||
Comment 19•6 years ago
|
||
Converted uses of defer to new Promise/async await in devtools dom directory
Comment 20•6 years ago
|
||
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
Comment 21•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e8b97f68a7fa
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
You need to log in
before you can comment on or make changes to this bug.
Description
•