Closed Bug 1283522 Opened 7 years ago Closed 7 years ago

Reps: support -0 grip in number rep


(DevTools :: Shared Components, defect, P1)



(firefox50 fixed)

Firefox 50
50.3 - Jul 18
Tracking Status
firefox50 --- fixed


(Reporter: linclark, Assigned: steveck)



(Whiteboard: [devtools-html])


(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
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]

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 =, -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]

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.

The only failure is unrelated, so this is good to go.
Attachment #8770794 - Flags: review?(lclark) → review+
Pushed by
Reps: support -0 grip in number rep. r=linclark
Closed: 7 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.