Implement and test DOMTokenList.replace (fixes #8511) #9353

nox · 2016-01-17T13:13:40Z

Thanks to @sankha93 for the original work in #8525.

nox · 2016-01-17T13:16:10Z

This is a WIP: ~~replace actually needs supports to be implemented~~.

frewsxcv · 2016-01-18T04:44:41Z

I know it's a WIP, but a couple suggestions if you weren't already going to do them:

explicitly add in the spec steps
create a separate internal function run_update_steps that performs the steps outlined here and link to that

nox · 2016-01-18T08:19:16Z

There is no run_update_steps because the associated attribute's value is actually the only storage for their list of tokens. We already do this.

bors-servo · 2016-02-12T15:39:42Z

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

jdm · 2016-02-24T16:51:38Z

@nox What's the status here? Is this still classified a WIP and needs more work, or should @asajeffrey be reviewing it?

nox · 2016-02-24T16:53:26Z

I'll look at it tonight or tomorrow while rebasing it.

asajeffrey · 2016-02-24T17:00:38Z

I've been holding off while it has conflicts, let me know when you're ready.

nox · 2016-02-25T13:20:30Z

@asajeffrey This is ok to go.

asajeffrey · 2016-02-25T14:48:27Z

Mostly this looks fine, there's a spec issue plus a couple of extra tests I think we should add, but otherwise cool

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

components/script/dom/domtokenlist.rs, line 135 [r1] (raw file):
It looks like this is slightly different from the spec in the case list.replace("a b", ""). The spec says it should throw a SyntaxError, it will actually throw an InvalidCharacterError.

tests/wpt/web-platform-tests/dom/nodes/Element-classlist.html, line 84 [r1] (raw file):
Can we add tests:

assert_throws( 'SYNTAX_ERR', function () { elem.classList.replace('', 'a b'); }
assert_throws( 'SYNTAX_ERR', function () { elem.classList.replace('a b', ''); }

tests/wpt/web-platform-tests/dom/nodes/Element-classlist.html, line 263 [r1] (raw file):
Can we add a test for the case where we're replacing a token with one that's already in the list:

  secondelem.className = 'token1 token2 token3'
  secondelem.classList.replace('token1', 'token3');
  assert_equals( secondelem.classList.length, 2 );
  assert_false( secondelem.classList.contains('token1') );
  assert_true( secondelem.classList.contains('token2') );
  assert_true( secondelem.classList.contains('token23') );

Comments from the review on Reviewable.io

jdm · 2016-02-25T17:35:12Z

I think assert_throws(new SyntaxError(), function () { elem.classList.replace('', 'a b'); } is more idiomatic.

Ms2ger · 2016-02-25T17:44:30Z

That's an other kind of SyntaxError

bors-servo · 2016-03-01T03:58:46Z

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

asajeffrey · 2016-03-10T15:27:30Z

@nox: is this PR still open?

nox · 2016-03-10T17:18:47Z

It is still open, still blocked, still didn't address it, I'll try to do it soon.

nox · 2016-03-22T13:27:09Z

@asajeffrey Ready to go.

asajeffrey · 2016-03-22T13:37:03Z

Okay, I'll take a look later today.

asajeffrey · 2016-03-23T17:03:12Z

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

components/script/dom/domtokenlist.rs, line 142 [r2] (raw file):
Can you add comments for how the code lines up with the steps of the spec?

Comments from the review on Reviewable.io

nox · 2016-03-24T09:38:23Z

Done. r?

Review status: 3 of 4 files reviewed at latest revision, 4 unresolved discussions.

components/script/dom/domtokenlist.rs, line 135 [r1] (raw file):
Done.

components/script/dom/domtokenlist.rs, line 142 [r2] (raw file):
Done.

tests/wpt/web-platform-tests/dom/nodes/Element-classlist.html, line 84 [r1] (raw file):
Done.

tests/wpt/web-platform-tests/dom/nodes/Element-classlist.html, line 263 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

asajeffrey · 2016-03-25T12:55:59Z

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

asajeffrey · 2016-03-25T12:56:25Z

@bors-servo r+

bors-servo · 2016-03-25T12:56:26Z

📌 Commit d010df9 has been approved by asajeffrey

bors-servo · 2016-03-25T12:56:28Z

⌛ Testing commit d010df9 with merge 59ba00e...

Implement and test DOMTokenList.replace (fixes #8511) Thanks to @sankha93 for the original work in #8525.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9353)

bors-servo · 2016-03-25T13:44:08Z

💔 Test failed - linux-rel

asajeffrey · 2016-03-25T13:55:08Z

@bors-servo retry

Intermittent timeout in /old-tests/submission/Opera/script_scheduling/040.html #9724

bors-servo · 2016-03-25T13:55:09Z

⚡ Previous build results for android, arm32, gonk, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor are reusable. Rebuilding only linux-rel...

bors-servo · 2016-03-25T14:42:55Z

☀️ Test successful - android, arm32, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 17, 2016

nox mentioned this pull request Jan 17, 2016

Update our implementation of DOMTokenList #8525

Closed

asajeffrey self-assigned this Feb 4, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Feb 12, 2016

nox force-pushed the domtokenlist-replace branch from b1f5276 to 636c0fc Compare February 25, 2016 13:20

nox removed the S-needs-rebase There are merge conflict errors. label Feb 25, 2016

asajeffrey added S-awaiting-answer Someone asked a question that requires an answer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 25, 2016

Ms2ger added the S-needs-code-changes Changes have not yet been made that were requested by a reviewer. label Feb 26, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Mar 1, 2016

Test that any call to classList.add() create the class attribute

feddb52

nox force-pushed the domtokenlist-replace branch from 636c0fc to d848645 Compare March 22, 2016 13:26

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 Mar 22, 2016

nox removed S-awaiting-answer Someone asked a question that requires an answer. S-needs-rebase There are merge conflict errors. labels Mar 22, 2016

Implement and test DOMTokenList.replace (fixes servo#8511)

d010df9

nox force-pushed the domtokenlist-replace branch from d848645 to d010df9 Compare March 24, 2016 09:37

highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Mar 25, 2016

highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 25, 2016

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 Mar 25, 2016

bors-servo merged commit d010df9 into servo:master Mar 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement and test DOMTokenList.replace (fixes #8511) #9353

Implement and test DOMTokenList.replace (fixes #8511) #9353

nox commented Jan 17, 2016

nox commented Jan 17, 2016

frewsxcv commented Jan 18, 2016

nox commented Jan 18, 2016

bors-servo commented Feb 12, 2016

jdm commented Feb 24, 2016

nox commented Feb 24, 2016

asajeffrey commented Feb 24, 2016

nox commented Feb 25, 2016

asajeffrey commented Feb 25, 2016

jdm commented Feb 25, 2016

Ms2ger commented Feb 25, 2016

bors-servo commented Mar 1, 2016

asajeffrey commented Mar 10, 2016

nox commented Mar 10, 2016

nox commented Mar 22, 2016

asajeffrey commented Mar 22, 2016

asajeffrey commented Mar 23, 2016

nox commented Mar 24, 2016

asajeffrey commented Mar 25, 2016

asajeffrey commented Mar 25, 2016

bors-servo commented Mar 25, 2016

bors-servo commented Mar 25, 2016

bors-servo commented Mar 25, 2016

asajeffrey commented Mar 25, 2016

bors-servo commented Mar 25, 2016

bors-servo commented Mar 25, 2016

Implement and test DOMTokenList.replace (fixes #8511) #9353

Implement and test DOMTokenList.replace (fixes #8511) #9353

Conversation

nox commented Jan 17, 2016

nox commented Jan 17, 2016

frewsxcv commented Jan 18, 2016

nox commented Jan 18, 2016

bors-servo commented Feb 12, 2016

jdm commented Feb 24, 2016

nox commented Feb 24, 2016

asajeffrey commented Feb 24, 2016

nox commented Feb 25, 2016

asajeffrey commented Feb 25, 2016

jdm commented Feb 25, 2016

Ms2ger commented Feb 25, 2016

bors-servo commented Mar 1, 2016

asajeffrey commented Mar 10, 2016

nox commented Mar 10, 2016

nox commented Mar 22, 2016

asajeffrey commented Mar 22, 2016

asajeffrey commented Mar 23, 2016

nox commented Mar 24, 2016

asajeffrey commented Mar 25, 2016

asajeffrey commented Mar 25, 2016

bors-servo commented Mar 25, 2016

bors-servo commented Mar 25, 2016

bors-servo commented Mar 25, 2016

asajeffrey commented Mar 25, 2016

bors-servo commented Mar 25, 2016

bors-servo commented Mar 25, 2016