Closed Bug 1283522 Opened 4 years ago Closed 4 years ago

Reps: support -0 grip in number rep

Categories

(DevTools :: Shared Components, defect, P1)

defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.3 - Jul 18
Tracking Status
firefox50 --- fixed

People

(Reporter: linclark, Assigned: steveck)

References

Details

(Whiteboard: [devtools-html])

Attachments

(2 files, 2 obsolete files)

Currently the number rep supports -0 as a value but not as a grip. It should support both cases.
Depends on: 1264688
Whiteboard: [devtools-html] [triage]
Flags: qe-verify-
Attached patch Failing test (obsolete) — Splinter Review
Here's a failing test.
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
Blocks: 1283847
Assignee: nobody → schung
Status: NEW → ASSIGNED
Attached patch Bug-1283522-fix.patch (obsolete) — Splinter Review
Hi Lin,

Since it's my first patch in firefox/devtool and I'm not 100% sure that it's correct direction of fixing the issue, I just created a quick patch based on your failing test patch. I'll submit the patch by mozreview if you think the patch is ok, thanks!
Attachment #8769582 - Flags: feedback?(lclark)
Comment on attachment 8769582 [details] [diff] [review]
Bug-1283522-fix.patch

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

Overall this looks good, thanks!

You'll want to run the changes through our ESLint checker. To do that, run ./mach eslint --no-ignore <filename>

Once you've fixed any issues that that uncovers, you can post another patch and add me for review.

::: devtools/client/shared/components/reps/number.js
@@ +20,5 @@
>      displayName: "Number",
>  
>      stringify: function (object) {
> +      let isNegativeZero = Object.is(object, -0) ||
> +        (object.type && object.type == "-0"); //grip

Our code style doesn't allow for comments at the end of a line. I don't think that this needs a comment, since we use the grips extensively in this part of the system.

::: devtools/client/shared/components/test/mochitest/test_reps_number.html
@@ +56,5 @@
> +    let renderedComponent = renderComponent(Number.rep, { object: getGripStub("testNegZeroGrip") });
> +    is(renderedComponent.textContent, "-0", "Number rep has expected text content for negative zero");
> +
> +    renderedComponent = renderComponent(Number.rep, { object: getGripStub("testNegZeroValue") });
> +    is(renderedComponent.textContent, "-0", "Number rep has expected text content for negative zero");

For both of these, the string "Number rep has expected text content for negative zero" should have something at the end to indicate whether it's testing the value -0 or the grip form. That will make it easier to figure out which test is failing if it fails.
Attachment #8769582 - Flags: feedback?(lclark) → feedback+
Iteration: --- → 50.3 - Jul 18
Priority: P2 → P1
Thanks for the review! I addressed some issues and resent the patch again before my Mozreview permission is ready.
Attachment #8769582 - Attachment is obsolete: true
Attachment #8770460 - Flags: review?(lclark)
Attachment #8766808 - Attachment is obsolete: true
Comment on attachment 8770460 [details] [diff] [review]
Bug-1283522-fix.patch

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

This looks good to me. Can you push to try? If so, please make sure to push it up to try and paste the link here. If the ES check passes and none of the devtools tests fail, then you can add the "checkin-needed" keyword.

Thanks for your work on this!
Attachment #8770460 - Flags: review?(lclark) → review+
Comment on attachment 8770794 [details]
Bug 1283522 - Reps: support -0 grip in number rep.

https://reviewboard.mozilla.org/r/64134/#review61268

The only failure is unrelated, so this is good to go.
Attachment #8770794 - Flags: review?(lclark) → review+
Pushed by lclark@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e1db3f6c752
Reps: support -0 grip in number rep. r=linclark
https://hg.mozilla.org/mozilla-central/rev/5e1db3f6c752
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.