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

RESOLVED FIXED in Firefox 50

Status

P1
enhancement
RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: linclark, Assigned: steveck)

Tracking

Trunk
Firefox 50
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [reserve-html])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Whiteboard: [devtools-html] [triage]
Blocks: 1263741
Flags: qe-verify-
Priority: -- → P3
Whiteboard: [devtools-html] [triage] → [reserve-html]
(Assignee)

Comment 1

2 years ago
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)
(Reporter)

Comment 2

2 years ago
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
(Assignee)

Comment 5

2 years ago
Created attachment 8770452 [details] [diff] [review]
Bug-1285530.patch

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
(Assignee)

Comment 8

2 years ago
Created attachment 8771218 [details]
Bug 1285530 - Reps: Off by one error in grip-array max length.

Review commit: https://reviewboard.mozilla.org/r/64482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64482/
Attachment #8771218 - Flags: review?(odvarko)
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+
(Assignee)

Comment 12

2 years ago
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)
(Assignee)

Comment 13

2 years ago
(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+

Updated

2 years ago
Blocks: 1286259
(Assignee)

Comment 15

2 years ago
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
(Assignee)

Comment 17

2 years ago
The result showed only one unrelated(?) intermittent failed (bug 1283705), so request checkin-needed
Keywords: checkin-needed

Comment 18

2 years ago
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

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b99fa4e2c829
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Iteration: --- → 50.4 - Aug 1
Priority: P3 → P1

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.