699419 - Add callback support for expanding/collapsing property tree elements in the debugger

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

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

The debugger's property view needs a way to update the property tree when the user tries to isnpect the children of a node. The requirement is just a callback registration mechanism, like setting onExpand and onCollapse methods in the property. This could be extended to variables and scopes if it would simplify the view code, although there is no imminent need for that.

With this mechanism in place, we can complete the object inspection that is partially fixed in bug 694538, by reusing the code in the latter half of SF_selectFrame to populate the children nodes.

Comment 1

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

Attachment: v1

Any scope, var or property can have the following callback functions set:

.onexpand = function(aSender) {}
.oncollapse = function(aSender) {}
.ontoggle = function(aSender) {}
.onhide = function(aSender) {}
.onshow = function(aSender) {}
.onempty = function(aSender) {}
.onremove = function(aSender) {}

You can then use aSender for additional nesting in the properties tree:

aSender.addProperties(...)

or any other action you may want to perform.

Comment 2

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

Comment on attachment 571738 [details] [diff] [review]
v1

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

Looks good. If we're not doing the work for fixing object inspection here, let's file a followup bug to do so, dependent on this one and bug 694538.

::: browser/devtools/debugger/content/debugger-prefs.js
@@ -110,1 +108,1 @@
> >          getService(Ci.nsIPrefService).getBranch(this._branch);

You could use Services.prefs to simplify this and the following ones.

::: browser/devtools/debugger/content/debugger-view.js
@@ +724,5 @@
>       */
>      element.show = function DVP_element_show() {
>        element.style.display = "block";
> +
> +      if ("function" === typeof element.onshow) {

Just curious: do you know that putting the string literal first makes the comparison faster? I know that this is (was?) true for Java, but I've never heard anything similar for JS.

I haven't spotted this pattern anywhere in Firefox so far.

@@ +739,5 @@
>        element.style.display = "none";
> +
> +      if ("function" === typeof element.onhide) {
> +        element.onhide(element);
> +      }

Did you come up with a use case for the extra callbacks? YAGNI and all that...

Comment 3

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

(In reply to Panos Astithas [:past] from comment #2)
> Comment on attachment 571738 [details] [diff] [review] [diff] [details] [review]
> v1
> 
> Review of attachment 571738 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Looks good. If we're not doing the work for fixing object inspection here,
> let's file a followup bug to do so, dependent on this one and bug 694538.
> 
> ::: browser/devtools/debugger/content/debugger-view.js
> @@ +724,5 @@
> >       */
> >      element.show = function DVP_element_show() {
> >        element.style.display = "block";
> > +
> > +      if ("function" === typeof element.onshow) {
> 
> Just curious: do you know that putting the string literal first makes the
> comparison faster? I know that this is (was?) true for Java, but I've never
> heard anything similar for JS.
> 

It's just a personal habit. I find it more semantic to say "is this the type of that?" than "is type of this that", nothing more.

> I haven't spotted this pattern anywhere in Firefox so far.
> 
> @@ +739,5 @@
> >        element.style.display = "none";
> > +
> > +      if ("function" === typeof element.onhide) {
> > +        element.onhide(element);
> > +      }
> 
> Did you come up with a use case for the extra callbacks? YAGNI and all
> that...

Not quite, just for consistency, since it was very easy to add and didn't really slow things down. I can remove them for now and keep the patch for later use if it would be necessary.

Idea: for example, onshow can be used when the "with" (or "closure"?) scope needs to be made visible and populated.

Comment 4

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

(In reply to Victor Porof from comment #3)
> Idea: for example, onshow can be used when the "with" (or "closure"?) scope
> needs to be made visible and populated.

Yeah, I think that would be necessary then.

Comment 5

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

Comment on attachment 571738 [details] [diff] [review]
v1

As discussed at the meeting, my review should be enough for this.

Comment 6

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

http://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/8f6d95ca4d28