Last Comment Bug 694538 - Implement Object grip actor message handling
: Implement Object grip actor message handling
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Panos Astithas [:past]
:
:
Mentors:
Depends on: 694526
Blocks: minotaur 692405 695279 700351
  Show dependency treegraph
 
Reported: 2011-10-14 05:50 PDT by Panos Astithas [:past]
Modified: 2011-11-17 02:01 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (13.60 KB, patch)
2011-11-01 11:01 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch (20.42 KB, patch)
2011-11-02 09:02 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch v2 (24.57 KB, patch)
2011-11-03 08:31 PDT, Panos Astithas [:past]
dcamp: review+
Details | Diff | Splinter Review

Description Panos Astithas [:past] 2011-10-14 05:50:13 PDT
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 [:past] 2011-11-01 11:01:15 PDT
Created attachment 571064 [details] [diff] [review]
WIP

Almost there. Protocol, unit tests are done. UI integration still has some bugs and no mochitests.
Comment 2 Panos Astithas [:past] 2011-11-02 09:02:44 PDT
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?
Comment 3 Victor Porof [:vporof][:vp] 2011-11-02 10:23:36 PDT
(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 [:past] 2011-11-03 08:31:41 PDT
Created attachment 571654 [details] [diff] [review]
Working patch v2

Added browser test, filed followup bug 699419 to fix deep object inspection.
Comment 5 Dave Camp (:dcamp) 2011-11-15 14:19:50 PST
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 [:past] 2011-11-16 03:43:47 PST
(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 8 Dave Camp (:dcamp) 2011-11-16 13:24:36 PST
Shouldn't threadActor.valueGrip(null) return a proper null grip?
Comment 9 Panos Astithas [:past] 2011-11-17 02:01:13 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.