1286259 - Reps: grip-array rep should support limited preview

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P1)

Description

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

GripArray is limiting number of items in long mode to 300, but number of items in grip-preview is limited to 10.

This causes that the following array:
let arr = ["0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10"];

is rendered as follows in long mode:
["0", "1", "2", "3", "4", "5", "6", "7", "8", "9"]

The last "10" element is missing and there is no "more..." label.


The rep should:
1) look at grip.preview.length -> to see the total number of items
2) and display 'more...' if grip.preview.length > than the actual number of items in the preview.

---

Additionally (not as part of this bug) it could be great if the user can load the rest (by issuing 'getPrototypeAndProperties' packet) without going to the Variables view...

In case of the HTTP Inspector (the Console panel) there is a 'more...' link that allows to fetch the rest of the response body if it's bigger than LongStringGrip max limit.

Honza

Comment 1

[[e-mail]]

took it. 

> it could be great if the user can load the rest (by issuing 'getPrototypeAndProperties' packet) without going to the Variables view...

Sounds cool, can you point me any example that already provide such feature in devtools?

Comment 2

[[e-mail]]

Attachment: right result

I took the example on console and the result looks right to me

Comment 3

[[e-mail]]

Ooh it's object grip-array, I'll add related test in test_reps_grip-array.html to make sure things works well.

Comment 4

[[e-mail]]

Attachment: 0001-Bug-1286259-Reps-grip-array-rep-should-support-limit.patch

Comment 5

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

Comment on attachment 8771212 [details] [diff] [review]
0001-Bug-1286259-Reps-grip-array-rep-should-support-limit.patch

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

I don't see the `more...` link when testing this in RepTester.

Home come that the test passes...?

Honza

::: devtools/client/shared/components/reps/grip-array.js
@@ +50,5 @@
>          return items;
>        }
>  
> +      if (grip.preview.length) {
> +        max = max > grip.preview.length ? grip.preview.length : max;

This is wrong. You need to check (and use) number of items available in the preview (grip.preview.items.length)

@@ +91,4 @@
>          }
>        }
>  
> +      if (array.length > max) {

This also needs to use the preview.items array.

Comment 6

[Steve Chung [:steveck]]

Hi Fred, it's just a reminder that I'll also fix the length limit issue in Bug 1285530 and it might have some conflict wit your patch. Sorry if it cause any inconvenience to you.

Comment 7

[[e-mail]]

Honza said in comment 1
> number of items in grip-preview is limited to 10.

I can't neither find `grip-preview` related code and its limit, do you know how to reproduce this issue?

Comment 8

[Lin Clark [:linclark]]

It was a typo. It should be a '.', not a '-'.

Search for grip.preview in grip-array.js

Comment 9

[[e-mail]]

Attachment: rep result

seems the rep result already shows correctly in rep tester, should I just resolve this issue?

Comment 10

[Lin Clark [:linclark]]

Let's wait for Honza's opinion before closing this out.

Comment 11

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

(In reply to Fred Lin [:gasolin] from comment #9)
> Created attachment 8777216 [details]
> rep result
> 
> seems the rep result already shows correctly in rep tester, should I just
> resolve this issue?
I can still see the problem in rep-tester.

When evaluating `["0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10"]` using `long` mode, the result is:

`Array["0", "1", "2", "3", "4", "5", "6", "7", "8", "9"]`

While it should be:

`Array["0", "1", "2", "3", "4", "5", "6", "7", "8", "9", 1 more…]`

(In reply to Lin Clark [:linclark] from comment #8)
> It was a typo. It should be a '.', not a '-'.
Correct, sorry for confusion.

Honza

Comment 12

[[e-mail]]

thanks, I can reproduce it now!

Comment 13

[[e-mail]]

Attachment: Bug 1286259 - Reps: grip-array rep should support limited preview;

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

Comment 14

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

Comment on attachment 8779956 [details]
Bug 1286259 - Reps: grip-array rep should support limited preview;

https://reviewboard.mozilla.org/r/70818/#review68680

Looks good to me now!

R+ assuming all tests are green.

Thanks for working on this!
Honza

Comment 15

[[e-mail]]

Thanks for review!

tree herder green

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fccbaff0ea6f
Reps: grip-array rep should support limited preview; r=Honza

Comment 17

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

I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1936502&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/b0604cd3b072

Comment 18

[[e-mail]]

Bug 1284855 had add extra space between braces, will update the test case to reflect the change

Comment 19

[[e-mail]]

Attachment: 0001-Bug-1286259-Reps-grip-array-rep-should-support-limit.patch

The only change is add extra space for test case to pass all tests, so carry r+ here.

change format because mozreview push shows `Parent review request is submitted or discarded` message

Comment 20

[[e-mail]]

Please help check-in 0001-Bug-1286259-Reps-grip-array-rep-should-support-limit.patch

thanks

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/cc8350ff1a9b
Reps: grip-array rep should support limited preview, add extra space; r=Honza

Comment 22

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

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