Implement Object grip actor message handling

RESOLVED FIXED

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: past, Assigned: past)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

The remote protocol specification describes the messages that an Object grip actor must respond to:

https://wiki.mozilla.org/Remote_Debugging_Protocol#Property_Descriptors

We have to implement "prototypeAndProperties", "prototype", "property" and "ownPropertyNames".
Created attachment 571064 [details] [diff] [review]
WIP

Almost there. Protocol, unit tests are done. UI integration still has some bugs and no mochitests.
Assignee: nobody → past
Status: NEW → ASSIGNED
Created attachment 571345 [details] [diff] [review]
Working patch

This patch is fully functional, it's just missing mochitests to complement the unit tests.

One thing missing is deep object inspection in the UI (where deep == more than one level). This is because AFAICT the DebuggerView.Properties API expects to be handled the entire tree at once in order to properly generate all the twisties. This case is commonly handled by fetching two more levels besides the current at creation time, in order to get twisties in the current level and its children. But that requires having an expansion callback in the children that will fetch the next level when required. I can't seem to find such an API, so we'll have to file a followup for this. Victor, is this correct or am I just blind?
Attachment #571064 - Attachment is obsolete: true
Attachment #571345 - Flags: feedback?(vporof)
(In reply to Panos Astithas [:past] from comment #2)
I see two concerns regarding this:

1. "DebuggerView.Properties API expects to be handled the entire tree at once"
This is not exactly true, but it depends on how you interpret things. For example, if you do:

var aVar = localScope.addVar("someVar")
aVar.addProperties({"someProp":{"value":{"type":"object","class":"Object"}}});

then you can access someProp, via aVar.someProp, so you can do

aVar.someProp.addProperties(...)

at any time, not only on creation. Twisties are added whenever, and you don't need a empty() or remove() call to add new properties at any given moment.

AFAIC, the real problem is:

2. "having an expansion callback in the children that will fetch the next level when required"
This is indeed not available, thus it makes point 1 more troublesome if you're not saving a list of added variables yourself (which is obviously a bad idea). I guess you can hack it by

aVar.querySelector(".title").addEventListener("DOMAttrModified" ...)

and check for style changes, specifically display from "none" to "block", but this is ugly and (yet again) a bad idea as well.

The best thing to do is file a bug to add callback support for when an element in the properties tree is expanded/collapsed (and also shown/hidden, empty/remove for consistency?), regardless if it is a scope or variable.
For example, a good usecase would be:

aScope/aVar/aProperty .onexpand/.oncollapse/.ontoggle/etc. = function() { ... };

If you need to be able to add more than one callback, please specify it too.
Created attachment 571654 [details] [diff] [review]
Working patch v2

Added browser test, filed followup bug 699419 to fix deep object inspection.
Attachment #571345 - Attachment is obsolete: true
Attachment #571345 - Flags: feedback?(vporof)
Attachment #571654 - Flags: review?(dcamp)
Blocks: 700351
Depends on: 694526
Blocks: 695279
Blocks: 692405

Comment 5

6 years ago
Comment on attachment 571654 [details] [diff] [review]
Working patch v2

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

::: browser/devtools/debugger/content/debugger.js
@@ -237,3 +239,4 @@
> >  
> >        // Add variables for every argument.
> >        let objClient = this.activeThread.pauseGrip(frame.callee);
> > +      let self = this;

Easier to just bind() the callback maybe?

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ -671,0 +671,32 @@
> > +   * Handle a protocol request to provide the names of the properties defined on
> > +   * the object and not its prototype.
> > +   */
> > +  onOwnPropertyNames: function OA_onOwnPropertyNames(aRequest) {
NaN more ...

If proto is undefined, shouldn't we return an undefined grip?  Why is this.threadActor.valueGrip(proto) not sufficient?
Attachment #571654 - Flags: review?(dcamp) → review+
(In reply to Dave Camp (:dcamp) from comment #5)
> Comment on attachment 571654 [details] [diff] [review] [diff] [details] [review]
> Working patch v2
> 
> Review of attachment 571654 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/content/debugger.js
> @@ -237,3 +239,4 @@
> > >  
> > >        // Add variables for every argument.
> > >        let objClient = this.activeThread.pauseGrip(frame.callee);
> > > +      let self = this;
> 
> Easier to just bind() the callback maybe?

I don't mind either way. I'll do that as part of followup bug 700351 which also changes this code, in order to avoid rebasing the rest of the patches in the queue, and the modularization patch in particular.

> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ -671,0 +671,32 @@
> > > +   * Handle a protocol request to provide the names of the properties defined on
> > > +   * the object and not its prototype.
> > > +   */
> > > +  onOwnPropertyNames: function OA_onOwnPropertyNames(aRequest) {
> NaN more ...
> 
> If proto is undefined, shouldn't we return an undefined grip?  Why is
> this.threadActor.valueGrip(proto) not sufficient?

It would have been nice, but it's actually null, not undefined:

https://wiki.mozilla.org/Debugger#Accessor_Properties_of_the_Debugger.Object_prototype

...and there is an explicit mention of returning the proper null grip in the protocol spec:

https://wiki.mozilla.org/Remote_Debugging_Protocol#Finding_An_Object.27s_Prototype_And_Properties
https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/c81b0314c9a2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 8

6 years ago
Shouldn't threadActor.valueGrip(null) return a proper null grip?
(In reply to Dave Camp (:dcamp) from comment #8)
> Shouldn't threadActor.valueGrip(null) return a proper null grip?

You are absolutely right! It should and it does. I must have been blind. Fixed in bug 700351, along with the other requested change.
You need to log in before you can comment on or make changes to this bug.