786070 - The window object has too many properties to scroll through

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It would be nice if the debugger hid some by default, or maybe included them at the end of the list.

Comment 1

[Victor Porof [:vporof][:vp]]

Maybe hide the non-enumerable globals or move them at the end of the list?
I don't know if we should do this by default though. Could have a toggle switch.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Another option would be to move properties set on the object at the top and leave properties on the prototype chain at the bottom. This seems to make no difference than what Victor suggests for the window object, but it might make more sense for all types of objects. I'd like us to avoid special-casing |window| if possible.

Comment 3

[Rob Campbell [:rc] (:robcee)]

I liked the suggestion of user-defined properties appearing first. That seems like The Thing You Would Want. A toggle to hide non-enumerable globals might be a decent solution too. Another option for a gear menu?

Comment 4

[Anton Kovalyov (:anton)]

I see a property |InstallTrigger| when inspecting a Window object in Firefox but I don't see it when I do the same in Chrome. Is this something Firefox specific (or maybe something NightlyDebug specific?) Also I see |chrome://global/content/bindings/resizer.xml| there.

In general, though, I compared our output with Chrome and Safari and it's not that much different.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Anton Kovalyov (:anton) from comment #4)
> I see a property |InstallTrigger| when inspecting a Window object in Firefox
> but I don't see it when I do the same in Chrome. Is this something Firefox
> specific (or maybe something NightlyDebug specific?) Also I see
> |chrome://global/content/bindings/resizer.xml| there.

InstallTrigger is Firefox-specific, part of the XPInstall API:
https://developer.mozilla.org/en-US/docs/Installing_Extensions_and_Themes_From_Web_Pages

Where exactly do you see the resizer.xml URL? In the script list or as the value of some variable in the variables view?

> In general, though, I compared our output with Chrome and Safari and it's
> not that much different.

That is true, so this is a nice opportunity to innovate :-)

Comment 6

[Anton Kovalyov (:anton)]

> Where exactly do you see the resizer.xml URL? In the script list or
> as the value of some variable in the variables view?

I see it as a property for the window object in the list of global variables.

> That is true, so this is a nice opportunity to innovate :-)

Yep. :)

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Anton Kovalyov (:anton) from comment #6)
> > Where exactly do you see the resizer.xml URL? In the script list or
> > as the value of some variable in the variables view?
> 
> I see it as a property for the window object in the list of global variables.

This sounds like a bug. Can you file it with the steps I can take to reproduce it?

Comment 8

[Anton Kovalyov (:anton)]

> This sounds like a bug. Can you file it with the steps I can take to reproduce it?

There is a bug for that already: 756086. Seems like it's not a debugger specific issue.

Comment 9

[Anton Kovalyov (:anton)]

Attachment: Move user-defined properties to the top of the list

Proof-of-concept. I think it already looks way better (especially for the |window| object) and can be the default and non-configurable option. I will work on playing with the hiding (opt-in) option.

Comment 10

[Anton Kovalyov (:anton)]

Attachment: Moved user-defined properties to the top of the list, added option to hide non-enums completely

Made a checkbox to hide/show non-enumerable properties. I couldn't find a settings menu/dialog so I placed the checkbox next to "pause on exceptions" one. Just as a quick-and-dirty implementation so that we could see how it feels.

Feedback?

Comment 11

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 664301 [details] [diff] [review]
Moved user-defined properties to the top of the list, added option to hide non-enums completely

yeah, that sounds like a good improvement.

We can figure out a proper place to put that checkbox (or just not bother with UI).

anybody have any suggestions there?

Comment 12

[Anton Kovalyov (:anton)]

By not bother with UI, do you mean make it a preference?

Comment 13

[Victor Porof [:vporof][:vp]]

Comment on attachment 664301 [details] [diff] [review]
Moved user-defined properties to the top of the list, added option to hide non-enums completely

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

This is ok, I like it. Apart from a few comments, needs a test and should be ready to go.
We really need to add a gear menu soon :)

::: browser/devtools/debugger/debugger-view.js
@@ +2589,5 @@
> +    if (aFlags && !aFlags.enumerable) {
> +      parent = aScope.childNodes[2];
> +    }
> +    else {
> +      parent = aScope.childNodes[1];

I wouldn't be too sure about the structure of the underlying tree (which may change at any time), so let's use getElementsByClassName. Add specific classes to the children if necessary.

Nit: no newline before "else {", here and everywhere else.

@@ +2847,5 @@
> +      parent = aVar.childNodes[2];
> +    }
> +    else {
> +      parent = aVar.childNodes[1];
> +    }

Previous comment applies here as well. Use getElementsByClassName.

@@ +3127,4 @@
>  
>      let title = document.createElement("box");
>      let details = document.createElement("vbox");
> +    let nonEnum = document.createElement("vbox");

This is a very good thing to do (separating the two types of variables into different containers), good thinking! My initial idea was filtering them beforehand, but this is better.

@@ +3147,4 @@
>  
>      // The node element which will contain any added scope variables.
>      details.className = "details";
> +    nonEnum.className = "details nonenum";

Maybe we could make this delimitation obvious? Like adding ".nonenum { border-top: 1px dotted #666; }" or something similar in the theme css.

@@ +3217,4 @@
>        if (!aSkipAnimationFlag) {
>          details.setAttribute("animated", "");
> +
> +        if (!this._hideNonEnums) {

I think it's safe to loose this conditional here. It doesn't affect visibility, and it's an attribute I'd like to retain regardless of the current open state.

@@ +3272,5 @@
>       */
>      element.showArrow = function DVP_element_showArrow() {
> +      let len = details.childNodes.length + nonEnum.childNodes.length;
> +
> +      if (element._forceShowArrow || len) {

Nit: inline the length here and put it on the next line, since we're not using it anywhere else in the function.

if (element._forceShowArrow ||
    details.childNodes.length + nonEnum.childNodes.length) {
  ...
}

@@ +3471,4 @@
>  
>        let node = aParent.parentNode;
>        let arrow = node.getElementsByClassName("arrow")[0];
> +      let children = node.querySelectorAll(".details > vbox").length;

Nice.

@@ +3592,5 @@
> +  _hideNonEnums: false,
> +
> +  _onHideNonEnums: function DVP__onHideNonEnums() {
> +    let option = document.getElementById("hide-nonenum");
> +    this._hideNonEnums = option.checked;

It'll be good to have a pref for this. I probably don't want to keep checking/unchecking that thing each time I start the debugger.

See how prefs are implemented in debugger-controller.js, and restore the value on initialization. You need to add the default value (probably false, I guess?) in firefox.js, in browser/app/profile/

@@ +3594,5 @@
> +  _onHideNonEnums: function DVP__onHideNonEnums() {
> +    let option = document.getElementById("hide-nonenum");
> +    this._hideNonEnums = option.checked;
> +
> +    var els = document.querySelectorAll(".details.nonenum");

Let's avoid expensive queries. How about:
this._vars.getElementsByClassName("details nonenum")

@@ +3596,5 @@
> +    this._hideNonEnums = option.checked;
> +
> +    var els = document.querySelectorAll(".details.nonenum");
> +
> +    for (var i = 0; i < els.length; i++) {

let i = this._vars.getElementsByClassName("details nonenum").iterator();

for (let element of i) {
}

https://developer.mozilla.org/en-US/docs/JavaScript/Guide/Iterators_and_Generators
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for...of

;)

@@ +3611,5 @@
>    /**
>     * Initialization function, called when the debugger is initialized.
>     */
>    initialize: function DVP_initialize() {
> +    let hideNonEnums = document.getElementById("hide-nonenum");

Might want to cache those nodes, at least to keep the same code patterns throughout the file.

::: browser/devtools/debugger/debugger.xul
@@ +93,5 @@
>                  label="&debuggerUI.pauseExceptions;"/>
> +      <checkbox id="hide-nonenum"
> +                type="checkbox"
> +                tabindex="0"
> +                label="Hide non-enumerable properties"/>

String should be in debugger.dtd, but I'm sure you knew about this :)

Comment 14

[Anton Kovalyov (:anton)]

Thanks!

> I wouldn't be too sure about the structure of the underlying tree
> (which may change at any time), so let's use getElementsByClassName.
> Add specific classes to the children if necessary.

This was my initial approach but I couldn't figure out how to query elements only when they are direct children of the current element:


vbox: [x]
 vbox.details:
   vbox:
     vbox.details
     vbox.nonenum [z]
   ...
 vbox.nonenum: [y]
   vbox:
     vbox.details
     vbox.nonenum

So with this structure, if I hold a reference to an element [x], I can't query for [y] because both |getElementsByClassName("nonenum")| and |querySelector(".nonenum")| always return a reference to [z] instead of [y]. jQuery has "> .nonenum" syntax (note the lack of a left-hand operand) but its a jQuery-specific hack AFAIK.

> Nit: no newline before "else {", here and everywhere else.

Uhm, Mihai made me do this in previous patches. Who to listen? :)

> Let's avoid expensive queries.

I didn't know querySelector is expensive.

Comment 15

[Victor Porof [:vporof][:vp]]

(In reply to Anton Kovalyov (:anton) from comment #14)
> Thanks!
> 
> > I wouldn't be too sure about the structure of the underlying tree
> > (which may change at any time), so let's use getElementsByClassName.
> > Add specific classes to the children if necessary.
> 
> This was my initial approach but I couldn't figure out how to query elements
> only when they are direct children of the current element:
> 
> vbox: [x]
>  vbox.details:
>    vbox:
>      vbox.details
>      vbox.nonenum [z]
>    ...
>  vbox.nonenum: [y]
>    vbox:
>      vbox.details
>      vbox.nonenum
> 
> So with this structure, if I hold a reference to an element [x], I can't
> query for [y] because both |getElementsByClassName("nonenum")| and
> |querySelector(".nonenum")| always return a reference to [z] instead of [y].
> jQuery has "> .nonenum" syntax (note the lack of a left-hand operand) but
> its a jQuery-specific hack AFAIK.

Not really it's a standard pattarn [0]. You can safely element.querySelector(".parent > .firstChild).

> 
> > Nit: no newline before "else {", here and everywhere else.
> 
> Uhm, Mihai made me do this in previous patches. Who to listen? :)

It's a nit, so perhaps nobody? :)
All joking aside, we generally tend to keep the same style throughout a file or module. But don't worry too much about it.

> 
> > Let's avoid expensive queries.
> 
> I didn't know querySelector is expensive.

Indeed, it's a very tiny optimization, but querySelector would be redundant since only a class name is queried. Yes, getElementsByClassName is a mouthful, but a more direct search :)

[0] http://www.w3.org/TR/CSS2/selector.html

Comment 16

[Victor Porof [:vporof][:vp]]

s/pattarn/pattern. DAARP.

Comment 17

[Rob Campbell [:rc] (:robcee)]

(In reply to Anton Kovalyov (:anton) from comment #14)
> Thanks!
> 
> > I wouldn't be too sure about the structure of the underlying tree
> > (which may change at any time), so let's use getElementsByClassName.
> > Add specific classes to the children if necessary.
> 
> This was my initial approach but I couldn't figure out how to query elements
> only when they are direct children of the current element:
> 
> 
> vbox: [x]
>  vbox.details:
>    vbox:
>      vbox.details
>      vbox.nonenum [z]
>    ...
>  vbox.nonenum: [y]
>    vbox:
>      vbox.details
>      vbox.nonenum
> 
> So with this structure, if I hold a reference to an element [x], I can't
> query for [y] because both |getElementsByClassName("nonenum")| and
> |querySelector(".nonenum")| always return a reference to [z] instead of [y].
> jQuery has "> .nonenum" syntax (note the lack of a left-hand operand) but
> its a jQuery-specific hack AFAIK.

that's... hard.

I think if you know your depth, you could nth-child pseudo-class selectors.

Failing that, I might start asking myself if these class definitions are strict enough for what you're trying to do. Of course, it's entirely possible you're in a place where you don't want to mess with the classes for other reasons.

> > Nit: no newline before "else {", here and everywhere else.
> 
> Uhm, Mihai made me do this in previous patches. Who to listen? :)

In general we try to maintain consistency across files. In this case, I'd defer to Victor who wrote a big chunk of debugger-view.js. I'm always available for tie-breakers! ;)

> > Let's avoid expensive queries.
> 
> I didn't know querySelector is expensive.

I wouldn't think querySelector for a number of classes would be noticeably more expensive than getElementsByClassname. Certainly, you can construct a more expensive query if you do things like ancestor selection and *, but I didn't see that here.

Victor, can you elaborate?

Comment 18

[Victor Porof [:vporof][:vp]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #17)
> (In reply to Anton Kovalyov (:anton) from comment #14)
> that's... hard.
> 
> I think if you know your depth, you could nth-child pseudo-class selectors.
> 

Since each node in the tree has a specific id assigned based on the path-to-root:
querySelector("#" + aVar.id + " > .nonenum")
to avoid the lack of a left-hand operand and get the direct .nonenum child?

You're right, it's a bit awkward. Let's just keep the aVar.childNodes[index] for now and if it will ever cause any trouble, we'll deal with it later on.

> I wouldn't think querySelector for a number of classes would be noticeably
> more expensive than getElementsByClassname. Certainly, you can construct a
> more expensive query if you do things like ancestor selection and *, but I
> didn't see that here.
> 
> Victor, can you elaborate?

Yes, in this case it *seems* like a premature optimization. In theory, the performance differences are massive:
http://jsperf.com/getelementbyid-vs-queryselector or
jsperf.com/getelementbyid-vs-getelementsbyclassname

Those tests may be poorly written, so I don't have actual real debuggerUI related numbers to back my wild claims :) So I'll just defer to my non-argumented preference that
querySelector*(".somthing") is harder to read that
getElementsByClassName("something")

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 664301 [details] [diff] [review]
Moved user-defined properties to the top of the list, added option to hide non-enums completely

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

Yes, this is a definite improvement. I agree that keeping user-defined-properties-on-top non-configurable is best. We'll merge the two checkboxes in a gear button in bug 796148.

Comment 20

[Anton Kovalyov (:anton)]

Attachment: "Show hidden properties" feature

Patch ready for review. I changed "Hide non-enumerable properties" to "Show hidden properties", I think it looks simpler and better this way. The checkbox is now persistent, I save its state into a pref variable. Also I had to update a few tests because they were assuming that all variables/properties are in one |vbox| element.

Comment 21

[Anton Kovalyov (:anton)]

I think I addressed all comments from above, let me know if I missed anything.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 667286 [details] [diff] [review]
"Show hidden properties" feature

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

I found one bug with this patch and I have comments for a few minor issues. The bug is that the toggle does not appear to work for variables not yet expanded. STR:

1) Open http://htmlpad.org/debugger/
2) Click 'Click me'
3) Expand aArg
4) Toggle checkbox
5) Expand cArg
6) Observe that the updated checkbox state does not affect the display of cArg

::: browser/app/profile/firefox.js
@@ +1040,5 @@
>  pref("devtools.debugger.ui.variables-width", 300);
>  pref("devtools.debugger.ui.variables-pane-visible", true);
>  
> +// Debugger: show non-enumerable properties and variables
> +pref("devtools.debugger.non-enum-visible", false);

devtools.debugger.ui.non-enum-visible would fit better with the rest of the prefs. I think we should keep true as the default value, and have advanced users look for the toggle.

You will also need a browser peer to review the changes to this file.

::: browser/devtools/debugger/debugger-view.js
@@ +17,4 @@
>  const SEARCH_LINE_FLAG = ":";
>  const SEARCH_TOKEN_FLAG = "#";
>  
> +Cu.import("resource://gre/modules/Services.jsm");

This is redundant, since debugger-view.js shares the same global with debugger-controller.js, which already loads the services module.

@@ +3592,5 @@
> +
> +  _onShowNonEnums: function DVP__onShowNonEnums() {
> +    let option = document.getElementById("show-nonenum");
> +    this._nonEnumsVisible = option.checked;
> +    Services.prefs.setBoolPref("devtools.debugger.non-enum-visible",

We use a Prefs object defined in the controller to handle preferences. Could you move the handling for this pref over there for consistency?

Comment 23

[Anton Kovalyov (:anton)]

Attachment: "Show hidden properties" feature

I removed internal |_nonEnumsVisible| property in favor of |Pref.noEnumsVisible| since we're caching these values anyway. This change also fixed a bug you described above.

Who from the browser team should I ping for a review?

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

The patch seems unchanged, are you sure you qref'd?

Browser peers are listed here: https://wiki.mozilla.org/Modules/Firefox
This is a trivial change so it doesn't really matter, but if you can find someone who is local to SF, you will have the opportunity to make more friends :-)

Comment 25

[Anton Kovalyov (:anton)]

Attachment: "Show hidden properties" feature

You are right, my bad.

Comment 26

[Jason Orendorff [:jorendorff]]

(In reply to Victor Porof [:vp] from comment #13)
> @@ +3596,5 @@
> > +    var els = document.querySelectorAll(".details.nonenum");
> > +
> > +    for (var i = 0; i < els.length; i++) {
> 
> let i = this._vars.getElementsByClassName("details nonenum").iterator();
> 
> for (let element of i) {
> }

You don't need to call .iterator(); the for-of loop does it for you.

This will work:

    for (let element of document.querySelectorAll(".details.nonenum")) {
      ...
    }

Comment 27

[Robert Strong (they/them - no direct email)]

Comment on attachment 667999 [details] [diff] [review]
"Show hidden properties" feature

I won't be able to get to this review anytime soon. Moving request over to gavin so he can either review it or assign it to someone else.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 667999 [details] [diff] [review]
"Show hidden properties" feature

It's generally a bad idea to combine many whitespace changes and substantive changes - it makes future blame lookup and patch reviewing a lot more annoying. I'm fine with the changes, but do them as part of a preliminary patch, with the substantive change on top of it.

For future reference, I don't think you need a browser peer to sign off on devtools-specific default prefs being added to firefox.js, FWIW.

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #28)
> For future reference, I don't think you need a browser peer to sign off on
> devtools-specific default prefs being added to firefox.js, FWIW.


Thanks, I wasn't sure if that was the case or not.

Comment 30

[Anton Kovalyov (:anton)]

Attachment: "Show hidden properties" feature

Removed whitespace changes from |firefox.js|. Everything else is the same.

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/646f2d44e019

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/646f2d44e019