808371 - There's no way to add new properties to objects in the variables view

Status: RESOLVED FIXED

(DevTools :: Object Inspector, defect, P1)

Comment 1

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

Having this would essentially make the VariablesView suitable for the Rules view in the style inspector. It should already work out of the box with the Computed view.

Comment 2

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

Moving stuff to the Object Inspector component.
Filter on COSMICMIGRAINE.

Comment 3

[Brandon Benvie [:benvie]]

In terms of the UI for this, one thought I had was to add a "+" icon (or whatever) next to the "x" for deleting properties on hover for things which are objects. So it would be like

> fooString: "bar"           x
> fooObject: Object        + x
>   fooNumber: 5             x
>   fooObjectProp: Object  + x
>   fooNumber2: 15           x

So clicking the "+" on fooObject adds a property to it. This might not be intuitive though, since clicking "x" deletes the property from the parent object while clicking the "+" would add it as a property to that object (they change different objects).

The other option is making a single global add property button at the top or bottom or somewhere, which required you to select an object in the view and then adds to it.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Something inline seems more intuitive to me, but you are right that it conflicts a bit with how the "x" functions.

Darrin, any thoughts on this?

Comment 5

[Darrin Henein [:darrin]]

Here is a quick first draft of what I had in mind... http://cl.ly/image/1B2s1b0c2Y2c

(ignore the actual text/properties, I started with a screenshot of the current editor)

- on the left is very similar to our current variables view
- hovering a line/property would show a hint/type/description inline next to it, and a plus/minus button right aligned (to add a child node or remove this parent node)
- errors/warnings would highlight the line and include an icon/message right aligned

The whole editor would be in a visually separate panel, to help indicate that this is an editable area. Perhaps the editable value/text would have a yellow highlight to further indicate that it is an editable value?

Feedback would be great and we can refine from here. Not sure how the +/- buttons interact with the warning/error icons if they are present (maybe they just replace whats there on hover?)

Comment 6

[Brandon Benvie [:benvie]]

(In reply to Darrin Henein [:darrin] from comment #5)
> The whole editor would be in a visually separate panel, to help indicate
> that this is an editable area. Perhaps the editable value/text would have a
> yellow highlight to further indicate that it is an editable value?

What do you mean in a separate panel? It looks like in your mockup you have highlighting only during hover, which I think is good for discoverability over what we currently have (just the edit cursor). Do you mean something besides this?


> Feedback would be great and we can refine from here. Not sure how the +/-
> buttons interact with the warning/error icons if they are present (maybe
> they just replace whats there on hover?)

Perhaps instead of having those icons on the right, there could be a small gutter on the left where they could go.

Comment 7

[Darrin Henein [:darrin]]

> What do you mean in a separate panel? It looks like in your mockup you have
> highlighting only during hover, which I think is good for discoverability
> over what we currently have (just the edit cursor). Do you mean something
> besides this?

I was referring only to the inset shadow/lighter background that the whole bottom editor section has (everything below the Manifest Editor title).

> Perhaps instead of having those icons on the right, there could be a small
> gutter on the left where they could go.

That's a great idea... closer to the other edit targets (key/value text, etc.) as well. I will move the +/- there.

Comment 8

[Darrin Henein [:darrin]]

Here is a minor revision with the =/- buttons moved: http://cl.ly/image/1O1Y1K142U27

Comment 9

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

That's awesome.

Comment 10

[Brandon Benvie [:benvie]]

I like it! I agree that it's better having the +/- on the left and having the alerts on the right.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Yes, looks great!

Comment 12

[Brandon Benvie [:benvie]]

Attachment: WIP1

Very rough first iteration of this. Running into an issue where if an object is not already expanded, I expand it and don't have a way to know when it's actually finished expanding (done roundtripping in the case of a remote object). So when it actually does finish expanding, the currently selected textbox (the property name of the newly added property) becomes deselected and is at the top of the list instead of the bottom of the list.

Todo list:

* add graphics for +/-
* improve editing interface so editing the property name doesn't cover the value
* allow tabbing from editing the property name to the value

Comment 13

[Brandon Benvie [:benvie]]

Attachment: WIP2

Updates key editing to not hide the separator and value, and also to expand with the text. A problem with this is in order to not do it in a completely hackey way (like using a dummy element to measure text width) I had to use a monospace font in the textbox.

Comment 14

[Brandon Benvie [:benvie]]

Attachment: WIP3

Testing out changing everything in the VariablesView to be monospace (using the "devtools-monospace" class). I like how it looks, and it allows the textbox to fit in the exact same spot as the label without moving (when combined with some css tweaks).

Comment 15

[Brandon Benvie [:benvie]]

Attachment: WIP4

Still rough, but this breaks off managing the input fields to its own class. The purpose of this is to make it easier to implement creating a new field that allows editing both the name and value before committing (not implemented as of yet).

Comment 16

[Brandon Benvie [:benvie]]

Attachment: WIP4

Making progress but it's still all kinds of buggy. I'm thinking about maybe just adding the feature for non-remote use for now. It's really hard to make this work with remote objects given how it works right now.

Comment 17

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

(In reply to Brandon Benvie [:bbenvie] from comment #16)

> just adding the feature for non-remote use for now. It's really hard to make
> this work with remote objects given how it works right now.

Can you elaborate? I might be able to help.

Comment 18

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vp] from comment #17)
> (In reply to Brandon Benvie [:bbenvie] from comment #16)
> 
> > just adding the feature for non-remote use for now. It's really hard to make
> > this work with remote objects given how it works right now.
> 
> Can you elaborate? I might be able to help.

In order to add a property to an object, it must be expanded first. Currently there's no way to determine when the expansion is completed, since that machinery happens outside of the VariablesView itself (i.e. it happens in the VariablesViewController if you're working with a remote object). To support situations where the expansion isn't completed synchronously (AKA remote targets) there needs to be someway to get feedback into the VariablesView about when the expansion is completed.

Do you have any thoughts on a good way to do this? Everything I've come up with so far is clunky.

Comment 19

[Brandon Benvie [:benvie]]

A solution I'm looking at that might make sense is to simply disallow adding properties until an object is expanded.

Comment 20

[Brandon Benvie [:benvie]]

Nevermind, that still doesn't solve the problem that you don't know when the expanding has finished.

Comment 21

[Brandon Benvie [:benvie]]

Attachment: WIP5

This version cleans up the bugs with the interaction when adding properties and adds comments.

Victor, I'd like your feedback on the way I'm going about this. I've tried a number of different approaches, and what I currently have is the cleanest way to do it and avoids repeating large swaths of code.

Right now the only problem is when used with remote targets (webconsole, debugger) and attempting to add properties to unexpanded objects. As I said above, there's no simple way to know when an object has finished expanding to know when the newly added item can be appended. However, we'd like to get this landed for 27 so it can be used in the manifest editor which only uses local objects.

After 27 we can revisit how best to make this work in the debugger and webconsole. In this patch it does actually work with them if you expand the object first, but before final review I plan to remove that (as simple as not providing a `variablesView.new` callback).

The remaining thing I need to do is rebase against bug 922144, which includes styles changes that will affect how the new button in this patch is positioned and styled.

Comment 22

[Brandon Benvie [:benvie]]

Attachment: WIP6

Fixes an issue that prevented it from working.

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

If you'd like to grab my styles for the "add property" button, they're part of bug 930092 (attachment 821245 [details] [diff] [review]).

Comment 24

[Brandon Benvie [:benvie]]

Yeah, I'll do that. I'm fitting my patch to work on top of bug 922144 now and the icon will be helpful for that.

Comment 25

[Brandon Benvie [:benvie]]

Attachment: WIP7

Rebases on top of bug 922144 and integrates the bits needed to make it work in the manifest editor. The only thing I'm having trouble with is figuring out why the + button shows all the time instead of only on hover.

Comment 26

[Brandon Benvie [:benvie]]

Attachment: WIP8

Accidentally left in some debug code.

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Brandon Benvie [:bbenvie] from comment #25)
> The only thing I'm having trouble with is figuring
> out why the + button shows all the time instead of only on hover.

I think you need to change this block in the shared widget.css:

*:not(:hover) .variables-view-delete {
  visibility: hidden;
}

to: 

*:not(:hover) .variables-view-delete,
*:not(:hover) .variables-view-add-property {
  visibility: hidden;
}

Sorry for the confusion... I thought I had seen it in a previous rev of your patch, so I left it out of mine.

Comment 28

[Brandon Benvie [:benvie]]

(In reply to J. Ryan Stinnett [:jryans] from comment #27)
> (In reply to Brandon Benvie [:bbenvie] from comment #25)
> > The only thing I'm having trouble with is figuring
> > out why the + button shows all the time instead of only on hover.
> 
> I think you need to change this block in the shared widget.css:
> 
> *:not(:hover) .variables-view-delete {
>   visibility: hidden;
> }
> 
> to: 
> 
> *:not(:hover) .variables-view-delete,
> *:not(:hover) .variables-view-add-property {
>   visibility: hidden;
> }
> 
> Sorry for the confusion... I thought I had seen it in a previous rev of your
> patch, so I left it out of mine.

Ah yes, it got lost in the rebase and I missed adding it back in. Thanks!

Comment 29

[Brandon Benvie [:benvie]]

Attachment: WIP9

Rebase against bug 922144. I need to get a corresponding "+" .svg for the icon.

Comment 30

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The updated patch in bug 930092 (attachment 821896 [details] [diff] [review]) contains such an image.

Comment 31

[Brandon Benvie [:benvie]]

Attachment: WIP10

Stole the svg icon from bug 930092.

Comment 32

[Brandon Benvie [:benvie]]

Attachment: WIP11

Fix the debugger test failures.

Comment 33

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

Comment on attachment 822620 [details] [diff] [review]
WIP11

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

After a first pass through all of this, I think it's pretty sweet! Looks good, but I have a few comments, see below. Ask me again for review after addressing them.

::: browser/devtools/debugger/test/browser_dbg_variables-view-data.js
@@ +503,5 @@
>  }
>  
>  function testPropertyInheritance(fooScope, anonymousVar, anonymousScope, barVar, bazProperty) {
> +  is(fooScope.preventDisableOnChange, gVariablesView.preventDisableOnChange,
> +    "The preventDisableOnChange property should persist from the view to all scopes.");

Check for the propagation of the 'new' function in this test, similarly to eval/switch/delete.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +51,5 @@
>    lazyEmptyDelay: 10, // ms
>    searchEnabled: true,
>    editableValueTooltip: "",
>    editableNameTooltip: "",
> +  preventDisableOnChange: true,

For some reason now, in the Network Monitor, all headers contain an expander node, even though they're not expandable. Probably not related to the preventDisableOnChange typo fix, but please look into why this is happening.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +75,5 @@
>    this._prevHierarchy = new Map();
>    this._currHierarchy = new Map();
>  
>    this._parent = aParentNode;
> +  this._parent.classList.add("variables-view-container", "devtools-monospace");

I don't think we should add devtools-monospace by default just yet. Make this an option please, as a setter, like actionsFirst and alignedValues.

@@ +1166,5 @@
>    this.deleteButtonTooltip = aView.deleteButtonTooltip;
>    this.contextMenuId = aView.contextMenuId;
>    this.separatorStr = aView.separatorStr;
>  
> +  this._init(aName, aFlags);

Why remove the trim()?

@@ +2437,2 @@
>      }
> +    this._title.appendChild(deleteNode);

Delete nodes now appear in the debugger's variables view, which doesn't have a delete function attached. I also don't fully understand why you're always creating a delete node, regardless whether the delete function exists or not. This is a bit costly, especially when creating many variables or properties at once. Any reason not to keep everything in a conditional?

@@ +2437,4 @@
>      }
> +    this._title.appendChild(deleteNode);
> +
> +    let addPropertyNode = this._addPropertyNode = this.document.createElement("toolbarbutton");

Add a _addPropertyNode: null along with _deleteNode and friends at the end of this object literal.

@@ +2438,5 @@
> +    this._title.appendChild(deleteNode);
> +
> +    let addPropertyNode = this._addPropertyNode = this.document.createElement("toolbarbutton");
> +    addPropertyNode.className = "plain variables-view-add-property";
> +    addPropertyNode.addEventListener("mousedown", this._onAddProperty.bind(this), false);

Why mousedown as opposed to click? You'll also have to add this in the scope's _onClick handler, preventing an expand when clicked.

@@ +2442,5 @@
> +    addPropertyNode.addEventListener("mousedown", this._onAddProperty.bind(this), false);
> +    if (!ownerView.new || !descriptor.value || VariablesView.isPrimitive(descriptor)) {
> +      addPropertyNode.setAttribute("invisible", "");
> +    }
> +    this._title.appendChild(addPropertyNode);

Ditto on always creating a node. Only do it if a function is available.

You should also make sure that adding a property is keyboard accessible. Should be pretty straight forward, just add the new rules in _onViewKeyPress and the assertions in browser_dbg_variables-view-accessibility.js Maybe VK_INSERT would be a good key?

@@ +2605,5 @@
>     */
>    _activateNameInput: function(e) {
> +    var editable = new NameEditable(this, aKey => {
> +      this.ownerView.switch(this, aKey);
> +      if (!this._variablesView.preventDisableOnChange) {

You're handing preventDisableOnChange 'after' eval-ing, in the this case, but 'before' in _activateValueInput. Pick only one. It used to be 'before', so maybe stick with that.

@@ +2613,2 @@
>      });
> +    editable.activate(e);

Doesn't look like you really need the `editable` variable. This makes me think that it may be better for NameEditable to have a static method, like "handle" or something, versus being a constructor.

@@ +2677,5 @@
>          this.hide();
>        }
>      }
>    },
> + 

Nit: unnecessary whitespace at the beginning of the line.

@@ +2717,5 @@
>    _initialDescriptor: null,
>    _separatorLabel: null,
>    _spacer: null,
>    _valueLabel: null,
>    _inputNode: null,

The _inputNode is never assigned anymore, remove it from here and all over the variables view.

@@ -2895,4 @@
>      this._idString = generateId(this._nameString = aName);
>      this._displayScope(aName, "variables-view-property variable-or-property");
>  
> -    // Don't allow displaying property information there's no name available.

Why remove this comment?

@@ +3141,5 @@
> +}
> +
> +Editable.prototype = {
> +  /**
> +   * The class name for targeting this Editable type's label element.

Add that these properties are overwritten in inheriting objects.

@@ +3156,5 @@
> +   */
> +  label: null,
> +
> +  /**
> +   * Activate this editable by replaving the input box it overlays and

typo: replacing.

@@ +3184,5 @@
> +    if (!this._variable._variablesView.alignedValues) {
> +      input.setAttribute("flex", "1");
> +    }
> +    let size = Math.max(1, this.label.getAttribute("value").length);
> +    input.setAttribute("size", size);

Why is the use of Math.max necessary?

Also, this will never work properly when there's a flex attribute. I think you can now safely remove that.

@@ +3222,5 @@
> +    this._input.removeEventListener("blur", this.deactivate);
> +    this._input.parentNode.replaceChild(this.label, this._input);
> +    this._input = null;
> +
> +    this._variable._variablesView._boxObject.scrollBy(-this._variable._target.clientWidth, 0);

Nit: Split this into two lines. let boxObject = ... then scrollBy.

@@ +3293,5 @@
> +        break;
> +      default:
> +        // The value of the node will not be updated until *after* this event
> +        // completes.
> +        this._variable.window.setTimeout(() => {

Set a named timeout for this, to avoid the (unlikely, but possible) scenario of another timeout being set before the previous one has finished. See setNamedTimeout in ViewHelpers.

@@ +3307,5 @@
> +
> +/**
> + * An Editable specific to editing the name of a Variable or Property.
> + */
> +function NameEditable(aVariable, aOnSave) {

EditableName, EditableValue sound better to me.

@@ +3347,5 @@
> +
> +/**
> + * An Editable specific to editing the key and value of a new property.
> + */
> +function NewPropertyEditable(aVariable, aOnSave) {

Having "New" as part of a constructor name is weird. Maybe just EditableNameAndValue, also making it obvious that it inherits from EditableName?

@@ +3353,5 @@
> +}
> +
> +NewPropertyEditable.prototype = Heritage.extend(NameEditable.prototype, {
> +  _reset: function(e) {
> +    // Hide the Varible or Property if the user presses escape.

Does this mean the variable/property will still be there, but with missing data? Wouldn't it be better to remove it instead of hiding?

::: browser/devtools/webconsole/webconsole.js
@@ +3625,5 @@
>      this.requestEvaluation(code, evalOptions).then(onEval, onEval);
>    },
>  
> +  _variablesViewNew:
> +  function JST__variablesViewNew(aOptions, aVar, aNewName, aNewValue)

I don't see this working in the variables view in the webconsole. There's no button.

::: browser/themes/linux/devtools/widgets.css
@@ +646,5 @@
> +  visibility: hidden;
> +}
> +
> +.variables-view-delete > .toolbarbutton-text,
> +.variables-view-add-property > .toolbarbutton-text {

These two should be in content css, not theme.

@@ +670,5 @@
>  }
>  
>  .element-name-input {
>    -moz-margin-start: -2px !important;
> +  -moz-margin-end: -3px !important;

This only works properly with monospace fonts. Both scenarios should be handled here.

@@ +705,5 @@
>  .arrow[open] {
>    -moz-appearance: treetwistyopen;
>  }
>  
> +.variables-view-property [invisible] {

I don't understand this. Children and descending nodes of a variables view property can be invisible? If you were referring to the delete and new buttons, see my other comment about not creating them at all.

@@ -699,5 @@
>  .arrow[open] {
>    -moz-appearance: treetwistyopen;
>  }
>  
> -.arrow[invisible] {

Why did you remove .arrow[invisible]?

Comment 34

[Brandon Benvie [:benvie]]

Thanks for the feedback! A few questions/comments on fixing this:

> I don't think we should add devtools-monospace by default just yet. Make
> this an option please, as a setter, like actionsFirst and alignedValues.

Essentially, monospace will have to be linked to whether the edit textboxes use the whole horizontal space or not. In order for the textbox width to match the width of the actual text, monospace has to be used (or some horrendous CSS/JS hack to measure the current width of the text). Absolutely can be optional, but these two things are linked one way or the other.

> 
> @@ +2437,2 @@
> >      }
> > +    this._title.appendChild(deleteNode);
> 
> Delete nodes now appear in the debugger's variables view, which doesn't have
> a delete function attached. I also don't fully understand why you're always
> creating a delete node, regardless whether the delete function exists or
> not. This is a bit costly, especially when creating many variables or
> properties at once. Any reason not to keep everything in a conditional?

So the issue here was when left aligning the add/remove buttons. The problem is that if they are only conditionally added, the columns no longer match up because there's less stuff on the left. It looks pretty ugly. My solution was to unconditionally add them, but conditionally hide/show them. That way the space was even. I also see the issues with this, and I hadn't come up with a great solution that solved both sets of issues. Do you have an idea for how I can solve this better?

> 
> @@ +2438,5 @@
> > +    this._title.appendChild(deleteNode);
> > +
> > +    let addPropertyNode = this._addPropertyNode = this.document.createElement("toolbarbutton");
> > +    addPropertyNode.className = "plain variables-view-add-property";
> > +    addPropertyNode.addEventListener("mousedown", this._onAddProperty.bind(this), false);
> 
> Why mousedown as opposed to click? You'll also have to add this in the
> scope's _onClick handler, preventing an expand when clicked.

Yeah, I used mousedown to sidestep around the issue whether the click event expands the thing, which interrupted the add-property stuff. Preventing expand on click on the add-property button will solve the issue.


> @@ +3184,5 @@
> > +    if (!this._variable._variablesView.alignedValues) {
> > +      input.setAttribute("flex", "1");
> > +    }
> > +    let size = Math.max(1, this.label.getAttribute("value").length);
> > +    input.setAttribute("size", size);
> 
> Why is the use of Math.max necessary?

If you set the "size" to 0, it goes to the default width of like ~20 characters, so 1 is the minimum value that won't break it. I'll make a comment noting that.


> Also, this will never work properly when there's a flex attribute. I think
> you can now safely remove that.

This is true, in the manifest editor the size does nothing. In the webconsole and debugger it still works though since they don't use flex. Now that the new design for the manifest editor has landed and the fixed width column thing is, there's some stuff I can trim from here since it is either useless or requires the used of fixed-width fonts.


> @@ +3353,5 @@
> > +}
> > +
> > +NewPropertyEditable.prototype = Heritage.extend(NameEditable.prototype, {
> > +  _reset: function(e) {
> > +    // Hide the Varible or Property if the user presses escape.
> 
> Does this mean the variable/property will still be there, but with missing
> data? Wouldn't it be better to remove it instead of hiding?

Indeed. My first instinct was to find a way to simply remove them, but I couldn't find a convenient way to do it. I was actually surprised to find no Variable#removeChild function. I will add this or something to the effect and use it.

> 
> ::: browser/devtools/webconsole/webconsole.js
> @@ +3625,5 @@
> >      this.requestEvaluation(code, evalOptions).then(onEval, onEval);
> >    },
> >  
> > +  _variablesViewNew:
> > +  function JST__variablesViewNew(aOptions, aVar, aNewName, aNewValue)
> 
> I don't see this working in the variables view in the webconsole. There's no
> button.

Yes, I removed the two lines that set `variablesView.new = this.variablesViewNew` in my last edit. This is the function needed to actually do it, but it doesn't work perfectly (although it does actually work pretty well as of my most recent iteration). I wasn't sure whether to just remove all traces of it, or leave the mostly completed but inactive version of it.


> @@ +670,5 @@
> >  }
> >  
> >  .element-name-input {
> >    -moz-margin-start: -2px !important;
> > +  -moz-margin-end: -3px !important;
> 
> This only works properly with monospace fonts. Both scenarios should be
> handled here.

Yeah, the problem is that it really can't properly work without monospace fonts at all, since they will have variable widths across platforms, systems, etc. I agree that this should be toggleable though, essentially on or off.

> 
> @@ +705,5 @@
> >  .arrow[open] {
> >    -moz-appearance: treetwistyopen;
> >  }
> >  
> > +.variables-view-property [invisible] {
> 
> I don't understand this. Children and descending nodes of a variables view
> property can be invisible? If you were referring to the delete and new
> buttons, see my other comment about not creating them at all.

This was simply a way to make the "invisible" attribute work with any child, rather than a specific class.

> 
> @@ -699,5 @@
> >  .arrow[open] {
> >    -moz-appearance: treetwistyopen;
> >  }
> >  
> > -.arrow[invisible] {
> 
> Why did you remove .arrow[invisible]?

The above style ".variables-view-property [invisible]" would cover .arrow[invisible].

Comment 35

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

(In reply to Brandon Benvie [:bbenvie] from comment #34)
> Thanks for the feedback! A few questions/comments on fixing this:
> 
> > I don't think we should add devtools-monospace by default just yet. Make
> > this an option please, as a setter, like actionsFirst and alignedValues.
> 
> Essentially, monospace will have to be linked to whether the edit textboxes
> use the whole horizontal space or not. In order for the textbox width to
> match the width of the actual text, monospace has to be used (or some
> horrendous CSS/JS hack to measure the current width of the text). Absolutely
> can be optional, but these two things are linked one way or the other.
> 

The current approach works well enough for both monospace and kerned text, even though it's not perfectly exact. I'd say you shouldn't worry too much about it for now.

> > 
> > @@ +2437,2 @@
> > >      }
> > > +    this._title.appendChild(deleteNode);
> > 
> > Delete nodes now appear in the debugger's variables view, which doesn't have
> > a delete function attached. I also don't fully understand why you're always
> > creating a delete node, regardless whether the delete function exists or
> > not. This is a bit costly, especially when creating many variables or
> > properties at once. Any reason not to keep everything in a conditional?
> 
> So the issue here was when left aligning the add/remove buttons. The problem
> is that if they are only conditionally added, the columns no longer match up
> because there's less stuff on the left. It looks pretty ugly. My solution
> was to unconditionally add them, but conditionally hide/show them. That way
> the space was even. I also see the issues with this, and I hadn't come up
> with a great solution that solved both sets of issues. Do you have an idea
> for how I can solve this better?
> 

Yes, but I think that will have a non-negligeable perf hit, especially when lots of nodes are added. Ideally we should keep the number of instantiated nodes for each scope/var/property to an absolute minimum. AFAICT, there are no current scenarios in which one button might be added and the other not, assuming aligned values as well. Debugger and NetMonitor won't suffer from the weird alignment issues you mention, and the manifest editor will ahve both of them enabled.

> > 
> > @@ +2438,5 @@
> > > +    this._title.appendChild(deleteNode);
> > > +
> > > +    let addPropertyNode = this._addPropertyNode = this.document.createElement("toolbarbutton");
> > > +    addPropertyNode.className = "plain variables-view-add-property";
> > > +    addPropertyNode.addEventListener("mousedown", this._onAddProperty.bind(this), false);
> > 
> > Why mousedown as opposed to click? You'll also have to add this in the
> > scope's _onClick handler, preventing an expand when clicked.
> 
> Yeah, I used mousedown to sidestep around the issue whether the click event
> expands the thing, which interrupted the add-property stuff. Preventing
> expand on click on the add-property button will solve the issue.
> 

Cool.

> 
> > @@ +3184,5 @@
> > > +    if (!this._variable._variablesView.alignedValues) {
> > > +      input.setAttribute("flex", "1");
> > > +    }
> > > +    let size = Math.max(1, this.label.getAttribute("value").length);
> > > +    input.setAttribute("size", size);
> > 
> > Why is the use of Math.max necessary?
> 
> If you set the "size" to 0, it goes to the default width of like ~20
> characters, so 1 is the minimum value that won't break it. I'll make a
> comment noting that.
> 

Good point.

> 
> > Also, this will never work properly when there's a flex attribute. I think
> > you can now safely remove that.
> 
> This is true, in the manifest editor the size does nothing. In the
> webconsole and debugger it still works though since they don't use flex. Now
> that the new design for the manifest editor has landed and the fixed width
> column thing is, there's some stuff I can trim from here since it is either
> useless or requires the used of fixed-width fonts.
> 

Great, let me know how it goes.

> 
> > @@ +3353,5 @@
> > > +}
> > > +
> > > +NewPropertyEditable.prototype = Heritage.extend(NameEditable.prototype, {
> > > +  _reset: function(e) {
> > > +    // Hide the Varible or Property if the user presses escape.
> > 
> > Does this mean the variable/property will still be there, but with missing
> > data? Wouldn't it be better to remove it instead of hiding?
> 
> Indeed. My first instinct was to find a way to simply remove them, but I
> couldn't find a convenient way to do it. I was actually surprised to find no
> Variable#removeChild function. I will add this or something to the effect
> and use it.
> 

True, it wasn't needed until now. Adding a Scope.prototype.remove function would be best thing to do here IMHO. Shouldn't be that hard, but you just need to be sure to remove all references from arrays, maps etc.

> > 
> > ::: browser/devtools/webconsole/webconsole.js
> > @@ +3625,5 @@
> > >      this.requestEvaluation(code, evalOptions).then(onEval, onEval);
> > >    },
> > >  
> > > +  _variablesViewNew:
> > > +  function JST__variablesViewNew(aOptions, aVar, aNewName, aNewValue)
> > 
> > I don't see this working in the variables view in the webconsole. There's no
> > button.
> 
> Yes, I removed the two lines that set `variablesView.new =
> this.variablesViewNew` in my last edit. This is the function needed to
> actually do it, but it doesn't work perfectly (although it does actually
> work pretty well as of my most recent iteration). I wasn't sure whether to
> just remove all traces of it, or leave the mostly completed but inactive
> version of it.
> 

I think we shouldn't touch the webconsole for now. Let's keep this bug's reach minimal and have a followup for adding 'new' to the webconsole.

> 
> > @@ +670,5 @@
> > >  }
> > >  
> > >  .element-name-input {
> > >    -moz-margin-start: -2px !important;
> > > +  -moz-margin-end: -3px !important;
> > 
> > This only works properly with monospace fonts. Both scenarios should be
> > handled here.
> 
> Yeah, the problem is that it really can't properly work without monospace
> fonts at all, since they will have variable widths across platforms,
> systems, etc. I agree that this should be toggleable though, essentially on
> or off.
> 

How about having inputs always be in monospace, versus all the variables view? It'd be an interesting experiment.

> > 
> > @@ +705,5 @@
> > >  .arrow[open] {
> > >    -moz-appearance: treetwistyopen;
> > >  }
> > >  
> > > +.variables-view-property [invisible] {
> > 
> > I don't understand this. Children and descending nodes of a variables view
> > property can be invisible? If you were referring to the delete and new
> > buttons, see my other comment about not creating them at all.
> 
> This was simply a way to make the "invisible" attribute work with any child,
> rather than a specific class.
> 

Doesn't seem to do what you want though :) I think you might mean variable-or-property, or..?

> > 
> > @@ -699,5 @@
> > >  .arrow[open] {
> > >    -moz-appearance: treetwistyopen;
> > >  }
> > >  
> > > -.arrow[invisible] {
> > 
> > Why did you remove .arrow[invisible]?
> 
> The above style ".variables-view-property [invisible]" would cover
> .arrow[invisible].

Not for scopes and variables.

Comment 36

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Benvie, do you think you'll be able to keep making progress on this?

Comment 37

[Brandon Benvie [:benvie]]

Yep! I have something cooking up now that addresses Victor's feedback. I'll get it cleaned up ASAP.

Comment 38

[Brandon Benvie [:benvie]]

Attachment: WIP12

Addresses most of the feedback:

* Add Scope#remove and Varible#remove
* Add testing for propagation of "new" property
* Removes monospace by default
* Make it so delete/add property aren't added unless they can work
* Add keyboard accessibility
* Removal of input width adjustment
* Fixes to the CSS and removal of some of the adjustments that were made and are not longer needed
* Remove changes to webconsole
* Various naming, cleanup, and comments

The maining issues I'm having are
* Something's screwed up with the normal VariableView's name input's size
* Alignment of columns in manifest edit. The problem is that the add property button is optional (it only shows up for objects) where the delete button is shown for every property. The extra button causes alignment issues when they're not always added. I might have to have an option for always adding (but sometimes hiding) the button in order to fix the alignment issue.
* Need to check the behavior in the netmonitor and debugger.

Comment 39

[Brandon Benvie [:benvie]]

Attachment: WIP12

Woops that one didn't have the style fixes.

Comment 40

[Brandon Benvie [:benvie]]

Attachment: WIP13

* Creates Editable `create` factory function
* Fix the sizing of the name textbox in the normal VariablesView
* change the Editables to have two callbacks, onSave and onCleanup

Remaining bits:
* Add tests for Scope/Variable#remove
* Add tests for adding properties
* Figure out how to align the left side when there's an add property button

Comment 41

[Brandon Benvie [:benvie]]

Attachment: WIP14

* Add test for removing VariablesView Scopes/Variables
* Add test for adding property to manifest editor
* Fix the alignment issue

Comment 42

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8335048 [details] [diff] [review]
WIP14

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

Note: I have only reviewed the manifest editor / app manager parts.

Overall, this looks good!  But one question about functionality...

It seems the add buttons appear for all properties, even if they are not objects, but then you can only successfully add things to those which are indeed objects.  Should the add button be hidden for non-object types?  It seems confusing to allow the user to "add" a property to something like a string, but then it disappears and is not applied after they finish entering a value.

::: browser/themes/shared/devtools/app-manager/manifest-editor.inc.css
@@ +76,5 @@
> +
> +.manifest-editor .variables-view-add-property::before {
> +  background-image: url("app-manager/add.svg");
> +  -moz-margin-end: 2px;
> +}

Since you're landing the add button appearance for manifest editor, it would be good to add this change too:

 .variables-view-container.manifest-editor {
   background-color: #F5F5F5;
-  padding: 20px 13px;
+  padding: 20px 2px;
 }

so that the +/x buttons fit snugly in the gutter.

Comment 43

[Brandon Benvie [:benvie]]

(In reply to J. Ryan Stinnett [:jryans] from comment #42)
> It seems the add buttons appear for all properties, even if they are not
> objects, but then you can only successfully add things to those which are
> indeed objects.  Should the add button be hidden for non-object types?  It
> seems confusing to allow the user to "add" a property to something like a
> string, but then it disappears and is not applied after they finish entering
> a value.

The add button is indeed added for every property, but it is visibility: hidden for non-objects. The button has to be added to every item when actionsFirst is true in order to keep the items aligned. If you're seeing the add button on everything then something got missed when I last refreshed my patch, though I could swear I uploaded the most recent changes. I'll check tomorrow first thing.

Comment 44

[Brandon Benvie [:benvie]]

Attachment: WIP15

Figured it out. I forgot to add my last css change to osx and linux. This patch fixes that and also adds your suggested padding change to .variables-view-container.manifest-editor.

Comment 45

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8335376 [details] [diff] [review]
WIP15

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

Manifest Editor / App Manager changes look good to me!

Comment 46

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

Comment on attachment 8335376 [details] [diff] [review]
WIP15

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

Some preliminary testing results:

This looks very nice in the manifest editor. Great work! r+ with comments addressed.

This subtly break watch expressions in the debugger. STR
1. Open debugger on http://htmlpad.org/debugger/
2. Click me
3. Add a watch expression, like |window|, notice how it "-> Window" as the evaluation result
4. Double click the expression, to enter input mode
5. Press RETURN

The evaluation result is removed/hidden. This wasn't the case before, the expression was re-evaluated.

If you add a property with a value that throws when evaluating, that property can't be edited anymore. You'd have to delete it first. Try adding a property with the value |x|, or something else that throws.

When adding a new property, pressing TAB without entering a name works and switches to the value input. It shouldn't, because the property name is now an empty string, and can't be modified anymore without starting from scratch.

While editing an already existing property's name, it'd be nice to have TAB save and switch to editing the value. Followup material, for sure.

Code comments below.

::: browser/devtools/app-manager/test/browser_manifest_editor.js
@@ +34,5 @@
>  }
>  
> +// Wait until the animation from commitHierarchy has completed
> +function waitForUpdate() {
> +  return waitForTime(gManifestEditor.editor.lazyEmptyDelay + 1);

Is this really necessary? Would a simple executeSoon work instead?

::: browser/devtools/debugger/test/browser_dbg_variables-view-04.js
@@ +118,5 @@
> +       "VariablesView should have a record of the parent.");
> +    is(variables.getItemForNode(child.target), child,
> +       "VariablesView should have a record of the child.");
> +    is([...parent].length, 1,
> +       "Parent should have one child.");

Sexy!

@@ +123,5 @@
> +
> +    parent.remove();
> +
> +    is(variables.getItemForNode(parent.target), undefined,
> +       "VariablesView should have a record of the parent.");

should *not* ... anymore

@@ +127,5 @@
> +       "VariablesView should have a record of the parent.");
> +    is(parent.target.parentNode, null,
> +       "Parent element should not have a parent.")
> +    is(variables.getItemForNode(child.target), undefined,
> +       "VariablesView should have a record of the child.");

ditto.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +1286,5 @@
> +   */
> +  remove: function() {
> +    let view = this._variablesView;
> +    let index = view._store.indexOf(this);
> +    view._store.splice(index, 1);

You could inline index here, since it's only used once.

@@ +1288,5 @@
> +    let view = this._variablesView;
> +    let index = view._store.indexOf(this);
> +    view._store.splice(index, 1);
> +    view._itemsByElement.delete(this._target);
> +    view._currHierarchy.delete(this._absoluteName);

Are you sure scopes have _absoluteNames? I don't remember this being the case, but please double check.

Please also explicitly test the removal of these elements from the maps and arrays, in browser_dbg_variables-view-04.js.

@@ +1290,5 @@
> +    view._store.splice(index, 1);
> +    view._itemsByElement.delete(this._target);
> +    view._currHierarchy.delete(this._absoluteName);
> +    this._target.remove();
> +    for (let variable of this._store.values()) {

Is .values() standard? If not, I'd suggest sticking to "let [, variable] of ..."

@@ +2172,5 @@
> +   */
> +  remove: function() {
> +    this.ownerView._store.delete(this._nameString);
> +    this._variablesView._itemsByElement.delete(this._target);
> +    this._variablesView._currHierarchy.delete(this._absoluteName);

Ditto for explicitly testing whether those are removed from the owner maps/arrays.

@@ -2309,5 @@
>      this._idString = generateId(this._nameString = aName);
>      this._displayScope(aName, "variables-view-variable variable-or-property");
>  
> -    // Don't allow displaying variable information there's no name available.
> -    if (this._nameString) {

Why was this conditional removed? (Not saying it's necessarily bad, I just want to better understand).

@@ +2481,5 @@
> +      let addPropertyNode = this._addPropertyNode = this.document.createElement("toolbarbutton");
> +      addPropertyNode.className = "plain variables-view-add-property";
> +      addPropertyNode.addEventListener("mousedown", this._onAddProperty.bind(this), false);
> +      if (actionsFirst &&
> +          (!descriptor.value || VariablesView.isPrimitive(descriptor))) {

I think you might mean "value" in descriptor here. Would it make sense to make isPrimitive always return true when there's not value in the descriptor? I would vote yes.

@@ +2656,5 @@
> +      onSave: aKey => {
> +        this.ownerView.switch(this, aKey);
> +        if (!this._variablesView.preventDisableOnChange) {
> +          this._disable();
> +          this._name.setAttribute("value", aKey);

Just a thought: can we remove this
> this._name.setAttribute("value", aKey);
since it's going to be applied anyway after evalling? If so, let's do it here.

@@ +2762,5 @@
> +      onSave: ([aKey, aValue]) => {
> +        this.ownerView.new(this, aKey, aValue);
> +        if (!this._variablesView.preventDisableOnChange) {
> +          this._disable();
> +          this._name.setAttribute("value", aKey);

Ditto for this call.

@@ -2895,5 @@
>      this._idString = generateId(this._nameString = aName);
>      this._displayScope(aName, "variables-view-property variable-or-property");
>  
> -    // Don't allow displaying property information there's no name available.
> -    if (this._nameString) {

Ditto for "why was this conditional removed?".

@@ +3184,5 @@
> + * allowing the user to edit the value.
> + *
> + * @param Variable aVariable
> + *        The Variable or Property to make editable.
> + * @param Function aOptions

I think you mean @param object aOptions here.

@@ +3185,5 @@
> + *
> + * @param Variable aVariable
> + *        The Variable or Property to make editable.
> + * @param Function aOptions
> + *                 - onSave

Super Nit: can you move the options description to the left in this comment? It feel's like there's too much wasted space. Something like this: 

@param Function aOptions
         - onSave: ...
         - onCleanup: ...

@@ +3302,5 @@
> +  _save: function() {
> +    let initial = this.label.getAttribute("value");
> +    let current = this._input.value.trim();
> +    this.deactivate();
> +    if (initial != current) {

I think this might be the thing affecting the debugger's watch expressions.

@@ +3409,5 @@
> +
> +EditableNameAndValue.prototype = Heritage.extend(EditableName.prototype, {
> +  _reset: function(e) {
> +    // Hide the Varible or Property if the user presses escape.
> +    this._variable.hide();

Wasn't this supposed to be remove()?

::: browser/devtools/shared/widgets/widgets.css
@@ +72,5 @@
>    display: none;
>  }
> +
> +.variables-view-delete > .toolbarbutton-text,
> +.variables-view-add-property > .toolbarbutton-text {

Super nit: can you move these rules right below "*:not(:hover) ... "? It makes it easier to follow how the delete and add-property buttons behave.

::: browser/themes/shared/devtools/app-manager/manifest-editor.inc.css
@@ +24,1 @@
>    font-family: monospace;

Shouldn't we use .devtools-monospace for things like these? Not critical, but it would make more sense to have such things consistent across tools.

@@ +55,5 @@
>  .manifest-editor .variables-view-delete:hover,
> +.manifest-editor .variables-view-delete:active,
> +.manifest-editor .variables-view-add-property,
> +.manifest-editor .variables-view-add-property:hover,
> +.manifest-editor .variables-view-add-property:active {

Why wouldn't these belong in widgets.css ? (just curious)

@@ +61,5 @@
>    -moz-image-region: initial;
>  }
>  
> +.manifest-editor .variables-view-delete::before,
> +.manifest-editor .variables-view-add-property::before {

Ditto for this and the other rules in this file.
I think there's no icon for adding new properties in the variables view right? The only place it's defined is here. I guess it'd be ok to use them everywhere?

Comment 47

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Victor Porof [:vp] from comment #46)
> @@ +61,5 @@
> >    -moz-image-region: initial;
> >  }
> >  
> > +.manifest-editor .variables-view-delete::before,
> > +.manifest-editor .variables-view-add-property::before {
> 
> Ditto for this and the other rules in this file.
> I think there's no icon for adding new properties in the variables view
> right? The only place it's defined is here. I guess it'd be ok to use them
> everywhere?

You're right that there is no icon currently, except for the manifest editor...  We could use that icon in the general case, though the appearance was taken from manifest editor specific mockup, so it may or may not fit in with the usual appearance of the variables view.

Comment 48

[Brandon Benvie [:benvie]]

Thanks for the thorough review! Responses below where needed.

(In reply to Victor Porof [:vp] from comment #46)
> This subtly break watch expressions in the debugger. STR ...

Looks like this was caused by moved `this.ownerView.eval(...)` to before `this._disable()`. Swapping the order back how it originally was fixes the issue.

> ::: browser/devtools/app-manager/test/browser_manifest_editor.js
> @@ +34,5 @@
> >  }
> >  
> > +// Wait until the animation from commitHierarchy has completed
> > +function waitForUpdate() {
> > +  return waitForTime(gManifestEditor.editor.lazyEmptyDelay + 1);
> 
> Is this really necessary? Would a simple executeSoon work instead?

The manifest editor tests run with view.lazyEmpty = true (unlike the debugger tests which disable lazy addition). LAZY_EMPTY_DELAY defaults to 150ms so executeSoon wouldn't work.

> @@ +1288,5 @@
> > +    let view = this._variablesView;
> > +    let index = view._store.indexOf(this);
> > +    view._store.splice(index, 1);
> > +    view._itemsByElement.delete(this._target);
> > +    view._currHierarchy.delete(this._absoluteName);
> 
> Are you sure scopes have _absoluteNames? I don't remember this being the
> case, but please double check.

Correct you are, should be `this._nameString`.

> @@ +1290,5 @@
> > +    view._store.splice(index, 1);
> > +    view._itemsByElement.delete(this._target);
> > +    view._currHierarchy.delete(this._absoluteName);
> > +    this._target.remove();
> > +    for (let variable of this._store.values()) {
> 
> Is .values() standard? If not, I'd suggest sticking to "let [, variable] of
> ..."

Thankfully it is! {keys, values, entries} are in the ES6 spec for Array, Map, and Set.

> @@ -2309,5 @@
> >      this._idString = generateId(this._nameString = aName);
> >      this._displayScope(aName, "variables-view-variable variable-or-property");
> >  
> > -    // Don't allow displaying variable information there's no name available.
> > -    if (this._nameString) {
> 
> Why was this conditional removed? (Not saying it's necessarily bad, I just
> want to better understand).

Two reasons. The reason specific to this bug is that when adding a new property the key is empty (since the user hasn't had a chance to enter one yet). The second is that it prohibits showing properties with empty string keys from showing up in the variables view despite that being a legitimate key in JS (bug 924295). Two bugs, one patch.

> @@ +2481,5 @@
> > +      let addPropertyNode = this._addPropertyNode = this.document.createElement("toolbarbutton");
> > +      addPropertyNode.className = "plain variables-view-add-property";
> > +      addPropertyNode.addEventListener("mousedown", this._onAddProperty.bind(this), false);
> > +      if (actionsFirst &&
> > +          (!descriptor.value || VariablesView.isPrimitive(descriptor))) {
> 
> I think you might mean "value" in descriptor here. Would it make sense to
> make isPrimitive always return true when there's not value in the
> descriptor? I would vote yes.

Actually, isPrimitive already does all the checking needed here, so there's no reason to even do this extra check. Not sure why I added it.

> 
> @@ +2656,5 @@
> > +      onSave: aKey => {
> > +        this.ownerView.switch(this, aKey);
> > +        if (!this._variablesView.preventDisableOnChange) {
> > +          this._disable();
> > +          this._name.setAttribute("value", aKey);
> 
> Just a thought: can we remove this
> > this._name.setAttribute("value", aKey);
> since it's going to be applied anyway after evalling? If so, let's do it
> here.

You're right, this can be removed. It's probably good to remove it too so we don't have multiple places setting the raw value.

> @@ +3409,5 @@
> > +
> > +EditableNameAndValue.prototype = Heritage.extend(EditableName.prototype, {
> > +  _reset: function(e) {
> > +    // Hide the Varible or Property if the user presses escape.
> > +    this._variable.hide();
> 
> Wasn't this supposed to be remove()?

Righto!


> @@ +55,5 @@
> >  .manifest-editor .variables-view-delete:hover,
> > +.manifest-editor .variables-view-delete:active,
> > +.manifest-editor .variables-view-add-property,
> > +.manifest-editor .variables-view-add-property:hover,
> > +.manifest-editor .variables-view-add-property:active {
> 
> Why wouldn't these belong in widgets.css ? (just curious)

I believe these styles were just specifically needed for the manifest editor due to it having the actions on the left (jryans can correct me if I'm wrong here).

> @@ +61,5 @@
> >    -moz-image-region: initial;
> >  }
> >  
> > +.manifest-editor .variables-view-delete::before,
> > +.manifest-editor .variables-view-add-property::before {
> 
> Ditto for this and the other rules in this file.
> I think there's no icon for adding new properties in the variables view
> right? The only place it's defined is here. I guess it'd be ok to use them
> everywhere?

As jryans said, the only styles for it were from the manifest editor design, which differs from the other VariablesView usage. A followup to enable adding properties to the rest of the Variables View usage would need to include a bit of style (along with the changes to handle asynchronous resolution). We'd need to get a "+" analog to the "x" png we're using for these.

Comment 49

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vp] from comment #46)
> @@ +1288,5 @@
> > +    let view = this._variablesView;
> > +    let index = view._store.indexOf(this);
> > +    view._store.splice(index, 1);
> > +    view._itemsByElement.delete(this._target);
> > +    view._currHierarchy.delete(this._absoluteName);
> 
> Are you sure scopes have _absoluteNames? I don't remember this being the
> case, but please double check.
> 
> Please also explicitly test the removal of these elements from the maps and
> arrays, in browser_dbg_variables-view-04.js.

Most of those were being tested, but `view._itemsByElement` and `view._currHierarchy` were not being checked for emptiness, which is how this snuck through. I've added checks for those to the test.

Comment 50

[Brandon Benvie [:benvie]]

Attachment: WIP16

Addresses victor's feedback.

Comment 51

[Brandon Benvie [:benvie]]

Attachment: WIP17

Fixes the test (needed to use Cu.nondeterministicGetWeakMapKeys on _itemsByElement). https://tbpl.mozilla.org/?tree=Try&rev=d288db32e5b5

Comment 52

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

This is excellent. Thanks benvie! o/

Comment 53

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

(In reply to Brandon Benvie [:benvie] from comment #48)
> 
> The manifest editor tests run with view.lazyEmpty = true (unlike the
> debugger tests which disable lazy addition). LAZY_EMPTY_DELAY defaults to
> 150ms so executeSoon wouldn't work.
> 

Thanks, got it.

Sounds like a potential cause for oranges, but hopefully it won't be the case. If things start timing out we'll know it's probably because of this, in which case it'd be a good idea to disable lazyEmpty in tests.

Comment 54

[Brandon Benvie [:benvie]]

Looks like those busted builds are unrelated (it's due to bug 940541 bustage).

Comment 55

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vp] from comment #53)
> (In reply to Brandon Benvie [:benvie] from comment #48)
> > 
> > The manifest editor tests run with view.lazyEmpty = true (unlike the
> > debugger tests which disable lazy addition). LAZY_EMPTY_DELAY defaults to
> > 150ms so executeSoon wouldn't work.
> > 
> 
> Thanks, got it.
> 
> Sounds like a potential cause for oranges, but hopefully it won't be the
> case. If things start timing out we'll know it's probably because of this,
> in which case it'd be a good idea to disable lazyEmpty in tests.

I'm hoping not, since the timer is just a bit longer than the longest possible wait for lazyEmpty to happen. But yeah, we should keep an eye on this.

Comment 56

[Brandon Benvie [:benvie]]

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

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e0572cb4eb82