676592 - Add a property viewer to the script debugger

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

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

The debugger should display a property viewer for inspecting the frames of a stack trace.

Comment 1

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

Attachment: Wip screenshot 0

First work in progress ideas:
The properties container on the right can show details about variables and/or the inspected stackframe. Each object's properties are listed. If a property is another object, clicking it could show information about that particular object (a.k.a "jump to"). Following Cedric's mockup, the stackframes panel was moved to the left.

Comment 2

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

Nice. I assume that objects will not be expanded initially, otherwise this will get crowded. What about adding a first level grouping of local vs. global properties, the way WebKit does? The former are usually more useful, while the latter can be numerous, so having them grouped separately can help get to the important data faster.

Comment 3

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

(In reply to Panos Astithas [:past] from comment #2)
> Nice. I assume that objects will not be expanded initially, otherwise this
> will get crowded. What about adding a first level grouping of local vs.
> global properties, the way WebKit does? The former are usually more useful,
> while the latter can be numerous, so having them grouped separately can help
> get to the important data faster.

Yes, grouping and expanding objects are necessary and good ideas. Although not so used anymore, how do you think we should handle 'with' blocks?

Comment 4

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

(In reply to Victor Porof from comment #3)
> Yes, grouping and expanding objects are necessary and good ideas. Although
> not so used anymore, how do you think we should handle 'with' blocks?

Good question. WebKit apparently provides a third 'With Scope' top-level group, which seems nice. Try setting a breakpoint in line 10 at:

http://htmlpad.org/debugger-with/

I'm not sure whether SpiderMonkey exposes the necessary information in Debugger.Object or Debugger.Environment though. CCing jorendorff for his thoughts on this.

Comment 5

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

Attachment: Wip screenshot 1

Added a clear separation between global and local scope. Adding a 'with' block should be easy, but I'm waiting for more thoughts on this.

Comment 6

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

The current API to control the property viewer UI (as seen in the screenshot) is:

var globalScope = DebuggerView.PropertyView.addScope("Global"),
    localScope = DebuggerView.PropertyView.addScope("Local"),
    withScope = DebuggerView.PropertyView.addScope("With"),

    windowVar = globalScope.addVar("window"),
    documentVar = globalScope.addVar("document"),
    localVar0 = localScope.addVar("myVariable"),
    localVar1 = localScope.addVar("anotherVariable"),
    localVar2 = localScope.addVar("randomNumber");

windowVar.addProperties({ Type: "object",
                          Class: "Window" });

documentVar.addProperties({ Type: "object",
                            Class: "HTMLDocument" });

localVar0.addProperties({ Type: "object",
                          Class: "Fox",
                          someProp1: "0",
                          someProp2: "\"random string\"",
                          someProp3: "{object Function}",
                          someProp4: "\"another string\"",
                          someProp5: "{object MyCustomObject}" });

localVar1.addProperties({ Type: "object",
                          Class: "Object",
                          someProp1: "31",
                          someProp2: "{object Function}" });

localVar2.addProperties({ Type: "number",
                          Value: "5" });

Any feedback on what might be missing, or any improvements I could make?

Comment 7

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

Attachment: Wip screenshot 2

Added specific coloring based on the variable type and class (the API changed a bit to reflect these additions). Also cleaned up the Type and Class properties to be displayed inline with the variable name to save up vertical space. The scope/variable expand and collapse abilities are now more obvious.

Comment 8

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

Attachment: Wip screenshot 2a

Fixed a small glitch when handling "null" strings (not adding "" quotes)

Comment 9

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

Attachment: Wip patch 0 (no tests yet + populating with dummy variables)

Comment 10

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

(In reply to Victor Porof from comment #8)
> Created attachment 565783 [details]
> Wip screenshot 2a

Very nice. One thing that occurred to me is that it may be better to use the same colors as the orion highlighter (e.g. for string literals), in order to make scanning faster, as the mind won't have to "context switch" between "parsing" colors among the two panes.

Comment 11

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

Attachment: Wip patch 1 (+ tests)

Comment 12

[Dave Camp (:dcamp)]

Comment on attachment 565940 [details] [diff] [review]
Wip patch 1 (+ tests)

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

::: browser/devtools/debugger/content/debugger-view.js
@@ +335,5 @@
> +
> +    // the expand/collapse arrow
> +    arrow.className = "arrow";
> +    arrow.style.visibility = "hidden";
> +    arrow.appendChild(document.createTextNode("►"));

You can use a div with class: inline-block and -moz-appearance: treetwisty/treetwistyopen.

@@ +411,5 @@
> +      if (!scope.isExpanded()) {
> +        scope.expand();
> +      }
> +      else {
> +        scope.collapse();

Mozilla (and devtools module) coding style is } else {

@@ +454,5 @@
> +   *                 optional, an id for the variable html node
> +   *
> +   * @return {Object} the newly created html node representing the added var.
> +   */
> +  addVar: function DVV_addVariable(aScope, aName, aId) {

As we discussed earlier, the distinction between variables and properties here is a bit off - we should be able to dig deeper into object/array properties.

@@ +521,5 @@
> +     * See the DebuggerView.Variables.addProperty function for more info.
> +     *
> +     * @param {Array} aProperty
> +     *                the property defined as a [key, [type, value]] array
> +     *

Would be nice if this were passed in as:
{ key: /key/, value: /grip, as defined in the remote debugging protocol/ }

@@ +786,5 @@
> +  },
> +
> +  /**
> +   * Returns a custom hex color for a type and a value.
> +   *

We should just set appropriate classes on given types/values and let css plug in colors here.  We should avoid hard-coding colors.

Comment 13

[Dave Camp (:dcamp)]

(In reply to Dave Camp (:dcamp) from comment #12)

> You can use a div with class: inline-block and -moz-appearance:
> treetwisty/treetwistyopen.

err, display: inline-block

Comment 14

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

(In reply to Dave Camp (:dcamp) from comment #12)

A few more notes from our talk on IRC:
Instead of -moz-appearance: treetwisty/treetwistyopen, using [0] is more suitable; also, the API requires an .empty() function (depending on the implementation, it may be necessary to recreate the entire property view each frame).

[0] http://hg.mozilla.org/users/dcamp_campd.org/remote-debug/file/d8c599f418f9/browser/themes/pinstripe/browser/devtools/csshtmltree.css#l123

Comment 15

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

Attachment: Wip screenshot 2b

Comment 16

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

Attachment: Property viewer

Comment 17

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

For the record, I'm posting the property viewer API reference:

The viewer creates 3 scopes where variables can be added: 'global', 'local' and 'with block'. The 'with block' is initially hidden. To get these public objects:
var globalScope = DebuggerView.Properties.globalScope,
    localScope = DebuggerView.Properties.localScope,
    withScope = DebuggerView.Properties.withScope;

To create a variable in a scope, use:
var myVar = theScope.addVar(aName);

You can perform two types of operations to the added variable (setting a grip, as defined in [0] and adding properties, as described in [1]). In both cases, if the type is not specified, it will be inferred from the passed params.
myVar.setGrip(aGrip);
myVar.addProperties(aProperties);

ex:
To set the grip (type and value, or type and class of a variable):
myVar.setGrip(42); /* or myVar.setGrip({ "value": 42 });
                    *    myVar.setGrip({ "type": "number", "value": "42" }); */
myVar.setGrip(true);
myVar.setGrip("nasu");
myVar.setGrip({ "type": "undefined" }); 
myVar.setGrip({ "type": "null" });      /* although null is not a type, it is used 
                                         * this way for consistency with 'undefined'
                                         * as specified in the protocol [0] */
myVar.setGrip({ "type": "object", "class": "Object" });

To avoid getting inferred values when setting a grip, use explicit descriptors:
myVar.setGrip({ "type": "string", "value": true }); // value does not imply type
myVar.setGrip({ "type": "string", "value": 42 });

To add properties to a variable:
myVar.addProperties({ "someProp0": { "value": 42 },
                      "someProp1": { "value": true },
                      "someProp2": { "value": "nasu" },
                      "someProp3": { "type": "undefined" },
                      "someProp4": { "type": "null" },
                      "someProp5": { "type": "object", "class": "Object" } });

Properties can be nested any number of levels deep and are saved as a key in the parent object (thus accessible via dot syntax):
myVar.someProp5.addProperties({ "innerProp0": { "value": 42 },
                                "innerProp1": { "value": true },
                                "innerProp2": { "value": "nasu" },
                                "innerProp3": { "type": "undefined" },
                                "innerProp4": { "type": "null" } });

Adding a getter and/or setter property:
myVar.addProperties({ "myProp": { "get": { "type": "object", "class": "Function" },
                                  "set": { "type": "undefined" } } });

Both scopes, variables and properties share the following functions:
.show();     // shows the element
.hide();     // hides the element
.expand();   // expands the element, showing all the added details
.collapse(); // collapses the element, hiding all the added details
.toggle();   // toggles between the element collapse/expand state
.empty();    // removes all added children vars or props from the element
.remove();   // removes the element from the parent var, prop or scope

You can also use the following getters and setters to inspect/modify the state:
myElement.visible  // gets or sets if the element is shown or hidden
myElement.expanded // gets or sets if the element is expanded or collapsed

An in-depth example handling all use cases can be found in the browser_dbg_propertyview-06.js test from the patch.

[0] https://wiki.mozilla.org/Remote_Debugging_Protocol#Grips
[1] https://wiki.mozilla.org/Remote_Debugging_Protocol#Objects

Comment 18

[Dave Camp (:dcamp)]

Comment on attachment 566920 [details] [diff] [review]
Property viewer

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

r- mostly for the grip disagreement with Remote_Debugging_Protocol.

::: browser/devtools/debugger/content/debugger-view.js
@@ +327,5 @@
> +       */
> +      element.addVar = function DVP_scope_addVar(aName) {
> +        return this._addVar(element, aName);
> +      }.bind(this);
> +    }

Can't you do element.addVar = this._addVar.bind(this, element) ?

@@ +366,5 @@
> +                                      aScope.querySelector(".details"));
> +
> +    // make sure the element was created successfully
> +    if (element !== null) {
> +

Let's either early-return in this case, or have _createElement throw if element isn't created.

@@ +421,5 @@
> +   *                is not specified, it will be inferred from the value)
> +   *                e.g. ["someProp0": { value: 42 }]
> +   *                     ["someProp1": { value: true }]
> +   *                     ["someProp2": { value: "nasu" }]
> +   *                     ["someProp3": { type: "undefined" }]

This still doesn't quite match the Remote Debugging Protocol specification.  Is there a reason we can't just pass 42 rather than { value: 42 } ?

@@ +449,5 @@
> +    var element = this._createElement(aName, aId, "property",
> +                                      aVar.querySelector(".details"));
> +
> +    // make sure the element was created successfully
> +    if (element !== null) {

Same thing here, either early return or throw from _createElement().

@@ +650,5 @@
> +   *
> +   * @return {String} the formatted string
> +   */
> +  _propertyString: function DVP_propertyString(aGrip) {
> +    var aType = aGrip["type"],

I think this method will be greatly simplified if we assume the remote debugging protocol's grip format.

@@ +691,5 @@
> +   *
> +   * @return {String} the class style
> +   */
> +  _propertyColor: function DVP_propertyColor(aGrip) {
> +    var aType = aGrip["type"],

... same with this.

@@ +696,5 @@
> +        aValue = aGrip["value"] !== undefined ? aGrip["value"] :
> +                                                aGrip["class"];
> +
> +    if (aType === undefined && aValue === undefined) {
> +      return "token_unknown";

css classes are typically dash-separated.

@@ +738,5 @@
> +   *                 the parent node which will contain the element
> +   *
> +   * @return {Object} the newly created html node representing the generic elem.
> +   */
> +  _createElement: function DVP__createElement(aName, aId, aClass, aParent) {

Can we rename this?  This is much more involved than a typical document.createElement, would be nice for the name to hint at how.

@@ +740,5 @@
> +   * @return {Object} the newly created html node representing the generic elem.
> +   */
> +  _createElement: function DVP__createElement(aName, aId, aClass, aParent) {
> +    // make sure we don't duplicate anything and the parent exists
> +    if (document.getElementById(aId) || !aParent) {

Seems like these should both be pretty vocally complained about (at least a warning to the console?)

@@ +894,5 @@
> +     * @param {Array} aArguments
> +     *                optional arguments array to be applied to aFunction
> +     */
> +    element.refresh = function DVP_element_add(aFunction, aArguments) {
> +      if ("function" === typeof aFunction) {

Function name doesn't match here.

::: browser/devtools/debugger/content/debugger.css
@@ -39,1 @@
>   * ***** END LICENSE BLOCK ***** */

(I'm not going to closely review the css.  We'll have someone that knows what they're doing review the css before we land on m-c).

::: browser/devtools/debugger/content/debugger.js
@@ +167,4 @@
>     * Handler for the thread client's framescleared notification.
>     */
>    onFramesCleared: function SF_onFramesCleared() {
> +    DebuggerView.Stackframes.emptyText("Empty");

Do we have a bug on file yet to property internationalize the UI?

Comment 19

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

(In reply to Dave Camp (:dcamp) from comment #18)
> ::: browser/devtools/debugger/content/debugger.js
> @@ +167,4 @@
> >     * Handler for the thread client's framescleared notification.
> >     */
> >    onFramesCleared: function SF_onFramesCleared() {
> > +    DebuggerView.Stackframes.emptyText("Empty");
> 
> Do we have a bug on file yet to property internationalize the UI?

I thought I did that ages ago, but apparently I didn't. Filed bug 695279.

Comment 20

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

(In reply to Dave Camp (:dcamp) from comment #18)
> Comment on attachment 566920 [details] [diff] [review] [diff] [details] [review]
> Property viewer
> 
> Review of attachment 566920 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> This still doesn't quite match the Remote Debugging Protocol specification. 
> Is there a reason we can't just pass 42 rather than { value: 42 } ?

A quick note here: the API supports both .setGrip(42) and .setGrip({ value: 42 }), so it does match the specification. The true question should be "why support them both?". My concern was regarding the remote debugger protocol handling undefined and null as types, when null is in fact an object. With the current API, one can specify both .setGrip({ value: null, type: object}), and .setGrip({ type: null }) as in the spec. Thus, if the debugger spec changes to exactly match what typeof null and undefined are, the property viewer API won't need to change.
However, if the remote debugger protocol is unlikely to change this soon, supporting them in the property view API is redundant and should be removed.

Comment 21

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

(In reply to Victor Porof from comment #20)
> (In reply to Dave Camp (:dcamp) from comment #18)
> > Comment on attachment 566920 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Property viewer
> > 
> > Review of attachment 566920 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]:
> > -----------------------------------------------------------------
> > 
> > This still doesn't quite match the Remote Debugging Protocol specification. 
> > Is there a reason we can't just pass 42 rather than { value: 42 } ?
> 
> A quick note here: the API supports both .setGrip(42) and .setGrip({ value:
> 42 }), so it does match the specification. The true question should be "why
> support them both?". My concern was regarding the remote debugger protocol
> handling undefined and null as types, when null is in fact an object. With
> the current API, one can specify both .setGrip({ value: null, type:
> object}), and .setGrip({ type: null }) as in the spec. Thus, if the debugger
> spec changes to exactly match what typeof null and undefined are, the
> property viewer API won't need to change.
> However, if the remote debugger protocol is unlikely to change this soon,
> supporting them in the property view API is redundant and should be removed.

What would be the benefit of using the setGrip({ value: null, type: object}) form, instead of the simpler setGrip({ type: null }) one?

Comment 22

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

(In reply to Panos Astithas [:past] from comment #21)
> What would be the benefit of using the setGrip({ value: null, type: object})
> form, instead of the simpler setGrip({ type: null }) one?

No real benefit, just consistency. The idea is that typeof undefined === "undefined" and typeof null === "object", therefore null is not a type like undefined, and treating them the same may be confusing. I agree it's simpler to use { type: undefined } and { type: null } for symmetry, but if I wouldn't be completely familiar with the protocol spec, I would use { value: null, type: object }.

Comment 23

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

Attachment: Property viewer v2

Addressed review comments. Example API usage can be found in browser_dbg_propertyview-06.js

Comment 24

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

https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/27c3e993850f

Comment 25

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

Apparently WebKit also displays a "Closure" scope in some cases. For example, when the execution pauses at the first debugger statement in:

http://htmlpad.org/debugger/

This is in the eval code. I'm not sure whether it's more helpful than putting it in the local scope, nor if our engine will expose that bit of information.

Comment 26

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

Created bug 696830 to manage the "Closure" scope issue.