Use the VariablesView in webconsole

RESOLVED FIXED in Firefox 23

Status

P2
normal
RESOLVED FIXED
6 years ago
2 months ago

People

(Reporter: vporof, Assigned: msucan)

Tracking

(Blocks: 2 bugs, {dev-doc-needed})

Trunk
Firefox 23
dev-doc-needed
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sharing-code])

Attachments

(5 attachments, 8 obsolete attachments)

(Reporter)

Description

6 years ago
See bug 794823.
(Reporter)

Updated

6 years ago
Whiteboard: [sharing-code]
(Reporter)

Comment 1

6 years ago
Not actually needed by this bug, but to avoid confusion: note that there's a typo in the VariablesView constructor description: 'To allow replacing variable or property values, provide a "switch" function.'

s/values/names/

This is only needed for watch expressions in the debugger.

For changing values there's the eval function property, as described in the same comment.
(Assignee)

Updated

6 years ago
Assignee: nobody → mihai.sucan
Blocks: 783499
Status: NEW → ASSIGNED
Version: unspecified → Trunk
(Assignee)

Updated

6 years ago
Blocks: 817547
(Assignee)

Updated

6 years ago
Blocks: 720177
(Assignee)

Updated

6 years ago
Depends on: 660197
(Assignee)

Updated

6 years ago
Depends on: 831724
(Reporter)

Updated

6 years ago
Depends on: 830702
(Assignee)

Updated

6 years ago
Depends on: 832473
(Assignee)

Updated

6 years ago
Blocks: 830609
(Assignee)

Updated

6 years ago
Depends on: 833411
(Assignee)

Updated

6 years ago
Blocks: 586246
(Reporter)

Updated

6 years ago
Depends on: 820878
(Assignee)

Updated

6 years ago
Depends on: 830818
(Assignee)

Comment 2

6 years ago
Created attachment 706571 [details] [diff] [review]
wip 1

Work in progress patch. All functionality seems to be working well, except most of the tests. Tests are broken for several reasons: the switch to the debugger API has its toll, the temporary failures caused by bug 820524 (until this patch is ready), and the switch to the variables view breaks a bunch of property panel-related tests.

This patch is almost ready for review - missing some new tests and fixes for existing web console tests in browser/.

There are no known-to-me issues related to functionality in this patch. I do have a bunch of ideas for follow up bugs to report. Will do that, later on, as we progress with the reviews.

Before you apply this patch you need the patches from: bug 820524 and bug 783499.

Due to the size of the patch I suggest we start the reviews before all of the tests are fixed. Thank you!
Attachment #706571 - Flags: feedback?(vporof)
Attachment #706571 - Flags: feedback?(past)
(Assignee)

Updated

6 years ago
Depends on: 720180
(Reporter)

Comment 3

6 years ago
Comment on attachment 706571 [details] [diff] [review]
wip 1

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

::: browser/devtools/jar.mn
@@ +7,5 @@
>      content/browser/devtools/markup-view.css      (markupview/markup-view.css)
>      content/browser/NetworkPanel.xhtml            (webconsole/NetworkPanel.xhtml)
>      content/browser/devtools/webconsole.js        (webconsole/webconsole.js)
>      content/browser/devtools/webconsole.xul       (webconsole/webconsole.xul)
> +    content/browser/devtools/VariablesView.xul    (webconsole/VariablesView.xul)

Hooray!

::: browser/devtools/shared/VariablesView.jsm
@@ +590,5 @@
>    _emptyTextNode: null,
>    _emptyTextValue: ""
>  };
>  
> +this.VariablesView.NON_SORTABLE_CLASSES = [

Nit: "this." is not necessary.

@@ +1607,5 @@
>      if (!isUndefined && (descriptor.get || descriptor.set)) {
>        // FIXME: editing getters and setters is not allowed yet. Bug 831794.
>        this.eval = null;
> +      // Deleting getters and setters individually is not allowed.
> +      this.delete = null;

This is not the case anymore after bug 831794 and bug 837052. You can edit or delete getters and setters. There's a new (public) evaluationMacro method called when determining what string is passed to the eval function. Feel free to bend it to your needs :)

::: browser/devtools/webconsole/VariablesView.xul
@@ +11,5 @@
> +  %viewDTD;
> +]>
> +<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" title="&PropertiesViewWindowTitle;">
> +  <vbox id="variables" flex="1"/>
> +</window>

Why not move this file directly in shared?

::: browser/devtools/webconsole/webconsole.js
@@ +3164,5 @@
> +      frame: this.SELECTED_FRAME,
> +      bindObjectActor: aOptions.objectActor.actor,
> +    };
> +
> +    this.requestEvaluation("_self" + aString, evalOptions).then(onEval, onEval);

Maybe use evaluationMacro, since you never know what you might end up getting as aString? For example, if it weren't trimmed, this eval would fail.

@@ +3187,5 @@
> +      bindObjectActor: aOptions.objectActor.actor,
> +    };
> +
> +    this.requestEvaluation("delete _self" + aVar.symbolicName, evalOptions)
> +        .then(onEval, onEval);

Should something happen to inform the user when delete fails?

@@ +3216,5 @@
> +
> +    let newSymbolicName = aVar.ownerView.symbolicName + '["' + aNewName + '"]';
> +
> +    let code = "_self" + newSymbolicName + " = _self" + aVar.symbolicName + ";" +
> +               "delete _self" + aVar.symbolicName;

What happens if the same name is given? Something tells me that you'll forever loose the variable.

@@ +3276,5 @@
> +    if (!grip) {
> +      throw new Error("No object actor grip was given for the variable.");
> +    }
> +
> +    let actors = aVar._variablesView._consoleObjectActors;

Hmm. Although harmless, you're using a private property here. Why wouldn't this._variablesView work?

On a related note, why are you storing _consoleObjectActors as a member on the view instance? Are you anticipating having multiple variable views in the sidebar (you probably wouldn't need that)? If it's because of console.dir, then.. ok, I guess?

@@ +3311,5 @@
> +      if (ownProperties) {
> +        aVar.addProperties(ownProperties, { sorted: sortable });
> +
> +        for (let name in ownProperties) {
> +          onNewProperty(name);

Nit: you can pass a callback to the options object to avoid the extra loop.

@@ +3323,5 @@
> +      }
> +
> +      aVar._retrieved = true;
> +
> +      aVar._variablesView.commitHierarchy();

Again, aVar._variablesView?

@@ +3356,5 @@
> +        }
> +
> +        aVar.onexpand = null;
> +        aVar.setGrip(grip.initial + aResponse.substring);
> +        aVar.hideArrow();

Should the arrow be hidden automatically when passing a primitive as a grip? I could make the API handle that, but it may be "too much magic" under the hood.
Attachment #706571 - Flags: feedback?(vporof) → feedback+
(Assignee)

Updated

6 years ago
Blocks: 688400
This seems to have bitrotted? It doesn't apply cleanly on top of bugs 720180, 820524 and 783499.
(Assignee)

Comment 5

6 years ago
(In reply to Panos Astithas [:past] from comment #4)
> This seems to have bitrotted? It doesn't apply cleanly on top of bugs
> 720180, 820524 and 783499.

Yes, landing of vview patches bitrotted this one. I will submit an updated patch tomorrow.
Comment on attachment 706571 [details] [diff] [review]
wip 1

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

I couldn't try it out due to bit rot, but it looks nice. I can't wait to see what the console now looks like!

::: browser/devtools/framework/Sidebar.jsm
@@ +66,5 @@
>        }
>        this.emit(id + "-ready");
>      }.bind(this);
>  
> +    iframe.addEventListener("load", onIFrameLoaded, true);

You should probably run the changes in this file by Paul.

::: browser/devtools/jar.mn
@@ +7,5 @@
>      content/browser/devtools/markup-view.css      (markupview/markup-view.css)
>      content/browser/NetworkPanel.xhtml            (webconsole/NetworkPanel.xhtml)
>      content/browser/devtools/webconsole.js        (webconsole/webconsole.js)
>      content/browser/devtools/webconsole.xul       (webconsole/webconsole.xul)
> +    content/browser/devtools/VariablesView.xul    (webconsole/VariablesView.xul)

Since the code is in shared, the markup should move there, too.

::: browser/devtools/webconsole/webconsole.js
@@ +1115,5 @@
>      if (level == "trace") {
>        node._stacktrace = aMessage.stacktrace;
>  
>        this.makeOutputMessageLink(node, function _traceNodeClickCallback() {
> +        this.jsterm.openVariablesView({ rawObject: node._stacktrace });

Stack trace in the variables view, eh? Interesting!

@@ +2811,5 @@
>  
> +    node._objectActors = new Set();
> +
> +    let error = aResponse.exception;
> +    if (error && typeof error == "object" && error.actor) {

I don't believe this is strictly necessary, but OK.

@@ +3263,5 @@
> +   * @param object [aGrip]
> +   *        Optional, the object actor grip of the variable. If the grip is not
> +   *        provided, then the aVar.value is used as the object actor grip.
> +   */
> +  _fetchVarProperties: function JST__fetchVarProperties(aVar, aGrip)

Most of these new methods look very similar to the ones used by the debugger frontend. It would be a shame if we can't factor them out into a common shared utility jsm.
Attachment #706571 - Flags: feedback?(past) → feedback+
Priority: -- → P2
Blocks: 640225
Blocks: 640653
(Assignee)

Comment 7

6 years ago
(In reply to Victor Porof [:vp] from comment #3)
> @@ +1607,5 @@
> >      if (!isUndefined && (descriptor.get || descriptor.set)) {
> >        // FIXME: editing getters and setters is not allowed yet. Bug 831794.
> >        this.eval = null;
> > +      // Deleting getters and setters individually is not allowed.
> > +      this.delete = null;
> 
> This is not the case anymore after bug 831794 and bug 837052. You can edit
> or delete getters and setters. There's a new (public) evaluationMacro method
> called when determining what string is passed to the eval function. Feel
> free to bend it to your needs :)

Great! I have an updated patch that works nicely with getters and setters.


> ::: browser/devtools/webconsole/VariablesView.xul
> @@ +11,5 @@
> > +  %viewDTD;
> > +]>
> > +<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" title="&PropertiesViewWindowTitle;">
> > +  <vbox id="variables" flex="1"/>
> > +</window>
> 
> Why not move this file directly in shared?

Good point. Done.


> ::: browser/devtools/webconsole/webconsole.js
> @@ +3164,5 @@
> > +      frame: this.SELECTED_FRAME,
> > +      bindObjectActor: aOptions.objectActor.actor,
> > +    };
> > +
> > +    this.requestEvaluation("_self" + aString, evalOptions).then(onEval, onEval);
> 
> Maybe use evaluationMacro, since you never know what you might end up
> getting as aString? For example, if it weren't trimmed, this eval would fail.

Yep, but the API wasn't available when I submitted the patch. You added it slightly later. :)

> @@ +3187,5 @@
> > +      bindObjectActor: aOptions.objectActor.actor,
> > +    };
> > +
> > +    this.requestEvaluation("delete _self" + aVar.symbolicName, evalOptions)
> > +        .then(onEval, onEval);
> 
> Should something happen to inform the user when delete fails?

Good point. I had this planned. Fixed now.


> @@ +3216,5 @@
> > +
> > +    let newSymbolicName = aVar.ownerView.symbolicName + '["' + aNewName + '"]';
> > +
> > +    let code = "_self" + newSymbolicName + " = _self" + aVar.symbolicName + ";" +
> > +               "delete _self" + aVar.symbolicName;
> 
> What happens if the same name is given? Something tells me that you'll
> forever loose the variable.

Good catch. Fixed.

> @@ +3276,5 @@
> > +    if (!grip) {
> > +      throw new Error("No object actor grip was given for the variable.");
> > +    }
> > +
> > +    let actors = aVar._variablesView._consoleObjectActors;
> 
> Hmm. Although harmless, you're using a private property here. Why wouldn't
> this._variablesView work?
> 
> On a related note, why are you storing _consoleObjectActors as a member on
> the view instance? Are you anticipating having multiple variable views in
> the sidebar (you probably wouldn't need that)? If it's because of
> console.dir, then.. ok, I guess?

I use aVar._variablesView and I add the _consoleObjectActors property to vviews because I have multiple instances of vviews throughout the web console, one for each console.dir(), and one for the sidebar. |this._variablesView| is a definite no-no.


> @@ +3311,5 @@
> > +      if (ownProperties) {
> > +        aVar.addProperties(ownProperties, { sorted: sortable });
> > +
> > +        for (let name in ownProperties) {
> > +          onNewProperty(name);
> 
> Nit: you can pass a callback to the options object to avoid the extra loop.

This is new API you added! Fixed. :)

> @@ +3356,5 @@
> > +        }
> > +
> > +        aVar.onexpand = null;
> > +        aVar.setGrip(grip.initial + aResponse.substring);
> > +        aVar.hideArrow();
> 
> Should the arrow be hidden automatically when passing a primitive as a grip?
> I could make the API handle that, but it may be "too much magic" under the
> hood.

This is fine. Too much magic might be a problem...

Thanks for the feedback and for the vview fixes!
(Assignee)

Comment 8

6 years ago
(In reply to Panos Astithas [:past] from comment #6)
> Comment on attachment 706571 [details] [diff] [review]
> wip 1
> 
> Review of attachment 706571 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I couldn't try it out due to bit rot, but it looks nice. I can't wait to see
> what the console now looks like!

Thanks for the feedback!


> ::: browser/devtools/framework/Sidebar.jsm
> @@ +66,5 @@
> >        }
> >        this.emit(id + "-ready");
> >      }.bind(this);
> >  
> > +    iframe.addEventListener("load", onIFrameLoaded, true);
> 
> You should probably run the changes in this file by Paul.

Good point.


> ::: browser/devtools/jar.mn
> @@ +7,5 @@
> >      content/browser/devtools/markup-view.css      (markupview/markup-view.css)
> >      content/browser/NetworkPanel.xhtml            (webconsole/NetworkPanel.xhtml)
> >      content/browser/devtools/webconsole.js        (webconsole/webconsole.js)
> >      content/browser/devtools/webconsole.xul       (webconsole/webconsole.xul)
> > +    content/browser/devtools/VariablesView.xul    (webconsole/VariablesView.xul)
> 
> Since the code is in shared, the markup should move there, too.

Done.

> ::: browser/devtools/webconsole/webconsole.js
> @@ +1115,5 @@
> >      if (level == "trace") {
> >        node._stacktrace = aMessage.stacktrace;
> >  
> >        this.makeOutputMessageLink(node, function _traceNodeClickCallback() {
> > +        this.jsterm.openVariablesView({ rawObject: node._stacktrace });
> 
> Stack trace in the variables view, eh? Interesting!

We had it in the property panel...


> @@ +3263,5 @@
> > +   * @param object [aGrip]
> > +   *        Optional, the object actor grip of the variable. If the grip is not
> > +   *        provided, then the aVar.value is used as the object actor grip.
> > +   */
> > +  _fetchVarProperties: function JST__fetchVarProperties(aVar, aGrip)
> 
> Most of these new methods look very similar to the ones used by the debugger
> frontend. It would be a shame if we can't factor them out into a common
> shared utility jsm.

This is something I already discussed with Victor. We have bug 828680 for this aspect of code sharing.
(Assignee)

Comment 9

6 years ago
Created attachment 711391 [details] [diff] [review]
getting-there patch

This patch addresses feedback comments from Victor and Panos and fixes all of the browser webconsole tests.

Remaining issues:

- bug 820524 has several tests that continue to fail and they break in here as well.
- some tests fail because when we inspect objects like |document| and |window| we do not have any of the usually expected property and methods: |body|, |querySelectorAll| and so on. Panos, how should we fix these? Will bug 830818 fix this problem?
- I need to add a couple of tests for the new variables view, and for eval-in-frame.
- I need to push this to the try server, but I cannot do this yet, given the known failures.

This is pretty much all.

Paul: please review the sidebar changes and how it is used.
Victor: please look into the variables view changes.
Panos: everything else. :)

Thank you guys!
Attachment #706571 - Attachment is obsolete: true
Attachment #711391 - Flags: review?(vporof)
Attachment #711391 - Flags: review?(paul)
Attachment #711391 - Flags: review?(past)
(Reporter)

Comment 10

6 years ago
(In reply to Mihai Sucan [:msucan] from comment #7)
> > 
> > On a related note, why are you storing _consoleObjectActors as a member on
> > the view instance? Are you anticipating having multiple variable views in
> > the sidebar (you probably wouldn't need that)? If it's because of
> > console.dir, then.. ok, I guess?
> 
> I use aVar._variablesView and I add the _consoleObjectActors property to
> vviews because I have multiple instances of vviews throughout the web
> console, one for each console.dir(), and one for the sidebar.
> |this._variablesView| is a definite no-no.
> 

Maybe you could keep a WeakMap of variable views? Either that or publicize the method. I would prefer the former, but it's your decision.
(Reporter)

Comment 11

6 years ago
Comment on attachment 711391 [details] [diff] [review]
getting-there patch

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

Variables View usage looks good to me, I have just a few nits, see below.

I couldn't get to play with all the shiny new functionalities because no matter what incantations I made, I always ended up with
> make[4]: *** No rule to make target `browser_webconsole_property_panel.js', needed by `libs'.  Stop.
So I need to get back to this.

::: browser/devtools/shared/VariablesView.jsm
@@ +972,5 @@
>        // If the parent object will end up without any getter or setter,
>        // morph it into a plain value.
>        if ((type == "set" && propertyObject.getter.type == "undefined") ||
>            (type == "get" && propertyObject.setter.type == "undefined")) {
> +        return propertyObject.evaluationMacro(propertyObject, "undefined");

Having a generic evaluationMacro method called on the property object may seem surprising to people first reading the code. Since it should *always* be the case that a macro dealing with overriding getter/setter properties to plain values should be called here, add a comment suggesting something like "Make sure the right getter/setter to value override macro is applied to the target object".

@@ +1030,5 @@
>   *        The current getter or setter property.
>   */
>  VariablesView.getterOrSetterDeleteCallback = function(aItem) {
>    aItem._disable();
> +  aItem.ownerView.eval(aItem.evaluationMacro(aItem, ""));

Ditto, "Make sure the right getter/setter removal macro is applied to the target object".

@@ +1898,5 @@
>     *                 someProp3: { value: { type: "undefined" } },
>     *                 someProp4: { value: { type: "null" } },
>     *                 someProp5: { value: { type: "object", class: "Object" } },
>     *                 someProp6: { get: { type: "object", class: "Function" },
> +   *                              set: { type: "undefined" } } }

OCD!

@@ +2162,5 @@
>        this.switch = null;
>  
>        // Getter/setter properties require special handling when it comes to
>        // evaluation and deletion.
> +      this.evaluationMacro = VariablesView.overrideValueEvalMacro;

Keep this inside the conditional and add
> this.evaluationMacro = null;
in the else branch.

::: browser/devtools/webconsole/test/head.js
@@ +319,5 @@
>    });
>  }
> +
> +/**
> + * Find variables or properties in a VariablesView instance.

I love this!

@@ +344,5 @@
> + */
> +function findVariableViewProperties(aView, aRules, aOptions)
> +{
> +  let outstanding = [];
> +  let rules = [].concat(aRules); // keep a copy of the rules array

Nit: Array.slice(aRules); ?

@@ +424,5 @@
> +  }
> +
> +  if (aRule.value) {
> +    let displayValue = aProp.displayValue;
> +    if (aProp._valueClassName == "token-string") {

Since you added a getter for _valueString, you should also add one for _valueClassName and use it here. Maybe call it displayClassName?

@@ +439,5 @@
> +
> +  if ("isGetter" in aRule) {
> +    let isGetter = !!(aProp.getter && aProp.get("get"));
> +    if (aRule.isGetter != isGetter) {
> +      console.debug("isGetter blaaa", aRule.isGetter, isGetter);

"is Getter blaaa"
leftover? :)
(Reporter)

Comment 12

6 years ago
Created attachment 712104 [details]
width-issue

Managed to finally get the build built :) Thanks mihai for updating your github patch queue. If anyone else gets into the same issues I did, make sure you have the following queue from [0]:

 0 A bug-837723
 1 A bug-820524
 2 A bug-783499
 3 A bug-808370

That said, there are a few oddities with how the webconsole now behaves on OS X. I tested everything on [1]. I'm still playing with it, but here are some usability issues I found so far:

* There webconsole "content" width is larger than the owner window, so a scrollbar appears below and the header is offset a few pixels on the right. See the "width-issue" screenshot. 
  Also, I found the grippy separating the logging output and the sidebar extremely buggy and awkward; most of the times clicking it wouldn't have any effect (either that or I couldn't get my clicks pixel-perfect accurate enough). I think we need to find a different way of collapsing/showing the variables view, maybe something like what the debugger is using? The grippy is frustrating, and exactly the reason I avoided it with the debugger.

* When inspecting an object in the sidebar, I immediately got dizzy visually parsing the 5 toolbars stacked on top of each other. See the "all-the-toolbars!" screenshot. I feel a bit uneasy about this; maybe getting rid of showing "Properties" header, and scope separator when not necessary would help, but having 3 separators stacked on top of each other is still too much. It would also be more efficient to combine the sidebar header with the main webconsole header (similar to what the inspector is doing).
  Having two "Filter" filter boxes on top of each other is also a bit spooky :) If I'm not familiar with the webconsole, it's not immediately obvious that one filters output and one filters variables.

* When clicking on an object to inspect it (from the output), the logs got scrolled upwards and the "[object Object]" line got hidden. I understand why this happens, but I find it may be a bit annoying.

* console.dir() output is much too restricted (vertically) imo. I originally thought this was a bug and wanted to scroll the main output to see more properties. See the "awkward-scrolling" screenshot. I think you should allow for at least twice the current height for the inlined variables view (or something like 75% of the visible logging area height).
  Also, adding some margin below the view to suggest that "this is a subregion of scrollable content, don't try to scroll the main output" would help a lot.

* "debugger eval code:1" links shouldn't be present in the webconsole output. Clicking them shows an empty view source window.

* There is an unexpectedly large amount of flicker when modifying properties (through eval, switch, delete etc.). I think it's a good idea to make LAZY_EMPTY_DELAY a configurable property on the VariablesView.prototype and double it for the webconsole.

[0] https://github.com/mihaisucan/mozilla-patch-queue/tree/master/patches-webconsole
[1] http://todomvc.com/architecture-examples/backbone/
(Reporter)

Comment 13

6 years ago
Created attachment 712105 [details]
awkward-inline-scrolling
(Reporter)

Comment 14

6 years ago
Created attachment 712106 [details]
all-the-toolbars!
(Reporter)

Comment 15

6 years ago
Comment on attachment 711391 [details] [diff] [review]
getting-there patch

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

Variables View usage looks good to me, I have just a few nits, see below.

I couldn't get to play with all the shiny new functionalities because no matter what incantations I made, I always ended up with
> make[4]: *** No rule to make target `browser_webconsole_property_panel.js', needed by `libs'.  Stop.
So I need to get back to this.

::: browser/devtools/shared/VariablesView.jsm
@@ +972,5 @@
>        // If the parent object will end up without any getter or setter,
>        // morph it into a plain value.
>        if ((type == "set" && propertyObject.getter.type == "undefined") ||
>            (type == "get" && propertyObject.setter.type == "undefined")) {
> +        return propertyObject.evaluationMacro(propertyObject, "undefined");

Having a generic evaluationMacro method called on the property object may seem surprising to people first reading the code. Since it should *always* be the case that a macro dealing with overriding getter/setter properties to plain values should be called here, add a comment suggesting something like "Make sure the right getter/setter to value override macro is applied to the target object".

@@ +1030,5 @@
>   *        The current getter or setter property.
>   */
>  VariablesView.getterOrSetterDeleteCallback = function(aItem) {
>    aItem._disable();
> +  aItem.ownerView.eval(aItem.evaluationMacro(aItem, ""));

Ditto, "Make sure the right getter/setter removal macro is applied to the target object".

@@ +1898,5 @@
>     *                 someProp3: { value: { type: "undefined" } },
>     *                 someProp4: { value: { type: "null" } },
>     *                 someProp5: { value: { type: "object", class: "Object" } },
>     *                 someProp6: { get: { type: "object", class: "Function" },
> +   *                              set: { type: "undefined" } } }

OCD!

@@ +2162,5 @@
>        this.switch = null;
>  
>        // Getter/setter properties require special handling when it comes to
>        // evaluation and deletion.
> +      this.evaluationMacro = VariablesView.overrideValueEvalMacro;

Keep this inside the conditional and add
> this.evaluationMacro = null;
in the else branch.

::: browser/devtools/webconsole/test/head.js
@@ +319,5 @@
>    });
>  }
> +
> +/**
> + * Find variables or properties in a VariablesView instance.

I love this!

@@ +344,5 @@
> + */
> +function findVariableViewProperties(aView, aRules, aOptions)
> +{
> +  let outstanding = [];
> +  let rules = [].concat(aRules); // keep a copy of the rules array

Nit: Array.slice(aRules); ?

@@ +424,5 @@
> +  }
> +
> +  if (aRule.value) {
> +    let displayValue = aProp.displayValue;
> +    if (aProp._valueClassName == "token-string") {

Since you added a getter for _valueString, you should also add one for _valueClassName and use it here. Maybe call it displayClassName?

@@ +439,5 @@
> +
> +  if ("isGetter" in aRule) {
> +    let isGetter = !!(aProp.getter && aProp.get("get"));
> +    if (aRule.isGetter != isGetter) {
> +      console.debug("isGetter blaaa", aRule.isGetter, isGetter);

"is Getter blaaa"
leftover? :)
Attachment #711391 - Flags: review?(vporof)
(Assignee)

Comment 16

6 years ago
Created attachment 712114 [details] [diff] [review]
address review from victor
Attachment #711391 - Attachment is obsolete: true
Attachment #711391 - Flags: review?(paul)
Attachment #711391 - Flags: review?(past)
Attachment #712114 - Flags: review?(vporof)
Attachment #712114 - Flags: review?(paul)
Attachment #712114 - Flags: review?(past)
(Assignee)

Comment 17

6 years ago
(In reply to Victor Porof [:vp] from comment #15)
> Comment on attachment 711391 [details] [diff] [review]
> getting-there patch
> 
> Review of attachment 711391 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Variables View usage looks good to me, I have just a few nits, see below.
> 
> I couldn't get to play with all the shiny new functionalities because no
> matter what incantations I made, I always ended up with
> > make[4]: *** No rule to make target `browser_webconsole_property_panel.js', needed by `libs'.  Stop.
> So I need to get back to this.

Argh! This is what I get from using smartmake. Fixed.


> ::: browser/devtools/shared/VariablesView.jsm
> @@ +972,5 @@
> >        // If the parent object will end up without any getter or setter,
> >        // morph it into a plain value.
> >        if ((type == "set" && propertyObject.getter.type == "undefined") ||
> >            (type == "get" && propertyObject.setter.type == "undefined")) {
> > +        return propertyObject.evaluationMacro(propertyObject, "undefined");
> 
> Having a generic evaluationMacro method called on the property object may
> seem surprising to people first reading the code. Since it should *always*
> be the case that a macro dealing with overriding getter/setter properties to
> plain values should be called here, add a comment suggesting something like
> "Make sure the right getter/setter to value override macro is applied to the
> target object".

Good point. Updated.


> @@ +1898,5 @@
> >     *                 someProp3: { value: { type: "undefined" } },
> >     *                 someProp4: { value: { type: "null" } },
> >     *                 someProp5: { value: { type: "object", class: "Object" } },
> >     *                 someProp6: { get: { type: "object", class: "Function" },
> > +   *                              set: { type: "undefined" } } }
> 
> OCD!

This broke my vim jumps between { and }. I had to be fix it! :)


> @@ +2162,5 @@
> >        this.switch = null;
> >  
> >        // Getter/setter properties require special handling when it comes to
> >        // evaluation and deletion.
> > +      this.evaluationMacro = VariablesView.overrideValueEvalMacro;
> 
> Keep this inside the conditional and add
> > this.evaluationMacro = null;
> in the else branch.

Fixed.


> ::: browser/devtools/webconsole/test/head.js
> @@ +319,5 @@
> >    });
> >  }
> > +
> > +/**
> > + * Find variables or properties in a VariablesView instance.
> 
> I love this!

I hope we can reuse this code for debugger tests as well.

> @@ +344,5 @@
> > + */
> > +function findVariableViewProperties(aView, aRules, aOptions)
> > +{
> > +  let outstanding = [];
> > +  let rules = [].concat(aRules); // keep a copy of the rules array
> 
> Nit: Array.slice(aRules); ?

Fixed.


> @@ +424,5 @@
> > +  }
> > +
> > +  if (aRule.value) {
> > +    let displayValue = aProp.displayValue;
> > +    if (aProp._valueClassName == "token-string") {
> 
> Since you added a getter for _valueString, you should also add one for
> _valueClassName and use it here. Maybe call it displayClassName?

Sounds good to me. Fixed.


> @@ +439,5 @@
> > +
> > +  if ("isGetter" in aRule) {
> > +    let isGetter = !!(aProp.getter && aProp.get("get"));
> > +    if (aRule.isGetter != isGetter) {
> > +      console.debug("isGetter blaaa", aRule.isGetter, isGetter);
> 
> "is Getter blaaa"
> leftover? :)

Hehe, good catch. :)


Thanks for your review comments! I have updated the patch to address these comments.
(Assignee)

Comment 18

6 years ago
(In reply to Victor Porof [:vp] from comment #12)
> That said, there are a few oddities with how the webconsole now behaves on
> OS X. I tested everything on [1]. I'm still playing with it, but here are
> some usability issues I found so far:

Thanks for your testing and comments here. I have not yet addressed these because I'd like us to definite the breadth and scope of further changes.

I feel we could do a lot of work to improve things, but we also need to be able to finish this patch soon - I would like us to target Firefox 21.

> * There webconsole "content" width is larger than the owner window, so a
> scrollbar appears below and the header is offset a few pixels on the right.
> See the "width-issue" screenshot. 

Oh, I had this issue on Linux - I fixed it on my system. I need to try Mac OS as well. Thanks!

>   Also, I found the grippy separating the logging output and the sidebar
> extremely buggy and awkward; most of the times clicking it wouldn't have any
> effect (either that or I couldn't get my clicks pixel-perfect accurate
> enough). I think we need to find a different way of collapsing/showing the
> variables view, maybe something like what the debugger is using? The grippy
> is frustrating, and exactly the reason I avoided it with the debugger.

I added the grippy because it was the easiest/quickest choice. Can we fix this in a separate bug? Or do you believe this is important enough to be fixed here?

If the debugger solution is quick to implement, we can do that here. Please ping me on IRC so we can discuss.

There's also a bug about adding closable/removable tabs to the toolbox sidebars, as an option.


> * When inspecting an object in the sidebar, I immediately got dizzy visually
> parsing the 5 toolbars stacked on top of each other. See the
> "all-the-toolbars!" screenshot. I feel a bit uneasy about this; maybe
> getting rid of showing "Properties" header, and scope separator when not
> necessary would help, but having 3 separators stacked on top of each other
> is still too much. It would also be more efficient to combine the sidebar
> header with the main webconsole header (similar to what the inspector is
> doing).
>   Having two "Filter" filter boxes on top of each other is also a bit spooky
> :) If I'm not familiar with the webconsole, it's not immediately obvious
> that one filters output and one filters variables.

Yes, good point. This is something we need to polish. I'll look into hiding the tab bar. I feel even like this we can safely say it's a huge improvement in UI looks and functionality over the old Property Panel.

Combining the two filter inputs is something that takes a lot more work and I would like us to do it in a separate bug. If you feel that two filter inputs are too much, I can simply take it out. I wouldn't prefer that - I think the benefit of having a way to filter properties/values in vview strongly outweighs the UI awkwardness of having two filter inputs. I leave the decision to you.


> * When clicking on an object to inspect it (from the output), the logs got
> scrolled upwards and the "[object Object]" line got hidden. I understand why
> this happens, but I find it may be a bit annoying.

I'll try to fix this. Thanks for pointing it out.


> * console.dir() output is much too restricted (vertically) imo. I originally
> thought this was a bug and wanted to scroll the main output to see more
> properties. See the "awkward-scrolling" screenshot. I think you should allow
> for at least twice the current height for the inlined variables view (or
> something like 75% of the visible logging area height).

I can look into assigning the vview a dynamic-at-creation time width. For example, I can compute the output height, then assign 75% of that for the vview.


>   Also, adding some margin below the view to suggest that "this is a
> subregion of scrollable content, don't try to scroll the main output" would
> help a lot.

Not sure I understand this. Sounds simple, we should discuss it on IRC.


> * "debugger eval code:1" links shouldn't be present in the webconsole
> output. Clicking them shows an empty view source window.

I think we should show "debugger eval code" but without any link.


> * There is an unexpectedly large amount of flicker when modifying properties
> (through eval, switch, delete etc.). I think it's a good idea to make
> LAZY_EMPTY_DELAY a configurable property on the VariablesView.prototype and
> double it for the webconsole.

Heh, yes.
(Reporter)

Comment 19

6 years ago
(In reply to Mihai Sucan [:msucan] from comment #18)
> 
> I feel we could do a lot of work to improve things, but we also need to be
> able to finish this patch soon - I would like us to target Firefox 21.
> 

Completely agreed. Don't interpret all these comments as blocking for landing, just some usability issues I happened to notice.

> >   Also, I found the grippy separating the logging output and the sidebar
> > extremely buggy and awkward; most of the times clicking it wouldn't have any
> > effect (either that or I couldn't get my clicks pixel-perfect accurate
> > enough). I think we need to find a different way of collapsing/showing the
> > variables view, maybe something like what the debugger is using? The grippy
> > is frustrating, and exactly the reason I avoided it with the debugger.
> 
> I added the grippy because it was the easiest/quickest choice. Can we fix
> this in a separate bug? Or do you believe this is important enough to be
> fixed here?
> 
> If the debugger solution is quick to implement, we can do that here. Please
> ping me on IRC so we can discuss.
> 

It's not a toggle available through an API if that's what you mean, but the code itself is reusable enough (just set a certain margin and you're all set, see [0]). However we can leave this as a followup.

> There's also a bug about adding closable/removable tabs to the toolbox
> sidebars, as an option.

That's most likely the better long term solution, but I worry that having different ways-of-hiding-panes in the debugger and webconsole would feel awkward in the grand scheme of things. The debugger doesn't use a sidepanel, maybe it should?

> > * When inspecting an object in the sidebar, I immediately got dizzy visually
> > parsing the 5 toolbars stacked on top of each other. See the
> > "all-the-toolbars!" screenshot. I feel a bit uneasy about this; maybe
> > getting rid of showing "Properties" header, and scope separator when not
> > necessary would help, but having 3 separators stacked on top of each other
> > is still too much. It would also be more efficient to combine the sidebar
> > header with the main webconsole header (similar to what the inspector is
> > doing).
> >   Having two "Filter" filter boxes on top of each other is also a bit spooky
> > :) If I'm not familiar with the webconsole, it's not immediately obvious
> > that one filters output and one filters variables.
> 
> Yes, good point. This is something we need to polish. I'll look into hiding
> the tab bar. I feel even like this we can safely say it's a huge improvement
> in UI looks and functionality over the old Property Panel.
> 

Indeed. Maybe you could do the following:
1. hide the "Properties" tab bar since it doesn't offer any useful information
2. hide the Scope header because there can only be 1 inspected variable at a time (at this point)
3. make the sidepanel appear on the far right (and not below the webconsole toolbar), just like in the Inspector.

> Combining the two filter inputs is something that takes a lot more work and
> I would like us to do it in a separate bug. If you feel that two filter
> inputs are too much, I can simply take it out. I wouldn't prefer that - I
> think the benefit of having a way to filter properties/values in vview
> strongly outweighs the UI awkwardness of having two filter inputs. I leave
> the decision to you.
> 

No, definitely don't take the filtering out. Also, combining search functionalities into a single filter box is a whole discussion topic on itself, certainly not the scope of this bug.

Maybe just change the placeholder text to say "Filter console output" and "Filter properties" respectively?

> 
> >   Also, adding some margin below the view to suggest that "this is a
> > subregion of scrollable content, don't try to scroll the main output" would
> > help a lot.
> 
> Not sure I understand this.
> 

There is no distance between the inlined variables view iframe and the bottom of the console output container. Therefore, the iframe "touches" the js input box, and this makes it hard to understand that it's a separate scrollable container. There should be some margin at the bottom of the iframe.
(Reporter)

Comment 20

6 years ago
One more thing I found: the variables view is shown on a gray-ish background on OS X (presumably the default OS window background). It should always be white, since none of the colors are system theme dependent and you may end up with weird scenarios like purple text on black background :)
(Reporter)

Comment 21

6 years ago
Comment on attachment 712114 [details] [diff] [review]
address review from victor

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

Thank you! Apart from the UX nits discussed above, I love it.
Attachment #712114 - Flags: review?(vporof) → review+
(Assignee)

Updated

6 years ago
Blocks: 840093

Comment 22

6 years ago
>diff --git a/browser/devtools/framework/Sidebar.jsm b/browser/devtools/framework/Sidebar.jsm
>-    iframe.addEventListener("DOMContentLoaded", onIFrameLoaded, true);
>+    iframe.addEventListener("load", onIFrameLoaded, true);
> [...]
>     if (selected) {
>       // For some reason I don't understand, if we call this.select in this
>       // event loop (after inserting the tab), the tab will never get the
>       // the "selected" attribute set to true.
>       this._panelDoc.defaultView.setTimeout(function() {
>         this.select(id);
>-      }.bind(this), 0);
>+      }.bind(this), 10);

/me shudders a little.
That looks like some magic code.
Not blaming you, but it's sad.

Comment 23

6 years ago
Comment on attachment 712114 [details] [diff] [review]
address review from victor

Do we want to hide the single tab in the sidebar?
Attachment #712114 - Flags: review?(paul) → review+
(Assignee)

Comment 24

6 years ago
(In reply to Paul Rouget [:paul] from comment #23)
> Comment on attachment 712114 [details] [diff] [review]
> address review from victor
> 
> Do we want to hide the single tab in the sidebar?

Yes, too much UI clutter, as pointed out by Victor. Next patch update hides the single tab in the sidebar.
Comment on attachment 712114 [details] [diff] [review]
address review from victor

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

::: browser/devtools/webconsole/HUDService.jsm
@@ +220,5 @@
>     *         A Promise for the initialization.
>     */
>    init: function WC_init()
>    {
> +    Promise._reportErrors = true; // FIXME: debug-only

Don't forget to remove this before committing.

::: browser/devtools/webconsole/webconsole.js
@@ +3293,5 @@
> +  /**
> +   * A noop callback for JavaScript evaluation. This method releases any
> +   * result ObjectActors that come from the server for evaluation requests. This
> +   * is used for editing, renaming and deleting properties in the variables
> +   * view.

I couldn't test this, but from reading it, it sounds like you ignore the return value? I can't see how this could be possible, so I assume the comment needs a clarification to that effect.

::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
@@ +203,5 @@
>  connectionTimeout=Connection timeout. Check the Error Console on both ends for potential error messages. Reopen the Web Console to try again.
> +
> +# LOCALIZATION NOTE (propertiesFilterPlaceholder): This is the text that
> +# appears in the filter text box for the properties view container.
> +propertiesFilterPlaceholder=Filter

Maybe "Filter properties" to differentiate from the other one? Also, the font looks slightly larger than the other filter input box.

::: toolkit/devtools/webconsole/WebConsoleUtils.jsm
@@ +877,5 @@
> +   *         True if the given value is a grip with an actor.
> +   */
> +  isActorGrip: function WCU_isActorGrip(aGrip)
> +  {
> +    return aGrip && typeof(aGrip) == "object" && aGrip.actor;

If you wanted to be nitpicky you'd have tested that the "actor" property was set on the object and not somewhere in its prototype chain.
Attachment #712114 - Flags: review?(past) → review+
(Assignee)

Comment 26

6 years ago
Created attachment 713400 [details] [diff] [review]
address more feedback from victor

Changes:

- the sidebar tab bar is now hidden.
- moved the sidebar to the far right, as suggested by Victor.
  - took the grippy is out, because it is too wide and it looks weird now.
- "debugger eval code" location link for console API calls is no longer clickable.
- console.dir() height is now 60% of output height, at creation.
- add margin-bottom for inlined vviews.
- changed strings: "Filter output" and "Filter properties".

I kept the scope header, otherwise we have a section without any heading, which is weird. Height seems fine for me, even with the scope header.

Remaining work: see comment #9. I still need to test the patch on a Mac. Can someone please test this on a Windows machine?

Thank you all for the r+s.
Attachment #712114 - Attachment is obsolete: true
(Reporter)

Comment 27

6 years ago
(In reply to Mihai Sucan [:msucan] from comment #26)


Oooh, can't wait to try this!
(Assignee)

Comment 28

6 years ago
(In reply to Victor Porof [:vp] from comment #12)
> * There webconsole "content" width is larger than the owner window, so a
> scrollbar appears below and the header is offset a few pixels on the right.
> See the "width-issue" screenshot. 
>   Also, I found the grippy separating the logging output and the sidebar
> extremely buggy and awkward; most of the times clicking it wouldn't have any
> effect (either that or I couldn't get my clicks pixel-perfect accurate
> enough). I think we need to find a different way of collapsing/showing the
> variables view, maybe something like what the debugger is using? The grippy
> is frustrating, and exactly the reason I avoided it with the debugger.

Please retest. I changed how the sidebar is placed in the latest patch.

> * When inspecting an object in the sidebar, I immediately got dizzy visually
> parsing the 5 toolbars stacked on top of each other. See the
> "all-the-toolbars!" screenshot. I feel a bit uneasy about this; maybe
> getting rid of showing "Properties" header, and scope separator when not
> necessary would help, but having 3 separators stacked on top of each other
> is still too much. It would also be more efficient to combine the sidebar
> header with the main webconsole header (similar to what the inspector is
> doing).

Should be better now. Let me know if it's fine.


> * When clicking on an object to inspect it (from the output), the logs got
> scrolled upwards and the "[object Object]" line got hidden. I understand why
> this happens, but I find it may be a bit annoying.

I tried an approach to fix this and it didn't work. This needs a follow-up bug report. Added to my TODO to open it.

> * console.dir() output is much too restricted (vertically) imo. I originally
> thought this was a bug and wanted to scroll the main output to see more
> properties. See the "awkward-scrolling" screenshot. I think you should allow
> for at least twice the current height for the inlined variables view (or
> something like 75% of the visible logging area height).
>   Also, adding some margin below the view to suggest that "this is a
> subregion of scrollable content, don't try to scroll the main output" would
> help a lot.

Fixed.

> * "debugger eval code:1" links shouldn't be present in the webconsole
> output. Clicking them shows an empty view source window.

Fixed.

> * There is an unexpectedly large amount of flicker when modifying properties
> (through eval, switch, delete etc.). I think it's a good idea to make
> LAZY_EMPTY_DELAY a configurable property on the VariablesView.prototype and
> double it for the webconsole.

This is more of a variables view bug. Please open a follow up bug and when you have time, a patch is welcome. :)

Thanks for your comments.
(Assignee)

Comment 29

6 years ago
(In reply to Panos Astithas [:past] from comment #25)
> Comment on attachment 712114 [details] [diff] [review]
> address review from victor
> 
> Review of attachment 712114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/webconsole/HUDService.jsm
> @@ +220,5 @@
> >     *         A Promise for the initialization.
> >     */
> >    init: function WC_init()
> >    {
> > +    Promise._reportErrors = true; // FIXME: debug-only
> 
> Don't forget to remove this before committing.

Thanks for the reminder!


> ::: browser/devtools/webconsole/webconsole.js
> @@ +3293,5 @@
> > +  /**
> > +   * A noop callback for JavaScript evaluation. This method releases any
> > +   * result ObjectActors that come from the server for evaluation requests. This
> > +   * is used for editing, renaming and deleting properties in the variables
> > +   * view.
> 
> I couldn't test this, but from reading it, it sounds like you ignore the
> return value? I can't see how this could be possible, so I assume the
> comment needs a clarification to that effect.

Not sure I understand your request. The comment says I ignore the return value and release the ObjectActor. Is it confusing? If yes, I can try to rephrase it in a less confusing manner.

> ::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
> @@ +203,5 @@
> >  connectionTimeout=Connection timeout. Check the Error Console on both ends for potential error messages. Reopen the Web Console to try again.
> > +
> > +# LOCALIZATION NOTE (propertiesFilterPlaceholder): This is the text that
> > +# appears in the filter text box for the properties view container.
> > +propertiesFilterPlaceholder=Filter
> 
> Maybe "Filter properties" to differentiate from the other one? Also, the
> font looks slightly larger than the other filter input box.

Yep, fixed in the updated patch.

Thanks for the review!
(Reporter)

Comment 30

6 years ago
(In reply to Mihai Sucan [:msucan] from comment #28)

I'm testing the patch. Things are feeling much much better than before! Thanks!
I'll be back with more comments.

> > * When clicking on an object to inspect it (from the output), the logs got
> > scrolled upwards and the "[object Object]" line got hidden. I understand why
> > this happens, but I find it may be a bit annoying.
> 
> I tried an approach to fix this and it didn't work. This needs a follow-up
> bug report. Added to my TODO to open it.
> 

I'm probably stating the obvious (or suggesting a dumb thing), but won't a setTimeout(scroll to bottom, very soon) work?

> > * "debugger eval code:1" links shouldn't be present in the webconsole
> > output. Clicking them shows an empty view source window.
> 
> Fixed.
> 

Somehow, I still think we shouldn't show "debugger eval code:1" at all, not just make it unclickable. Panos, what do you say? (I don't feel strongly about this).

> > * There is an unexpectedly large amount of flicker when modifying properties
> > (through eval, switch, delete etc.). I think it's a good idea to make
> > LAZY_EMPTY_DELAY a configurable property on the VariablesView.prototype and
> > double it for the webconsole.
> 
> This is more of a variables view bug. Please open a follow up bug and when
> you have time, a patch is welcome. :)
> 

Filed bug 841008. I'll leave it up to you to decide what timeout to use in the webconsole.
(Reporter)

Updated

6 years ago
Blocks: 841008
(Reporter)

Comment 31

6 years ago
(In reply to Mihai Sucan [:msucan] from comment #29)

> > Also, the font looks slightly larger than the other filter input box.
> 
> Yep, fixed in the updated patch.
> 

The text-is-too-large problem still happens with the latest patch on OS X, but bug 802546 has a very simple fix in common.css.
(In reply to Mihai Sucan [:msucan] from comment #29)
> (In reply to Panos Astithas [:past] from comment #25)
> > I couldn't test this, but from reading it, it sounds like you ignore the
> > return value? I can't see how this could be possible, so I assume the
> > comment needs a clarification to that effect.
> 
> Not sure I understand your request. The comment says I ignore the return
> value and release the ObjectActor. Is it confusing? If yes, I can try to
> rephrase it in a less confusing manner.

It's OK, now that I managed to build it successfully, I can see that another request is made afterwards to get the updated object. Sorry for the noise.

> > ::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
> > @@ +203,5 @@
> > >  connectionTimeout=Connection timeout. Check the Error Console on both ends for potential error messages. Reopen the Web Console to try again.
> > > +
> > > +# LOCALIZATION NOTE (propertiesFilterPlaceholder): This is the text that
> > > +# appears in the filter text box for the properties view container.
> > > +propertiesFilterPlaceholder=Filter
> > 
> > Maybe "Filter properties" to differentiate from the other one? Also, the
> > font looks slightly larger than the other filter input box.
> 
> Yep, fixed in the updated patch.

The larger font remains, and my guess is that the cause is the slightly larger input box (the old one is constrained by the toolbar height, apparently). There are a few other visual nits I've observed, like the width of the splitter which is larger than the ones we use in the debugger, or the 1 px border of the scope header that leaks into the console output area.

I think we should fix all of these in followups.


(In reply to Victor Porof [:vp] from comment #30)
> Somehow, I still think we shouldn't show "debugger eval code:1" at all, not
> just make it unclickable. Panos, what do you say? (I don't feel strongly
> about this).

I don't think it is of any use, especially if it's not clickable. The same holds for the Scratchpad pseudo-URL. I think we should keep a map of all those dummy script locations and hide them from the output.
(Reporter)

Comment 33

6 years ago
Created attachment 713595 [details]
os x, latest patch, just minor ui oddities

(In reply to Panos Astithas [:past] from comment #32)
> 
> The larger font remains, and my guess is that the cause is the slightly
> larger input box (the old one is constrained by the toolbar height,
> apparently).

No, it's because of type="search". Don't ask, I know it's delightfully unexpected.
A simple font-size: inherit does the trick, I'm taking care of it in a different bug.

A few more oddities I noticed (most of these can be fixed in followups, but please file bugs):
* the variables view "leaks" UI elements inside the webconsole output window (see [object Proxy] header in the screenshot)
* the expander on a scope header (arrow/twisty) should be hidden, since it can't be collapsed
* there's a gray border around the variables view container (especially noticeable on the top
* the side margin is 1 pixel too thick (probably related to the gray border)
* there's one weird black pixel on the far right of the toolbar (immediately above the clear button)
* I noticed sometimes the filtering doesn't work on the first try; I don't understand why this is happening in the webconsole and not in the debugger, we need to investigate!
* the variables view from a console.dir output should probably have a 1px light gray border; the sizing and margins are perfect, thanks for the update!
* the debugger uses accel+shift+v to focus the variables view; maybe the webconsole should do the same?

I attached a big resolution screenshot so the "imperfections" are more obvious.
(Assignee)

Comment 34

6 years ago
Created attachment 715676 [details] [diff] [review]
rebased patch

Will address review comments ASAP.
Attachment #713400 - Attachment is obsolete: true
(Assignee)

Comment 35

6 years ago
(In reply to Victor Porof [:vp] from comment #30)
> (In reply to Mihai Sucan [:msucan] from comment #28)
> 
> I'm testing the patch. Things are feeling much much better than before!

Great!

> > > * When clicking on an object to inspect it (from the output), the logs got
> > > scrolled upwards and the "[object Object]" line got hidden. I understand why
> > > this happens, but I find it may be a bit annoying.
> > 
> > I tried an approach to fix this and it didn't work. This needs a follow-up
> > bug report. Added to my TODO to open it.
> > 
> 
> I'm probably stating the obvious (or suggesting a dumb thing), but won't a
> setTimeout(scroll to bottom, very soon) work?

No. scrollToBottom() would be the wrong fix when you inspect an object from backscroll. The fix I tried was to maintain the relative scroll by noticing how much height was added by the sidebar, and fix the scroll position. I tried that, but it was far from accurate - one needs to keep track of where the message you click on is relative to the viewport. This is when I decided it's too much work for this bug.


> > > * There is an unexpectedly large amount of flicker when modifying properties
> > > (through eval, switch, delete etc.). I think it's a good idea to make
> > > LAZY_EMPTY_DELAY a configurable property on the VariablesView.prototype and
> > > double it for the webconsole.
> > 
> > This is more of a variables view bug. Please open a follow up bug and when
> > you have time, a patch is welcome. :)
> > 
> 
> Filed bug 841008. I'll leave it up to you to decide what timeout to use in
> the webconsole.

Not greater than 350ms.


(In reply to Panos Astithas [:past] from comment #32)
> > Yep, fixed in the updated patch.
> 
> The larger font remains, and my guess is that the cause is the slightly
> larger input box (the old one is constrained by the toolbar height,
> apparently). There are a few other visual nits I've observed, like the width
> of the splitter which is larger than the ones we use in the debugger, or the
> 1 px border of the scope header that leaks into the console output area.
> 
> I think we should fix all of these in followups.

Agreed. I will open a follow-up for visual nits.


> (In reply to Victor Porof [:vp] from comment #30)
> > Somehow, I still think we shouldn't show "debugger eval code:1" at all, not
> > just make it unclickable. Panos, what do you say? (I don't feel strongly
> > about this).
> 
> I don't think it is of any use, especially if it's not clickable. The same
> holds for the Scratchpad pseudo-URL. I think we should keep a map of all
> those dummy script locations and hide them from the output.

I can agree with the removal of "debugger eval code" but the scratchpad pseudo-url is clickable, it has a point (or am I mistaken?). It will be removed once Scratchpad becomes remotable (IIANM).

Thank you both for the reviews!
(In reply to Mihai Sucan [:msucan] from comment #35)
> (In reply to Victor Porof [:vp] from comment #30)
> > (In reply to Victor Porof [:vp] from comment #30)
> > > Somehow, I still think we shouldn't show "debugger eval code:1" at all, not
> > > just make it unclickable. Panos, what do you say? (I don't feel strongly
> > > about this).
> > 
> > I don't think it is of any use, especially if it's not clickable. The same
> > holds for the Scratchpad pseudo-URL. I think we should keep a map of all
> > those dummy script locations and hide them from the output.
> 
> I can agree with the removal of "debugger eval code" but the scratchpad
> pseudo-url is clickable, it has a point (or am I mistaken?). It will be
> removed once Scratchpad becomes remotable (IIANM).

You are right, we use it to focus the right scratchpad if more than one are open, but it's not that useful (c.f. bug 813087). But yes, let's leave it out of this.
(Assignee)

Updated

6 years ago
Blocks: 842995
(Assignee)

Updated

6 years ago
Blocks: 842996
(Assignee)

Updated

6 years ago
Blocks: 843004
(Assignee)

Updated

6 years ago
Blocks: 843019
(Assignee)

Updated

6 years ago
Blocks: 843022
(Assignee)

Updated

6 years ago
Blocks: 843091
(Assignee)

Updated

6 years ago
Blocks: 843094
(Assignee)

Updated

6 years ago
Blocks: 843260
(Assignee)

Updated

6 years ago
Blocks: 843280
(Assignee)

Updated

6 years ago
Blocks: 843287
(Assignee)

Comment 37

6 years ago
Created attachment 717295 [details] [diff] [review]
test fixes

All browser tests pass now. Changes:

- "debugger eval code" no longer shows, as requested.
- the two tests that failed because querySelectorAll, body and other properties were not found are now fixed. As discussed, we now consider that they should always show on __proto__ - thanks to the fixes in bug 830818.
- there was a test that failed intermittently on my machine - due to event timings - fixed it.
- added helpers in head.js for easier testing of nested properties (like __proto__.body).

First try push: https://tbpl.mozilla.org/?tree=Try&rev=8013503956cf

I expect failures in the toolkit tests for the Web Console.


Remaining work:

- fix the toolkit tests (waiting on feedback from jimb/bz).
- write new tests (been too busy with other work and I didn't yet do this yet).
Attachment #715676 - Attachment is obsolete: true
(Assignee)

Comment 38

6 years ago
Created attachment 719067 [details] [diff] [review]
patch 7: rebase
Attachment #717295 - Attachment is obsolete: true
(Assignee)

Comment 39

6 years ago
Created attachment 721426 [details] [diff] [review]
patch 8: minor fixes and new tests

Asking for a quick review so you can check the changes that I made since your last review.

Please let me know if we need more tests.

All tests pass on Linux and Mac OS (locally). Try push:

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

I also included a fix for the console.error message I saw with the patch for bug 820524. executeSoon() is undefined after the toolbox is destroyed, in some tests. This happens because finish() tries to clean up, but the test already started the toolbox close. By the time the closeConsole() callback for the promise executes, the test harness pulls executeSoon() out, so it's undefined.

Victor: please check the tests and the latest changes as well. I added a new property.focus() method to allow tests to focus properties, so I can send them keyboard events - for changing values, for delete and for rename.

Thank you both!
Attachment #719067 - Attachment is obsolete: true
Attachment #721426 - Flags: review?(vporof)
Attachment #721426 - Flags: review?(past)
(Reporter)

Comment 40

6 years ago
Comment on attachment 721426 [details] [diff] [review]
patch 8: minor fixes and new tests

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

Still loving it. There's a bit of carnage in the try run, r+ with that sorted out.

::: browser/devtools/framework/Sidebar.jsm
@@ +84,5 @@
>        // event loop (after inserting the tab), the tab will never get the
>        // the "selected" attribute set to true.
>        this._panelDoc.defaultView.setTimeout(function() {
>          this.select(id);
> +      }.bind(this), 10);

I know this has been here for a while, but I'm getting more and more nervous each time I see it.

::: browser/devtools/shared/VariablesView.jsm
@@ +1411,5 @@
>    /**
> +   * Focus this scope.
> +   */
> +  focus: function S_focus() {
> +    this._variablesView._focusItem(this);

<3

::: browser/devtools/shared/VariablesView.xul
@@ +10,5 @@
> +  <!ENTITY % viewDTD SYSTEM "chrome://browser/locale/devtools/VariablesView.dtd">
> +  %viewDTD;
> +]>
> +<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +        title="&PropertiesViewWindowTitle;">

Nit: do we still need this title, since the tab isn't visible anymore?
Attachment #721426 - Flags: review?(vporof) → review+
Comment on attachment 721426 [details] [diff] [review]
patch 8: minor fixes and new tests

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

Looks good. As discussed earlier, if reverting the sidebar timeout to 0 fixes the failing test, then you probably just need to add a similar timeout to it.

::: browser/themes/linux/devtools/webconsole.css
@@ +251,5 @@
> +}
> +
> +#webconsole-sidebar > tabs {
> +  height: 0;
> +  overflow: hidden;

Nit: shouldn't this be in content CSS?
Attachment #721426 - Flags: review?(past) → review+
(Assignee)

Comment 42

6 years ago
(In reply to Victor Porof [:vp] from comment #40)
> Comment on attachment 721426 [details] [diff] [review]
> patch 8: minor fixes and new tests
> 
> Review of attachment 721426 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Still loving it. There's a bit of carnage in the try run, r+ with that
> sorted out.

Indeed. I am working out through the issues.

> ::: browser/devtools/shared/VariablesView.xul
> @@ +10,5 @@
> > +  <!ENTITY % viewDTD SYSTEM "chrome://browser/locale/devtools/VariablesView.dtd">
> > +  %viewDTD;
> > +]>
> > +<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> > +        title="&PropertiesViewWindowTitle;">
> 
> Nit: do we still need this title, since the tab isn't visible anymore?

Yeah, it's still a document that needs a title. Later on we'll show the tab bar - eg. when a network request info tab is also open.






(In reply to Panos Astithas [:past] from comment #41)
> Comment on attachment 721426 [details] [diff] [review]
> patch 8: minor fixes and new tests
> 
> Review of attachment 721426 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. As discussed earlier, if reverting the sidebar timeout to 0
> fixes the failing test, then you probably just need to add a similar timeout
> to it.
> 
> ::: browser/themes/linux/devtools/webconsole.css
> @@ +251,5 @@
> > +}
> > +
> > +#webconsole-sidebar > tabs {
> > +  height: 0;
> > +  overflow: hidden;
> 
> Nit: shouldn't this be in content CSS?

Probably yes, but the web console doesn't yet have a content CSS.


Thanks for the review.
(Reporter)

Updated

6 years ago
Depends on: 834273
(Assignee)

Updated

6 years ago
Blocks: 855058
(Assignee)

Comment 43

6 years ago
Created attachment 731505 [details] [diff] [review]
patch 9: rebase and fixes for oranges

Rebased the patch and included all the fixes needed for the try server oranges I saw. This is ready to land - all tests pass, no oranges on try. https://tbpl.mozilla.org/?tree=Try&rev=f05d8fb72edb

Before we can land this, we still need bug 837723 which is not yet ready.
Attachment #721426 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 587757
(Assignee)

Comment 44

5 years ago
Landed:
https://hg.mozilla.org/integration/fx-team/rev/dfc808a01756
Whiteboard: [sharing-code] → [sharing-code][fixed-in-fx-team]
(Assignee)

Updated

5 years ago
Blocks: 859858
https://hg.mozilla.org/mozilla-central/rev/dfc808a01756
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [sharing-code][fixed-in-fx-team] → [sharing-code]
Target Milestone: --- → Firefox 23
(Assignee)

Comment 46

5 years ago
We should probably mention in MDN that the new variables view allows property renaming, value edits and deletion. When you edit values you can also use the jsterm helpers [1] and you can do all of this while debugging a content page. We should make sure that the old object inspector is no longer mentioned/show-cased by the docs we have.

Thank you!

[1] https://developer.mozilla.org/en-US/docs/Tools/Web_Console/Helpers
Keywords: dev-doc-needed
No longer depends on: 820878

Updated

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