1275556 - Reps: Obj rep should support 'tiny' mode

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P1)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Lin Clark [:linclark] from comment #5)
> Right, sorry if my wording of that question confused.
> 
> As I said above, Nicholas's work would make it output:
> 
> > Object { objProp: Object {}, strProp: "test string" }
> 
> But this is not what we want... we want it to just be object, correct?
Yes, correct.

Honza

Comment 1

[Brian Grinstead [:bgrins]]

OK, sounds like with the planned updates doing console.log({objProp: {id: 1}, strProp: "test string"}) will continue to output:

Object { objProp: Object, strProp: "test string" }

That works for me.

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1275556.patch

I am updating the patch so, `Object` is rendered.
Not that this will also affect JSON rendering (JSON view and JSON HTTP responses in the Console panel).

Honza

Comment 3

[Lin Clark [:linclark]]

Comment on attachment 8756389 [details] [diff] [review]
bug1275556.patch

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

I didn't get a chance to manually test this, but it looks good to me.

Comment 4

[Lin Clark [:linclark]]

Forgot to mention, we should make sure this passes tests before committing. I'm not sure if any of the JSON viewer or DOM Panel tests depends on the current output.

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The Obj rep should support 'tiny' mode just like the 'Grip' rep does.

Honza

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1275556.patch

Lin, here is a patch adding support for 'tiny' mode to the Obj rep.

We might want to yet discuss/change how JS objects are rendered:

Two options:

`{}` - used by Reps atm (and Firebug), it's closer to JS syntax and requires less space (I recall the FB users required that).

`Object` - this is used by the Console panel atm and Chrome devtools use it too.

Honza

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Brian, what do you think about JS object representation?
See the comment above.

Honza

Comment 8

[Lin Clark [:linclark]]

Brian, for some more context, let's say we have this object... {objProp: {id: 1}, strProp: "test string"}

---

here's what we currently show in console:

> Object { objProp: Object, strProp: "test string" }

Here's what we would show with Reps as they are:

> { objProp: {}, strProp: "test string" }

And here's what we would show with the changes Nicholas is putting in to place in https://github.com/bgrins/gecko-dev/pull/85:

> Object { objProp: Object {}, strProp: "test string" }

---

In the meeting this morning, we were leaning towards handling it as we currently do (which matches Chrome). Honza, given the work that Nicholas is doing, this would mean that in tiny mode the grip would only output its title (which is linked), right?

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Lin Clark [:linclark] from comment #3)
> Honza, given the work that Nicholas is
> doing, this would mean that in tiny mode the grip would only output its
> title (which is linked), right?

Nicholas's patch is changing getTitle() method that is used in 'tiny' and other modes. So, it would output `Object {}` - where `Object` is a link.

See the following code that renders Grip rep. The 'tiny' mode is using getTitle() and `{}` string.

https://github.com/bgrins/gecko-dev/blob/console-frontend/devtools/client/shared/components/reps/grip.js#L142

Honza

Comment 10

[Lin Clark [:linclark]]

Right, sorry if my wording of that question confused.

As I said above, Nicholas's work would make it output:

> Object { objProp: Object {}, strProp: "test string" }

But this is not what we want... we want it to just be object, correct?

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1275556.patch

(In reply to Lin Clark [:linclark] from comment #10)
> Forgot to mention, we should make sure this passes tests before committing.
> I'm not sure if any of the JSON viewer or DOM Panel tests depends on the
> current output.
All JSON and DOM tests pass for me, and here is a link to try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b6e5510862f

I changed one thing in the patch. Obj.getTitle() should not use `object.class` since the `object` is not a grip and so, "Object" title should always be used.

Honza

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

New try push (the previous has wrong commit message)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=603a3056b97e

Honza

Comment 13

[Lin Clark [:linclark]]

I downloaded the previous patch (the one that I reviewed yesterday) to use with tests. I noticed that nested objects are rendering like this.

> Object{objProp: Object, strProp: "test string"}

I think we need a space in there after the first title.

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1275556.patch

(In reply to Lin Clark [:linclark] from comment #13)
> I downloaded the previous patch (the one that I reviewed yesterday) to use
> with tests. I noticed that nested objects are rendering like this.
> 
> > Object{objProp: Object, strProp: "test string"}
> 
> I think we need a space in there after the first title.
Agreed, done.

Honza

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

There are clipboard failures, but otherwise it looks good.

Honza

Comment 16

[Pulsebot]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5f457ebb26a9