1283522 - Reps: support -0 grip in number rep

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P1)

Description

[Lin Clark [:linclark]]

Currently the number rep supports -0 as a value but not as a grip. It should support both cases.

Comment 1

[Lin Clark [:linclark]]

Attachment: Failing test

Here's a failing test.

Comment 2

[Steve Chung [:steveck]]

Attachment: Bug-1283522-fix.patch

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!

Comment 3

[Lin Clark [:linclark]]

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.

Comment 4

[Steve Chung [:steveck]]

Attachment: Bug-1283522-fix.patch

Thanks for the review! I addressed some issues and resent the patch again before my Mozreview permission is ready.

Comment 5

[Lin Clark [:linclark]]

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!

Comment 6

[Steve Chung [:steveck]]

Attachment: Bug 1283522 - Reps: support -0 grip in number rep.

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

Comment 7

[Lin Clark [:linclark]]

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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e1db3f6c752
Reps: support -0 grip in number rep. r=linclark

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e1db3f6c752