694538 - Implement Object grip actor message handling

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

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

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".

Comment 1

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

Attachment: WIP

Almost there. Protocol, unit tests are done. UI integration still has some bugs and no mochitests.

Comment 2

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

Attachment: 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?

Comment 3

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

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

Comment 4

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

Attachment: Working patch v2

Added browser test, filed followup bug 699419 to fix deep object inspection.

Comment 5

[Dave Camp (:dcamp)]

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?

Comment 6

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

(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

Comment 7

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

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

Comment 8

[Dave Camp (:dcamp)]

Shouldn't threadActor.valueGrip(null) return a proper null grip?

Comment 9

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

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