Do not accept negative time values in transition durations #15658

simon-whitehead · 2017-02-20T11:37:32Z

Adds Time::parse_non_negative which errors on negative values. This new method is now used by transition_duration::parse.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #15343

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-02-20T11:37:38Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

highfive · 2017-02-20T11:37:38Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/longhand/box.mako.rs, components/style/values/specified/mod.rs
@emilio: components/style/properties/longhand/box.mako.rs, components/style/values/specified/mod.rs

emilio · 2017-02-20T17:54:38Z

Hi, this looks great, could you squash the commits so the commit message doesn't end up being "Fix #xxx"?

r=me with that.

Thanks again! :)

simon-whitehead · 2017-02-22T21:36:41Z

Sorry for the delay @emilio .. my new job uses BitBucket so I don't see these notifications quite so often anymore!

The commit message has a newline between "Fixes #xxx" and the actual message. Would you like me to amend that specific commit message or squash all commits together into one?

KiChjang · 2017-02-22T21:47:29Z

Squash all of them into one, and rename the commit, please.

emilio · 2017-02-23T00:45:41Z

Sorry for the delay @emilio .. my new job uses BitBucket so I don't see these notifications quite so often anymore!

No worries, hope your new job is great :)

The commit message has a newline between "Fixes #xxx" and the actual message. Would you like me to amend that specific commit message or squash all commits together into one?

Yeah, if you could squash them all with a meaningful message, that'd be neat! It's generally frowned upon to have commits with a message like "Fixes tidy", or other kind of partial fixes/commits.

If you need any help, don't bother to ask :). Alternatively, you can also check the "Allow edits from collaborators" checkbox, and I can squash and land them for you if you're busy with other stuff.

Thanks again for doing this! :)


        Add Time::parse_non_negative and add it to transition_duration::parse

simon-whitehead · 2017-02-23T12:35:14Z

Thanks @emilio - all done :) (the job is great thanks! Just a busy first week!)

I wasn't sure what you meant by r=me so I just tagged you again. I hope that is okay. After this first week I'm sure I can dedicate more time to my contributions. Sorry for the delay.

emilio · 2017-02-23T15:58:16Z

The job is great thanks! Just a busy first week!

Glad to hear that! :)

I wasn't sure what you meant by r=me so I just tagged you again. I hope that is okay.

Sure it is :). r=me is just saying that "this can land with my signoff with the comments addressed". I try to be around to sign-off myself, but if I'm not around for any reason and any other reviewer sees that, they can land it with r=emilio.

After this first week I'm sure I can dedicate more time to my contributions. Sorry for the delay.

That'd be awesome, and no worries! This looks great to me, thanks! :)

@bors-servo r+

bors-servo · 2017-02-23T15:58:17Z

📌 Commit 8c661fe has been approved by emilio

bors-servo · 2017-02-23T15:58:22Z

⌛ Testing commit 8c661fe with merge ba45384...


        Auto merge of #15658 - simon-whitehead:fix-15343, r=emilio

Fix #15343  Adds `Time::parse_non_negative` which errors on negative values. This new method is now used by `transition_duration::parse`. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #15343  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15658)

bors-servo · 2017-02-23T16:33:14Z

💔 Test failed - linux-rel-css

jdm · 2017-02-23T17:25:46Z

  ▶ Unexpected subtest result in /css-transitions-1_dev/html/transition-duration-001.htm:
  └ PASS [expected FAIL] parse '-5s'

  ▶ Unexpected subtest result in /css-transitions-1_dev/html/transition-duration-001.htm:
  └ PASS [expected FAIL] parse '-500ms'

  ▶ Unexpected subtest result in /css-transitions-1_dev/html/transition-delay-001.htm:
  │ FAIL [expected PASS] parse '-5s'
  │   → assert_equals: Expected computed value expected "-5s" but got "0s"
  │ FAIL [expected PASS] parse '-500ms'
  │   → assert_equals: Expected computed value expected "-0.5s" but got "0s"
  │ 
  │ @http://web-platform.test:8000/css-transitions-1_dev/html/transition-delay-001.htm:96:25
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1403:20
  │ test@http://web-platform.test:8000/resources/testharness.js:501:9
  └ @http://web-platform.test:8000/css-transitions-1_dev/html/transition-delay-001.htm:91:21

  ▶ Unexpected subtest result in /css-values-3_dev/html/transition-delay-001.htm:
  │ FAIL [expected PASS] parse '-5s'
  │   → assert_equals: Expected computed value expected "-5s" but got "0s"
  │ FAIL [expected PASS] parse '-500ms'
  │   → assert_equals: Expected computed value expected "-0.5s" but got "0s"
  │ 
  │ @http://web-platform.test:8000/css-values-3_dev/html/transition-delay-001.htm:96:25
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1403:20
  │ test@http://web-platform.test:8000/resources/testharness.js:501:9
  └ @http://web-platform.test:8000/css-values-3_dev/html/transition-delay-001.htm:91:21

  ▶ Unexpected subtest result in /css-values-3_dev/html/transition-duration-001.htm:
  └ PASS [expected FAIL] parse '-5s'

  ▶ Unexpected subtest result in /css-values-3_dev/html/transition-duration-001.htm:
  └ PASS [expected FAIL] parse '-500ms'

Good news - clearly this code is being executed. Bad news is we need to figure out how to avoid the failures in transition-delay-001.htm.

emilio · 2017-02-24T00:35:23Z

Right, negative values in transition delay are allowed.

simon-whitehead · 2017-02-25T05:33:03Z

How were those tests run? I can give fixing it a go but I need to know how to run the tests that are failing. Sorry if thats an obvious question.

emilio · 2017-02-25T18:52:29Z

./mach test-css /css-values-3_dev/html/transition-delay-001.htm should do it :)

emilio · 2017-03-26T09:30:28Z

@simon-whitehead are you planning to finish this one? Thanks!

simon-whitehead · 2017-03-26T09:33:25Z

I am so sorry @emilio, I have been distracted with my new job.

I am also getting married next week so this week and next week are a write off for me too.

I hate to be "that guy" who leaves it in this state but realistically this just all happened at a bad time. I am happy to finish it off in a few weeks but if that isnt suitable I will have to hand it off to someone else.

I am sorry for the inconvenience.

emilio · 2017-03-26T09:36:53Z

No worries at all! We can probably try to finish this in a few weeks, thanks for replying :)

nox · 2017-03-28T07:09:39Z

@bors-servo r-

bors-servo · 2017-04-19T13:37:41Z

☔ The latest upstream changes (presumably #16527) made this pull request unmergeable. Please resolve the merge conflicts.

hiikezoe · 2017-05-07T08:49:36Z

@simon-whitehead would you mind finishing this up?

simon-whitehead · 2017-05-08T02:38:04Z

Oh sure @hiikezoe - I completely forgot, sorry about that. I should get a chance in the next few days.

Sorry again everyone.

hiikezoe · 2017-05-08T02:43:54Z

That's really good to know. Thank you!

chenpighead · 2017-05-10T03:22:31Z

Filed gecko bug for tracking this: Bug 1363592.

cc @chenpighead

simon-whitehead · 2017-05-12T06:18:43Z

I'm going to have to be "that guy" @hiikezoe and pass this on to someone else.

I am sorry. Both personally and professionally I am just swamped at the moment and I don't want to hold anyone else up. I apologise immensely for half-baking this feature.

I hope to have more time soon to help with more tasks. Again, I am really really sorry.

hiikezoe · 2017-05-12T06:34:12Z

@simon-whitehead you don't have to apologize. You did a good job. The unit test can be still used as it is. Thanks for doing! I will do the rest of work.

jdm · 2017-05-12T21:02:57Z

Closing in favour of #16829.

highfive assigned Manishearth Feb 20, 2017

highfive added the S-awaiting-review label Feb 20, 2017

jdm added S-needs-squash and removed S-awaiting-review labels Feb 21, 2017

Add Time::parse_non_negative and add it to transition_duration::parse

Loading status checks…

8c661fe

simon-whitehead force-pushed the simon-whitehead:fix-15343 branch from d8ec53c to 8c661fe Feb 23, 2017

highfive added the S-awaiting-review label Feb 23, 2017

highfive assigned emilio and unassigned Manishearth Feb 23, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Feb 23, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 23, 2017

jdm changed the title ~~Fix #15343~~ Do not accept negative time values in transition durations Feb 23, 2017

highfive added the S-needs-rebase label Apr 19, 2017

hiikezoe mentioned this pull request May 12, 2017

Disallow negative duration for animation and transition #16829

Merged

4 of 4 tasks complete

jdm closed this May 12, 2017

servo / servo

Do not accept negative time values in transition durations #15658

Do not accept negative time values in transition durations #15658

simon-whitehead commented Feb 20, 2017 •

edited by larsbergstrom

highfive commented Feb 20, 2017

highfive commented Feb 20, 2017

emilio commented Feb 20, 2017

simon-whitehead commented Feb 22, 2017

KiChjang commented Feb 22, 2017

emilio commented Feb 23, 2017

simon-whitehead commented Feb 23, 2017

emilio commented Feb 23, 2017

bors-servo commented Feb 23, 2017

bors-servo commented Feb 23, 2017

bors-servo commented Feb 23, 2017

jdm commented Feb 23, 2017

emilio commented Feb 24, 2017

simon-whitehead commented Feb 25, 2017

emilio commented Feb 25, 2017

emilio commented Mar 26, 2017

simon-whitehead commented Mar 26, 2017

emilio commented Mar 26, 2017

nox commented Mar 28, 2017

bors-servo commented Apr 19, 2017

hiikezoe commented May 7, 2017

simon-whitehead commented May 8, 2017

hiikezoe commented May 8, 2017

chenpighead commented May 10, 2017

simon-whitehead commented May 12, 2017

hiikezoe commented May 12, 2017

jdm commented May 12, 2017

servo / servo

Join GitHub today

Do not accept negative time values in transition durations #15658

Do not accept negative time values in transition durations #15658

Conversation

simon-whitehead commented Feb 20, 2017 • edited by larsbergstrom

highfive commented Feb 20, 2017

highfive commented Feb 20, 2017

emilio commented Feb 20, 2017

simon-whitehead commented Feb 22, 2017

KiChjang commented Feb 22, 2017

emilio commented Feb 23, 2017

simon-whitehead commented Feb 23, 2017

emilio commented Feb 23, 2017

bors-servo commented Feb 23, 2017

bors-servo commented Feb 23, 2017

bors-servo commented Feb 23, 2017

jdm commented Feb 23, 2017

emilio commented Feb 24, 2017

simon-whitehead commented Feb 25, 2017

emilio commented Feb 25, 2017

emilio commented Mar 26, 2017

simon-whitehead commented Mar 26, 2017

emilio commented Mar 26, 2017

nox commented Mar 28, 2017

bors-servo commented Apr 19, 2017

hiikezoe commented May 7, 2017

simon-whitehead commented May 8, 2017

hiikezoe commented May 8, 2017

chenpighead commented May 10, 2017

simon-whitehead commented May 12, 2017

hiikezoe commented May 12, 2017

jdm commented May 12, 2017

simon-whitehead commented Feb 20, 2017 •

edited by larsbergstrom