Closed
Bug 1436076
Opened 5 years ago
Closed 5 years ago
Refactor HUDService Browser Console opening/toggling functions
Categories
(DevTools :: Console, defect, P1)
DevTools
Console
Tracking
(firefox60 fixed)
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: bgrins, Assigned: bgrins)
References
Details
(Whiteboard: [newconsole-mvp])
Attachments
(3 files, 1 obsolete file)
As mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1435084#c2, the functions to open and toggle the BC could be simplified, at least to use async / await.
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Updated•5 years ago
|
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•5 years ago
|
||
mozreview-review |
Comment on attachment 8948806 [details] Bug 1436076 - Part 2 - Use async/await instead of defer in HUDService; https://reviewboard.mozilla.org/r/218168/#review224004 Code analysis found 6 defects in this patch: - 6 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/webconsole/hudservice.js:193 (Diff revision 2) > - return new Promise(resolve => { > - let browserConsoleURL = Tools.webConsole.browserConsoleURL; > + let browserConsoleURL = Tools.webConsole.browserConsoleURL; > - let win = Services.ww.openWindow(null, browserConsoleURL, "_blank", > + let win = Services.ww.openWindow(null, browserConsoleURL, "_blank", > - BC_WINDOW_FEATURES, null); > + BC_WINDOW_FEATURES, null); > - win.addEventListener("DOMContentLoaded", () => { > + await new Promise(resolve => { > + win.addEventListener("DOMContentLoaded", resolve, {once:true}); Error: Missing space before value for key 'once'. [eslint: key-spacing] ::: devtools/client/webconsole/hudservice.js:204 (Diff revision 2) > - ); > - } > + } > - }, {once: true}); > + > + let iframe = win.document.querySelector("iframe"); > + await new Promise(resolve => { > + iframe.addEventListener("DOMContentLoaded", resolve, {once:true}); Error: Missing space before value for key 'once'. [eslint: key-spacing] ::: devtools/client/webconsole/hudservice.js:212 (Diff revision 2) > - }); > - }, console.error.bind(console)); > > - return this._browserConsoleDefer.promise; > + // Temporarily cache the async startup sequence so that if toggleBrowserConsole > + // gets called again we can return this console instead of opening another one. > + this._browserConsoleInitializing = new Promise(async function() { Error: Missing space before function parentheses. [eslint: space-before-function-paren] ::: devtools/client/webconsole/hudservice.js:538 (Diff revision 2) > * Console is closed. > * > * @return object > * A promise object that is resolved once the Web Console is closed. > */ > - destroy() { > + async destroy() { Error: Expected to return a value at the end of async method 'destroy'. [eslint: consistent-return] ::: devtools/client/webconsole/hudservice.js:543 (Diff revision 2) > - destroy() { > + async destroy() { > if (this._destroyer) { > - return this._destroyer.promise; > + return this._destroyer; > } > > + this._destroyer = new Promise(async function() { Error: Missing space before function parentheses. [eslint: space-before-function-paren] ::: devtools/client/webconsole/hudservice.js:654 (Diff revision 2) > destroy() { > if (this._bcDestroyer) { > - return this._bcDestroyer.promise; > + return this._bcDestroyer; > } > > + this._bcDestroyer = new Promise(async function() { Error: Missing space before function parentheses. [eslint: space-before-function-paren]
Comment 11•5 years ago
|
||
mozreview-review |
Comment on attachment 8948806 [details] Bug 1436076 - Part 2 - Use async/await instead of defer in HUDService; https://reviewboard.mozilla.org/r/218168/#review224006 Code analysis found 6 defects in this patch: - 6 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/webconsole/hudservice.js:193 (Diff revision 1) > - return new Promise(resolve => { > - let browserConsoleURL = Tools.webConsole.browserConsoleURL; > + let browserConsoleURL = Tools.webConsole.browserConsoleURL; > - let win = Services.ww.openWindow(null, browserConsoleURL, "_blank", > + let win = Services.ww.openWindow(null, browserConsoleURL, "_blank", > - BC_WINDOW_FEATURES, null); > + BC_WINDOW_FEATURES, null); > - win.addEventListener("DOMContentLoaded", () => { > + await new Promise(resolve => { > + win.addEventListener("DOMContentLoaded", resolve, {once:true}); Error: Missing space before value for key 'once'. [eslint: key-spacing] ::: devtools/client/webconsole/hudservice.js:204 (Diff revision 1) > - ); > - } > + } > - }, {once: true}); > + > + let iframe = win.document.querySelector("iframe"); > + await new Promise(resolve => { > + iframe.addEventListener("DOMContentLoaded", resolve, {once:true}); Error: Missing space before value for key 'once'. [eslint: key-spacing] ::: devtools/client/webconsole/hudservice.js:212 (Diff revision 1) > - }); > - }, console.error.bind(console)); > > - return this._browserConsoleDefer.promise; > + // Temporarily cache the async startup sequence so that if toggleBrowserConsole > + // gets called again we can return this console instead of opening another one. > + this._browserConsoleInitializing = new Promise(async function() { Error: Missing space before function parentheses. [eslint: space-before-function-paren] ::: devtools/client/webconsole/hudservice.js:538 (Diff revision 1) > * Console is closed. > * > * @return object > * A promise object that is resolved once the Web Console is closed. > */ > - destroy() { > + async destroy() { Error: Expected to return a value at the end of async method 'destroy'. [eslint: consistent-return] ::: devtools/client/webconsole/hudservice.js:543 (Diff revision 1) > - destroy() { > + async destroy() { > if (this._destroyer) { > - return this._destroyer.promise; > + return this._destroyer; > } > > + this._destroyer = new Promise(async function() { Error: Missing space before function parentheses. [eslint: space-before-function-paren] ::: devtools/client/webconsole/hudservice.js:654 (Diff revision 1) > destroy() { > if (this._bcDestroyer) { > - return this._bcDestroyer.promise; > + return this._bcDestroyer; > } > > + this._bcDestroyer = new Promise(async function() { Error: Missing space before function parentheses. [eslint: space-before-function-paren]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•5 years ago
|
Attachment #8948804 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•5 years ago
|
||
mozreview-review |
Comment on attachment 8948805 [details] Bug 1436076 - Part 1 - Fix eslint for hudservice.js; https://reviewboard.mozilla.org/r/218166/#review224110 Looks so much better, thanks Brian !
Attachment #8948805 -
Flags: review?(nchevobbe) → review+
Comment 18•5 years ago
|
||
mozreview-review |
Comment on attachment 8948806 [details] Bug 1436076 - Part 2 - Use async/await instead of defer in HUDService; https://reviewboard.mozilla.org/r/218168/#review224112 Thanks a lot Brian, this is so much more readable now ! I only have a few nits, but this is good to land. ::: devtools/client/webconsole/hudservice.js:192 (Diff revision 4) > - win.addEventListener("DOMContentLoaded", () => { > + await new Promise(resolve => { > + win.addEventListener("DOMContentLoaded", resolve, {once: true}); > + }); I wonder if we don't have a helper function for that, and if not, if we should have one. That could be helpful ::: devtools/client/webconsole/hudservice.js:212 (Diff revision 4) > - }); > - }, console.error.bind(console)); > > - return this._browserConsoleDefer.promise; > + // Temporarily cache the async startup sequence so that if toggleBrowserConsole > + // gets called again we can return this console instead of opening another one. > + this._browserConsoleInitializing = (async function () { nit: i think we can do ```js (async () => { ... })() ``` so we don't have to bind to this. ::: devtools/client/webconsole/hudservice.js:233 (Diff revision 4) > */ > openBrowserConsoleOrFocus() { > let hud = this.getBrowserConsole(); > if (hud) { > hud.iframeWindow.focus(); > - return promise.resolve(hud); > + return new Promise(resolve => resolve(hud)); nit: directly return `Promise.resolve(hud)` ::: devtools/client/webconsole/hudservice.js:543 (Diff revision 4) > - destroy() { > + async destroy() { > if (this._destroyer) { > - return this._destroyer.promise; > + return this._destroyer; > } > > + this._destroyer = (async function () { ditto about using an arrow function to be able to not bind to this ::: devtools/client/webconsole/hudservice.js:656 (Diff revision 4) > destroy() { > if (this._bcDestroyer) { > - return this._bcDestroyer.promise; > + return this._bcDestroyer; > } > > + this._bcDestroyer = (async function () { ditto about using an arrow function to be able to not bind to this
Attachment #8948806 -
Flags: review?(nchevobbe) → review+
Comment 19•5 years ago
|
||
mozreview-review |
Comment on attachment 8948807 [details] Bug 1436076 - Part 3 - Miscellaneous HUDService cleanups; https://reviewboard.mozilla.org/r/218170/#review224114 Nice :)
Attachment #8948807 -
Flags: review?(nchevobbe) → review+
Assignee | ||
Comment 20•5 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #18) > Comment on attachment 8948806 [details] Thanks, updated the patch to address nits. > Thanks a lot Brian, this is so much more readable now ! > I only have a few nits, but this is good to land. > > ::: devtools/client/webconsole/hudservice.js:192 > (Diff revision 4) > > - win.addEventListener("DOMContentLoaded", () => { > > + await new Promise(resolve => { > > + win.addEventListener("DOMContentLoaded", resolve, {once: true}); > > + }); > > I wonder if we don't have a helper function for that, and if not, if we > should have one. > That could be helpful We have the `once` function for tests (https://dxr.mozilla.org/mozilla-central/rev/0d806b3230fe4767fa70cdee57db87d1e9a5ba49/devtools/client/framework/test/shared-head.js#347), but I'm not aware of one for the app. With async/await and the `once` option on addEventListeners I'm pretty happy with it as-is, but I could see adding something on devtoolsutils if we wanted to make it even shorter.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•5 years ago
|
||
Not sure what happened with the osx opt build here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c5a776ef33ea0edfe99b7eb623aa6700f63d262. Since everything else looks good and I've seen a green try on a previous version of the patches I'm going to land it.
Comment 25•5 years ago
|
||
Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a2affaecd775 Part 1 - Fix eslint for hudservice.js;r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/6aaa957e550e Part 2 - Use async/await instead of defer in HUDService;r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/ea1b54a019d0 Part 3 - Miscellaneous HUDService cleanups;r=nchevobbe
Comment 26•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a2affaecd775 https://hg.mozilla.org/mozilla-central/rev/6aaa957e550e https://hg.mozilla.org/mozilla-central/rev/ea1b54a019d0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•