1264693 - [rep tests] Add tests for object rep

Status: RESOLVED FIXED

(DevTools :: Shared Components, enhancement, P1)

Description

[Lin Clark [:linclark]]

See Bug 1257552

Comment 1

[Lin Clark [:linclark]]

Under what circumstances would Rep return the Object rep? For any objects that the console is working with, I think it would always result in the Grip rep.

Comment 2

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

Yes, the Console never uses/renders JS objects directly and only access it remotely through grips.

I don't know if this counts, but: e.g. The HTTP Inspector uses it for rendering JSON HTTP responses. JSON HTTP response is parsed into an object on the client side and rendered using Object rep.

Honza

Comment 3

[Lin Clark [:linclark]]

Attachment: Bug1264693.patch

It looks like this is still outputing Object{props} instead of having a space between the title and props, e.g. Object {props}. It also looks like the properties are being reordered in an unexpected way in testNested (unless I misunderstand).

I think both of those can be addressed in follow-ups.

Comment 4

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

Comment on attachment 8765546 [details] [diff] [review]
Bug1264693.patch

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

Looks good to me. There is one comment, please fix, otherwise R+

Thanks Lin for doing these tests!
Honza

::: devtools/client/shared/components/test/mochitest/test_reps_object.html
@@ +158,5 @@
> +        arr: [2]
> +      },
> +      strProp: "test string",
> +      arrProp: [1]
> +    };

Please put at least 4 items into the `arr` and `arrProp`. In such case we can also cover what mode is actually used for inner objects/arrays. And let it pass...

Currently Grip rep and GripArray rep are both using 'tiny' for inner props/items, but Obj rep and Array rep are not doing this. I am not entirely sure what's the right approach here. Using 'tiny' seems to restrictive 'short' renders more info. But at least we should be consistent (across local and remote objects).

The difference is as follows:

An example object:

let obj = {
  p1: "John",
  p2: [1,2,3,4],
  p3: true
}

// Inner items are using 'tiny' - see the `p2` array
Object {p1: "John", p3: true, p2: [3]}

// Inner items are not using 'tiny' - see the `p2` array
// But it's using 'long' mode!
Object {p1: "John", p3: true, p2: [1, 2, 3, 4]}

Please create a report for this, the inner mode should definitely not be 'long'. I'll fix it and adopt this test.

Honza

Comment 5

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

(In reply to Lin Clark [:linclark] from comment #3)
> Created attachment 8765546 [details] [diff] [review]
> Bug1264693.patch
> 
> It looks like this is still outputing Object{props} instead of having a
> space between the title and props, e.g. Object {props}.
Please report it and I'll fix it (and I'll also adopt this test)

> It also looks like
> the properties are being reordered in an unexpected way in testNested
> (unless I misunderstand).
This is what I explained here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1264676#c2
... and yes it should be fixed. Please make sure there is a bug reported and I can fix it.

This ordering issue might be covered in this test (not by test for the Array rep). I'll let you to decide.

> I think both of those can be addressed in follow-ups.
Yes

Honza

Comment 6

[Lin Clark [:linclark]]

Attachment: Bug 1264693 - [rep tests] Add tests for object rep.

Review commit: https://reviewboard.mozilla.org/r/63704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63704/

Comment 7

[Lin Clark [:linclark]]

Comment on attachment 8770139 [details]
Bug 1264693 - [rep tests] Add tests for object rep.

https://reviewboard.mozilla.org/r/63704/#review60654

Carrying over review from patch

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/fcb309979c71
[rep tests] Add tests for object rep. r=Honza

Comment 9

[Carsten Book [:Tomcat]]

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