Currently, if you have a grip array of 301 items, all 301 show up even though there is a limit of 300. When you have 302, then you get an array of 300 with the more caption. This isn't a huge problem, but is a little confusing.

Hi Lin, Seems like it only need to modify this part[1] to show the caption for length = max + 1 case, but it'll also affect the short array render mode(max length is 3). Could I assume that we should change the short mode as well? BTW, I couldn't find any existing test about the array reps. Should I add another identical test file for array, or add the test to the existing test file? And please let me know if I'm wrong about the testing, thanks! [1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/reps/array.js#63

Honza would probably be able to help more on this issue because he ran into it before. For the test for array reps, that should land today in Bug 1264676.

(In reply to Steve Chung [:steveck] from comment #1) > Hi Lin, > > Seems like it only need to modify this part[1] to show the caption for > length = max + 1 case, but it'll also affect the short array render mode(max > length is 3). Could I assume that we should change the short mode as well? Yes, looks like the place we need to change. Btw. for me both 'short' and 'long' modes are broken. I.e. there must be 5 items to see the 'more...' link in the short mode (instead of 4). Also note that there are two array reps: 1) ArrayRep -> for JS arrays that are directly accessible (living in the same process) 2) GripArray -> for JS arrays that are accessible remotely through RDP and grips. This case is related to #2, but I think we should fix both. > BTW, I couldn't find any existing test about the array reps. Should I add > another identical test file for array, or add the test to the existing test > file? And please let me know if I'm wrong about the testing, thanks! The test for GripArray just landed. look for test_reps_grip-array.html in devtools/client/shared/components/test directory. This test should be extended to cover also this reported problem. There is also test for ArrayRep (see test_reps_array.html, just landed) this should also be extended. Honza

Steve, I am assigning this to you so, it appears nicely on the tracking board. Honza

Hi Honza, Thanks for the suggestions, here is the patch about array/grip array length fixing for both short and long render mode. Will ask for review once the permission setup for Mozreview is ready(Still waiting for level 1 access...).

Comment on attachment 8770452 [details] [diff] [review] Bug-1285530.patch Review of attachment 8770452 [details] [diff] [review]: ----------------------------------------------------------------- Looks good R+, just please fix the two comments I've made. Thanks, Honza ::: devtools/client/shared/components/test/mochitest/test_reps_array.html @@ +18,5 @@ > let { Rep } = browserRequire("devtools/client/shared/components/reps/rep"); > let { ArrayRep } = browserRequire("devtools/client/shared/components/reps/array"); > > let componentUnderTest = ArrayRep; > + const maxlength = { Please use camelCase notation: maxlength => maxLength ::: devtools/client/shared/components/test/mochitest/test_reps_grip-array.html @@ +18,5 @@ > let { Rep } = browserRequire("devtools/client/shared/components/reps/rep"); > let { GripArray } = browserRequire("devtools/client/shared/components/reps/grip-array"); > > let componentUnderTest = GripArray; > + const maxlength = { The same, camelCase should be used.

Also, since you are working on Array reps, can you please take a look at Bug 1286259 Honza

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

Comment on attachment 8771218 [details] Bug 1285530 - Reps: Off by one error in grip-array max length. https://reviewboard.mozilla.org/r/64482/#review61566

Note that I there is a collision with bug 1286259. I can't apply if patch from bug 1286259 is in the tree. Please synchronize this with Fred. Honza

Comment on attachment 8771218 [details] Bug 1285530 - Reps: Off by one error in grip-array max length. https://reviewboard.mozilla.org/r/64482/#review61568 R- in the end, see my inline comment. Honza ::: devtools/client/shared/components/test/mochitest/test_reps_array.html:102 (Diff revision 1) > > testRepRenderModes(modeTests, "testMaxProps", componentUnderTest, stub); > } > > - function testMoreThanMaxProps() { > - const stub = Array(302).fill("foo"); > + function testMoreThanShortMaxProps() { > + const stub = Array(maxlength.short + 1).fill("foo"); maxlength is not defined.

Comment on attachment 8771218 [details] Bug 1285530 - Reps: Off by one error in grip-array max length. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64482/diff/1-2/

(In reply to Jan Honza Odvarko [:Honza] from comment #11) > maxlength is not defined. Oops! Sorry about the stupid error :/ > Note that I there is a collision with bug 1286259. I can't apply if patch from bug 1286259 is in the tree. > Please synchronize this with Fred. Yeah thanks for the reminder, I'll ni? him on bug 1286259 about this.

Comment on attachment 8771218 [details] Bug 1285530 - Reps: Off by one error in grip-array max length. https://reviewboard.mozilla.org/r/64482/#review61576 Works for me now! One last thing, bug 1286710 introduces ellipsis character instead of three dotes "..." Please wait till that but lands, adopt your test appropriatelly and land (or mark as checkin-needed) Thanks Steve! Honza

Comment on attachment 8771218 [details] Bug 1285530 - Reps: Off by one error in grip-array max length. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64482/diff/2-3/

Great thanks this is ready to land. Honza

The result showed only one unrelated(?) intermittent failed (bug 1283705), so request checkin-needed

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/b99fa4e2c829 Reps: Off by one error in grip-array max length. r=Honza