1111601 - Avoid using hiddendomwindow where possible in DevTools code

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

In the majority of places that we use hiddenDOMWindow it tends to be a hack to get around missing functionality. This tends to create a lot of spew in test logs and can quite dramatically slow down our tools.

We should avoid using these hidden windows where possible and create / use APIs as needed. For now we will just use APIs that are currently available and in subsequent bugs we can make use of other APIs.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1111601-stop-using-hiddenDOMWindow.patch

Try:
https://tbpl.mozilla.org/?tree=Try&rev=b357d00304d4
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b357d00304d4

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1111601-stop-using-hiddenDOMWindow.patch

Try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a85eaf055547
https://tbpl.mozilla.org/?tree=Try&rev=a85eaf055547

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I am interested to see how much shorter the test logs are with these changes.

Comment 4

[Patrick Brosset <:pbro>]

Wow, these changes look great and not having these very annoying hiddenWindow logs when running tests would be amazing.
I'll try and get to the review tomorrow.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

So with these changes there are obvious performance improvements plus the number of lines in test logs goes down from 14900 to 11970 === 2930 (20%) less lines which means faster, easier to debug tests.

I have other patches that give us more APIs that will give us more performance improvements, better CSS property type detection, better autocompletion etc. but I will add them as separate bugs.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8536609 [details] [diff] [review]
1111601-stop-using-hiddenDOMWindow.patch

Review of attachment 8536609 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/css-color.js
@@ +235,5 @@
>      }
>      return this._hslNoAlpha().replace("hsl", "hsla").replace(")", ", 1)");
>    },
>  
> +  _checkSpecialOrValid() {

This should be checkSpecialOrInvalid()

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1111601-stop-using-hiddenDOMWindow.patch

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #6)
> Comment on attachment 8536609 [details] [diff] [review]
> 1111601-stop-using-hiddenDOMWindow.patch
> 
> Review of attachment 8536609 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/css-color.js
> @@ +235,5 @@
> >      }
> >      return this._hslNoAlpha().replace("hsl", "hsla").replace(")", ", 1)");
> >    },
> >  
> > +  _checkSpecialOrValid() {
> 
> This should be checkSpecialOrInvalid()

Fixed

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1111601-stop-using-hiddenDOMWindow.patch

Removed _validateColor() as we can do the whole validation using a one liner from valid() instead:
```
  get valid() {
    return DOMUtils.isValidCSSColor(this.authored);
  },
```

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #8)
> Created attachment 8537101 [details] [diff] [review]
> 1111601-stop-using-hiddenDOMWindow.patch
> 
> Removed _validateColor() as we can do the whole validation using a one liner
> from valid() instead:
> ```
>   get valid() {
>     return DOMUtils.isValidCSSColor(this.authored);
>   },
> ```
This makes sense.
For info, I have started to review the previous patch. I'm almost done, so I'll probably publish that review anyway, see if bugzilla accepts reviews on obsolete patches.
Anyway, if the only difference between the patch I'm reviewing and the one you just uploading is the _validateColor function, then my comments will still apply.

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8537066 [details] [diff] [review]
1111601-stop-using-hiddenDOMWindow.patch

Review of attachment 8537066 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, we need this a lot. Thanks for working on it.
I only have 2 remarks:
- one about the _checkSpecialOrInvalid function which, to me, isn't really defined and used in a way that makes it easy to understand. I've made some suggestions bellow for changing a few things, but you might have better ideas. In any case, this won't require a new review once changed.
- The other thing is about checking for null from colorToRGBA.

::: toolkit/devtools/css-color.js
@@ +235,5 @@
>      }
>      return this._hslNoAlpha().replace("hsl", "hsla").replace(")", ", 1)");
>    },
>  
> +  _checkSpecialOrInvalid() {

It took me a while to understand this function. I think it requires both a new name, a new return value and some comment:

- for the name, maybe something like _getInvalidOrSpecialValue would be better,
- for the return value, if you make it return false if it's neither a special value nor invalid, it would make it more obvious to consumers (and readers) what the function does,
- and finally, a bit of jsdoc comment would be great.

If you do this, the hsla, hsl, rgba, ... getters could be changed to:

get hsla() {
  let invalidOrSpecialValue = this._getInvalidOrSpecialValue();
  if (invalidOrSpecialValue !== false) {
    return invalidOrSpecialValue;
  }
  ...
}

I think this conveys the intent a bit better.

@@ +290,5 @@
>     * Returns a RGBA 4-Tuple representation of a color or transparent as
>     * appropriate.
>     */
>    _getRGBATuple: function() {
> +    let tuple = DOMUtils.colorToRGBA(this.authored);

Note that colorToRGBA returns null if the parameter isn't a recognized color.
Before, with the hidden window implementation, we never had that case, because win.getComputedStyle(span).color would always return a default color.
With this new implementation, we need to check for null.

Comment 11

[Patrick Brosset <:pbro>]

So, bugzilla freaked out a little bit, but didn't loose my review. And it marked the obsolete patch as non obsolete.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8537101 [details] [diff] [review]
1111601-stop-using-hiddenDOMWindow.patch

Review of attachment 8537101 [details] [diff] [review]:
-----------------------------------------------------------------

R+'ing the new patch. See my remarks in the last review.

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1111601-stop-using-hiddenDOMWindow.patch

Addressed review comments.

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8537815 [details] [diff] [review]
1111601-stop-using-hiddenDOMWindow.patch

https://hg.mozilla.org/integration/fx-team/rev/766d501a2066

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #14)
> Comment on attachment 8537815 [details] [diff] [review]
> 1111601-stop-using-hiddenDOMWindow.patch
> 
> https://hg.mozilla.org/integration/fx-team/rev/766d501a2066

I believe this patch causes the Browser Toolbox inspector tab to crash.  See bug 1112700.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/766d501a2066

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to J. Ryan Stinnett [:jryans] from comment #15)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #14)
> > Comment on attachment 8537815 [details] [diff] [review]
> > 1111601-stop-using-hiddenDOMWindow.patch
> > 
> > https://hg.mozilla.org/integration/fx-team/rev/766d501a2066
> 
> I believe this patch causes the Browser Toolbox inspector tab to crash.  See
> bug 1112700.

Mike, should we revert this change?

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to J. Ryan Stinnett [:jryans] from comment #17)
> (In reply to J. Ryan Stinnett [:jryans] from comment #15)
> > (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #14)
> > > Comment on attachment 8537815 [details] [diff] [review]
> > > 1111601-stop-using-hiddenDOMWindow.patch
> > > 
> > > https://hg.mozilla.org/integration/fx-team/rev/766d501a2066
> > 
> > I believe this patch causes the Browser Toolbox inspector tab to crash.  See
> > bug 1112700.
> 
> Mike, should we revert this change?

I will fix it as part of bug 1112700... it is an obvious problem so I am happy to r=me and land it.