828680 - Variables view needs a controller of its own

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Mihai Sucan [:msucan]]

There's a lot of code in debugger-controller.js that deals with adding scopes, variables, properties, and expanding these. This code should be reusable by the Web Console and other projects that need to display ObjectActors in the VariablesView.

We should move out some code from debugger-controller.js into a separate controller for VV,

Comment 2

[Brandon Benvie [:benvie]]

I'm looking at tackling this bug, as the Scratchpad now also needs to remote the VariablesView. I was thinking of making a subclass of the VariablesView that bakes in the functionality needed to handle Actors for display. Is there any opinion on doing this vs. doing it as a controller?

Comment 3

[Brandon Benvie [:benvie]]

Attachment: WIP1

This is an in-progress refactor that starts with streamlining debugger-controller's interactions with the `VariablesView`. `Scope` and `Property` methods have been made to be more interchangeable. All tests still pass.

* `Scope#addVar` and `Property#addProperty` have been replaced with `Scope#addItem`.
* `Property#addProperties` has been moved to `Scope#addItems`
* `StackFrames#_mergeSafeGetterValues` moved off the prototype since it's not |this| sensitive or changed between instances
* `StackFrames#_addScopeExpander` and `StackFrames#_addVarExpander` merged into a single `StackFrames#_addExpander`
* `StackFrames#_insertScopeArguments` inlined since it's only used in one spot
* `StackFrames#_insertScopeVariables` removed and replaced by using `Scope#addItems`
* `StackFrames#_populateProperties` added which adds the properties from an object grip to a target `Scope` or `Variable` and returns a promise
* `StackFrames#_fetchScopeVariables` and `StackFrames#_fetchVarProperties` merged into single `StackFrames#_fetchChildren` which takes either a `Scope` or `Variable` and creates children for all its properties/bindings

Comment 4

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

(In reply to Brandon Benvie [:bbenvie] from comment #2)
> I'm looking at tackling this bug, as the Scratchpad now also needs to remote
> the VariablesView. I was thinking of making a subclass of the VariablesView
> that bakes in the functionality needed to handle Actors for display. Is
> there any opinion on doing this vs. doing it as a controller?

Sorry for expressing an opinion in this bug after two weeks. Just as I said in bug 867458, I think having a separate controller for the VariablesView is what we need. IIRC msucan was suggesting this as well, because it's best to separate the idea of "views" and "controllers" and not mash everything into a single thing.

Comment 5

[Brandon Benvie [:benvie]]

Attachment: WIP2

This patch moves the plumbing for grip client expanders to VariablesView and friends. The VariablesView now has the basic necessary plumbing to internally handle remote values.

Remaining work is to fix some bugs causing failing tests (something to do with the events I think), upgrade the webconsole to use the modified API, and further refinement.

Comment 6

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vp] from comment #4)
> (In reply to Brandon Benvie [:bbenvie] from comment #2)
> > I'm looking at tackling this bug, as the Scratchpad now also needs to remote
> > the VariablesView. I was thinking of making a subclass of the VariablesView
> > that bakes in the functionality needed to handle Actors for display. Is
> > there any opinion on doing this vs. doing it as a controller?
> 
> Sorry for expressing an opinion in this bug after two weeks. Just as I said
> in bug 867458, I think having a separate controller for the VariablesView is
> what we need. IIRC msucan was suggesting this as well, because it's best to
> separate the idea of "views" and "controllers" and not mash everything into
> a single thing.

The changes I've made in the most recent patch should be easily adapted to being in a separate controller.

Comment 7

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

(In reply to Brandon Benvie [:bbenvie] from comment #6)
> 
> The changes I've made in the most recent patch should be easily adapted to
> being in a separate controller.

Thanks!

Comment 8

[Mihai Sucan [:msucan]]

(In reply to Brandon Benvie [:bbenvie] from comment #6)
> (In reply to Victor Porof [:vp] from comment #4)
> > (In reply to Brandon Benvie [:bbenvie] from comment #2)
> > > I'm looking at tackling this bug, as the Scratchpad now also needs to remote
> > > the VariablesView. I was thinking of making a subclass of the VariablesView
> > > that bakes in the functionality needed to handle Actors for display. Is
> > > there any opinion on doing this vs. doing it as a controller?
> > 
> > Sorry for expressing an opinion in this bug after two weeks. Just as I said
> > in bug 867458, I think having a separate controller for the VariablesView is
> > what we need. IIRC msucan was suggesting this as well, because it's best to
> > separate the idea of "views" and "controllers" and not mash everything into
> > a single thing.
> 
> The changes I've made in the most recent patch should be easily adapted to
> being in a separate controller.

Thank you! I confirm that what we both planned (me and Victor) was a separate controller that uses VariablesView.

Comment 9

[Brandon Benvie [:benvie]]

Attachment: WIP3

Add basic VariablesViewController.

Comment 10

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

hg add

Comment 11

[Brandon Benvie [:benvie]]

Attachment: WIP4

Add VariablesViewController.jsm, get all tests passing again.

Comment 12

[Brandon Benvie [:benvie]]

Attachment: WIP5

Unifies the VariablesViewController for use by both the webconsole and the debugger. Debugger tests are all passing but webconsole aren't yet (though it mostly seems to work). I expect to be able to reduce some redundancy beyond what's already been done.

* Add handling of long strings to the controller
* Make controller track actors and able to clear them
* Remove most of the VariablesView controller stuff from JSTerm and use VariablesViewController
* Add configuration bits needed to satisfy needs of JSTerm's usage of VariablesView

Comment 13

[Brandon Benvie [:benvie]]

Attachment: WIP6

Forgot to save a file.

* Add prefix handling for the evaluation macros.

Comment 14

[Rob Campbell [:rc] (:robcee)]

Is this ready for some feedback? Even if it isn't entirely tested yet, some initial review might be helpful.

Who wants it?

Comment 15

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #14)
> Is this ready for some feedback? Even if it isn't entirely tested yet, some
> initial review might be helpful.
> 
> Who wants it?

I could certainly take a look at it if benvie feels ok with that. However, I'd also be happy if msucan would check if the webconsole changes are good.

Comment 16

[Mihai Sucan [:msucan]]

When Brandon feels this patch is ready I am happy to review the webconsole changes.

Comment 17

[Brandon Benvie [:benvie]]

Attachment: WIP7

Excellent! I could use some feedback from you Victor. I know there's still refinement to do on this, but it's certainly feedbackable. Once I fix the failing tests in webconsole I'd also like feedback from Mihai.

Comment 18

[Brandon Benvie [:benvie]]

Attachment: WIP8

* change all uses of `addVar` and `addProperty` to `addItem`
* change all uses of `addProperties` to `addItems`
* remove the aliases for the above
* some method description comments

Comment 19

[Brandon Benvie [:benvie]]

Attachment: WIP9

* Change to single "variablesview-fetched" event
* Have the event include the target
* Update the debugger-controller's echoing of events to handle the new single event
* Add temporary plumbing for JSTerm to echo events to make tests work

Webconsole tests are still failing to timeouts where the variablesview-fetched event isn't firing for some reason.

Comment 20

[Brandon Benvie [:benvie]]

Attachment: WIP10

FINALLY tracked down the cause of webconsole tests timing out. Still have 11 webconsole tests failing to go, though.

* Change `VariablesViewController#_expand` to `expand` and have it take the source as a parameter instead of passing it around as a property
* Change `JSTerm#_updateVariablesView` to use `expand` instead of `_populateFromGrip` directly

Comment 21

[Brandon Benvie [:benvie]]

Attachment: WIP11

Remove some accidental debug code.

Comment 22

[Brandon Benvie [:benvie]]

Attachment: WIP12

Make `VariablesViewController#expand` return a promise (for bug 825039 rebase).

Comment 23

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

You're too fast for me! :)
GG.

Comment 24

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

Comment on attachment 758312 [details] [diff] [review]
WIP12

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

I don't have much to say about the implementation, it's really well thought out and in line with what's already there. Just a couple of comments below, but nothing drastic. Great work! f+++

::: browser/devtools/debugger/debugger-controller.js
@@ +73,5 @@
>  
>      DebuggerView.initialize(() => {
>        DebuggerView._isInitialized = true;
>  
> +      let variablesViewController = new VariablesViewController({

Are you going to use this variable anywhere? If not, then instantiating a VariablesViewController instance with 'new' seems a bit awkward. Don't get me wrong, I not opposed to the fact that you're attaching a 'controller' property to the view instance, but using a constructor in this case seems weird.

We used to have the same issue with EventEmitter. Maybe follow it, with 'decorate'?

@@ +81,5 @@
> +        }
> +      });
> +
> +
> +      // TODO: update tests and remove this

Are you planning to do this in this bug? If not, file a followup and add the bug number to this comment.

@@ -973,5 @@
> -
> -    DebuggerView.StackFrames.addFrame(frameTitle, frameLocation, line, depth);
> -  },
> -
> -  /**

So much red in here, loving it! The Debugger controller breaths again!

::: browser/devtools/debugger/test/browser_dbg_propertyview-filter-05.js
@@ +110,5 @@
>      is(gSearchBox.value, "*",
>        "Searchbox value is incorrect after 3 backspaces");
>  
> +    is(innerScope.querySelectorAll(".variables-view-variable:not([non-match])").length, 4,
> +      "There should be 4 variables displayed in the inner scope");

I can't entirely wrap my head on why this changed from 3 to 4. Please add a comment explaining the change.

@@ +140,5 @@
>      is(gSearchBox.value, "",
>        "Searchbox value is incorrect after 1 backspace");
>  
> +    is(innerScope.querySelectorAll(".variables-view-variable:not([non-match])").length, 4,
> +      "There should be 4 variables displayed in the inner scope");

Ditto.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +794,5 @@
>  
>        case e.DOM_VK_RETURN:
>        case e.DOM_VK_ENTER:
>          // Start editing the value or name of the variable or property.
> +        if (item instanceof Variable) {

The former 'item instanceof Property' here was mostly cosmetic, as you might have guessed. Removing it is fair, but the inheritance model may seem awkward to newcomers/contributors. Do you think it'd be useful to add an inline comment here and everywhere else with /* item is also instanceof Property */?

@@ +1057,5 @@
>  };
>  
> +
> +
> +

That's a lotta' whitespace.

::: browser/devtools/shared/widgets/VariablesViewController.jsm
@@ +13,5 @@
> +Cu.import("resource:///modules/devtools/VariablesView.jsm");
> +Cu.import("resource:///modules/devtools/ViewHelpers.jsm");
> +Cu.import("resource://gre/modules/devtools/WebConsoleUtils.jsm");
> +
> +// The maximum length of strings to be displayed by the Web Console.

I'm a little wary of having something specific to a 'Web Console' inside a generic VariablesView controller. I'm curious what Mihai thinks about this.

@@ +20,5 @@
> +this.EXPORTED_SYMBOLS = ["VariablesViewController"];
> +
> +
> +
> +function VariablesViewController(aOptions) {

I'd be happy if you added a short comment describing what this does, why it is here and how to use it.

@@ +98,5 @@
> +          ownProperties[name].getterValue = safeGetterValues[name].getterValue;
> +          ownProperties[name].getterPrototypeLevel = safeGetterValues[name]
> +                                                     .getterPrototypeLevel;
> +        }
> +        else {

Uber uber i'm-ashamed-to-mention-this style nit: I prefer } else {, and since this jsm is related to widget code, it'd be nice to keep things consistent.

@@ +139,5 @@
> +   *        The variable where the properties will be placed into.
> +   * @param any aGrip
> +   *        The grip of the variable.
> +   */
> +  addExpander: function(aTarget, aSource) {

Params here differ from the documentation.

@@ +147,5 @@
> +
> +    if (longString) {
> +      aTarget.showArrow();
> +    }
> +    else if (primitive) {

Ditto for } else if {

::: browser/devtools/webconsole/webconsole.js
@@ +3253,5 @@
>      view.searchEnabled = !aOptions.hideFilterInput;
>      view.lazyEmpty = this._lazyVariablesView;
>      view.lazyAppend = this._lazyVariablesView;
> +
> +    new VariablesViewController({

See, this is what I'm talking about.

Comment 25

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vp] from comment #24)
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +73,5 @@
> >  
> >      DebuggerView.initialize(() => {
> >        DebuggerView._isInitialized = true;
> >  
> > +      let variablesViewController = new VariablesViewController({
> 
> Are you going to use this variable anywhere? If not, then instantiating a
> VariablesViewController instance with 'new' seems a bit awkward. Don't get
> me wrong, I not opposed to the fact that you're attaching a 'controller'
> property to the view instance, but using a constructor in this case seems
> weird.

I agree. I've been trying to think of an elegant way to remove the new to `new` the VVC. Leaning toward `VariablesViewController.attach(view, options)`.


> @@ +81,5 @@
> > +        }
> > +      });
> > +
> > +
> > +      // TODO: update tests and remove this
> 
> Are you planning to do this in this bug? If not, file a followup and add the
> bug number to this comment.

I plan to do this before finalizing this patch. Leaving the test updates for last in case I decided to do it differently.

> ::: browser/devtools/debugger/test/browser_dbg_propertyview-filter-05.js
> @@ +110,5 @@
> >      is(gSearchBox.value, "*",
> >        "Searchbox value is incorrect after 3 backspaces");
> >  
> > +    is(innerScope.querySelectorAll(".variables-view-variable:not([non-match])").length, 4,
> > +      "There should be 4 variables displayed in the inner scope");
> 
> I can't entirely wrap my head on why this changed from 3 to 4. Please add a
> comment explaining the change.

I changed it so scopes that are object scopes (like the global scope) list `__proto__` (adding a property, so 3 -> 4) since they actually do have the property, and things on the prototype chain are "in scope" as well. I'll make a note in the tests unless you think I should just undo the change entirely.

> ::: browser/devtools/shared/widgets/VariablesViewController.jsm
> @@ +13,5 @@
> > +Cu.import("resource:///modules/devtools/VariablesView.jsm");
> > +Cu.import("resource:///modules/devtools/ViewHelpers.jsm");
> > +Cu.import("resource://gre/modules/devtools/WebConsoleUtils.jsm");
> > +
> > +// The maximum length of strings to be displayed by the Web Console.
> I'm a little wary of having something specific to a 'Web Console' inside a
> generic VariablesView controller. I'm curious what Mihai thinks about this.

I copied this from the webconsole and I think it's still useful here, I just need to update the comment (oops).

Comment 26

[Brandon Benvie [:benvie]]

Attachment: WIP13

Addresses Victor's feedback and a few other changes:

* Comment documenting VariablesViewController constructor
* Various commenting fixes/improvements
* Handful of style nits with curly braces
* Add and use `VariablesViewController.attach`
* Add `VariablesViewController#releaseActor`
* Automatically release long string actors when expanded
* Remove the `type` property from Scope/Variable
* Add `VariablesView.isVariable` to distinguish Variables/Properties from Scopes

Comment 27

[Brandon Benvie [:benvie]]

Attachment: WIP14

rebase

Comment 28

[Brandon Benvie [:benvie]]

Attachment: WIP15

This patch gets the webconsole down to 0 test failures, but it's a hack. For some reason, in some cases the webconsole's special `simpleValueEvalMacro` is not getting attached, self "_self" isn't getting prepended. This leads to eval macros like '["prop"] = value' which is a ReferenceError (invalid LHS assignment). The hack basically detects a missing prefix and a symbolic name starting with "[" and adds in "_self". This obviously isn't a real solution, but I haven't been able to determine the cause yet and I wanted to put up this jury-rigged-but-test-passing version in the meantime.

Comment 29

[Brandon Benvie [:benvie]]

Attachment: WIP16

I think it makes sense to rebroadcast the "fetched" event so I've removed the TODOs for that. I still haven't been able to determine why the changed simpleValueEvalMacro is not being respected. I have determined that it is *never* respected, though, so at least there's consistency.

This update contains some fixes and comment updates that I've done while trying to figure this bug out.

Comment 30

[Brandon Benvie [:benvie]]

Attachment: WIP17

Tracked down the cause of the issue to being `VariablesViewController#addExpander` returning for primitives before setting the evaluation macro. Temporary hack removed with all tests passing.

Comment 31

[Brandon Benvie [:benvie]]

https://tbpl.mozilla.org/?tree=Try&rev=b00a2712bac6

Comment 32

[Brandon Benvie [:benvie]]

Attachment: WIP18

Adds some comments, cleans up `VariablesViewController#addExpander` a bit.

Comment 33

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

Comment on attachment 761011 [details] [diff] [review]
WIP18

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

::: browser/devtools/shared/widgets/VariablesViewController.jsm
@@ +43,5 @@
> +  this._getLongStringClient = aOptions.getLongStringClient;
> +  this._releaseActor = aOptions.releaseActor;
> +
> +  function logWrap(name){
> +    return (...args) => {

All my likes. Take them.

@@ +224,5 @@
> +
> +  /**
> +   * Adds properties to a Scope, Variable, or Property in the view. Triggered
> +   * when a scope is expanded or certain variables are hovered. It does not
> +   * expand the target.

I think it's pretty wild having an "expand" method for which the documentation says "it does not expand the target". Maybe onExpand is better, or something else (you choose).

Comment 34

[Brandon Benvie [:benvie]]

Attachment: WIP19

Forgot to qrefresh after the last save. Removes debug code, updates some comments. `expand` does actually expand, so I have updated the comment. Given that, do you still think it should be renamed to 'onExpand'? If so I'll change it.

Comment 35

[Brandon Benvie [:benvie]]

Attachment: WIP20

Blarg this actually fixes it.

Comment 36

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

(In reply to Brandon Benvie [:bbenvie] from comment #34)
> Given that, do you still think it should be renamed to 'onExpand'? If so
> I'll change it.

Nope, don't worry about it.

Comment 37

[Brandon Benvie [:benvie]]

Attachment: WIP21

Rebase for bug 773590 landing. Four of the new tests are failing, however, and I'm trying to figure out why.

Comment 38

[Mihai Sucan [:msucan]]

Comment on attachment 761548 [details] [diff] [review]
WIP21

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

This is a really well done patch, Brandon! Great work. Not much to comment, just a couple of nits. r+

I suggest you also ask for a review from Victor for the VariablesView changes, for Debugger stuff and for the new controller. I did check those and they look good to me, but he knows the subtle details better than I do. As I see he already went through the patch during feedback.

::: browser/devtools/webconsole/webconsole.js
@@ +2746,5 @@
> +/**
> + * @private
> + * @see VariablesView.simpleValueEvalMacro
> + */
> +function simpleValueEvalMacro(aItem, aCurrentString)

Since these functions are not part of an object, you should remove @private from the comment.

@@ +2749,5 @@
> + */
> +function simpleValueEvalMacro(aItem, aCurrentString)
> +{
> +  return VariablesView.simpleValueEvalMacro(aItem, aCurrentString, "_self");
> +};

nit: function with ; at the end?

Comment 39

[Mihai Sucan [:msucan]]

oh, r+ with the failing tests fixed. (obviously)

Web Console tests passed for me here, but I don't have quite the very latest fx-team. (I only pulled once this morning!)

Comment 40

[Brandon Benvie [:benvie]]

Thanks Mihai! I'll address those comments, fix the tests, and get Victor's r+ on this.

Comment 41

[Brandon Benvie [:benvie]]

Attachment: WIP22

False alarm on those test failures (arg).

* remove @private from eval macro comments and accidental left over semicolon
* use the pref for "variables-sorting-enabled" instead of hardcoding it to true
* DRY up the scope looper in `StackFrames#selectFrame` (did this while trying to debug it and seems like a good change)

Comment 42

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

Comment on attachment 762242 [details] [diff] [review]
WIP22

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

You know your stuff Brandon. Keep up the good work!

::: browser/devtools/debugger/debugger-controller.js
@@ +711,2 @@
>  
>        // Handle additions to the innermost scope.

I like the fact that you ironed out this stuff. But let's keep them comments since they might shed some light for contributors on why things are done the way they are. See below:
 
In this case you might want to change this comment to read
// Handle special additions to the innermost scope.

@@ +715,5 @@
> +      }
> +
> +      DebuggerView.Variables.controller.addExpander(scope, environment);
> +
> +      if (innermost || this.autoScopeExpand) {

..and a comment before this conditional saying
// The innermost scope is always automatically expanded, because it contains the variables in the current stack frame which are always likely to be inspected.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +81,3 @@
>  };
>  
>  VariablesView.prototype = {

Might want to add a "controller: null" to this prototype to 1. Avoid adding ad-hoc unexpected properties to prototypes, maybe speeding things up a bit and 2. Help with documentation and avoid VariablesView consumers setting a controller property by mistake and interfering with VariablesViewController.attach.

@@ +1117,5 @@
> +   *        The variable's descriptor.
> +   * @return Variable
> +   *         The newly created child Variable.
> +   */
> +  createChild: function(aName, aDescriptor) {

At a second glance, having both "createChild" and "addItem" as public methods screams "confusing API" to me.

I think this should at least be private, what do you say? Maybe add an underscore before the property name, or give it a more unlikely-to-touch-this-from-outside name like _childFactory.

@@ +2066,2 @@
>     */
> +  createChild: function(aName, aDescriptor) {

Ditto, obviously.

::: browser/devtools/shared/widgets/VariablesViewController.jsm
@@ +13,5 @@
> +Cu.import("resource:///modules/devtools/VariablesView.jsm");
> +Cu.import("resource:///modules/devtools/ViewHelpers.jsm");
> +Cu.import("resource://gre/modules/devtools/WebConsoleUtils.jsm");
> +
> +const VARIABLES_SORTING_ENABLED = Services.prefs.getBoolPref("devtools.debugger.ui.variables-sorting-enabled");

Use a lazy getter for this.

XPCOMUtils.defineLazyGetter(this, "VARIABLES_SORTING_ENABLED", () =>
  Services.prefs.getBoolPref("devtools.debugger.ui.variables-sorting-enabled");
);

Comment 43

[Brandon Benvie [:benvie]]

Attachment: WIP23

(In reply to Victor Porof [:vp] from comment #42)
> I like the fact that you ironed out this stuff. But let's keep them comments
> since they might shed some light for contributors on why things are done the
> way they are.

Agree! Added comments.


> >  VariablesView.prototype = {
> 
> Might want to add a "controller: null" to this prototype to 1.

Agree! Added.


> > +  createChild: function(aName, aDescriptor) {
> 
> At a second glance, having both "createChild" and "addItem" as public
> methods screams "confusing API" to me.
> 
> I think this should at least be private, what do you say? Maybe add an
> underscore before the property name, or give it a more
> unlikely-to-touch-this-from-outside name like _childFactory.

Agree, it should be private (it's never used externally, not even in tests). I changed it to "_createChild" instead of "_childFactory" since the theme of the VariablesView method naming mostly seems to be "describe what you do" vs. "say what you are".


> > +const VARIABLES_SORTING_ENABLED = Services.prefs.getBoolPref("devtools.debugger.ui.variables-sorting-enabled");
> 
> Use a lazy getter for this.

Done!

Comment 44

[Brandon Benvie [:benvie]]

Attachment: WIP23

Arg qrefresh.

Comment 45

[Brandon Benvie [:benvie]]

Attachment: WIP23

actually update it this time <:|

Comment 46

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

https://hg.mozilla.org/integration/fx-team/rev/19b3d7dc3d7c

Comment 47

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/19b3d7dc3d7c