Variables view needs a controller of its own

RESOLVED FIXED in Firefox 24

Status

DevTools
Debugger
P3
normal
RESOLVED FIXED
6 years ago
a month ago

People

(Reporter: msucan, Assigned: benvie)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 24
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 24 obsolete attachments)

91.08 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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,
Priority: -- → P3
(Assignee)

Updated

5 years ago
Duplicate of this bug: 867458
(Assignee)

Comment 2

5 years ago
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?
(Assignee)

Updated

5 years ago
Blocks: 825039
(Assignee)

Updated

5 years ago
Assignee: nobody → bbenvie
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Depends on: 843091
(Assignee)

Updated

5 years ago
No longer depends on: 843091
(Assignee)

Updated

5 years ago
Blocks: 843091
(Assignee)

Comment 3

5 years ago
Created attachment 750112 [details] [diff] [review]
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
(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.
(Assignee)

Comment 5

5 years ago
Created attachment 750199 [details] [diff] [review]
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.
Attachment #750112 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
(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.
(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!
(Reporter)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
Created attachment 753478 [details] [diff] [review]
WIP3

Add basic VariablesViewController.
Attachment #750199 - Attachment is obsolete: true
hg add
(Assignee)

Comment 11

5 years ago
Created attachment 753548 [details] [diff] [review]
WIP4

Add VariablesViewController.jsm, get all tests passing again.
Attachment #753478 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Created attachment 755479 [details] [diff] [review]
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
Attachment #753548 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
Created attachment 755484 [details] [diff] [review]
WIP6

Forgot to save a file.

* Add prefix handling for the evaluation macros.
Attachment #755479 - Attachment is obsolete: true
Is this ready for some feedback? Even if it isn't entirely tested yet, some initial review might be helpful.

Who wants it?
(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.
(Reporter)

Comment 16

5 years ago
When Brandon feels this patch is ready I am happy to review the webconsole changes.
(Assignee)

Comment 17

5 years ago
Created attachment 757446 [details] [diff] [review]
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.
Attachment #755484 - Attachment is obsolete: true
Attachment #757446 - Flags: feedback?(vporof)
(Assignee)

Comment 18

5 years ago
Created attachment 757630 [details] [diff] [review]
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
Attachment #757446 - Attachment is obsolete: true
Attachment #757446 - Flags: feedback?(vporof)
Attachment #757630 - Flags: feedback?(vporof)
(Assignee)

Comment 19

5 years ago
Created attachment 758169 [details] [diff] [review]
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.
Attachment #757630 - Attachment is obsolete: true
Attachment #757630 - Flags: feedback?(vporof)
(Assignee)

Updated

5 years ago
Attachment #758169 - Flags: feedback?(vporof)
(Assignee)

Comment 20

5 years ago
Created attachment 758266 [details] [diff] [review]
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
Attachment #758169 - Attachment is obsolete: true
Attachment #758169 - Flags: feedback?(vporof)
Attachment #758266 - Flags: feedback?(vporof)
(Assignee)

Comment 21

5 years ago
Created attachment 758295 [details] [diff] [review]
WIP11

Remove some accidental debug code.
Attachment #758266 - Attachment is obsolete: true
Attachment #758266 - Flags: feedback?(vporof)
Attachment #758295 - Flags: feedback?(vporof)
(Assignee)

Comment 22

5 years ago
Created attachment 758312 [details] [diff] [review]
WIP12

Make `VariablesViewController#expand` return a promise (for bug 825039 rebase).
Attachment #758295 - Attachment is obsolete: true
Attachment #758295 - Flags: feedback?(vporof)
(Assignee)

Updated

5 years ago
Attachment #758312 - Flags: feedback?(vporof)
You're too fast for me! :)
GG.
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.
Attachment #758312 - Flags: feedback?(vporof) → feedback+
(Assignee)

Comment 25

5 years ago
(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).
(Assignee)

Comment 26

5 years ago
Created attachment 758688 [details] [diff] [review]
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
Attachment #758312 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
Created attachment 759180 [details] [diff] [review]
WIP14

rebase
Attachment #758688 - Attachment is obsolete: true
(Assignee)

Comment 28

5 years ago
Created attachment 759333 [details] [diff] [review]
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.
Attachment #759180 - Attachment is obsolete: true
(Assignee)

Comment 29

5 years ago
Created attachment 759877 [details] [diff] [review]
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.
Attachment #759333 - Attachment is obsolete: true
(Assignee)

Comment 30

5 years ago
Created attachment 760532 [details] [diff] [review]
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.
Attachment #759877 - Attachment is obsolete: true
(Assignee)

Comment 32

5 years ago
Created attachment 761011 [details] [diff] [review]
WIP18

Adds some comments, cleans up `VariablesViewController#addExpander` a bit.
Attachment #760532 - Attachment is obsolete: true
Attachment #761011 - Flags: review?(mihai.sucan)
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).
(Assignee)

Comment 34

5 years ago
Created attachment 761149 [details] [diff] [review]
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.
Attachment #761011 - Attachment is obsolete: true
Attachment #761011 - Flags: review?(mihai.sucan)
Attachment #761149 - Flags: review?(mihai.sucan)
(Assignee)

Comment 35

5 years ago
Created attachment 761152 [details] [diff] [review]
WIP20

Blarg this actually fixes it.
Attachment #761149 - Attachment is obsolete: true
Attachment #761149 - Flags: review?(mihai.sucan)
Attachment #761152 - Flags: review?(mihai.sucan)
(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.
(Assignee)

Comment 37

5 years ago
Created attachment 761548 [details] [diff] [review]
WIP21

Rebase for bug 773590 landing. Four of the new tests are failing, however, and I'm trying to figure out why.
Attachment #761152 - Attachment is obsolete: true
Attachment #761152 - Flags: review?(mihai.sucan)
(Reporter)

Comment 38

5 years ago
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?
Attachment #761548 - Flags: review+
(Reporter)

Comment 39

5 years ago
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!)
(Assignee)

Comment 40

5 years ago
Thanks Mihai! I'll address those comments, fix the tests, and get Victor's r+ on this.
(Assignee)

Comment 41

5 years ago
Created attachment 762242 [details] [diff] [review]
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)
Attachment #761548 - Attachment is obsolete: true
Attachment #762242 - Flags: review?(vporof)
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");
);
Attachment #762242 - Flags: review?(vporof) → review+
(Assignee)

Comment 43

5 years ago
Created attachment 762767 [details] [diff] [review]
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!
Attachment #762242 - Attachment is obsolete: true
(Assignee)

Comment 44

5 years ago
Created attachment 762768 [details] [diff] [review]
WIP23

Arg qrefresh.
Attachment #762767 - Attachment is obsolete: true
(Assignee)

Comment 45

5 years ago
Created attachment 762779 [details] [diff] [review]
WIP23

actually update it this time <:|
Attachment #762768 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/19b3d7dc3d7c
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/19b3d7dc3d7c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.