Closed Bug 1436076 Opened 3 years ago Closed 3 years ago

Refactor HUDService Browser Console opening/toggling functions

Categories

(DevTools :: Console, defect, P1)

Product:
DevTools
Component:
Console
Type:
defect

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

(Whiteboard: [newconsole-mvp])

Attachments

(3 files, 1 obsolete file)

Bug 1436076 - Part 1 - Fix eslint for hudservice.js;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1436076 - Part 2 - Use async/await instead of defer in HUDService;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1436076 - Part 3 - Miscellaneous HUDService cleanups;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
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: nobody → bgrinstead
Status: NEW → ASSIGNED
Priority: -- → P1
Duplicate of this bug: 1256771
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 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]
Attachment #8948804 - Attachment is obsolete: true
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 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 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+
(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.
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.
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.