794823 - Refactor and move the debugger's PropertyView in shared, so that it can replace PropertyPanel.jsm soon

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Comment 1

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

There will be some overlap here with bug 618320

Comment 2

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

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #1)
> There will be some overlap here with bug 618320

I think they are dupes, actually :) The only concerning aspect is agreeing on a common API between the two implementations. Since the debugger's VariableView will replace the PropertyPanel, we could either

1] Match the VariableView's API as much as reasonable/possible to the PropertyPanel's implementation.

2] Keep the grip-like API of the VariableView and adapt the other tools to use it instead.

No mater which route we take, all of it can be implemented in this bug.
Mike, if this makes sense, I think we should dupe the other bug. Also, we should have a chat on which of the 2 options sounds more reasonable (probably [1] is more convenient for the majority).

Comment 4

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

You don't need to match anything to the current property panel as you would be effectively reimplementing it anyhow.

The only things that matter to me:
- The panel should be capable of being passed an iframe that it can then build itself in.
- It should be filterable
- It should be capable of being passed objects that are then displayed in a tree.

All other implementation details are up to you.

Comment 5

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

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #4)
> You don't need to match anything to the current property panel as you would
> be effectively reimplementing it anyhow.

\o/

> The only things that matter to me:
> - The panel should be capable of being passed an iframe that it can then
> build itself in.

Easy.

> - It should be filterable

Not possible yet. Follow-up?

> - It should be capable of being passed objects that are then displayed in a
> tree.

Ok, I need to put together some docs or examples on how this is done with the debugger's VariableView. You can also take a look in browser_dbg_propertyview-06.js to see how the API looks like. There are events fired for almost every action. Let me know if this scales for you.

Comment 6

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

(In reply to Victor Porof [:vp] from comment #5)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #4)
> > - It should be filterable
> 
> Not possible yet. Follow-up?

definitely.

\o/

Comment 7

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #6)
> (In reply to Victor Porof [:vp] from comment #5)
> > (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #4)
> > > - It should be filterable
> > 
> > Not possible yet. Follow-up?
> 
> definitely.
> 
> \o/

Started working on filtering before this bug, because I'd like to have everything ready before any integration with other tools. Patch in bug 798874 is working, only needs a test, so this shouldn't take long.

Comment 8

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

Attachment: v1, part1

This is probably the smallest patch I ever wrote.

However, there may be more parts that will follow based on the discussion below. The only downsides that I can think of for the current API in the variables view are:

1. No way to pass an object which automagically populates the view. Of course, there are methods for adding scopes, variables and properties, properties for properties and so on, but they all rely on a quite async-esque behavior. I've read how the property panel works (and I may have misunderstood the API), but it seems it can handle being passed an object and be smart enough to populate itself. It's rather trivial to implement such a behavior in the variables view as well, and hide the internal complexities, but please let me know if I'm right about this.

2. In the variables view, a property must always live in a variable, and a variable must live in a scope. While this hierarchy is necessary to be shown in the debugger, we probably don't to show this granularity in other cases. So while the hierarchy will still exist, there will be only one scope and one variable, with properties that can inspected. And because it's rather redundant to do otherwise, the scope and variable headers can be hidden, showing only properties. This is also trivial to implement.

For the webconsole, I believe things are a bit different. Mihai, let me know what you need.

Comment 9

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

(In reply to Victor Porof [:vp] from comment #7)
> Started working on filtering before this bug, because I'd like to have
> everything ready before any integration with other tools. Patch in bug
> 798874 is working, only needs a test, so this shouldn't take long.

Filtering is finished and waiting for review in bug 798874 and bug 793375.

Comment 10

[Mihai Sucan [:msucan]]

(In reply to Victor Porof [:vp] from comment #8)
> Created attachment 676513 [details] [diff] [review]
> v1
> 
> This is probably the smallest patch I ever wrote.
> 
> However, there may be more parts that will follow based on the discussion
> below. The only downsides that I can think of for the current API in the
> variables view are:
> 
> 1. No way to pass an object which automagically populates the view. Of
> course, there are methods for adding scopes, variables and properties,
> properties for properties and so on, but they all rely on a quite
> async-esque behavior. I've read how the property panel works (and I may have
> misunderstood the API), but it seems it can handle being passed an object
> and be smart enough to populate itself. It's rather trivial to implement
> such a behavior in the variables view as well, and hide the internal
> complexities, but please let me know if I'm right about this.

We should have a way to display a local object that doesn't come from the server. I could imagine the quick prototyping of a devtool would not be remoteable from the start and one could want to use this variables view. Take the JSTerm addon example - Paul could use it if he doesn't use the remote debugging protocol yet.

> 2. In the variables view, a property must always live in a variable, and a
> variable must live in a scope. While this hierarchy is necessary to be shown
> in the debugger, we probably don't to show this granularity in other cases.
> So while the hierarchy will still exist, there will be only one scope and
> one variable, with properties that can inspected. And because it's rather
> redundant to do otherwise, the scope and variable headers can be hidden,
> showing only properties. This is also trivial to implement.

Good point. Sounds like we should do this.

> For the webconsole, I believe things are a bit different. Mihai, let me know
> what you need.

The WebConsoleObjectActor is slightly different from the ObjectActor, so I'll expect some small changes to reconcile the two. I can't remember the exact differences from memory - we get to those when we implement the actual Web Console switch to this new VariablesView.jsm. I don't expect much trouble.

Thanks for the patch!

Comment 11

[Mihai Sucan [:msucan]]

Comment on attachment 676513 [details] [diff] [review]
v1, part1

Do you have tests for this new jsm?

Comment 12

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

(In reply to Mihai Sucan [:msucan] from comment #11)
> Comment on attachment 676513 [details] [diff] [review]
> v1
> 
> Do you have tests for this new jsm?

Yes, all the preexisting browser_dbg_propertyview-* tests and the newly added ones from bug 707302. I think there are currently around 17 of them. I plan on adding two more tests for the features described in comment #8.

Comment 13

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

(In reply to Mihai Sucan [:msucan] from comment #10)

> We should have a way to display a local object that doesn't come from the
> server. I could imagine the quick prototyping of a devtool would not be
> remoteable from the start and one could want to use this variables view.
> Take the JSTerm addon example - Paul could use it if he doesn't use the
> remote debugging protocol yet.
> 

Indeed. Something similar to the data setter in the property panel should suffice. Thus, a minimal use of the variables view would only require:

let view = new VariablesView(parentNode);
view.data = { object: myObject };

(no need for a objectProperties)

> Good point. Sounds like we should do this.
> 

Cool.

> The WebConsoleObjectActor is slightly different from the ObjectActor, so
> I'll expect some small changes to reconcile the two. I can't remember the
> exact differences from memory - we get to those when we implement the actual
> Web Console switch to this new VariablesView.jsm. I don't expect much
> trouble.
> 

The current variables view doesn't rely on specific instances of objects being passed in. It defines a standard way of receiving variables and properties pretty much based on object property descriptors. The fact that it's identical to what the debugging protocol offers was makes things convenient on my part, but afaict it should be sane enough to (maybe) not require any translations. We'll see.

> Thanks for the patch!

Thanks for the review!

Comment 14

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

Attachment: v1, part2

All of the extra required functionality and half of the test. Will add more to that test, but uploading the patch here for now.

Comment 15

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

Attachment: v1, part2

Added tests.
Mihai, please also take a quick look at the data setter and let me know if it can handle any additional meta properties required by the webconsole. It mimics the property panel setter, but I hope I'm not missing anything.

Comment 16

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

Try looks pretty green. https://tbpl.mozilla.org/?tree=Try&rev=dd33b55b6f9d

Comment 17

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

Attachment: v1, part3

One inconsistency with the debugger that I noticed when using the data setter shortcut added in part2 was the lack of a __proto__ item added when available. This part3 adds it.

Comment 18

[Mihai Sucan [:msucan]]

Comment on attachment 677314 [details] [diff] [review]
v1, part2

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

Patch looks good to me. Thanks!

::: browser/devtools/debugger/test/browser_dbg_propertyview-data.js
@@ +120,5 @@
> +  let someProp5 = gVariable.get("someProp5");
> +  let someProp6 = gVariable.get("someProp6");
> +  let someProp7 = gVariable.get("someProp7");
> +
> +  is(someProp0.visible, true, "The first property visible state is correct.");

The test looks fine, but all these checks could be less repetitive. ;)

::: browser/devtools/shared/VariablesView.jsm
@@ +47,5 @@
> +   *        - object:
> +   *          This is the raw object you want to display. You can only provide
> +   *          this object if you want the variables view to work in sync mode.
> +   */
> +  set data(aData) {

This bring some legacy from PropertyPanel that I'd like to avoid. In PP I used this setter as a way to pass around the remote object provider (what is called when something is expanded), and other functions (like the method for releasing object actors).

VariablesView usage model is different - onexpand is the way to add more stuff, as the user tries to expand anything.

I suggest a simplification - the setter is defined as:

set rawObject(aRawObject) { ... }

... so you can call populate(aRawObject) directly.

Comment 19

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

(In reply to Mihai Sucan [:msucan] from comment #18)
> Comment on attachment 677314 [details] [diff] [review]
> v1, part2
> 
> Review of attachment 677314 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

Thanks!

Comment 20

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

Attachment: v2

Qfolded everything in a single patch. Asking past for review since robcee is pretty tied up in profiler reviews.

Comment 21

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

Comment on attachment 677487 [details] [diff] [review]
v2

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

Looks good, just make sure you fix the bug in getGrip().

::: browser/devtools/debugger/VariablesView.jsm
@@ +933,5 @@
> +      }
> +    }
> +    // Add the variable's __proto__.
> +    if (prototype) {
> +      this._addRawValueProperty("__proto__ ", {}, prototype);

Could you remind me why we need this trailing space?

@@ +1443,5 @@
> +  if (aValue === null) {
> +    return { type: "null" };
> +  }
> +  if (typeof aValue == "object" || typeof aValue == "function") {
> +    return { type: "object", class: aValue.constructor.name };

aValue.constructor can be undefined here:

var foo = Object.create(null);
typeof foo; // == "object"
foo.constructor; // == undefined

::: browser/themes/pinstripe/devtools/debugger.css
@@ +257,5 @@
>  .property > .title > .value {
>    -moz-padding-start: 6px;
>  }
>  
> +.property:not([non-header]) > .details {

Don't you need to make this change to the ".scope > .details" rule as well?

Comment 22

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

(In reply to Panos Astithas [:past] from comment #21)
> Comment on attachment 677487 [details] [diff] [review]
> v2
> 
> Review of attachment 677487 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, just make sure you fix the bug in getGrip().
> 
> ::: browser/devtools/debugger/VariablesView.jsm
> @@ +933,5 @@
> > +      }
> > +    }
> > +    // Add the variable's __proto__.
> > +    if (prototype) {
> > +      this._addRawValueProperty("__proto__ ", {}, prototype);
> 
> Could you remind me why we need this trailing space?
> 

We don't. We used in the first place because these were properties added to the variable itself (thus accessing child nodes via their name, so var.__proto__ would've been a bad thing to attach), but now we're storing these in maps so there's no need. It's pure legacy style, and I'll remove it.

> @@ +1443,5 @@
> > +  if (aValue === null) {
> > +    return { type: "null" };
> > +  }
> > +  if (typeof aValue == "object" || typeof aValue == "function") {
> > +    return { type: "object", class: aValue.constructor.name };
> 
> aValue.constructor can be undefined here:
> 

Shit. I knew there's something I was missing.

> var foo = Object.create(null);
> typeof foo; // == "object"
> foo.constructor; // == undefined
> 

As a sidenote, did you know that writing Object.create(null) in scratchpad and attempting to inspect it yelds:
Error: TypeError: can't convert null to primitive type or
Error: TypeError: can't convert Object to string or
Error: Error: First argument must have an objectActor or an object property!

Filing bug 807924.

> ::: browser/themes/pinstripe/devtools/debugger.css
> @@ +257,5 @@
> >  .property > .title > .value {
> >    -moz-padding-start: 6px;
> >  }
> >  
> > +.property:not([non-header]) > .details {
> 
> Don't you need to make this change to the ".scope > .details" rule as well?

Not at this point, we don't offset variables in scope, only properties in variables and properties in properties.

Comment 23

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

Attachment: v2.1

Addressed comments.

Comment 24

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

https://hg.mozilla.org/integration/fx-team/rev/36e6c3ed3ca2

Comment 25

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

Filed:
bug 808369 - Use the VariablesView in Scratchpad
bug 808370 - Use the VariablesView in webconsole

Mike, Mihai, what's our strategy? :)

Comment 26

[Mihai Sucan [:msucan]]

(In reply to Victor Porof [:vp] from comment #25)
> Filed:
> bug 808369 - Use the VariablesView in Scratchpad
> bug 808370 - Use the VariablesView in webconsole

Thank you for filing these bugs!

> Mike, Mihai, what's our strategy? :)

Volunteers to take any of the bugs are welcome! ;)

If you have any questions please let me know.

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/36e6c3ed3ca2