Closed Bug 1285530 Opened 8 years ago Closed 8 years ago

Reps: Off by one error in grip-array max length

Categories

(DevTools :: Shared Components, enhancement, P1)

enhancement

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.4 - Aug 1
Tracking Status
firefox50 --- fixed

People

(Reporter: linclark, Assigned: steveck)

References

Details

(Whiteboard: [reserve-html])

Attachments

(2 files)

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.
Whiteboard: [devtools-html] [triage]
Flags: qe-verify-
Priority: -- → P3
Whiteboard: [devtools-html] [triage] → [reserve-html]
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
Flags: needinfo?(lclark)
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.
Flags: needinfo?(lclark) → needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
Steve, I am assigning this to you so, it appears nicely on the tracking board.

Honza
Assignee: nobody → schung
Status: NEW → ASSIGNED
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...).
Attachment #8770452 - Flags: feedback?(odvarko)
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.
Attachment #8770452 - Flags: feedback?(odvarko) → review+
Also, since you are working on Array reps, can you please take a look at Bug 1286259

Honza
Comment on attachment 8771218 [details]
Bug 1285530 - Reps: Off by one error in grip-array max length.

https://reviewboard.mozilla.org/r/64482/#review61566
Attachment #8771218 - Flags: review?(odvarko) → review+
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.
Attachment #8771218 - Flags: review+
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/
Attachment #8771218 - Flags: review?(odvarko)
(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
Attachment #8771218 - Flags: review?(odvarko) → review+
Blocks: 1286259
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
Keywords: 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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b99fa4e2c829
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Iteration: --- → 50.4 - Aug 1
Priority: P3 → P1
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: