Reps: support -0 grip in number rep

RESOLVED FIXED in Firefox 50

Status

defect
P1
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: linclark, Assigned: steveck)

Tracking

(Blocks 1 bug)

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

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [devtools-html])

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

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

Updated

3 years ago
Depends on: 1264688
Whiteboard: [devtools-html] [triage]
Flags: qe-verify-
Reporter

Comment 1

3 years ago
Posted patch Failing test (obsolete) — Splinter Review
Here's a failing test.
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
Reporter

Updated

3 years ago
Blocks: 1283847
Assignee: nobody → schung
Status: NEW → ASSIGNED
Assignee

Comment 2

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

Comment 3

3 years ago
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
Assignee

Comment 4

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

Updated

3 years ago
Attachment #8766808 - Attachment is obsolete: true
Reporter

Comment 5

3 years ago
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+
Reporter

Comment 7

3 years ago
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+

Comment 8

3 years ago
Pushed by lclark@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e1db3f6c752
Reps: support -0 grip in number rep. r=linclark

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5e1db3f6c752
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Updated

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