Closed Bug 1111601 Opened 8 years ago Closed 8 years ago

Avoid using hiddendomwindow where possible in DevTools code

Categories

(DevTools :: Inspector, defect)

30 Branch
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: miker, Assigned: miker)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
I am interested to see how much shorter the test logs are with these changes.
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.
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 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()
(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
Attachment #8536609 - Attachment is obsolete: true
Attachment #8536609 - Flags: review?(pbrosset)
Attachment #8537066 - Flags: review?(pbrosset)
Removed _validateColor() as we can do the whole validation using a one liner from valid() instead:
```
  get valid() {
    return DOMUtils.isValidCSSColor(this.authored);
  },
```
Attachment #8537066 - Attachment is obsolete: true
Attachment #8537066 - Flags: review?(pbrosset)
Attachment #8537101 - Flags: review?(pbrosset)
(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 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.
Attachment #8537066 - Attachment is obsolete: false
So, bugzilla freaked out a little bit, but didn't loose my review. And it marked the obsolete patch as non obsolete.
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.
Attachment #8537101 - Flags: review?(pbrosset) → review+
Addressed review comments.
Attachment #8537066 - Attachment is obsolete: true
Attachment #8537101 - Attachment is obsolete: true
Attachment #8537815 - Flags: review+
(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.
https://hg.mozilla.org/mozilla-central/rev/766d501a2066
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
(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?
Flags: needinfo?(mratcliffe)
(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.
Flags: needinfo?(mratcliffe)
Blocks: 1112700
No longer depends on: 1112700
Duplicate of this bug: 986003
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.