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

RESOLVED FIXED

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: past, Assigned: vporof)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
(Assignee)

Updated

6 years ago
Assignee: nobody → vporof
(Assignee)

Comment 1

6 years ago
Created attachment 571738 [details] [diff] [review]
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.
Attachment #571738 - Flags: review?(dcamp)
Attachment #571738 - Flags: feedback?(past)
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...
Attachment #571738 - Flags: feedback?(past) → feedback+
(Assignee)

Comment 3

6 years ago
(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.
(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 on attachment 571738 [details] [diff] [review]
v1

As discussed at the meeting, my review should be enough for this.
Attachment #571738 - Flags: review?(dcamp) → review+
Blocks: 700351
(Assignee)

Comment 6

6 years ago
http://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/8f6d95ca4d28
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.