1436076 - Refactor HUDService Browser Console opening/toggling functions

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Brian Grinstead [:bgrins]]

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.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1436076 - temp changeset until this hits mc

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

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: Bug 1436076 - Part 1 - Fix eslint for hudservice.js;

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

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: Bug 1436076 - Part 2 - Use async/await instead of defer in HUDService;

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

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: Bug 1436076 - Part 3 - Miscellaneous HUDService cleanups;

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

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8948804 [details]
Bug 1436076 - temp changeset until this hits mc

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

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8948805 [details]
Bug 1436076 - Part 1 - Fix eslint for hudservice.js;

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

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8948806 [details]
Bug 1436076 - Part 2 - Use async/await instead of defer in HUDService;

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

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8948807 [details]
Bug 1436076 - Part 3 - Miscellaneous HUDService cleanups;

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

Comment 10

[Code Review Bot [:reviewbot]]

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

[Code Review Bot [:reviewbot]]

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 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8948805 [details]
Bug 1436076 - Part 1 - Fix eslint for hudservice.js;

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

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8948806 [details]
Bug 1436076 - Part 2 - Use async/await instead of defer in HUDService;

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

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8948807 [details]
Bug 1436076 - Part 3 - Miscellaneous HUDService cleanups;

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

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8948806 [details]
Bug 1436076 - Part 2 - Use async/await instead of defer in HUDService;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218168/diff/3-4/

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8948807 [details]
Bug 1436076 - Part 3 - Miscellaneous HUDService cleanups;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218170/diff/3-4/

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

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 !

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8948807 [details]
Bug 1436076 - Part 3 - Miscellaneous HUDService cleanups;

https://reviewboard.mozilla.org/r/218170/#review224114

Nice :)

Comment 20

[Brian Grinstead [:bgrins]]

(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 21

[Brian Grinstead [:bgrins]]

Comment on attachment 8948805 [details]
Bug 1436076 - Part 1 - Fix eslint for hudservice.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218166/diff/3-4/

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8948806 [details]
Bug 1436076 - Part 2 - Use async/await instead of defer in HUDService;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218168/diff/4-5/

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 8948807 [details]
Bug 1436076 - Part 3 - Miscellaneous HUDService cleanups;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218170/diff/4-5/

Comment 24

[Brian Grinstead [:bgrins]]

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

[Pulsebot]

Pushed by [e-mail]:
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

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/a2affaecd775
https://hg.mozilla.org/mozilla-central/rev/6aaa957e550e
https://hg.mozilla.org/mozilla-central/rev/ea1b54a019d0