Closed Bug 810966 Opened 7 years ago Closed 6 years ago

Display closed over variables in the variables view

Categories

(DevTools :: Object Inspector, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: past, Assigned: past)

References

Details

Attachments

(2 files, 12 obsolete files)

38.77 KB, image/png
Details
64.95 KB, patch
past
: review+
Details | Diff | Splinter Review
The Debugger API mentions that ECMAScript environments form a tree:
https://wiki.mozilla.org/Debugger#Debugger.Environment

Debugger.Environment objects are associated with functions and stack frames, however we only display the list of environments associated with the selected stack frame in the debugger frontend. We should add the ability to inspect environments associated with functions when the user expands a function object in the variables view, in the same vein as FireClosure.
Priority: -- → P3
As for a more concrete case, the following page has an example that can be used to test the implementation against:

http://jsbin.com/iqijok/1

Setting a breakpoint at line 29 and then clicking the 'test' button, should pause execution and display the value of the 'person' object in the variable view. Expanding any of the function properties of 'person' should present an additional 'closure' pseudo-property or similar, that will contain the variables inside that closure: name, age and foo.

Or maybe we should reuse the existing scope labels and call it 'Function scope [getFoo]' or similar. Links (properties) to any existing parent scopes should be displayed as well.
Summary: Display function scopes in the variable view → Display closed over variables in the variables view
Attached patch Patch v1 (obsolete) — Splinter Review
Works, but needs tests.
Assignee: nobody → past
Status: NEW → ASSIGNED
Attached patch Patch v2 (obsolete) — Splinter Review
Bug 828680 heavily bitrotted this. I've rebased it, but there are still some things to fix.
Attachment #759172 - Attachment is obsolete: true
Attached patch Patch v3 (obsolete) — Splinter Review
Mostly works in the debugger, but getting the web console to show closures properly needs more work. Scratchpad hasn't integrated the variables view controller yet, so I'll wait for that first.

I ended up implementing a few more methods in the debugger client for requests that were unused until now, but which this patch now needs. I expect to actually use the "assign" protocol request in a followup patch though. I can live with a read-only view of closures for now.
Attachment #764278 - Attachment is obsolete: true
Attached patch Patch v4 (obsolete) — Splinter Review
Finishing touches to the debugger, no more errors logged. Output is comparable to Chrome's. Next stop: web console.
Attachment #764914 - Attachment is obsolete: true
Attached patch Patch v5 (obsolete) — Splinter Review
This patches lets the web console continue working albeit in a suboptimal way, by disabling closure inspection in the console's property inspector. The problem is that the web console relies on non pause-scoped object actors that can't support "scope" requests. The same will hold for Scratchpad as well, since it will reuse the web console backend.

I'm not happy about this and I'm going to think about what it would take to make things just work everywhere, but it may require substantial refactoring in the backend. All tests pass with this patch, but I haven't created new ones yet.
Attachment #765568 - Attachment is obsolete: true
Comment on attachment 766041 [details] [diff] [review]
Patch v5

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

::: browser/devtools/debugger/debugger-controller.js
@@ +78,5 @@
>          getGripClient: aObject => {
>            return this.activeThread.pauseGrip(aObject);
> +        },
> +        getEnvironmentClient: aObject => {
> +          return this.activeThread.environment(aObject);

All VView initialization stuff has moved in a single function (_initializeVariablesView) in debugger-view.js. Just letting you know, and sorry for making you rebase.
Moving stuff to the Object Inspector component.
Filter on COSMICMIGRAINE.
Component: Developer Tools: Debugger → Developer Tools: Object Inspector
I managed to get closures to appear in both the web console and scratchpad embeddings. I'm adding tests now, but this should be close to review soon.
Attachment #766041 - Attachment is obsolete: true
Added protocol tests. Next up: browser mochitests.
Attachment #781189 - Attachment is obsolete: true
I believe this is now ready for review. I added xpcshell unit tests and debugger browser mochitests and I'm reasonably confident that there are no more obvious bugs. I would still like to add a couple of console and scratchoad mochitests similar to the debugger one, but I won't have the time in the immediate future and this shouldn't hold up the review.

I'd like Victor to take a good look at the VariablesView changes and Mihai to look at the changes made to accommodate the web console. I have an idea for a future refactoring for the debugger/console actors that we can discuss during the workweek, but I think these changes are in accordance with the our current way of doing things. Rob, you get all the rest :-)

I sneaked in a couple of warning fixes, please don't hate me!
Attachment #782622 - Attachment is obsolete: true
Attachment #784464 - Flags: review?(vporof)
Attachment #784464 - Flags: review?(rcampbell)
Attachment #784464 - Flags: review?(mihai.sucan)
Comment on attachment 784464 [details] [diff] [review]
Display closed over variables in the variables view v8

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

The changed you asked me to look are exquisite. I just have a small suggestion about how the <Closure> pseudo property is displayed: it looks to me like it's very hard to distinguish, especially since it's considered nonconfigurable, nonenumerable and nonwritable (is this even true?) so it's semitransparent. I think some special style handling should be done here, just like with <exception> and <return>.

::: browser/devtools/debugger/debugger-toolbar.js
@@ -642,5 @@
>  
>  /**
> - * Utility functions for handling stackframes.
> - */
> -let StackFrameUtils = {

IMHO we should really just have some utils file for all of this, because it seems to be frantically moving from place to place. Some other time, perhaps.

::: browser/devtools/debugger/test/browser_dbg_closure-inspection.js
@@ +49,5 @@
> +      is(personNode.expanded, true, "person should be expanded at this point.");
> +
> +      // Poll every few milliseconds until the properties are retrieved.
> +      // It's important to set the timer in the chrome window, because the
> +      // content window timers are disabled while the debuggee is paused.

Oh man, not this again... Don't worry about it, I'm doing something better in bug 876277.

::: browser/devtools/shared/widgets/VariablesViewController.jsm
@@ +45,5 @@
>   *        - simpleValueEvalMacro: callback for creating a simple value eval macro
>   */
>  function VariablesViewController(aView, aOptions) {
>    this.addExpander = this.addExpander.bind(this);
> +  this._addVarScope = this._addVarScope.bind(this);

Why bind this?
Attachment #784464 - Flags: review?(vporof) → review+
Comment on attachment 784464 [details] [diff] [review]
Display closed over variables in the variables view v8

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

This is looking fine - wrt. web console-related changes. I couldn't test the patch - it needs a rebase.

::: toolkit/devtools/server/actors/webconsole.js
@@ +227,5 @@
> +   *        The lexical environment we want to extract.
> +   * @return The EnvironmentActor for aEnvironment or undefined for host
> +   *         functions or functions scoped to a non-debuggee global.
> +   */
> +  createEnvironmentActor: function WCA_createEnvironmentActor(aEnvironment) {

We should mention in a comment this is a "sister"/clone of TA_createEnvironmentActor(), or just reuse that function.
Attachment #784464 - Flags: review?(mihai.sucan) → review-
Attached image closures.png
(In reply to Victor Porof [:vp] from comment #12)
> The changed you asked me to look are exquisite. I just have a small
> suggestion about how the <Closure> pseudo property is displayed: it looks to
> me like it's very hard to distinguish, especially since it's considered
> nonconfigurable, nonenumerable and nonwritable (is this even true?) so it's
> semitransparent. I think some special style handling should be done here,
> just like with <exception> and <return>.

OK, how does this look?
I like it!
This is wonderful.
Addressed review comments, rebased and added a web console test. This exposed a SpiderMonkey bug that I will file a bug for, but I've worked around it in the test for now.
Attachment #784464 - Attachment is obsolete: true
Attachment #784464 - Flags: review?(rcampbell)
Attachment #791470 - Flags: review?(rcampbell)
Attachment #791470 - Flags: review?(mihai.sucan)
Attached patch Changes between v8 and v9 (obsolete) — Splinter Review
...for those who already took a look in v8 of the patch.
Comment on attachment 791470 [details] [diff] [review]
Display closed over variables in the variables view v9

Thanks for the updates. r+

Also, now I see that the v8 patch has an r- from me. That was a mistake - I did mean to give it r+ (in the comments I did not even mention any serious problems with the patch). I believe that the review flag field had focus when I pressed up/down arrow keys for scroll and I did not notice the selection changed to "-". Apologies for the error.
Attachment #791470 - Flags: review?(mihai.sucan) → review+
Comment on attachment 784464 [details] [diff] [review]
Display closed over variables in the variables view v8

(change to reflect actual intent)
Attachment #784464 - Flags: review- → review+
Comment on attachment 791470 [details] [diff] [review]
Display closed over variables in the variables view v9

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

I literally can't review this right now. Victor, did you review the whole thing? If so, I'm ok with your review.
Attachment #791470 - Flags: review?(rcampbell)
I reviewed everything except console changes, which seem to have been looked at by msucan.
Rebased and mentioned bug 912536 in the workaround comment. Carrying over r+.
Attachment #791470 - Attachment is obsolete: true
Attachment #791471 - Attachment is obsolete: true
Attachment #799534 - Flags: review+
Forgot to mention that all tests pass locally.
No longer blocks: 912536
Depends on: 912536
Rebased, modified the web console test to work when execution is paused and rewritten the debugger mochitest in the New Promisified World Order.
Comment on attachment 809910 [details] [diff] [review]
Display closed over variables in the variables view for functions that are not stack frames v11

Carry over r+.
Attachment #809910 - Flags: review+
Attachment #799534 - Attachment is obsolete: true
Comment on attachment 809910 [details] [diff] [review]
Display closed over variables in the variables view for functions that are not stack frames v11

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

::: browser/devtools/debugger/test/browser_dbg_closure-inspection.js
@@ +38,5 @@
> +    return waitForDebuggerEvents(gPanel, gDebugger.EVENTS.FETCHED_SCOPES, 1).then(() => {
> +      let deferred = promise.defer();
> +      executeSoon(() => {
> +        let localScope = gDebugger.DebuggerView.Variables._list.querySelectorAll(".variables-view-scope")[0],
> +            globalScope = gDebugger.DebuggerView.Variables._list.querySelectorAll(".variables-view-scope")[1],

You can now use Variables.getScopeAtIndex if you want.

@@ +58,5 @@
> +
> +        // Poll every few milliseconds until the properties are retrieved.
> +        // It's important to set the timer in the chrome window, because the
> +        // content window timers are disabled while the debuggee is paused.
> +        let count1 = 0;

There are much nicer ways of doing this now, if you take a look at the other variables view tests, for example browser_dbg_variables-view-filter-01.js. Not saying that you should necessarily change this, but it'd certainly be nice to not rely on setTimeouts.

::: browser/devtools/debugger/test/browser_dbg_variables-view-filter-01.js
@@ -81,5 @@
>      isnot(globalScope.target.querySelectorAll(".variables-view-variable:not([non-match])").length, 0,
>        "There should be some variables displayed in the global scope.");
>  
> -    is(localScope.target.querySelectorAll(".variables-view-property:not([non-match])").length, 3,
> -      "There should be 3 properties displayed in the local scope.");

Is it possible change this to test the new value instead of simply removing it?
Thanks for looking into this.

(In reply to Victor Porof [:vp] from comment #27)
> You can now use Variables.getScopeAtIndex if you want.

Done.

> There are much nicer ways of doing this now, if you take a look at the other
> variables view tests, for example browser_dbg_variables-view-filter-01.js.
> Not saying that you should necessarily change this, but it'd certainly be
> nice to not rely on setTimeouts.

I don't know if I'll find the time to do this, but I'll try.

> ::: browser/devtools/debugger/test/browser_dbg_variables-view-filter-01.js
> @@ -81,5 @@
> >      isnot(globalScope.target.querySelectorAll(".variables-view-variable:not([non-match])").length, 0,
> >        "There should be some variables displayed in the global scope.");
> >  
> > -    is(localScope.target.querySelectorAll(".variables-view-property:not([non-match])").length, 3,
> > -      "There should be 3 properties displayed in the local scope.");
> 
> Is it possible change this to test the new value instead of simply removing
> it?

I don't believe this is a worthy test. The extra properties are part of the closed-over variables in global scope, so they include constructors from various non-standard WebAPIs that will likely disappear in the future, or be acoompanied by constructors from new WebAPIs, making this test a moving target.
Attachment #809910 - Attachment is obsolete: true
Comment on attachment 809957 [details] [diff] [review]
Display closed over variables in the variables view for functions that are not stack frames v12

Carrying r+ forward.
Attachment #809957 - Flags: review+
Rebased and landed:
https://hg.mozilla.org/integration/fx-team/rev/d4b75b61d8c4
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d4b75b61d8c4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.