843004 - Improve output for object actor grips

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Mihai Sucan [:msucan]]

With bug 783499 and bug 808370 we are switching the web console to the debugger API and we are now using the Variables View way of displaying object actors in the console output.

However, the web console output had improved handling for different kinds of objects. For example instead of [object Function] we displayed either |function name(arguments, list)| or function.toSource() (the source of the function).

I discussed this change with Rob and we've agreed that it's worthwhile to improve output of various kinds in the future.

We should probably have two ways to display each type of object (where applicable):

- for functions we can output its source (toSource()) in the console output. In vview we can display |function name(args)| only. This is what we did in the web console and its old property panel.

- for Date: we can show the ISO date.

- for RegExp: we can show the regex toString().

- for DOM elements we could show <tag id=elemid> or <tag class="class names"> in the web console output. In vview we can simplify: tag#id or tag.class.names.

- for generic JS objects we can show Class instead of [object Class] in vview (simplification to make it easier to see which kind of object you have).

We should also keep in mind extensibility - addon authors will want to be able to customize how their js libs are displayed. This is most-likely something for a follow-up bug.

(filing this bug in the debugger component because with bug 808370 the web console uses the vview methods for displaying objects. Marking this bug as a blocker for bug 778766 because we intend to improve how objects are displayed with the console output rewrite.)

Comment 1

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

Bug 753332 is related.

Comment 2

[Mihai Sucan [:msucan]]

As part of the work for the web console output rewrite I am taking this bug. This is related to a regression in output for objects since bug 783499 - we had better output for objects in the console. Also, many users are asking for improved output for objects (high priority bug for the console).

My current plan is to go through related bugs (eg. bug 753332) and start with a simpler patch that we can expand on, in later patches in other bugs. Through this work I also plan to fix bug 753332 -- since the console and the object inspector share the same code that displays Object Actors.

Comment 3

[heavy]

Hopefully this can include (or lead to) support for better output for jQuery objects too.

Comment 5

[Mihai Sucan [:msucan]]

Attachment: part 1 - console output changes

This patch replaces the old code for rendering some of the messages in the webconsole output with the new ConsoleOutput APIs. The message types: js input and output, console API calls for log, info, warn, error and debug. The rest of the messages are left untouched.

At this point migration of the rest of the messages rendering to the new APIs should proceed faster.

Part 2 patch will include toolkit changes to improve the representation of various JS objects in ObjectActor grips.

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

Comment 6

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

Comment on attachment 8336384 [details] [diff] [review]
part 1 - console output changes

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

nice moves from webconsole.js.

Lots of change for no visible output improvements!

::: browser/devtools/webconsole/console-output.js
@@ +682,5 @@
> +  /**
> +   * Message timestamp.
> +   *
> +   * @type number
> +   *       Milliseconds elapsed since 1 January 1970 00:00:00 UTC.

should file a bug for the Y10K issue.

@@ +711,5 @@
> +  },
> +
> +  init: function()
> +  {
> +    Messages.BaseMessage.prototype.init.apply(this, arguments);

I wish we had a super keyword that worked. I guess when we get ES6 classes we can rewrite all this. Again!

@@ +1358,5 @@
>  gSequenceId.n = 0;
>  
>  exports.ConsoleOutput = ConsoleOutput;
>  exports.Messages = Messages;
> +exports.Widgets = Widgets;

nice changes to this file. Every time I see Heritage.extend I wish we had ES6 classes. Otherwise, this is a nice object oriented message structure in JS.

::: browser/devtools/webconsole/webconsole.js
@@ +2286,5 @@
>      }
>  
> +    if (methodOrNode == this.output._flushMessageQueue &&
> +        args[0]._objectActors) {
> +      for (let arg of args) {

argh.

@@ +2295,5 @@
> +          this._releaseObject(actor);
> +        }
> +        arg._objectActors.clear();
> +      }
> +    }

not a super-readable cleanup routine.

Comment 7

[David Bruant]

(In reply to Rob Campbell [:rc] (:robcee) from comment #6)
> @@ +711,5 @@
> > +  },
> > +
> > +  init: function()
> > +  {
> > +    Messages.BaseMessage.prototype.init.apply(this, arguments);
> 
> I wish we had a super keyword that worked. I guess when we get ES6 classes
> we can rewrite all this. Again!
Rumors suggest that super will be available outside classes:
https://twitter.com/awbjs/status/403403810015420416
We'll see what that's all about soon hopefully.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #6)
> Comment on attachment 8336384 [details] [diff] [review]
> part 1 - console output changes
> 
> Review of attachment 8336384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> nice moves from webconsole.js.
> 
> Lots of change for no visible output improvements!

This is fixing one of the long-standing webconsole issues: provide a sane API for adding new kinds of messages. This is in line with the goals for bug 778766. Some day I will write an MDN page about customizing the console output.


> ::: browser/devtools/webconsole/console-output.js
> @@ +682,5 @@
> > +  /**
> > +   * Message timestamp.
> > +   *
> > +   * @type number
> > +   *       Milliseconds elapsed since 1 January 1970 00:00:00 UTC.
> 
> should file a bug for the Y10K issue.

of course :)


> @@ +711,5 @@
> > +  },
> > +
> > +  init: function()
> > +  {
> > +    Messages.BaseMessage.prototype.init.apply(this, arguments);
> 
> I wish we had a super keyword that worked. I guess when we get ES6 classes
> we can rewrite all this. Again!
> 
> @@ +1358,5 @@
> >  gSequenceId.n = 0;
> >  
> >  exports.ConsoleOutput = ConsoleOutput;
> >  exports.Messages = Messages;
> > +exports.Widgets = Widgets;
> 
> nice changes to this file. Every time I see Heritage.extend I wish we had
> ES6 classes. Otherwise, this is a nice object oriented message structure in
> JS.

Yes, I also cant wait for the ES6 stuff.


> ::: browser/devtools/webconsole/webconsole.js
> @@ +2286,5 @@
> >      }
> >  
> > +    if (methodOrNode == this.output._flushMessageQueue &&
> > +        args[0]._objectActors) {
> > +      for (let arg of args) {
> 
> argh.
> 
> @@ +2295,5 @@
> > +          this._releaseObject(actor);
> > +        }
> > +        arg._objectActors.clear();
> > +      }
> > +    }
> 
> not a super-readable cleanup routine.

True. These cleanup routines will be removed in later patches when we drop the last messages that use createMessageNode().


Thanks for the quick review!

Comment 9

[Mihai Sucan [:msucan]]

Attachment: part 1 - console output changes v0.2

Landed: https://hg.mozilla.org/integration/fx-team/rev/80d7b20c8350

Comment 10

[Mihai Sucan [:msucan]]

Initial patch had try failures because I switched to Promise.jsm - for me it made work easier by reporting exceptions whenever something went wrong. Latest patch removes those changes. Brandon has work undergoing for switching devtools to Promise.jsm and fixing all the failures.

Comment 11

[Brandon Benvie [:benvie]]

Indeed, bug 943681.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80d7b20c8350

Comment 13

[Mihai Sucan [:msucan]]

Attachment: part 2 - pretty output for objects (text only) - wip1

This is a *rough* draft/prototype. Went through the object kinds we want to have pretty output for. Compared firebug and chrome's web inspector. I picked things I like. For example, I found chrome too rigid, you cannot inspect the actual JS objects for some types (DOM text nodes, anchors, <link>, etc), it outputs too many elements at once from arrays. I prefer firebug's output a lot more.

This patch adds text-only pretty output to the web console and variables view. Object kinds: some DOM lists, nodes and elements, some CSS OM, plain JS objects and arrays.

I only changed ObjectActor.grip() and VariablesView.getString(). I only focused on how the output looks and how the protocol messages look. Feedback on these is requested from anyone interested.

Next phase: I need to put the new code from grip() in their own methods and objects, to make this reusable and easy to extend. I will do something similar for getString().

Obligatory screenshot: http://i.imgur.com/KBpcQhi.png

Comment 14

[Mihai Sucan [:msucan]]

Comment on attachment 8340102 [details] [diff] [review]
part 2 - pretty output for objects (text only) - wip1

(forgot to ask for feedback)

Please see comment 13. Feedback wanted for the getString() text result, eg. if you'd like different output for certain objects, and for the ObjectActor grip JSON changes (the protocol changes).

Thank you!

Comment 15

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #13)
> Obligatory screenshot: http://i.imgur.com/KBpcQhi.png

\o/

So effing awesome..

Comment 16

[Mihai Sucan [:msucan]]

Attachment: part 2 - pretty output for objects (text only) - wip2

Changes:

- added pretty output for Set and Map.

- cleanups, moved the code out of ObjectActor.grip() into functions for every kind of object we want to have pretty output for. This is looking better now.

Jim: I tested if the raw object returned by unsafeDereference() is an xray wrapper and the result is confusing. See the code in this patch for ObjectActor.grip(). isXrayWrapper(raw) returns false. Also, I can access content properties from the object. I tried to specifically wrap the raw object in an XPCNativeWrapper() and I still get false from isXrayWrapper(). Am I missing something? Thank you!

Comment 17

[Brandon Benvie [:benvie]]

Comment on attachment 8340553 [details] [diff] [review]
part 2 - pretty output for objects (text only) - wip2

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

This is great! I'm still interested to see the performance impact, but this is definitely where we want to go from a feature perspective.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +3187,5 @@
>          return aGrip.type;
>        case "longString":
>          return "\"" + aGrip.initial + "\"";
>        default:
> +        if (aGrip.type != "object") {

When would this happen? This seems like it should only happen when something is broken. If so, we should error report here.

@@ +3228,5 @@
> +            return prefix + "[" + result.join(", ") + "]";
> +          }
> +
> +          if (aGrip.class == "Array") {
> +            return "Array(" + preview.length + ")";

This seems unnecessary. You could just display Arrays like any other ArrayLike. The class name gives enough differentiation.

@@ +3312,5 @@
> +          return result + ">";
> +        }
> +
> +        if (preview.kind == "DOMNode" &&
> +            preview.nodeType == Ci.nsIDOMNode.ATTRIBUTE_NODE) {

Should do `if (preview.kind == "DOMNode") {` and then put all the DOMNode subclass checks under that.

@@ +3359,5 @@
> +            }
> +            props.push(key + ": " + valueString);
> +          }
> +          if (preview.names > props.length) {
> +            props.push("more\u2026");

I think this should display how many more properties.

::: toolkit/devtools/server/actors/script.js
@@ +2772,5 @@
>    SyntaxError: errorStringify,
>    TypeError: errorStringify,
>    URIError: errorStringify,
>    Boolean: createBuiltinStringifier(Boolean),
> +  Date: createBuiltinStringifier(Date),

Presumably this stringifier stuff can be removed in a later patch once the previewer stuff lands.

@@ +2877,5 @@
>      };
>  
> +    /// XXX: Cu.isDeadWrapper() doesn't work with Debugger.Objects, but we have
> +    // code in this file that relies on it working...
> +    if (this.obj.class != "DeadObject") {

This should probably be `if (doPreview && this.obj.class != "DeadObject") {`.

@@ +3290,5 @@
>    "scope": ObjectActor.prototype.onScope,
>  };
>  
> +// Number of items to preview in an array.
> +const OBJECT_PREVIEW_MAX_ARRAY_ITEMS = 10;

Should rename this to just OBJECT_PREVIEW_MAX_ITEMS, or create and use two constants (properties/array items). Right now you're using this for both key:value pairs and indices.

@@ +3300,5 @@
> + * arguments: the ObjectActor instance and the grip object being prepared for
> + * the client. Functions must return false if they cannot provide preview
> + * information for the debugger object, or true otherwise.
> + */
> +// XXX: should we just flatten this structure into an array?

I don't think it should be a flat array. That would mean every object has to go through the entire set of previewers rather than skipping most of them.

It might be worth turning this into a full blown API to manage the list, something like:

DebuggerServer.ObjectActorPreviewers = {
  addPreviewer: function(className, previewer) { /***/ },
  findPreviewer: function(actor) { /***/ },
  createPreview: function(actor) { /***/ },
  /* etc. */
};

@@ +3331,5 @@
> +
> +    return true;
> +  }], // Function
> +
> +  // TODO: try to avoid the use of unsafeDereference

I think the use of unsafeDeference is fine. However, we need to use known-safe builtin methods. For example, `RegExp.prototype.toString.call(raw)` should be guaranteed to not run debuggee code.

@@ +3385,5 @@
> +      return true;
> +    }
> +
> +    let entries = aGrip.preview.entries = [];
> +    for (let [key, value] of raw) {

A safe way to iterate over Maps and Sets would be something like: `Map.prototype.forEach.call(raw, (value, key) => { /***/ })`. Using for-of has the potential to run debuggee code in a couple places.

@@ +3426,5 @@
> +      }
> +    }
> +
> +    return true;
> +  }], // DOMStringMap

Need a previewer for DOMException. I'm not sure if it should be normalized to the same fields are preview.kind = "Error" or its own special kind, since it has some different properties.

@@ +3454,5 @@
> +    }
> +  },
> +
> +  function CSSMediaRule({obj, threadActor}, aGrip) {
> +    let raw = obj.unsafeDereference();

obj.unsafeDereference() looks like it's used in many/most of the previewers. Perhaps it should just be done upfront once and passed as an argument to each previewer?

@@ +3484,5 @@
> +      return false;
> +    }
> +    aGrip.preview = {
> +      kind: "CSSImportRule",
> +      href: threadActor.createValueGrip(raw.href),

I don't know if we can do it, but it'd get great if we could get direct access to the DOM getter (in this case CSSImportRule#href) and call it on `raw` (similar to the above RegExp#toString example). If we can do this then we can avoid any accidental debuggee code running. The other option is to use `getProperty` but that's going to be slower and can be foiled if someone overwrites the builtin getters or something.

@@ +3556,5 @@
> +      return true;
> +    }
> +
> +    let items = aGrip.preview.items = [];
> +    for (let i = 0; i < raw.length && i < OBJECT_PREVIEW_MAX_ARRAY_ITEMS; i++) {

This won't handle holes in arrays, so the client side won't be able to differentiate between `[,,]` and `[undefined, undefined]`.

@@ +3578,5 @@
> +      kind: "MapLike",
> +      entries: [],
> +      size: raw.length,
> +    };
> +    let entries = aGrip.preview.entries;

Nit: `let {entries} = aGrip.preview`

@@ +3647,5 @@
> +    };
> +
> +    // XXX not sure if it makes sense to use the number of own properties. we
> +    // need it to determine if an object preview should show an ellipsis or
> +    // not. thoughts?

I think you should keep this.. It might be worth showing how many more properties there are for generic objects like you do with arrays. `Window { location: whatever, ... 500 more properties }`. Or, like for the VariablesView (or some other consumer of this API), you might just want to put `Object(50)` as the preview.

Comment 18

[Brandon Benvie [:benvie]]

Comment on attachment 8340553 [details] [diff] [review]
part 2 - pretty output for objects (text only) - wip2

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

::: toolkit/devtools/server/actors/script.js
@@ +3639,5 @@
> +    if (aGrip.preview || aGrip.displayString || threadActor._gripDepth > 1) {
> +      return false;
> +    }
> +
> +    let i = 0, names = obj.getOwnPropertyNames();

We might want to filter this to only show enumerable properties. This is what Chrome devtools does and I've found it useful. Thoughts?

Comment 19

[Mihai Sucan [:msucan]]

Attachment: part 2 - pretty output for objects (text only) - wip3

Updates:

- more cleanups and fixes.
- it seems getProperty() from actors/script.js had two bugs. Both involved how the safe getter was executed. See patch.
- fixed the isDeadWrapper() call with a Debugger.Object in script.js.
- less use of unsafeDereference() and as already suggested by Brandon I am using method from native prototypes where possible.
- added preview for the safe getters. In previous patches window.getSelection() displayed [object Selection] because there were no ownProperties.

Still need to do more fixes and less calls to unsafeDereference(). I didn't yet do much for VariablesView.

Brandon, thanks for your feedback!

Comment 20

[Mihai Sucan [:msucan]]

(In reply to Brandon Benvie [:benvie] from comment #17)
> Comment on attachment 8340553 [details] [diff] [review]
> part 2 - pretty output for objects (text only) - wip2
> 
> Review of attachment 8340553 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is great! I'm still interested to see the performance impact, but this
> is definitely where we want to go from a feature perspective.

Indeed.

> ::: browser/devtools/shared/widgets/VariablesView.jsm
> @@ +3187,5 @@
> >          return aGrip.type;
> >        case "longString":
> >          return "\"" + aGrip.initial + "\"";
> >        default:
> > +        if (aGrip.type != "object") {
> 
> When would this happen? This seems like it should only happen when something
> is broken. If so, we should error report here.

For kinds we don't know yet about. Future. This is basically filtering out anything non-ObjectActor.

> @@ +3228,5 @@
> > +            return prefix + "[" + result.join(", ") + "]";
> > +          }
> > +
> > +          if (aGrip.class == "Array") {
> > +            return "Array(" + preview.length + ")";
> 
> This seems unnecessary. You could just display Arrays like any other
> ArrayLike. The class name gives enough differentiation.

Good point.

> @@ +3312,5 @@
> > +          return result + ">";
> > +        }
> > +
> > +        if (preview.kind == "DOMNode" &&
> > +            preview.nodeType == Ci.nsIDOMNode.ATTRIBUTE_NODE) {
> 
> Should do `if (preview.kind == "DOMNode") {` and then put all the DOMNode
> subclass checks under that.

Sure. This is ugly stuff. Will change entirely.

> @@ +3359,5 @@
> > +            }
> > +            props.push(key + ": " + valueString);
> > +          }
> > +          if (preview.names > props.length) {
> > +            props.push("more\u2026");
> 
> I think this should display how many more properties.

This is problematic. Own properties or enumerable properties? If own properties, that's easier. Enumerable properties need to be counted and that's slow. Not sure if the perf impact is worth the benefit.


> ::: toolkit/devtools/server/actors/script.js
> @@ +2772,5 @@
> >    SyntaxError: errorStringify,
> >    TypeError: errorStringify,
> >    URIError: errorStringify,
> >    Boolean: createBuiltinStringifier(Boolean),
> > +  Date: createBuiltinStringifier(Date),
> 
> Presumably this stringifier stuff can be removed in a later patch once the
> previewer stuff lands.

True. I already removed this change. It was a remnant of some older work I did.


> @@ +2877,5 @@
> >      };
> >  
> > +    /// XXX: Cu.isDeadWrapper() doesn't work with Debugger.Objects, but we have
> > +    // code in this file that relies on it working...
> > +    if (this.obj.class != "DeadObject") {
> 
> This should probably be `if (doPreview && this.obj.class != "DeadObject") {`.

Not really. Partial previews need to be allowed, otherwise you'd see [[object Window], [object HTMLDocument], ...]. Each previewer function will check _gripDepth. See updated patch.


> @@ +3290,5 @@
> >    "scope": ObjectActor.prototype.onScope,
> >  };
> >  
> > +// Number of items to preview in an array.
> > +const OBJECT_PREVIEW_MAX_ARRAY_ITEMS = 10;
> 
> Should rename this to just OBJECT_PREVIEW_MAX_ITEMS, or create and use two
> constants (properties/array items). Right now you're using this for both
> key:value pairs and indices.

Good point.


> @@ +3300,5 @@
> > + * arguments: the ObjectActor instance and the grip object being prepared for
> > + * the client. Functions must return false if they cannot provide preview
> > + * information for the debugger object, or true otherwise.
> > + */
> > +// XXX: should we just flatten this structure into an array?
> 
> I don't think it should be a flat array. That would mean every object has to
> go through the entire set of previewers rather than skipping most of them.

True. I'll keep this structure.


> It might be worth turning this into a full blown API to manage the list,
> something like:
> 
> DebuggerServer.ObjectActorPreviewers = {
>   addPreviewer: function(className, previewer) { /***/ },
>   findPreviewer: function(actor) { /***/ },
>   createPreview: function(actor) { /***/ },
>   /* etc. */
> };

I did think of this, but I'd rather keep things simple for now. Do you think we should do this? My intent was to keep it simple, while allowing extensions to customize all of this.


> @@ +3331,5 @@
> > +
> > +    return true;
> > +  }], // Function
> > +
> > +  // TODO: try to avoid the use of unsafeDereference
> 
> I think the use of unsafeDeference is fine. However, we need to use
> known-safe builtin methods. For example,
> `RegExp.prototype.toString.call(raw)` should be guaranteed to not run
> debuggee code.

True, but I believe it's safer to entirely avoid it and to not encourage its use. Patch wip3 already starts using the bultin methods like Date.prototype.getTime(). In the RegExp case I favored the debugger API.


> @@ +3385,5 @@
> > +      return true;
> > +    }
> > +
> > +    let entries = aGrip.preview.entries = [];
> > +    for (let [key, value] of raw) {
> 
> A safe way to iterate over Maps and Sets would be something like:
> `Map.prototype.forEach.call(raw, (value, key) => { /***/ })`. Using for-of
> has the potential to run debuggee code in a couple places.

forEach() doesn't allow limiting the number of elements you go through. Patch wip3 fixes this issue by using Map.prototype.entries.call().


> @@ +3426,5 @@
> > +      }
> > +    }
> > +
> > +    return true;
> > +  }], // DOMStringMap
> 
> Need a previewer for DOMException. I'm not sure if it should be normalized
> to the same fields are preview.kind = "Error" or its own special kind, since
> it has some different properties.

Do you have an example with a DOMException?


> @@ +3454,5 @@
> > +    }
> > +  },
> > +
> > +  function CSSMediaRule({obj, threadActor}, aGrip) {
> > +    let raw = obj.unsafeDereference();
> 
> obj.unsafeDereference() looks like it's used in many/most of the previewers.
> Perhaps it should just be done upfront once and passed as an argument to
> each previewer?

Heh, I noticed this as well and thought of it... and I decided against it for the aforementioned reason: unsafeDereference() should be avoided when possible. I'm disappointed I don't get an XrayWrapped object. If unsafeDereference() guarantees it gives us an object that doesn't allow us to execute content code by mistake, then sure, we can add it as an argument for each previewer. Until then, I would say no.

I talked to Jim last week about unsafeDereference() and it *should* return XrayWrapped objects, but as I pointed out there's something funky about this... Will see.


> @@ +3484,5 @@
> > +      return false;
> > +    }
> > +    aGrip.preview = {
> > +      kind: "CSSImportRule",
> > +      href: threadActor.createValueGrip(raw.href),
> 
> I don't know if we can do it, but it'd get great if we could get direct
> access to the DOM getter (in this case CSSImportRule#href) and call it on
> `raw` (similar to the above RegExp#toString example). If we can do this then
> we can avoid any accidental debuggee code running. The other option is to
> use `getProperty` but that's going to be slower and can be foiled if someone
> overwrites the builtin getters or something.

Good idea, I'll try to find something like that wherever possible. Yet, I don't mind using getProperty() because it wont execute content code, and even if the content code overwrites the builtin getter with some value, that's not a tragedy -- the user will see what is actually on the object. It's better than having |raw.href| which could execute content code.


> @@ +3556,5 @@
> > +      return true;
> > +    }
> > +
> > +    let items = aGrip.preview.items = [];
> > +    for (let i = 0; i < raw.length && i < OBJECT_PREVIEW_MAX_ARRAY_ITEMS; i++) {
> 
> This won't handle holes in arrays, so the client side won't be able to
> differentiate between `[,,]` and `[undefined, undefined]`.

How can we better handle this?


> @@ +3647,5 @@
> > +    };
> > +
> > +    // XXX not sure if it makes sense to use the number of own properties. we
> > +    // need it to determine if an object preview should show an ellipsis or
> > +    // not. thoughts?
> 
> I think you should keep this.. It might be worth showing how many more
> properties there are for generic objects like you do with arrays. `Window {
> location: whatever, ... 500 more properties }`. Or, like for the
> VariablesView (or some other consumer of this API), you might just want to
> put `Object(50)` as the preview.

wip3 removed this, because of the safe getters confusion. It's hard to give the 'correct' number of 'more' properties. See the above explanation. Your thoughts are welcome. I am willing to add back the number of own properties, if you'd like it.

(In reply to Brandon Benvie [:benvie] from comment #18)
> Comment on attachment 8340553 [details] [diff] [review]
> part 2 - pretty output for objects (text only) - wip2
> 
> Review of attachment 8340553 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +3639,5 @@
> > +    if (aGrip.preview || aGrip.displayString || threadActor._gripDepth > 1) {
> > +      return false;
> > +    }
> > +
> > +    let i = 0, names = obj.getOwnPropertyNames();
> 
> We might want to filter this to only show enumerable properties. This is
> what Chrome devtools does and I've found it useful. Thoughts?

Since this is devtools, I'd like to see even non-enumerable props. However, I see the point of limiting the preview to only enumerable properties. I will try it out and see how it improves things.


Thank you! I'll ask again for feedback when I have more updates.

Comment 21

[Thomas Andersen]

> We should also keep in mind extensibility - addon authors will want to be able to customize how their js libs are displayed. This is most-likely something for a follow-up bug.

Does such a issue exist today?

For my add-on I would like to write to the web console's security category
Thanks to Mihai on the IRC channel I was pointed to https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIScriptError which works great, but it would be nice to use a higher level api.

Also, I see in nightly that the security log items has a [Read more] link appended to the message. Is it possible to switch this off?

Comment 22

[Mihai Sucan [:msucan]]

(In reply to Thomas Andersen from comment #21)
> > We should also keep in mind extensibility - addon authors will want to be able to customize how their js libs are displayed. This is most-likely something for a follow-up bug.
> 
> Does such a issue exist today?

We deemed that important enough to not split it in a follow-up bug. This is what we are working on here, not just making pretty output for objects. Customization needs to be allowed in multiple places across the codebase.

> For my add-on I would like to write to the web console's security category
> Thanks to Mihai on the IRC channel I was pointed to
> https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/
> nsIScriptError which works great, but it would be nice to use a higher level
> api.

The part1 patch that landed allows you to output any customized message using the ConsoleOutput API.

> Also, I see in nightly that the security log items has a [Read more] link
> appended to the message. Is it possible to switch this off?

Not really, unless you use the ConsoleOutput API.

Let's keep this bug clean. Please ping on IRC or via email if you have questions. Once this bug is fixed, I will write an MDN page on how to use the various APIs for customizing the web console output.

Comment 23

[Jim Blandy :jimb]

It occurs to me that we really ought not to trust 'displayName' properties on functions as much as the server does now; at the very least, it ought to truncate them if they're longer than 500 characters, say. Debuggers ought to assume that debuggees are doing bizarre things.

Comment 24

[Jim Blandy :jimb]

Or... simply omit suspicious displayName properties from the grip altogether.

Comment 25

[Mihai Sucan [:msucan]]

Attachment: part 2 - pretty output for objects (text only) - wip4

Changes:

- added pretty output for DOM events.

- now I pass the raw JS object (unsafeDereference() result) to previewer functions if isXrayWrapper() is true.

- all previewers now use the xray wrapped objects, when available. For the cases where I don't have xray wrapping I use native methods (eg. Date/RegExp/Array/Set/Map.prototype.foo()). When those are not available, or if I need to read an unsafe property I use the debugger's getProperty() function (this uses the debugger API).

- addressed Brandon's feedback, except code styling changes for vview. That part of the code is still just the result of quick prototyping.

- added back the number of own properties for "generic" objects, as suggested by Brandon.

- only showing enumerable own properties in "generic" object previews.

I have only a couple of toolkit-related changes left to do: preview for DOMException (I was close to having this done today), and avoid previewing for prototypes, or at least I should handle exceptions in better ways during the execution of preview functions.

Once toolkit changes are ready. I'll ask for review on that code.

Comment 26

[Mihai Sucan [:msucan]]

Attachment: part 2 - objectactor changes - wip5

This is ready for review.

Changes: more fixes and added preview for DOMExceptions. Also, figured out in bug 946752 how to check more kinds of objects if they are safe to read properties from. Updated the patch to almost always use the dereferenced JS object. Only pure JS objects dont get any safe wrappers - for those I use method from native prototypes (eg. RegExp, Date, Array, Map, Set, etc). When needed I also use the debugger API-based approach of getProperty().

I'm splitting the patch into two. This patch holds only the ObjectActor-related changes for easier review.

I'll attach a separate patch for the VariablesView changes.

Comment 27

[Mihai Sucan [:msucan]]

Attachment: part 3 - pretty output for objects (text only) - wip5

This patch holds the VariablesView changes for pretty ObjectActor output. This is not yet ready for review, however feedback is always welcome.

Changes: added preview for DOMException.

Comment 28

[Brandon Benvie [:benvie]]

Comment on attachment 8343959 [details] [diff] [review]
part 2 - objectactor changes - wip5

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

Awesome! This will make it very easy to make custom previewers.

::: toolkit/devtools/server/actors/script.js
@@ +2803,5 @@
>    this.obj = aObj;
>    this.threadActor = aThreadActor;
> +
> +  if (!("_gripDepth" in this.threadActor)) {
> +    this.threadActor._gripDepth = 0;

Could you add `this._gripDepth = 0;` to the ThreadActor constructor and avoid doing the check here every time an ObjectActor is constructed?

@@ +3061,5 @@
> +    try {
> +      names = aObject.getOwnPropertyNames()
> +    } catch (ex) {
> +      // Calling getOwnPropertyNames() on wrapped native prototypes is not
> +      // allowed: "cannot modify properties of a WrappedNative".

Maybe note the bug number (bug 560072) here like the other comments that note this?

@@ +3130,5 @@
>     * A helper method that creates a property descriptor for the provided object,
>     * properly formatted for sending in a protocol response.
>     *
>     * @param string aName
>     *        The property that the descriptor is generated for.

Add @param comment for aOnlyEnumerable.

@@ +3290,5 @@
> +    // Check if the developer has added a de-facto standard displayName
> +    // property for us to use.
> +    try {
> +      let desc = obj.getOwnPropertyDescriptor("displayName");
> +      if (desc && desc.value && typeof desc.value == "string") {

Could just be `if (desc && typeof desc.value == "string")`. This would consider empty strings to be valid userDisplayNames.

@@ +3303,5 @@
> +    return true;
> +  }], // Function
> +
> +  RegExp: [function({obj, threadActor}, aGrip) {
> +    if (!obj.proto || obj.proto.class != "RegExp") {

Is this required? We know that the current obj has "RegExp" as its class, so calling RegExp methods on it should definitely work I think. For example, the following works:

let regexp = /./;
regexp.__proto__ = null;
RegExp.prototype.toString.call(regexp); // "/./"

Is there a situation where this isn't true?

@@ +3315,5 @@
> +  }],
> +
> +  Date: [function({obj, threadActor}, aGrip) {
> +    let time = Date.prototype.getTime.call(obj.unsafeDereference());
> +    if (isNaN(time)) {

The time value for a Date can be NaN (`isNaN(new Date("blarg").getTime()) === true`). I think if a Date object has NaN for its time we should still use the Date previewer rather than falling back to the Object previewer. Client side renderers for date previews just need to be aware that `NaN` is a possible value for the timestamp and be able to render accordingly.

@@ +3343,5 @@
> +
> +    let raw = obj.unsafeDereference();
> +    let items = aGrip.preview.items = [];
> +
> +    Array.some(raw, (item) => {

Array.some skips array holes. So making a preview for `[,,,,'foo']` will produce `{ items: ['foo'] }` which is incorrect. To take into account holes you need something like:

for (let i = 0; i < raw.length && items.length < OBJECT_PREVIEW_MAX_ITEMS; i++) {
  if (!hasOwnProperty.call(raw, i)) {
    // array hole
    items.push(null)
  } else {
    /* handle normal values */
  }
}

@@ +3435,5 @@
> +      size: keys.length,
> +    };
> +
> +    if (threadActor._gripDepth > 1) {
> +      return false;

This should return `true` shouldn't it?

@@ +3438,5 @@
> +    if (threadActor._gripDepth > 1) {
> +      return false;
> +    }
> +
> +    let entries = aGrip.preview.entries;

Nit: `let { entries } = aGrip.preview`

@@ +3508,5 @@
> +    };
> +    return true;
> +  },
> +
> +  function CSSImportRule({obj, threadActor}, aGrip, aRawObj) {

It may be worth combining CSSImportRule, CSSStyleSheet, and DOMLocation into the same rule since they all do the same thing by combining the instanceof checks and change preview.kind to be aObj.class. This would be similar to how ArrayLike handles multiple different classes of object.

@@ +3552,5 @@
> +    };
> +    return true;
> +  },
> +
> +  function ArrayLike({obj, threadActor}, aGrip, aRawObj) {

This should probably include typed arrays.

@@ +3605,5 @@
> +      kind: "MapLike",
> +      entries: [],
> +      size: aRawObj.length,
> +    };
> +    let entries = aGrip.preview.entries;

Nite: `let { entries } = aGrip.preview`.

Comment 29

[Mihai Sucan [:msucan]]

(In reply to Jim Blandy :jimb from comment #23)
> It occurs to me that we really ought not to trust 'displayName' properties
> on functions as much as the server does now; at the very least, it ought to
> truncate them if they're longer than 500 characters, say. Debuggers ought to
> assume that debuggees are doing bizarre things.

Sounds like a good idea. Should I go ahead and update the patch to limit the displayName to 500 characters?

Comment 30

[Mihai Sucan [:msucan]]

(In reply to Brandon Benvie [:benvie] from comment #28)
> Comment on attachment 8343959 [details] [diff] [review]
> part 2 - objectactor changes - wip5
> 
> Review of attachment 8343959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome! This will make it very easy to make custom previewers.

Thank you for the quick review!


> ::: toolkit/devtools/server/actors/script.js
> @@ +2803,5 @@
> >    this.obj = aObj;
> >    this.threadActor = aThreadActor;
> > +
> > +  if (!("_gripDepth" in this.threadActor)) {
> > +    this.threadActor._gripDepth = 0;
> 
> Could you add `this._gripDepth = 0;` to the ThreadActor constructor and
> avoid doing the check here every time an ObjectActor is constructed?

Sure. Will do.


> @@ +3061,5 @@
> > +    try {
> > +      names = aObject.getOwnPropertyNames()
> > +    } catch (ex) {
> > +      // Calling getOwnPropertyNames() on wrapped native prototypes is not
> > +      // allowed: "cannot modify properties of a WrappedNative".
> 
> Maybe note the bug number (bug 560072) here like the other comments that
> note this?

Will do.


> @@ +3130,5 @@
> >     * A helper method that creates a property descriptor for the provided object,
> >     * properly formatted for sending in a protocol response.
> >     *
> >     * @param string aName
> >     *        The property that the descriptor is generated for.
> 
> Add @param comment for aOnlyEnumerable.

Ah, meant to do it, but I forgot. Good catch.

> @@ +3290,5 @@
> > +    // Check if the developer has added a de-facto standard displayName
> > +    // property for us to use.
> > +    try {
> > +      let desc = obj.getOwnPropertyDescriptor("displayName");
> > +      if (desc && desc.value && typeof desc.value == "string") {
> 
> Could just be `if (desc && typeof desc.value == "string")`. This would
> consider empty strings to be valid userDisplayNames.

This is a copy/paste of code we already had. I believe not allowing an empty userDisplayName is a sensible choice here. There's no benefit in displaying an empty function name. If you believe this is a change I should make, I can do it.

> @@ +3303,5 @@
> > +    return true;
> > +  }], // Function
> > +
> > +  RegExp: [function({obj, threadActor}, aGrip) {
> > +    if (!obj.proto || obj.proto.class != "RegExp") {
> 
> Is this required? We know that the current obj has "RegExp" as its class, so
> calling RegExp methods on it should definitely work I think. For example,
> the following works:

This check here is added to avoid doing a preview for the RegExp prototype itself. That looks as /(?:)/. Should I change this?


> @@ +3315,5 @@
> > +  }],
> > +
> > +  Date: [function({obj, threadActor}, aGrip) {
> > +    let time = Date.prototype.getTime.call(obj.unsafeDereference());
> > +    if (isNaN(time)) {
> 
> The time value for a Date can be NaN (`isNaN(new Date("blarg").getTime())
> === true`). I think if a Date object has NaN for its time we should still
> use the Date previewer rather than falling back to the Object previewer.
> Client side renderers for date previews just need to be aware that `NaN` is
> a possible value for the timestamp and be able to render accordingly.

I had this working and the client rendered such dates as expected ("Invalid Date"). However, when a preview for the Date prototype itself is generated we also get "Invalid Date". I chose to have all invalid dates expressed as "Date".

I can change this check to do something like I did for RegExp to avoid making a preview for the Date prototype itself. Then we can display invalid dates. Which way do you prefer?


> @@ +3343,5 @@
> > +
> > +    let raw = obj.unsafeDereference();
> > +    let items = aGrip.preview.items = [];
> > +
> > +    Array.some(raw, (item) => {
> 
> Array.some skips array holes. So making a preview for `[,,,,'foo']` will
> produce `{ items: ['foo'] }` which is incorrect. To take into account holes
> you need something like:
> 
> for (let i = 0; i < raw.length && items.length < OBJECT_PREVIEW_MAX_ITEMS;
> i++) {
>   if (!hasOwnProperty.call(raw, i)) {
>     // array hole
>     items.push(null)
>   } else {
>     /* handle normal values */
>   }
> }

I'll update the patch accordingly. I forgot about the array holes...


> @@ +3508,5 @@
> > +    };
> > +    return true;
> > +  },
> > +
> > +  function CSSImportRule({obj, threadActor}, aGrip, aRawObj) {
> 
> It may be worth combining CSSImportRule, CSSStyleSheet, and DOMLocation into
> the same rule since they all do the same thing by combining the instanceof
> checks and change preview.kind to be aObj.class. This would be similar to
> how ArrayLike handles multiple different classes of object.

I would still need to have individual checks for every class, because each of these have a different property we need to add to the preview. Not much benefit to have these methods combined. In ArrayLike we do the same for all the supported classes.


> @@ +3552,5 @@
> > +    };
> > +    return true;
> > +  },
> > +
> > +  function ArrayLike({obj, threadActor}, aGrip, aRawObj) {
> 
> This should probably include typed arrays.

Ah, yes! I will test typed arrays.


Will update the patch next week. Thanks!

Comment 31

[Brandon Benvie [:benvie]]

(In reply to Mihai Sucan [:msucan] from comment #30)
> > @@ +3290,5 @@
> > > +    // Check if the developer has added a de-facto standard displayName
> > > +    // property for us to use.
> > > +    try {
> > > +      let desc = obj.getOwnPropertyDescriptor("displayName");
> > > +      if (desc && desc.value && typeof desc.value == "string") {
> > 
> > Could just be `if (desc && typeof desc.value == "string")`. This would
> > consider empty strings to be valid userDisplayNames.
> 
> This is a copy/paste of code we already had. I believe not allowing an empty
> userDisplayName is a sensible choice here. There's no benefit in displaying
> an empty function name. If you believe this is a change I should make, I can
> do it.

Filtering empty displayNames is fine.


> > @@ +3303,5 @@
> > > +    return true;
> > > +  }], // Function
> > > +
> > > +  RegExp: [function({obj, threadActor}, aGrip) {
> > > +    if (!obj.proto || obj.proto.class != "RegExp") {
> > 
> > Is this required? We know that the current obj has "RegExp" as its class, so
> > calling RegExp methods on it should definitely work I think. For example,
> > the following works:
> 
> This check here is added to avoid doing a preview for the RegExp prototype
> itself. That looks as /(?:)/. Should I change this?

You might as well just show RegExp.prototype as it shows up. That seems better than rendering it as a generic empty object.


> > @@ +3315,5 @@
> > > +  }],
> > > +
> > > +  Date: [function({obj, threadActor}, aGrip) {
> > > +    let time = Date.prototype.getTime.call(obj.unsafeDereference());
> > > +    if (isNaN(time)) {
> > 
> > The time value for a Date can be NaN (`isNaN(new Date("blarg").getTime())
> > === true`). I think if a Date object has NaN for its time we should still
> > use the Date previewer rather than falling back to the Object previewer.
> > Client side renderers for date previews just need to be aware that `NaN` is
> > a possible value for the timestamp and be able to render accordingly.
> 
> I had this working and the client rendered such dates as expected ("Invalid
> Date"). However, when a preview for the Date prototype itself is generated
> we also get "Invalid Date". I chose to have all invalid dates expressed as
> "Date".
> 
> I can change this check to do something like I did for RegExp to avoid
> making a preview for the Date prototype itself. Then we can display invalid
> dates. Which way do you prefer?

The latter is better, we should definitely be able to show an invalid date as a date.

Comment 32

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

Comment on attachment 8343961 [details] [diff] [review]
part 3 - pretty output for objects (text only) - wip5

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

Quick question here: do we still need aConciseFlag? I don't think so.

Comment 33

[Mihai Sucan [:msucan]]

(In reply to Victor Porof [:vp] from comment #32)
> Comment on attachment 8343961 [details] [diff] [review]
> part 3 - pretty output for objects (text only) - wip5
> 
> Review of attachment 8343961 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Quick question here: do we still need aConciseFlag? I don't think so.

Yes. Some of the output for some types of objects is really verbose, compared to what we can show in the rather compact variables view. Unless we decide to show a lot of stuff as previews for objects in vview.

I'd really like the option to open a new top-level object inspector (like new tool), to temporarily look at objects. Like we can in Firebug. In those cases, yes, a concise flag would not be needed.

Comment 34

[Mihai Sucan [:msucan]]

Attachment: part 3 - pretty output for objects (text only) - wip6

Changes:

- this is now the code structure I want. Instead of a big dump of stringify-related code in getString() now each stringifier lives in separate functions, organized by types, object classes and preview kinds.

This is now extensible - addons can choose to change ObjectActor grips to add preview info for any objects they need, and they can also change how they are displayed in VariablesView and in the web console.

- did some bug fixes. using uneval() for proper string output where needed.


Next:
- fix up some remaining issues marked with XXX based on Brandon's feedback.
- I have a couple of TODO items as well.
- then fixes for existing tests, and I need to write some new tests for the object types we now added support for.


Brandon: I will address your review comments for patch part 2 ASAP. Thank you!

Comment 35

[Brandon Benvie [:benvie]]

Comment on attachment 8344888 [details] [diff] [review]
part 3 - pretty output for objects (text only) - wip6

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

This looks good. Is the plan to have a future patch where some of this stuff gets more in depth rendering, like rendering numbers/strings/booleans with colors to match the VariablesView, etc?

A high level thing we might want to look at is to be able to provide a custom set of renderers all at once. Like say we have a more verbose set of renderers for the webconsole, one set of renderers that is more succinct like what would be used in the VariablesView. Right now we have that controlled with a concise flag, but this isn't extensible. Probably something for a future followup bug, but just a thought to consider.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +3091,5 @@
>   *        @see Variable.setGrip
> + * @param object aOptions
> + *        Options:
> + *        - concise: boolean that tells you want a concisely formatted string.
> + *        - noStringQuotes: boolean that tells to not quote strings.

I like that this was changed to an options object. Boolean params are too opaque anyway.

@@ +3140,5 @@
>        return aGrip + "";
>    }
>  };
>  
> +VariablesView.stringifiers = {};

Seems like this might belong in DevToolsUtils or something else to be used in common. This is true with a lot of the Variables.* static utility functions though, so probably should just be done in another bug.

@@ +3142,5 @@
>  };
>  
> +VariablesView.stringifiers = {};
> +
> +VariablesView.stringifiers.byType = {

Maybe we can combine byType and byObjectClass and just have types be lowercase and classes be uppercase. Separating them seems like perhaps unneeded complexity.

@@ +3152,5 @@
> +  },
> +
> +  longString: function({initial}, {noStringQuotes}) {
> +    // XXX: add an ellipsis after .initial? If yes, should we also add an option
> +    // to toggle the ellipsis?

Yes to ellipsis. Maybe have `noEllipsis` field on the options? That way we can default to having it on.

@@ +3182,5 @@
> +    let params = aGrip.parameterNames || "";
> +    if (!concise) {
> +      return "function " + name + "(" + params + ")";
> +    }
> +    return (name || "function") + "(" + params + ")";

Kind of a small thing, but this won't correctly represent arrow functions or ES6 generators correctly (and succinct methods when they're implemented in the future, like `{ method(){} }`). This is a limitation of the Debugger object itself (that we can't get what kind a function is).

Another future problem is the handling of parameterNames. Currently, `Debugger.Frame.prototype.parameterNames` only returns an array of strings, so your code will work. However, `parameterNames` is specified to potentially return an array of strings|arrays|objects to handle destructured parameter names (bug 936208). There's also rest parameters which are currently handled inadequately (bug 905359). So when either of those bugs is fixed we should also update this stringifier to handle the new data/formatting.

@@ +3196,5 @@
> +      return null;
> +    }
> +
> +    if (concise) {
> +      return "Date " + new Date(preview.timestamp).toISOString();

I almost feel as if this should be the display no matter what (I.E. don't use toLocaleString at all). ISO is short, makes the preview common across locales, and it's something developers are familiar with anyway. Thoughts?

@@ +3257,5 @@
> +    }
> +
> +    let {preview} = aGrip;
> +    let props = [];
> +    for (let [key, value] of Iterator(preview.ownProperties || {})) {

I believe `Iterator` is deprecated. It might be worth adding our own helper for this (to DevToolsUtils or something). Something simple like:

function* entries(obj) {
  for (let key of Object.keys(Object(obj))) {
    yield [key, obj[key]];
  }
}

@@ +3335,5 @@
> +      return null;
> +    }
> +
> +    if (concise) {
> +      return aGrip.class + " " + preview.type; // XXX: maybe just preview.type?

I think it's useful to show the class here. Everything else shows its class (except plain Objects).

@@ +3370,5 @@
> +    return result;
> +  },
> +
> +  CSSStyleRule: function(aGrip) {
> +    // XXX: should we output only |class| if |concise| is true?

I think we should only output class if concise is true, yeah.

@@ +3377,5 @@
> +  },
> +
> +  CSSMediaRule: function(aGrip) {
> +    // XXX: we could introduce a new preview.kind = "ObjectWithText" or
> +    // something, one that we would reuse for CSSStyleRule and CSSMediaRule.

Yes. I think we should give every opportunity for customization, and more generic kinds helps with this.

@@ +3445,5 @@
> +  }, // DOMNode
> +}; // VariablesView.stringifiers.byObjectKind
> +
> +// XXX: here we could also add a new preview.kind ObjectWithURL for reuse.
> +// thoughts?

Yes, same answer as ObjectWithText.

Comment 36

[Mihai Sucan [:msucan]]

(In reply to Brandon Benvie [:benvie] from comment #31)
> (In reply to Mihai Sucan [:msucan] from comment #30)
> > This check here is added to avoid doing a preview for the RegExp prototype
> > itself. That looks as /(?:)/. Should I change this?
> 
> You might as well just show RegExp.prototype as it shows up. That seems
> better than rendering it as a generic empty object.

RegExp prototype is currently rendered as "RegExp" (the obj.class is used, without quotes, obviously), not as an empty generic object "{}". Do you prefer "RegExp" or "/(?:)/". I preferred the former.


> > I had this working and the client rendered such dates as expected ("Invalid
> > Date"). However, when a preview for the Date prototype itself is generated
> > we also get "Invalid Date". I chose to have all invalid dates expressed as
> > "Date".
> > 
> > I can change this check to do something like I did for RegExp to avoid
> > making a preview for the Date prototype itself. Then we can display invalid
> > dates. Which way do you prefer?
> 
> The latter is better, we should definitely be able to show an invalid date
> as a date.

Sounds fine.


Thanks for your comments!

Comment 37

[Mihai Sucan [:msucan]]

(In reply to Brandon Benvie [:benvie] from comment #35)
> Comment on attachment 8344888 [details] [diff] [review]
> part 3 - pretty output for objects (text only) - wip6
> 
> Review of attachment 8344888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good. Is the plan to have a future patch where some of this stuff
> gets more in depth rendering, like rendering numbers/strings/booleans with
> colors to match the VariablesView, etc?

Yes. We need to keep things simple enough and reviewable for now. This patch does text-only pretty output. I'll open a separate bug for the web console changes.


> A high level thing we might want to look at is to be able to provide a
> custom set of renderers all at once. Like say we have a more verbose set of
> renderers for the webconsole, one set of renderers that is more succinct
> like what would be used in the VariablesView. Right now we have that
> controlled with a concise flag, but this isn't extensible. Probably
> something for a future followup bug, but just a thought to consider.

Good point. I think this is also a matter of context. For example, in the webconsole we want more verbose output, syntax highlighting, and interactivity with various elements in the object preview.

Overview of where we should get at (in my opinion):

- DebuggerServer has its own object previewers, responsible for populating object grips based on the object type and whatever the addon author wants. (this is patch part 2)

- VariablesView has its own renderers/stringifiers that are responsible for displaying object grips, based on the preview info from the server. (patch part 3)

- The Web Console will provide its own ConsoleOutput structure for adding custom renderers for Object Actor grips, similar to how Variables View does it here in patch 3. Also note that patch part 1 has added APIs / prepared the ground for this work to happen, and it also allows addons to have custom messages (finally!).

At that point, addon authors will be able to custom-render jquery objects or anything they choose, by making changes in the server, variables view and the console.

It is tempting to provide one unified API for customizing output across the devtools, but take into consideration that the server has its own arguments for each previewer strongly tied to how object actors and thread actors are structured for now. The same will be true with the ConsoleOutput API - each renderer will need to be passed the message object, access to the DOM, to the ConsoleOutput API and access even to the debugger client for communication with the server. Both of these APIs, the object previewers for the DebuggerServer and the API for object rendering in the console, are not easily reusable in other contexts - unless explicitly modeled/structured to work similarly. Only the VariablesView stringifiers are fairly reusable as generic "hey, i want to preview this object actor".

As the code is, VariablesView.getString() is reusable from anywhere. Moving it to DevToolsUtils would pile it up. (see what happened with webconsole/utils.js :) )

This is stuff for the future - for now I think shipping something that looks good and allows addon authors to customize output is very important. We'll need to see the kinds of requirements a unified API would need to meet.


> ::: browser/devtools/shared/widgets/VariablesView.jsm
> @@ +3091,5 @@
> >   *        @see Variable.setGrip
> > + * @param object aOptions
> > + *        Options:
> > + *        - concise: boolean that tells you want a concisely formatted string.
> > + *        - noStringQuotes: boolean that tells to not quote strings.
> 
> I like that this was changed to an options object. Boolean params are too
> opaque anyway.

Yep.


> @@ +3140,5 @@
> >        return aGrip + "";
> >    }
> >  };
> >  
> > +VariablesView.stringifiers = {};
> 
> Seems like this might belong in DevToolsUtils or something else to be used
> in common. This is true with a lot of the Variables.* static utility
> functions though, so probably should just be done in another bug.

True. See what I also said above.


> @@ +3142,5 @@
> >  };
> >  
> > +VariablesView.stringifiers = {};
> > +
> > +VariablesView.stringifiers.byType = {
> 
> Maybe we can combine byType and byObjectClass and just have types be
> lowercase and classes be uppercase. Separating them seems like perhaps
> unneeded complexity.

I would like to keep the two separate, to avoid collisions.


> @@ +3152,5 @@
> > +  },
> > +
> > +  longString: function({initial}, {noStringQuotes}) {
> > +    // XXX: add an ellipsis after .initial? If yes, should we also add an option
> > +    // to toggle the ellipsis?
> 
> Yes to ellipsis. Maybe have `noEllipsis` field on the options? That way we
> can default to having it on.

Yes, will do.


> @@ +3182,5 @@
> > +    let params = aGrip.parameterNames || "";
> > +    if (!concise) {
> > +      return "function " + name + "(" + params + ")";
> > +    }
> > +    return (name || "function") + "(" + params + ")";
> 
> Kind of a small thing, but this won't correctly represent arrow functions or
> ES6 generators correctly (and succinct methods when they're implemented in
> the future, like `{ method(){} }`). This is a limitation of the Debugger
> object itself (that we can't get what kind a function is).

True. We'll need to fix it when the object actor grip for functions and when the debugger object itself support all of those kinds of functions. Is there already a bug open about these limitations in the Debugger.Object? I should open a follow up as well.


> Another future problem is the handling of parameterNames. Currently,
> `Debugger.Frame.prototype.parameterNames` only returns an array of strings,
> so your code will work. However, `parameterNames` is specified to
> potentially return an array of strings|arrays|objects to handle destructured
> parameter names (bug 936208). There's also rest parameters which are
> currently handled inadequately (bug 905359). So when either of those bugs is
> fixed we should also update this stringifier to handle the new
> data/formatting.

Good points, thanks for the bug numbers. I saw in the D.O docs that parameterNames can return more complex structures, but I didn't get around to test these things. I will file a follow-up bug.


> @@ +3196,5 @@
> > +      return null;
> > +    }
> > +
> > +    if (concise) {
> > +      return "Date " + new Date(preview.timestamp).toISOString();
> 
> I almost feel as if this should be the display no matter what (I.E. don't
> use toLocaleString at all). ISO is short, makes the preview common across
> locales, and it's something developers are familiar with anyway. Thoughts?

Agreed, but the reason why I use toLocaleString() for the 'verbose' output is simple: I expect that the locale string representation of a date is easier to rad for humans (not only devs).

I know, this is probably a weird idea. So I'll just change to ISO format if you wish.


> @@ +3257,5 @@
> > +    }
> > +
> > +    let {preview} = aGrip;
> > +    let props = [];
> > +    for (let [key, value] of Iterator(preview.ownProperties || {})) {
> 
> I believe `Iterator` is deprecated. It might be worth adding our own helper
> for this (to DevToolsUtils or something). Something simple like:
> 
> function* entries(obj) {
>   for (let key of Object.keys(Object(obj))) {
>     yield [key, obj[key]];
>   }
> }

Oh, Iterator was nice ;). I'll just avoid its use. Thanks!


> @@ +3335,5 @@
> > +      return null;
> > +    }
> > +
> > +    if (concise) {
> > +      return aGrip.class + " " + preview.type; // XXX: maybe just preview.type?
> 
> I think it's useful to show the class here. Everything else shows its class
> (except plain Objects).

Good. Will keep it. 

> @@ +3370,5 @@
> > +    return result;
> > +  },
> > +
> > +  CSSStyleRule: function(aGrip) {
> > +    // XXX: should we output only |class| if |concise| is true?
> 
> I think we should only output class if concise is true, yeah.

Will do.


> @@ +3377,5 @@
> > +  },
> > +
> > +  CSSMediaRule: function(aGrip) {
> > +    // XXX: we could introduce a new preview.kind = "ObjectWithText" or
> > +    // something, one that we would reuse for CSSStyleRule and CSSMediaRule.
> 
> Yes. I think we should give every opportunity for customization, and more
> generic kinds helps with this.
> 
> @@ +3445,5 @@
> > +  }, // DOMNode
> > +}; // VariablesView.stringifiers.byObjectKind
> > +
> > +// XXX: here we could also add a new preview.kind ObjectWithURL for reuse.
> > +// thoughts?
> 
> Yes, same answer as ObjectWithText.

Good then. Will do.


Thanks for the quick feedback!

Comment 38

[Mihai Sucan [:msucan]]

Attachment: part 2 - objectactor changes - wip6

Changes:

- added support for typed arrays.
- limited displayName to 500 chars, as suggested by Jim.
- as discussed added preview kinds ObjectWithText and ObjectWithURL.
- addressed Brandon's comments.
  - did put _gripDepth in thread actor.
  - added bug 560072 in the comments.
  - updated the comment for _propertyDescriptor().
  - changed to allow string preview for invalid dates.
  - array holes in previews.

Jim: I have one concern here. Cu.GetGlobalForObject() is used every time in the new isSafeJSObject() and in the previewer for typed arrays. Is GetGlobalForObject a slow operation? Should we cache it?

Thank you!

Comment 39

[Mihai Sucan [:msucan]]

Attachment: part 3 - pretty output for objects (text only) - wip7

Changes:

- added an ellipsis for long string previews. also added the noEllipsis option as agreed.
- localization for the new strings.
- dropped the use of Iterator.
- bug fixes (escaping of various strings).
- updated to work with the changes in the previous patch, mainly "ObjectWithText" and "ObjectWithURL".

This is ready for review. Thank you!

Next up: tests.

Comment 40

[Mihai Sucan [:msucan]]

Gabor: Sorry to bother you again. I have the following code in my patch (part2): https://pastebin.mozilla.org/3756446

It works well with the Web Console, when working with objects from content tabs. isSafeJSObject() returns true for xray-wrapped objects, and it also returns true for objects with per-scope XPCWN.

My problem is with the Browser Console, where we work with chrome objects (eg. XUL DOM elements). isSafeJSObject() returns false, as expected, for all chrome objects. Which means I can't allow object previewers to access the raw object.

What I need is a way to tell if the given raw object comes from a chrome-privileged compartment. I need something like this to allow the object previewers to read properties from the supported object classes. Currently I can't tell if this is a chrome object or not. It's definitely not safe in the xray-wrapper sense, but that is not very important when working with chrome stuff in the browser console. Any suggestions? Thank you very much!

Comment 41

[Mihai Sucan [:msucan]]

Attachment: part 4 - tests

Here we go, first WIP. More tests still need to be updated.

Comment 42

[Brandon Benvie [:benvie]]

Comment on attachment 8346042 [details] [diff] [review]
part 3 - pretty output for objects (text only) - wip7

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

This looks mostly good.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +3173,5 @@
> +    let ellipsis = noEllipsis ? "" : "\u2026";
> +    if (noStringQuotes) {
> +      return initial + ellipsis;
> +    }
> +    return uneval(initial) + ellipsis;

This will end up with things like `"long string"...`. Is that what we want?

@@ +3238,5 @@
> +    }
> +
> +    let result = [];
> +    for (let item of preview.items) {
> +      result.push(VariablesView.getString(item, { concise: true }));

This doesn't handle null items, which stand in for array holes. Need like

if (item === null)
  result.push("");
else
  /* etc. */

@@ +3361,5 @@
> +      return preview.name || aGrip.class;
> +    }
> +
> +    // XXX; should we localize 'code", "result" and/or "location"? i'm tempted
> +    // to say yes...

If possible then I think so.

Comment 43

[Mihai Sucan [:msucan]]

(In reply to Brandon Benvie [:benvie] from comment #42)
> Comment on attachment 8346042 [details] [diff] [review]
> part 3 - pretty output for objects (text only) - wip7
> 
> Review of attachment 8346042 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks mostly good.
> 
> ::: browser/devtools/shared/widgets/VariablesView.jsm
> @@ +3173,5 @@
> > +    let ellipsis = noEllipsis ? "" : "\u2026";
> > +    if (noStringQuotes) {
> > +      return initial + ellipsis;
> > +    }
> > +    return uneval(initial) + ellipsis;
> 
> This will end up with things like `"long string"...`. Is that what we want?

This is what I asked about in the XXX: should we add an ellipsis after the long strings? To somehow indicate that this is a cropped string.

What do you suggest we should do here?



> @@ +3238,5 @@
> > +    }
> > +
> > +    let result = [];
> > +    for (let item of preview.items) {
> > +      result.push(VariablesView.getString(item, { concise: true }));
> 
> This doesn't handle null items, which stand in for array holes. Need like
> 
> if (item === null)
>   result.push("");
> else
>   /* etc. */

Did you test this updated code? Array holes have undefined elements, not null, based on my testing. I tried new Array(5), then push('foo'), first five elements are |undefined|, then 'foo'. This code displays null and undefined items as they are. Do you want it to explicitly do something like '[,,"foo"]' instead of '[undefined,undefined,"foo"]'? The latter is what these patches display. This is what Chrome and Firebug also do. I'd like to avoid special-casing |undefined| because authors may actually add undefined elements on purpose or by mistake and they wont be able to tell why those elements are empty, they wont know what value that emptiness might mean - such display could be confusing.


> @@ +3361,5 @@
> > +      return preview.name || aGrip.class;
> > +    }
> > +
> > +    // XXX; should we localize 'code", "result" and/or "location"? i'm tempted
> > +    // to say yes...
> 
> If possible then I think so.

Will do. Thank you for the quick review!

Comment 44

[Mihai Sucan [:msucan]]

Brandon: Meh. It seems testing / comparing how array holes are displayed in firebug and chrome's web inspector got me confused. I didn't notice they do anything special for such cases. I was under the impression that making sure undefined elements show up in the preview is sufficient. As you suggested, I can use the hasOwnProperty() check to determine if the element is set to undefined or if it's an array hole. I have a local patch update with this fix included and other fixes - thanks to the tests. I will update the patches later today. Thank you!

Comment 45

[Gabor Krizsanits (INACTIVE)]

(In reply to Mihai Sucan [:msucan] from comment #40)
> What I need is a way to tell if the given raw object comes from a
> chrome-privileged compartment. I need something like this to allow the
> object previewers to read properties from the supported object classes.
> Currently I can't tell if this is a chrome object or not. It's definitely
> not safe in the xray-wrapper sense, but that is not very important when
> working with chrome stuff in the browser console. Any suggestions? Thank you
> very much!

I don't think we have any API for that in JS. But I do think it would be quite simple
to implement something for it. I pass the question to Bobby maybe he has a solution.

You mention the XUL document case, well IF the global of the object is a window, maybe
you could use the nodePrincpal of its document.documentElement, but it would only cover
some cases, and I'm not even sure it's a good approach. Bobby, what do you think about
an API on Cu for these sort of things? Like GetPrincipalOfObject / bool Subsumes(principal1, principal2)
or something... It might make sense to have it for testing purposes anyway...

Comment 46

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #45)
> (In reply to Mihai Sucan [:msucan] from comment #40)
> > What I need is a way to tell if the given raw object comes from a
> > chrome-privileged compartment. I need something like this to allow the
> > object previewers to read properties from the supported object classes.
> > Currently I can't tell if this is a chrome object or not. It's definitely
> > not safe in the xray-wrapper sense, but that is not very important when
> > working with chrome stuff in the browser console. Any suggestions? Thank you
> > very much!
> 
> I don't think we have any API for that in JS. But I do think it would be
> quite simple
> to implement something for it. I pass the question to Bobby maybe he has a
> solution.
> 
> You mention the XUL document case, well IF the global of the object is a
> window, maybe
> you could use the nodePrincpal of its document.documentElement, but it would
> only cover
> some cases, and I'm not even sure it's a good approach.
>
> Bobby, what do you
> think about
> an API on Cu for these sort of things? Like GetPrincipalOfObject / bool

I don't understand. If we're privileged enough to access Cu, then chrome stuff should be same-origin. If we're not, then we shouldn't really be inspecting chrome stuff in the first place.

Can someone clarify the use case?

Comment 47

[Brandon Benvie [:benvie]]

(In reply to Mihai Sucan [:msucan] from comment #43)
> This is what I asked about in the XXX: should we add an ellipsis after the
> long strings? To somehow indicate that this is a cropped string.
> 
> What do you suggest we should do here?

I would use ellipses, but inside the quotes. So "long string...". Having it outside the quotes looks wrong.

Comment 48

[Brandon Benvie [:benvie]]

(In reply to Mihai Sucan [:msucan] from comment #44)
> Brandon: Meh. It seems testing / comparing how array holes are displayed in
> firebug and chrome's web inspector got me confused. I didn't notice they do
> anything special for such cases. I was under the impression that making sure
> undefined elements show up in the preview is sufficient. As you suggested, I
> can use the hasOwnProperty() check to determine if the element is set to
> undefined or if it's an array hole. I have a local patch update with this
> fix included and other fixes - thanks to the tests. I will update the
> patches later today. Thank you!

Yeah, they mark it with the number. An interesting one is `[, undefined, ,,]` which is displayed as "[undefined × 1, undefined, undefined × 2]".

Comment 49

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #46)
> I don't understand. If we're privileged enough to access Cu, then chrome
> stuff should be same-origin. If we're not, then we shouldn't really be
> inspecting chrome stuff in the first place.
> 
> Can someone clarify the use case?

Same origin sure but not same compartment. The check Mihai is using is a comparison of two globals, which is not very helpful in this situation... So the use case is: we are in a system scope, someone gives us an object and we must tell if it's same origin or not. (At least that's my grasp on it...) In either case we get the same transparent wrapper, no? But only in the system vs content case is where we really have to worry. The system vs system case is less of a problem.

By the way Mihai, chrome can be Add-on code as well right? (I don't have much context here...) Because if so be careful about the assumptions you make about it, since it can trick you unintentionally as well. An unexpected getter call can cause problems in the Add-on code flow, or in the debugger as well keep that in mind.

Comment 50

[Mihai Sucan [:msucan]]

Thank you Gabor and Bobby!

(In reply to Bobby Holley (:bholley) from comment #46)
> I don't understand. If we're privileged enough to access Cu, then chrome
> stuff should be same-origin. If we're not, then we shouldn't really be
> inspecting chrome stuff in the first place.
> 
> Can someone clarify the use case?

True, same-origin, system/chrome code that inspects and evals code in other chrome windows. Use case: see the browser console. I need to check if the object is from the same-origin or if it's not. If yes, then I expect it to be "safe" for inspection.

Comment 51

[Mihai Sucan [:msucan]]

Attachment: part 2 - objectactor changes - wip7

Changes:

- improved handling of array holes as discussed with Brandon.

- lower case tag names for html elements. (I forgot to do this sooner)

- call makeDebuggeeValue() for typeof obj == "function" as well.

Comment 52

[Mihai Sucan [:msucan]]

Attachment: part 3 - pretty output for objects (text only) - wip8

Changes:

- put the ellipsis inside quotes for long strings, as suggested by Brandon. I did put it outside, because putting it inside might mean the string actually includes the ellipsis.

- sync with part 2 patch wrt. array holes.

Comment 53

[Mihai Sucan [:msucan]]

Attachment: part 4 - tests - wip2

All webconsole tests pass now. Next: I need to check/fix the other devtools which use the VariablesView for potential breakage, and I need to add some more checks for a few types of objects we do pretty output for.

Comment 54

[Mihai Sucan [:msucan]]

(In reply to Brandon Benvie [:benvie] from comment #42)
> > +      return preview.name || aGrip.class;
> > +    }
> > +
> > +    // XXX; should we localize 'code", "result" and/or "location"? i'm tempted
> > +    // to say yes...
> 
> If possible then I think so.

I just decided against this idea. It's possible, AFAIK, to easily translate 'code', 'result' and 'location' to the languages I know (french and romanian), but trying to explain these strings in localization notes could be difficult - DOM exceptions are, perhaps, too technical. No point to translate this.

(my initial temptation to have these translated is because I actually like Firefox devtools in french :) )


(In reply to Brandon Benvie [:benvie] from comment #47)
> (In reply to Mihai Sucan [:msucan] from comment #43)
> > This is what I asked about in the XXX: should we add an ellipsis after the
> > long strings? To somehow indicate that this is a cropped string.
> > 
> > What do you suggest we should do here?
> 
> I would use ellipses, but inside the quotes. So "long string...". Having it
> outside the quotes looks wrong.

I just made the change as requested. Initially, I did put the ellipse outside of the quote to avoid confusion that the ellipsis might be an actual part of the string.


Thank you for your comments!

Comment 55

[Bobby Holley (:bholley)]

(In reply to Mihai Sucan [:msucan] from comment #50)
> True, same-origin, system/chrome code that inspects and evals code in other
> chrome windows. Use case: see the browser console. I need to check if the
> object is from the same-origin or if it's not. If yes, then I expect it to
> be "safe" for inspection.

Ok. I'd be up for adding Cu.getObjectPrincipal(obj) (where wrapper-valued |obj| arguments would be unwrapped before getting the principal). You could then do Cu.getObjectPrincipal(obj).equals(Cu.getObjectPrincipal(yourGlobal)).

Comment 56

[Mihai Sucan [:msucan]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #49)
> (In reply to Bobby Holley (:bholley) from comment #46)
> > I don't understand. If we're privileged enough to access Cu, then chrome
> > stuff should be same-origin. If we're not, then we shouldn't really be
> > inspecting chrome stuff in the first place.
> > 
> > Can someone clarify the use case?
> 
> Same origin sure but not same compartment. The check Mihai is using is a
> comparison of two globals, which is not very helpful in this situation... So
> the use case is: we are in a system scope, someone gives us an object and we
> must tell if it's same origin or not. (At least that's my grasp on it...) In
> either case we get the same transparent wrapper, no? But only in the system
> vs content case is where we really have to worry. The system vs system case
> is less of a problem.
> 
> By the way Mihai, chrome can be Add-on code as well right? (I don't have
> much context here...) Because if so be careful about the assumptions you
> make about it, since it can trick you unintentionally as well. An unexpected
> getter call can cause problems in the Add-on code flow, or in the debugger
> as well keep that in mind.

Addon code is already given max privilege over the browser's experience and yes, it can break things in many ways. I'm inclined to simply 'trust' it and let addons break the debugger, if that's the case. They can already break it. However, before I make any such decision, I'll ping my colleagues.


Thank you!

Comment 57

[Mihai Sucan [:msucan]]

(In reply to Bobby Holley (:bholley) from comment #55)
> (In reply to Mihai Sucan [:msucan] from comment #50)
> > True, same-origin, system/chrome code that inspects and evals code in other
> > chrome windows. Use case: see the browser console. I need to check if the
> > object is from the same-origin or if it's not. If yes, then I expect it to
> > be "safe" for inspection.
> 
> Ok. I'd be up for adding Cu.getObjectPrincipal(obj) (where wrapper-valued
> |obj| arguments would be unwrapped before getting the principal). You could
> then do Cu.getObjectPrincipal(obj).equals(Cu.getObjectPrincipal(yourGlobal)).

This sounds like what I would need here.

Comment 58

[Mihai Sucan [:msucan]]

(In reply to Bobby Holley (:bholley) from comment #55)
> (In reply to Mihai Sucan [:msucan] from comment #50)
> > True, same-origin, system/chrome code that inspects and evals code in other
> > chrome windows. Use case: see the browser console. I need to check if the
> > object is from the same-origin or if it's not. If yes, then I expect it to
> > be "safe" for inspection.
> 
> Ok. I'd be up for adding Cu.getObjectPrincipal(obj) (where wrapper-valued
> |obj| arguments would be unwrapped before getting the principal). You could
> then do Cu.getObjectPrincipal(obj).equals(Cu.getObjectPrincipal(yourGlobal)).

Before we go for such an API we need to understand it better. What kinds of principals do chrome privileged compartments get? Is it the system principal for all, or is it different based on some rules? Which principal(s) is given to addons? Can we tell the difference between browser code and addons?

This API is not blocking this bug, we should open a follow-up bug for the new API, once we decide on what we want.

Thank you!

Comment 59

[Bobby Holley (:bholley)]

(In reply to Mihai Sucan [:msucan] from comment #58)
> Before we go for such an API we need to understand it better. What kinds of
> principals do chrome privileged compartments get? Is it the system principal
> for all, or is it different based on some rules?

"chrome privileged" == "system principal"

> Which principal(s) is given
> to addons? Can we tell the difference between browser code and addons?

Jetpack addons, yes. Legacy overlay-based addons, no. But billm is working on sandoxing the latter, at which point we will be able to tell.

Not that regardless though, the principal will probably just be System Principal. But we can detect those cases based on the information attached to their global, if we need to.

> This API is not blocking this bug, we should open a follow-up bug for the
> new API, once we decide on what we want.

I'll let gabor take care of that.

Comment 60

[Mihai Sucan [:msucan]]

Attachment: part 4 - tests - wip3

More test fixes. All debugger, webconsole and scratchpad tests pass locally. Started writing more tests.

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

Comment 61

[Mihai Sucan [:msucan]]

Comment on attachment 8347440 [details] [diff] [review]
part 4 - tests - wip3

Jeff: one of our debugger server xpcshell tests fails because of a strict error, a builtin self-hosted script is accessing undefined properties. Can you please review the change I did? Thank you!

Please review the changes in the following file: js/src/builtin/Intl.js.

The xpcshell test in question, which IIANM is not of too much relevance: toolkit/devtools/server/tests/unit/test_stepping-06.js. Including these details for future reference.

The strict error is caught by the nsIConsoleService listener found in head_dbg.js, of the unit tests folder.

The reported stack trace points to the GenericObject() function from toolkit/devtools/server/actors/script.js, at the line where we do obj.getOwnPropertyNames(). obj is a Debugger.Object.

The error message referred to options.weekday - which was surprising, since getOwnPropertyNames() is a native function, and we don't have any weekday property.

MXR pointed me to Intl.js and hg blame pointed me to you as a potential reviewer. I hope this is fine, thanks!

Comment 62

[Jeff Walden [:Waldo]]

Comment on attachment 8347440 [details] [diff] [review]
part 4 - tests - wip3

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

::: js/src/builtin/Intl.js
@@ +2302,5 @@
>      if ((required === "date" || required === "any") &&
> +        (typeof options.weekday !== "undefined" ||
> +         typeof options.year !== "undefined" ||
> +         typeof options.month !== "undefined" ||
> +         typeof options.day !== "undefined"))

This isn't equivalent to the previous code.  The difference occurs when the value of any of these properties is an object that emulates |undefined| -- that is, an object whose |typeof| is "undefined" yet still is an object.  (In the browser this is only |document.all|, which makes it very hard to demonstrate the bug, but I think it's possible with enough effort.)  Here's a JS shell testcase (you can test with $objdir/dist/bin/js in a browser build):

[jwalden@find-waldo-now src]$ dbg/js
js> var obj = objectEmulatingUndefined();
js> obj.toString = function() { return "long"; };
(function () { return "long"; })
js> new Date(2013, 12 - 1, 14).toLocaleString("en-US", { weekday: obj }) 
"Saturday"

That's the expected behavior.  But if I change just the weekday component above to what you want to use, I get this instead:

[jwalden@find-waldo-now src]$ dbg/js
js> var obj = objectEmulatingUndefined();
js> obj.toString = function() { return "long"; };        
(function () { return "long"; })
js> new Date(2013, 12 - 1, 14).toLocaleString("en-US", { weekday: obj }) 
"Saturday, 12/14/2013, 12:00:00 AM"

instead of the expected result we'd get if |options.weekday| stringified to "long":

js> new Date(2013, 12 - 1, 14).toLocaleString("en-US", { weekday: "long" }) 
"Saturday"

Rather than "fix" this code this way, we should fix the engine to not warn about it.  It's a one-liner -- I'll post in a second.

Comment 63

[Jeff Walden [:Waldo]]

Attachment: Don't warn for comparisons against |undefined| in self-hosted code

For some reason, in a browser that assertEq(str, "Saturday") is "Friday" instead.  It's possible I'm just screwing something up locally, tho -- if a try run comes back positive on this I'll run with it, otherwise I'll change to assertEq(/day$/.test(str), true) instead.

But none of that really matters as far as the review clock goes, so here you go.  :-)

Comment 64

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

Comment on attachment 8346788 [details] [diff] [review]
part 3 - pretty output for objects (text only) - wip8

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

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +3117,2 @@
>        default:
> +        let stringifier = VariablesView.stringifiers.byType[aGrip.type];

Sexy!

@@ +3160,5 @@
> + * given the same two arguments as those given to VariablesView.getString().
> + */
> +VariablesView.stringifiers = {};
> +
> +VariablesView.stringifiers.byType = {

I really really like all of this. Very nicely written code.

@@ +3169,5 @@
> +    return uneval(aGrip);
> +  },
> +
> +  longString: function({initial}, {noStringQuotes, noEllipsis}) {
> +    let ellipsis = noEllipsis ? "" : "\u2026";

Localize this by using nsIPrefLocalizedString.

I'm adding the following in bug 830344, you can race me :)
https://bugzilla.mozilla.org/attachment.cgi?id=8347598&action=diff#a/browser/devtools/shared/widgets/VariablesView.jsm_sec9

// An ellipsis symbol (usually "…") used for localization.
XPCOMUtils.defineLazyGetter(Scope, "ellipsis", () =>
  Services.prefs.getComplexValue("intl.ellipsis", Ci.nsIPrefLocalizedString).data);

@@ +3282,5 @@
> +      key = VariablesView.getString(key, {
> +        concise: true,
> +        noStringQuotes: true,
> +      });
> +      value = VariablesView.getString(value, { concise: true });

Nit: please declare new variables for key and value.

@@ +3287,5 @@
> +      entries.push(key + ": " + value);
> +    }
> +
> +    if (typeof preview.size == "number" && preview.size > entries.length) {
> +      entries.push((preview.size - entries.length) + " more\u2026");

Localize the ellipsis.

@@ +3305,5 @@
> +  ObjectWithURL: function(aGrip, {concise}) {
> +    let result = aGrip.class;
> +    let url = aGrip.preview.url;
> +    if (!VariablesView.isFalsy({ value: url })) {
> +      result += " " + WebConsoleUtils.abbreviateSourceURL(url,

The debugger has its own SourceUtils as well. Can you please file a followup about moving some of these WebConsoleUtils and the debugger's into a more shared helper?

https://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#1171

Ideally, we should be reusing the same methods everywhere.

@@ +3364,5 @@
> +    let {preview} = aGrip;
> +    let msg = VariablesView.getString(preview.name, { noStringQuotes: true }) +
> +              ": " +
> +              VariablesView.getString(preview.message, { noStringQuotes: true });
> +    if (!VariablesView.isFalsy({ value: preview.stack })) {

Nit: add a newline before this conditional to make it easier to read.

@@ +3386,5 @@
> +
> +    if (preview.filename) {
> +      msg += "\nlocation: " + preview.filename;
> +      if (preview.lineNumber) {
> +        msg += "@" + preview.lineNumber;

The debugger uses ":" for specifying line numbers, and "@" for function locations. Let's use ":" here as well.

@@ +3460,5 @@
> +            result += " " + name + '="' + escapeHTML(value) + '"';
> +            n++;
> +          }
> +          if (preview.attributesLength > n) {
> +            result += " \u2026";

Localize.

@@ +3472,5 @@
> +        }
> +        return result + ">";
> +      }
> +
> +      default:

DOCUMENT_FRAGMENT_NODES aren't handled by this switch.

@@ +3554,5 @@
> +function escapeHTML(aString) {
> +  return aString.replace(/&/g, "&")
> +                .replace(/"/g, """)
> +                .replace(/</g, "&lt;")
> +                .replace(/>/g, "&gt;");

This is a pretty short list. Might want to change the function description to "Escape *some* HTML special characters" and explain why we do it only for a few cases.

If we'd want to support everything, I'm pretty sure there are better ways to do it.

::: browser/locales/en-US/chrome/browser/devtools/debugger.properties
@@ +263,5 @@
> +# This is a semi-colon list of plural forms.
> +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +# #1 number of remaining items in the object
> +# example: 3 more…
> +variablesViewMoreObjects=#1 more…;#1 more…

Nit: might want to keep these strings along with the other variables view strings, to keep this file structured. I'd place them after globalScopeLabel.

Comment 65

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

Another small thing I found while testing:

1. Go to http://htmlpad.org/debugger/
2. Open debugger
3. Click me

The "this" variable in the local scope is shown as "window debugger", which looks very funky to me. Might want to add a separator between them, like → or something, it can be confusing on certain pages.

Comment 66

[Mihai Sucan [:msucan]]

Attachment: part 2 - objectactor changes - wip8

Addressed Victor's review comment about adding pretty output for DOM document fragments.

Comment 67

[Mihai Sucan [:msucan]]

Attachment: part 3 - pretty output for objects (text only) - wip9

Addressed Victor's review comments.

Comment 68

[Mihai Sucan [:msucan]]

Attachment: part 4 - tests - wip4

Changes:

- removed my changes to the JS builtin Intl.js self-hosted script, based on Jeff's review. The debugger xpcshell tests pass with his patch as well.

- fixes for tests based on the updated part 3 patch.

- more tests for more kinds of objects.

Last week's try push was surprisingly green.

Comment 69

[Mihai Sucan [:msucan]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #62)
> Comment on attachment 8347440 [details] [diff] [review]
> part 4 - tests - wip3
> 
> Review of attachment 8347440 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/builtin/Intl.js
> @@ +2302,5 @@
> >      if ((required === "date" || required === "any") &&
> > +        (typeof options.weekday !== "undefined" ||
> > +         typeof options.year !== "undefined" ||
> > +         typeof options.month !== "undefined" ||
> > +         typeof options.day !== "undefined"))
> 
> This isn't equivalent to the previous code.  The difference occurs when the
> value of any of these properties is an object that emulates |undefined| --
> that is, an object whose |typeof| is "undefined" yet still is an object. 
> (In the browser this is only |document.all|, which makes it very hard to
> demonstrate the bug, but I think it's possible with enough effort.)  Here's
> a JS shell testcase (you can test with $objdir/dist/bin/js in a browser
> build):
> 
> [...]

Thanks for your quick review! I wasn't sure this code tries to distinguish between objects that emulate undefined and |undefined|.

> Rather than "fix" this code this way, we should fix the engine to not warn
> about it.  It's a one-liner -- I'll post in a second.

Thank you very much for the quick fix!

Comment 70

[Mihai Sucan [:msucan]]

(In reply to Victor Porof [:vp] from comment #64)
> @@ +3160,5 @@
> > + * given the same two arguments as those given to VariablesView.getString().
> > + */
> > +VariablesView.stringifiers = {};
> > +
> > +VariablesView.stringifiers.byType = {
> 
> I really really like all of this. Very nicely written code.

Thanks!


> @@ +3169,5 @@
> > +    return uneval(aGrip);
> > +  },
> > +
> > +  longString: function({initial}, {noStringQuotes, noEllipsis}) {
> > +    let ellipsis = noEllipsis ? "" : "\u2026";
> 
> Localize this by using nsIPrefLocalizedString.
> 
> I'm adding the following in bug 830344, you can race me :)
> https://bugzilla.mozilla.org/attachment.cgi?id=8347598&action=diff#a/browser/
> devtools/shared/widgets/VariablesView.jsm_sec9
> 
> // An ellipsis symbol (usually "…") used for localization.
> XPCOMUtils.defineLazyGetter(Scope, "ellipsis", () =>
>   Services.prefs.getComplexValue("intl.ellipsis",
> Ci.nsIPrefLocalizedString).data);

Ah, nice thing! Added this in the new patch.


> @@ +3282,5 @@
> > +      key = VariablesView.getString(key, {
> > +        concise: true,
> > +        noStringQuotes: true,
> > +      });
> > +      value = VariablesView.getString(value, { concise: true });
> 
> Nit: please declare new variables for key and value.

Fixed.


> @@ +3287,5 @@
> > +      entries.push(key + ": " + value);
> > +    }
> > +
> > +    if (typeof preview.size == "number" && preview.size > entries.length) {
> > +      entries.push((preview.size - entries.length) + " more\u2026");
> 
> Localize the ellipsis.

Eh, I had the _getNMoreString() function but somehow I missed updating this line of code. Good catch! Fixed.


> @@ +3305,5 @@
> > +  ObjectWithURL: function(aGrip, {concise}) {
> > +    let result = aGrip.class;
> > +    let url = aGrip.preview.url;
> > +    if (!VariablesView.isFalsy({ value: url })) {
> > +      result += " " + WebConsoleUtils.abbreviateSourceURL(url,
> 
> The debugger has its own SourceUtils as well. Can you please file a followup
> about moving some of these WebConsoleUtils and the debugger's into a more
> shared helper?
> 
> https://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/
> debugger-panes.js#1171
> 
> Ideally, we should be reusing the same methods everywhere.

Good point. Filed bug 950879.


> @@ +3364,5 @@
> > +    let {preview} = aGrip;
> > +    let msg = VariablesView.getString(preview.name, { noStringQuotes: true }) +
> > +              ": " +
> > +              VariablesView.getString(preview.message, { noStringQuotes: true });
> > +    if (!VariablesView.isFalsy({ value: preview.stack })) {
> 
> Nit: add a newline before this conditional to make it easier to read.

Done.


> @@ +3386,5 @@
> > +
> > +    if (preview.filename) {
> > +      msg += "\nlocation: " + preview.filename;
> > +      if (preview.lineNumber) {
> > +        msg += "@" + preview.lineNumber;
> 
> The debugger uses ":" for specifying line numbers, and "@" for function
> locations. Let's use ":" here as well.

Fixed.


> @@ +3460,5 @@
> > +            result += " " + name + '="' + escapeHTML(value) + '"';
> > +            n++;
> > +          }
> > +          if (preview.attributesLength > n) {
> > +            result += " \u2026";
> 
> Localize.

Fixed.


> @@ +3472,5 @@
> > +        }
> > +        return result + ">";
> > +      }
> > +
> > +      default:
> 
> DOCUMENT_FRAGMENT_NODES aren't handled by this switch.

Fixed.


> @@ +3554,5 @@
> > +function escapeHTML(aString) {
> > +  return aString.replace(/&/g, "&")
> > +                .replace(/"/g, """)
> > +                .replace(/</g, "&lt;")
> > +                .replace(/>/g, "&gt;");
> 
> This is a pretty short list. Might want to change the function description
> to "Escape *some* HTML special characters" and explain why we do it only for
> a few cases.
> 
> If we'd want to support everything, I'm pretty sure there are better ways to
> do it.

Updated the comment.


> ::: browser/locales/en-US/chrome/browser/devtools/debugger.properties
> @@ +263,5 @@
> > +# This is a semi-colon list of plural forms.
> > +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> > +# #1 number of remaining items in the object
> > +# example: 3 more…
> > +variablesViewMoreObjects=#1 more…;#1 more…
> 
> Nit: might want to keep these strings along with the other variables view
> strings, to keep this file structured. I'd place them after globalScopeLabel.

Done.


Thank you for the r+.

Comment 71

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #63)
> For some reason, in a browser that assertEq(str, "Saturday") is "Friday"
> instead.  It's possible I'm just screwing something up locally, tho -- if a
> try run comes back positive on this I'll run with it, otherwise I'll change
> to assertEq(/day$/.test(str), true) instead.

Horrifying. Rounding error maybe? If so, I think it would be a bug. Does it reproduce for you? A solid bug report for that would be brilliant.

As for the particular test, I don't suppose it's possible to make the [[DateValue]] of the date in this test not rely on anything machine-dependent? Couldn't think of anything myself. Bletch.

Comment 72

[Jason Orendorff [:jorendorff]]

Comment on attachment 8347692 [details] [diff] [review]
Don't warn for comparisons against |undefined| in self-hosted code

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

I think it would be better if document.all didn't fool self-hosted code at all. That is, Detecting should just return false.

Clearing the review flag for now.

Comment 73

[Mihai Sucan [:msucan]]

Comment on attachment 8348280 [details] [diff] [review]
part 2 - objectactor changes - wip8

If I am not mistaken, Jim seems to be on vacation. Asking Panos for a check on this patch. Thank you!

Comment 74

[Mihai Sucan [:msucan]]

Attachment: part 3 - pretty output for objects (text only) - wip10

Minor change for the updated part 4 patch that I will attach in a few moments.

Comment 75

[Mihai Sucan [:msucan]]

Attachment: part 4 - tests - wip5

Added more tests for the kinds of objects we have pretty output for.

I refactored the browser_webconsole_bug_598357_jsterm_output.js test file and I did split into 5 tests.

This is ready for review.

New try push: https://tbpl.mozilla.org/?tree=Try&rev=db9b2ec09445

Comment 76

[Jeff Walden [:Waldo]]

Attachment: Retry?

Comment 77

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

Bug 830344 just landed, Mihai, so you can remove the Scope.ellipsis addition.

Comment 78

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8348280 [details] [diff] [review]
part 2 - objectactor changes - wip8

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

I have some comments and questions, but this looks fine to me overall.

::: toolkit/devtools/server/actors/script.js
@@ +3063,5 @@
> +    try {
> +      names = aObject.getOwnPropertyNames()
> +    } catch (ex) {
> +      // Calling getOwnPropertyNames() on wrapped native prototypes is not
> +      // allowed: "cannot modify properties of a WrappedNative". See bug 560072.

Wait, it was getOwnPropertyDescriptor that was known to throw, wasn't it? Moreover bug 560072 is now fixed, so we should be removing these checks. If these methods still throw, we need to file a new bug if we want it fixed.

@@ +3072,5 @@
>        try {
>          desc = aObject.getOwnPropertyDescriptor(name);
>        } catch (e) {
>          // Calling getOwnPropertyDescriptor on wrapped native prototypes is not
>          // allowed (bug 560072).

That's right, I'm not senile yet :-) Can you try to remove this try/catch?

@@ +3136,5 @@
>     * @param string aName
>     *        The property that the descriptor is generated for.
> +   * @param boolean aOnlyEnumerable
> +   *        True if you want a descriptor only for an enumerable property, false
> +   *        otherwise.

Since the code only checks aOnlyEnumerable for a falsy value, we can mark it as optional.

@@ +3265,5 @@
>  
>  /**
> + * Functions for adding information to ObjectActor grips for the purpose of
> + * having customized output. This object holds arrays mapped by
> + * DebuggerObject.class.

This is actually Debugger.Object.prototype.class.

@@ +3304,5 @@
> +      dumpn(e);
> +    }
> +
> +    if (userDisplayName && userDisplayName.value &&
> +        typeof userDisplayName.value == "string") {

This should be enough, shouldn't it?

if (userDisplayName && typeof userDisplayName.value == "string")

@@ +3309,5 @@
> +      aGrip.userDisplayName = threadActor.createValueGrip(userDisplayName.value);
> +    }
> +
> +    return true;
> +  }], // Function

In some cases you have a trailing comment like this, in others you don't. Personally I wouldn't add them for such blocks that fit in a single page of my editor, but it's up to you.

@@ +3642,5 @@
> +    if (aRawObj instanceof Ci.nsIDOMDocument) {
> +      preview.location = threadActor.createValueGrip(aRawObj.location.href);
> +    }
> +
> +    if (aRawObj instanceof Ci.nsIDOMDocumentFragment) {

Why not use "else if" from this point forward for a tiny speed gain?

@@ +3707,5 @@
> +    if (aRawObj instanceof Ci.nsIDOMMouseEvent) {
> +      props.push("buttons", "clientX", "clientY", "layerX", "layerY");
> +    }
> +
> +    if (aRawObj instanceof Ci.nsIDOMKeyEvent) {

Same here ("else if").

@@ +3809,5 @@
> +    try {
> +      names = obj.getOwnPropertyNames();
> +    } catch (ex) {
> +      // Calling getOwnPropertyNames() on wrapped native prototypes is not
> +      // allowed: "cannot modify properties of a WrappedNative". See bug 560072.

See my previous comment about bug 560072.

@@ +5124,5 @@
>  }
> +
> +/**
> + * Make a debuggee value for the given object, if needed. Primitive values
> + * are left the same.

Can you expand a little bit on the uses for this helper function? It's not clear when it would be useful just from looking at the comment.

Comment 79

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #78)
> Comment on attachment 8348280 [details] [diff] [review]
> part 2 - objectactor changes - wip8
> 
> Review of attachment 8348280 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have some comments and questions, but this looks fine to me overall.
> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +3063,5 @@
> > +    try {
> > +      names = aObject.getOwnPropertyNames()
> > +    } catch (ex) {
> > +      // Calling getOwnPropertyNames() on wrapped native prototypes is not
> > +      // allowed: "cannot modify properties of a WrappedNative". See bug 560072.
> 
> Wait, it was getOwnPropertyDescriptor that was known to throw, wasn't it?
> Moreover bug 560072 is now fixed, so we should be removing these checks. If
> these methods still throw, we need to file a new bug if we want it fixed.

IIANM, that method continues to throw. I had this error with Components.interfaces (iirc).


> @@ +3072,5 @@
> >        try {
> >          desc = aObject.getOwnPropertyDescriptor(name);
> >        } catch (e) {
> >          // Calling getOwnPropertyDescriptor on wrapped native prototypes is not
> >          // allowed (bug 560072).
> 
> That's right, I'm not senile yet :-) Can you try to remove this try/catch?

I will try again. I was surprised, back then, to see the bug was marked as fixed.


> @@ +3136,5 @@
> >     * @param string aName
> >     *        The property that the descriptor is generated for.
> > +   * @param boolean aOnlyEnumerable
> > +   *        True if you want a descriptor only for an enumerable property, false
> > +   *        otherwise.
> 
> Since the code only checks aOnlyEnumerable for a falsy value, we can mark it
> as optional.

Will do.

> @@ +3265,5 @@
> >  
> >  /**
> > + * Functions for adding information to ObjectActor grips for the purpose of
> > + * having customized output. This object holds arrays mapped by
> > + * DebuggerObject.class.
> 
> This is actually Debugger.Object.prototype.class.

Yes, will fix.


> @@ +3304,5 @@
> > +      dumpn(e);
> > +    }
> > +
> > +    if (userDisplayName && userDisplayName.value &&
> > +        typeof userDisplayName.value == "string") {
> 
> This should be enough, shouldn't it?
> 
> if (userDisplayName && typeof userDisplayName.value == "string")

This is a copy/paste of what we already had. Do you want me to allow an empty userDisplayName?


> @@ +3309,5 @@
> > +      aGrip.userDisplayName = threadActor.createValueGrip(userDisplayName.value);
> > +    }
> > +
> > +    return true;
> > +  }], // Function
> 
> In some cases you have a trailing comment like this, in others you don't.
> Personally I wouldn't add them for such blocks that fit in a single page of
> my editor, but it's up to you.

True. Will fix.


> @@ +3642,5 @@
> > +    if (aRawObj instanceof Ci.nsIDOMDocument) {
> > +      preview.location = threadActor.createValueGrip(aRawObj.location.href);
> > +    }
> > +
> > +    if (aRawObj instanceof Ci.nsIDOMDocumentFragment) {
> 
> Why not use "else if" from this point forward for a tiny speed gain?

Will update. This code mostly ended-up like this due to its evolution.


> @@ +5124,5 @@
> >  }
> > +
> > +/**
> > + * Make a debuggee value for the given object, if needed. Primitive values
> > + * are left the same.
> 
> Can you expand a little bit on the uses for this helper function? It's not
> clear when it would be useful just from looking at the comment.

I will try.

Thank you for the review!

Comment 80

[Mihai Sucan [:msucan]]

Previous try push had some failures.

Updated push: https://tbpl.mozilla.org/?tree=Try&rev=d90b2cbd06a0

I will attach updated patches tomorrow, once I get a green try run and address Panos's comments.

Comment 81

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mihai Sucan [:msucan] from comment #79)
> This is a copy/paste of what we already had. Do you want me to allow an
> empty userDisplayName?

Ah, the joys of coercion, I didn't think of the empty string. Can you move the middle part to the end then? It should be more obvious that way that we know it's a string and we test its value. Or even test for the empty string explicitly if you prefer.

Comment 82

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #78)
> Comment on attachment 8348280 [details] [diff] [review]
> part 2 - objectactor changes - wip8
> 
> Review of attachment 8348280 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have some comments and questions, but this looks fine to me overall.
> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +3063,5 @@
> > +    try {
> > +      names = aObject.getOwnPropertyNames()
> > +    } catch (ex) {
> > +      // Calling getOwnPropertyNames() on wrapped native prototypes is not
> > +      // allowed: "cannot modify properties of a WrappedNative". See bug 560072.
> 
> Wait, it was getOwnPropertyDescriptor that was known to throw, wasn't it?
> Moreover bug 560072 is now fixed, so we should be removing these checks. If
> these methods still throw, we need to file a new bug if we want it fixed.

Just tested. It seems that for obj.getOwnPropertyNames(), at least, we still get an exception for some objects, eg. for Components.utils.

I filed bug 952093 for this problem.

Comment 83

[Jason Orendorff [:jorendorff]]

Comment on attachment 8349044 [details] [diff] [review]
Retry?

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

Sorry for the slow review here.

I think it would be even better to put this new test in GetPropertyHelperInline, with all the other "Don't warn if" cases.

If you feel like factoring out the unlikely parts of that block into a separate MaybeWarnAboutUndefinedProperty function, feel free... it'd be nice for the function to fit on a screen and a lot of that is totally peripheral to what it's trying to do.

Comment 84

[Mihai Sucan [:msucan]]

Attachment: part 2 - objectactor changes - wip9

Rebased and updated to address review comments from Panos. Thank you!

Comment 85

[Mihai Sucan [:msucan]]

Attachment: part 3 - pretty output for objects (text only) - wip11

Rebased the patch.

Comment 86

[Mihai Sucan [:msucan]]

Attachment: part 4 - tests - wip6

Updated the tests to fix some oranges. Had a green try push yesterday.

Here is a third try push that I just sent to the servers, to include today's changes: https://tbpl.mozilla.org/?tree=Try&rev=01707f4ad071

This should be ready to land, once reviewed. Thank you!

Comment 87

[Mihai Sucan [:msucan]]

(In reply to Jason Orendorff [:jorendorff] from comment #83)
> Comment on attachment 8349044 [details] [diff] [review]
> Retry?
> 
> Review of attachment 8349044 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> [...]

Jason and Jeff: thank you very much for the proper fix.

Comment 88

[Mihai Sucan [:msucan]]

Comment on attachment 8350118 [details] [diff] [review]
part 4 - tests - wip6

Brandon is on PTO. Nick volunteered for the review - thank you!

Comment 89

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8350118 [details] [diff] [review]
part 4 - tests - wip6

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

r+ with comments adressed and follow ups filed!

::: browser/devtools/webconsole/test/browser_webconsole_output_01.js
@@ +9,3 @@
>  
>  let dateNow = Date.now();
> +let DebuggerServer = Cu.import("resource://gre/modules/devtools/dbg-server.jsm", {}).DebuggerServer;

Nit:

  let { DebuggerServer } = Cu.import("...", {});

@@ +9,5 @@
>  
>  let dateNow = Date.now();
> +let DebuggerServer = Cu.import("resource://gre/modules/devtools/dbg-server.jsm", {}).DebuggerServer;
> +let longString = (new Array(DebuggerServer.LONG_STRING_LENGTH + 4)).join("a");
> +let initialString = longString.substring(0, DebuggerServer.LONG_STRING_INITIAL_LENGTH);

To make debugging this test and looking at its logs more sane, it could be useful to set LONG_STRING_LENGTH to something small like 50 just for this test. I've done this in other tests, and it's been helpful for the log's readability.

@@ +21,5 @@
> +//
> +// - inspectable: boolean, tells if the result is expected to be an object you
> +// can click on to inspect.
> +//
> +// - noClick: boolean, tells the test runner to not click the JS eval result.

"when true, the test runner does not click the JS eval result." <-- this let's people know down the line whether to use true or false to suppress a click, which is helpful when skimming IMO.

@@ +156,2 @@
>  
> +  function runner()

|function*| since it is a generator.

@@ +170,3 @@
>    }
>  
> +  function checkInput(i, entry)

function* here too.

::: browser/devtools/webconsole/test/browser_webconsole_output_02.js
@@ +14,5 @@
> +//
> +// - output: string|RegExp, expected JS eval result.
> +//
> +// - inspectable: boolean, tells if the result is expected to be an object you
> +// can click on to inspect.

See last comment I made about documenting boolean parameters.

Would it make sense to move this declarative console output test runner thing-y into head.js and consolidate all documentation there? If so, do it as a follow up. No need to block this from landing because of that.

@@ +179,5 @@
> +    browser.removeEventListener("load", onLoad, true);
> +    Task.spawn(runner);
> +  }, true);
> +
> +  function runner()

function*

----------------------------

Yeah this is all basically (exactly?) the same as the -01 test, right? Should definitely move the runner into head js, consolidate documentation, and just call it from each of the actual tests. As a follow up.

@@ +194,5 @@
> +    inputTests = null;
> +    finishTest();
> +  }
> +
> +  function checkInput(i, entry)

function*

Also, this function is freaking huge. Can we break it down into helpers and sub-functions a bit?

::: browser/devtools/webconsole/test/browser_webconsole_output_03.js
@@ +1,4 @@
> +/*
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */

I think every comment I've made on the previous output tests also applies here.

::: browser/devtools/webconsole/test/browser_webconsole_output_04.js
@@ +1,4 @@
> +/*
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */

ditto et all

::: browser/devtools/webconsole/test/browser_webconsole_output_events.js
@@ +15,5 @@
> +    browser.removeEventListener("load", onLoad, true);
> +    Task.spawn(runner);
> +  }, true);
> +
> +  function runner()

function*

::: browser/devtools/webconsole/test/head.js
@@ +8,5 @@
>  (() => {
>    gDevTools = Cu.import("resource:///modules/devtools/gDevTools.jsm", {}).gDevTools;
>    console = Cu.import("resource://gre/modules/devtools/Console.jsm", {}).console;
>    promise = Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js", {}).Promise;
> +  Task = Cu.import("resource://gre/modules/Task.jsm", {}).Task;

Whoa... shouldn't all of these be declared?

Also, destructuring cuts down on chars and is more readable IMO. Pretty sure we agreed that destructuring imports was a Good Thing(tm) at the last work week.

::: browser/devtools/webconsole/test/test-console-output.html
@@ +11,5 @@
> +<body>
> +  <p>hello world!</p>
> +  <script type="text/javascript">
> +
> +// For test browser_webconsole_output_02.js:

I'm going to tentatively suggest that we split this file into test-console-output-0N.html files for readability and maintainability for when someone else has to come in and debug these tests. Having stuff for other tests just adds noise for them to sift through.

If you really disagree, then whatever don't do it, but if you don't feel strongly, then please do.

Comment 91

[Mihai Sucan [:msucan]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #89)
> Comment on attachment 8350118 [details] [diff] [review]
> part 4 - tests - wip6
> 
> Review of attachment 8350118 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments adressed and follow ups filed!

Thank you for the really quick review! Really good suggestions.


> ::: browser/devtools/webconsole/test/browser_webconsole_output_01.js
> @@ +9,3 @@
> >  
> >  let dateNow = Date.now();
> > +let DebuggerServer = Cu.import("resource://gre/modules/devtools/dbg-server.jsm", {}).DebuggerServer;
> 
> Nit:
> 
>   let { DebuggerServer } = Cu.import("...", {});

Fixed.


> @@ +9,5 @@
> >  
> >  let dateNow = Date.now();
> > +let DebuggerServer = Cu.import("resource://gre/modules/devtools/dbg-server.jsm", {}).DebuggerServer;
> > +let longString = (new Array(DebuggerServer.LONG_STRING_LENGTH + 4)).join("a");
> > +let initialString = longString.substring(0, DebuggerServer.LONG_STRING_INITIAL_LENGTH);
> 
> To make debugging this test and looking at its logs more sane, it could be
> useful to set LONG_STRING_LENGTH to something small like 50 just for this
> test. I've done this in other tests, and it's been helpful for the log's
> readability.

Good point. Fixed.


> @@ +21,5 @@
> > +//
> > +// - inspectable: boolean, tells if the result is expected to be an object you
> > +// can click on to inspect.
> > +//
> > +// - noClick: boolean, tells the test runner to not click the JS eval result.
> 
> "when true, the test runner does not click the JS eval result." <-- this
> let's people know down the line whether to use true or false to suppress a
> click, which is helpful when skimming IMO.

Updated.


> @@ +156,2 @@
> >  
> > +  function runner()
> 
> |function*| since it is a generator.

Nice. When I first read about these, I saw the function* wasn't yet supported. Fixed now.


> ::: browser/devtools/webconsole/test/browser_webconsole_output_02.js
> @@ +14,5 @@
> > +//
> > +// - output: string|RegExp, expected JS eval result.
> > +//
> > +// - inspectable: boolean, tells if the result is expected to be an object you
> > +// can click on to inspect.
> 
> See last comment I made about documenting boolean parameters.
> 
> Would it make sense to move this declarative console output test runner
> thing-y into head.js and consolidate all documentation there? If so, do it
> as a follow up. No need to block this from landing because of that.

It does make sense, yes. No need for a followup. Fixed.

This was really an oversight. I started out with a huge test file, then I've split it into multiple files, without moving the generic code into head.js. I wasn't even sure it will turn out to be so much of reusable code.


> @@ +194,5 @@
> > +    inputTests = null;
> > +    finishTest();
> > +  }
> > +
> > +  function checkInput(i, entry)
> 
> function*
> 
> Also, this function is freaking huge. Can we break it down into helpers and
> sub-functions a bit?

True. Fixed.


> ::: browser/devtools/webconsole/test/head.js
> @@ +8,5 @@
> >  (() => {
> >    gDevTools = Cu.import("resource:///modules/devtools/gDevTools.jsm", {}).gDevTools;
> >    console = Cu.import("resource://gre/modules/devtools/Console.jsm", {}).console;
> >    promise = Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js", {}).Promise;
> > +  Task = Cu.import("resource://gre/modules/Task.jsm", {}).Task;
> 
> Whoa... shouldn't all of these be declared?

They are, see above, in the head.js file. Nonetheless, in the updated patch I'm making this slightly better.

> Also, destructuring cuts down on chars and is more readable IMO. Pretty sure
> we agreed that destructuring imports was a Good Thing(tm) at the last work
> week.

Fixed. This code was added quite some time ago...

> ::: browser/devtools/webconsole/test/test-console-output.html
> @@ +11,5 @@
> > +<body>
> > +  <p>hello world!</p>
> > +  <script type="text/javascript">
> > +
> > +// For test browser_webconsole_output_02.js:
> 
> I'm going to tentatively suggest that we split this file into
> test-console-output-0N.html files for readability and maintainability for
> when someone else has to come in and debug these tests. Having stuff for
> other tests just adds noise for them to sift through.
> 
> If you really disagree, then whatever don't do it, but if you don't feel
> strongly, then please do.

Fixed.

Will submit the updated patch in a few moments.

Comment 92

[Mihai Sucan [:msucan]]

Attachment: part 4 - tests - wip7

Updated the patch to address Nick's review.

Last try push was green.

New try push: https://tbpl.mozilla.org/?tree=Try&rev=c0eb7f9a8ac3

I plan to land these patches tomorrow if this try run is green again.

Comment 93

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c75e5f21cd86 for the JS engine change.

Comment 94

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 95

[Mihai Sucan [:msucan]]

Attachment: part 4 - tests - wip8

Had a minor orange in a test. Fixed now.

New try push looks green: https://tbpl.mozilla.org/?tree=Try&rev=84235fe48243

Comment 96

[Mihai Sucan [:msucan]]

Landed in fx-team:

https://hg.mozilla.org/integration/fx-team/rev/7b1a9fc65b07
https://hg.mozilla.org/integration/fx-team/rev/3ad3e3d57534
https://hg.mozilla.org/integration/fx-team/rev/76b62f2579ba

Thanks to everyone!

Comment 97

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7b1a9fc65b07
https://hg.mozilla.org/mozilla-central/rev/3ad3e3d57534
https://hg.mozilla.org/mozilla-central/rev/76b62f2579ba