Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add DateTime string attribute to Time #14086

Merged
merged 1 commit into from Dec 3, 2016
Merged

Add DateTime string attribute to Time #14086

merged 1 commit into from Dec 3, 2016

Conversation

chajath
Copy link
Contributor

@chajath chajath commented Nov 5, 2016 •
edited by larsbergstrom

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

This change is Reviewable

@highfive
Copy link

highfive commented Nov 5, 2016

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

@highfive
Copy link

highfive commented Nov 5, 2016

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/htmltimeelement.rs, components/script/dom/webidls/HTMLTimeElement.webidl
  • @KiChjang: components/script/dom/htmltimeelement.rs, components/script/dom/webidls/HTMLTimeElement.webidl

@highfive
Copy link

highfive commented Nov 5, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 5, 2016
@KiChjang
Copy link
Contributor

KiChjang commented Nov 6, 2016 •
edited

@chajath We generally only accept PRs that implement the full set of features as described by the HTML spec, rather than exposing interfaces to incomplete functionalities. You can either close this PR, or add more commits to it so that it conforms with the spec.

@KiChjang KiChjang added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 6, 2016
@chajath chajath changed the title Add DateTime string attribute to Time [WIP] Add DateTime string attribute to Time Nov 6, 2016
@nox
Copy link
Contributor

nox commented Nov 7, 2016

@KiChjang What is the rest of the functionality?

@KiChjang
Copy link
Contributor

KiChjang commented Nov 7, 2016

Parsing the input string into a machine-readable equivalent. Are there any WPT tests for this?

@nox
Copy link
Contributor

nox commented Nov 7, 2016

Parsing the input string into a machine-readable equivalent. Are there any WPT tests for this?

Where do you find this needed in the spec? The spec just says that the IDL attribute reflects the content attribute, AFAICT.

@KiChjang
Copy link
Contributor

KiChjang commented Nov 7, 2016

See whatwg/html#1702.

@nox
Copy link
Contributor

nox commented Nov 7, 2016

That's for authors, not UAs, AFAICT. At the very least the current spec doesn't say to parse anything in the dateTime setter.

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 13, 2016
@cbrewster
Copy link
Contributor

Sorry about being late on a review for this... This looks good to me, unless the datetime does need to be parsed, but I don't see exactly where the spec would be saying that. I think if that is the case, it would be ok to implement that in a followup PR.

Lets see what tests now pass
@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 735d104 with merge 1025184...

bors-servo pushed a commit that referenced this pull request Nov 16, 2016
Auto merge of #14086 - chajath:time-datetime-attribute, r=<try>
1025184 
[WIP] Add DateTime string attribute to Time

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix part of #12967, content parsing will come as a separate PR.

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->

---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14086)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💥 Test timed out

@chajath
Copy link
Contributor Author

chajath commented Nov 18, 2016

That's odd. Tests run fine on my local machine. Should we try again?

@chajath
Copy link
Contributor Author

chajath commented Nov 18, 2016

@bors-servo try

@cbrewster
Copy link
Contributor

@bors-servo retry

@bors-servo
Copy link
Contributor

⌛ Trying commit 735d104 with merge a37fd8d...

@bors-servo
Copy link
Contributor

💔 Test failed - linux-dev

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Nov 18, 2016
@KiChjang
Copy link
Contributor 

Checking files for tidiness...

./components/script/dom/htmltimeelement.rs:5: trailing whitespace

@KiChjang KiChjang added the S-fails-tidy `./mach test-tidy` reported errors. label Nov 18, 2016
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Nov 19, 2016
@chajath chajath changed the title [WIP] Add DateTime string attribute to Time Add DateTime string attribute to Time Nov 24, 2016
@cbrewster
Copy link
Contributor

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit bb2d8f5 has been approved by ConnorGBrewster

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Nov 25, 2016
@cbrewster cbrewster added S-awaiting-review There is new code that needs to be reviewed. S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. S-awaiting-review There is new code that needs to be reviewed. labels Nov 25, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit bb2d8f5 with merge 026b1a8...

bors-servo pushed a commit that referenced this pull request Nov 25, 2016
Auto merge of #14086 - chajath:time-datetime-attribute, r=ConnorGBrew…
026b1a8 
…ster

Add DateTime string attribute to Time

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix part of #12967, content parsing will come as a separate PR.

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->

---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14086)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

⛄ The build was interrupted to prioritize another pull request.

@bors-servo
Copy link
Contributor

⚡ Previous build results for mac-dev-unit are reusable. Rebuilding only arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev...

@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-wpt1

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 25, 2016
@cbrewster
Copy link
Contributor

There are still some test expectations that need updated.
You should not need to do this by hand. Refer to Updating Test Expectations.

Tests with unexpected results:
  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: setAttribute() to ""

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: setAttribute() to " \\0\\x01\\x02\\x03\\x04\\x05\\x06\\x07 \\b\\t\\n\\v\\f\\r\\x0e\\x0f \\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17 \\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f  foo "

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: setAttribute() to undefined

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: setAttribute() to 7

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: setAttribute() to 1.5

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: setAttribute() to true

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: setAttribute() to false

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: setAttribute() to object "[object Object]"

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: setAttribute() to NaN

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: setAttribute() to Infinity

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: setAttribute() to -Infinity

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: setAttribute() to "\\0"

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: setAttribute() to null

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: setAttribute() to object "test-toString"

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: setAttribute() to object "test-valueOf"

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: IDL set to ""

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: IDL set to " \\0\\x01\\x02\\x03\\x04\\x05\\x06\\x07 \\b\\t\\n\\v\\f\\r\\x0e\\x0f \\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17 \\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f  foo "

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: IDL set to undefined

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: IDL set to 7

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: IDL set to 1.5

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: IDL set to true

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: IDL set to false

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: IDL set to object "[object Object]"

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: IDL set to NaN

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: IDL set to Infinity

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: IDL set to -Infinity

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: IDL set to "\\0"

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: IDL set to null

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: IDL set to object "test-toString"

  ▶ Unexpected subtest result in /html/dom/reflection-text.html:
  └ PASS [expected FAIL] time.dateTime: IDL set to object "test-valueOf"

@chajath
Add DateTime string attribute to Time
25f0317 
Also handle datetime content attribute when attribute is absent.

Rephrase terms used in the test: dateTime IDL property, datetime
attribute and datetime content attribute.

Remove failure expectations for time element.
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 27, 2016
@cbrewster
Copy link
Contributor

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 25f0317 has been approved by ConnorGBrewster

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Dec 3, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit 25f0317 with merge 8992b65...

bors-servo pushed a commit that referenced this pull request Dec 3, 2016
Auto merge of #14086 - chajath:time-datetime-attribute, r=ConnorGBrew…
8992b65 
…ster

Add DateTime string attribute to Time

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix part of #12967, content parsing will come as a separate PR.

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->

---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14086)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

@bors-servo bors-servo merged commit 25f0317 into servo:master Dec 3, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Dec 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cbrewster cbrewster

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@chajath @highfive @KiChjang @nox @cbrewster @bors-servo