1224186 - Implement DOMTokenList's new replace() method

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Anne (:annevk)]

https://dom.spec.whatwg.org/#dom-domtokenlist-replace

We should probably leave the validation logic until there's a feature that actually makes use of that.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Proposed patch

Anne, are there existing tests for this? Do you know who should I ask for review?

Comment 2

[Anne (:annevk)]

You could ask :baku or :smaug maybe. Not aware of any tests. Perhaps you can update https://github.com/w3c/web-platform-tests/blob/master/dom/nodes/Element-classlist.html. Ms2ger might have ideas.

Comment 3

[n.oxyde]

We have some tests in this open PR: https://github.com/servo/servo/pull/9353

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: Proposed patch with tests

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Attachment: Proposed patch with tests (pressed intro too early)

nox, added a simple test, since that servo PR is covering most cases.

This one also includes the `replace(string_with_spaces, empty_string)` case.

Let me know if you think the test shouldn't be included.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8724415 [details] [diff] [review]
Proposed patch with tests (pressed intro too early)

Review of attachment 8724415 [details] [diff] [review]:
-----------------------------------------------------------------

Note that your test change was bitrotted in https://hg.mozilla.org/integration/mozilla-inbound/rev/3da6faf2cd54

Deferring to baku to see if code/tests are correct per spec.

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8724415 [details] [diff] [review]
Proposed patch with tests (pressed intro too early)

Review of attachment 8724415 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMTokenList.cpp
@@ +315,5 @@
> +{
> +  // Doing this here instead of using `CheckToken` because if aToken had invalid
> +  // characters, and aNewToken is empty, the returned error should be a
> +  // SyntaxError, not an InvalidCharacterError.
> +  if (aNewToken.IsEmpty()) {

NS_WARN_IF

@@ +321,5 @@
> +    return;
> +  }
> +
> +  aError = CheckToken(aToken);
> +  if (aError.Failed()) {

NS_WARN_IF

@@ +326,5 @@
> +    return;
> +  }
> +
> +  aError = CheckToken(aNewToken);
> +  if (aError.Failed()) {

NS_WARN_IF

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8724415 [details] [diff] [review]
Proposed patch with tests (pressed intro too early)

Review of attachment 8724415 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMTokenList.cpp
@@ +317,5 @@
> +  // characters, and aNewToken is empty, the returned error should be a
> +  // SyntaxError, not an InvalidCharacterError.
> +  if (aNewToken.IsEmpty()) {
> +    aError = NS_ERROR_DOM_SYNTAX_ERR;
> +    return;

you should throw SyntaxError also if atoken is empty.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

(In reply to Andrea Marchesini (:baku) from comment #8)

> you should throw SyntaxError also if atoken is empty.

That's implicitly done in the next CheckToken call. 

The explicit check for `aNewToken` is necessary because in the case string_with_whitespace, empty_string, the invalid character error would be thrown instead if it's omitted.

Re. the NS_WARN_IF, I didn't add them because they're not anywhere else in the fail (I guess because the thrown error is considered enough?).

If you want I can add it everywhere else in the file though :)

Comment 10

[Andrea Marchesini [:baku]]

It's ok. Thanks for the comment. r+

Comment 11

[Carsten Book [:Tomcat]]

has problems to apply:

renamed 1224186 -> 0001-Implement-DOMTokenList.replace.patch
applying 0001-Implement-DOMTokenList.replace.patch
patching file testing/web-platform/tests/dom/nodes/Element-classlist.html
Hunk #1 FAILED at 298
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/tests/dom/nodes/Element-classlist.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0001-Implement-DOMTokenList.replace.patch

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Attachment: Unbitrotted version of the patch

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Attachment: Unbitrotted version of the patch

Sorry, got distracted and didn't took the time to unbitrot it.

There it goes again :)

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 8731485 [details] [diff] [review]
Unbitrotted version of the patch

Review of attachment 8731485 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMTokenList.cpp
@@ +316,5 @@
> +  // Doing this here instead of using `CheckToken` because if aToken had invalid
> +  // characters, and aNewToken is empty, the returned error should be a
> +  // SyntaxError, not an InvalidCharacterError.
> +  if (aNewToken.IsEmpty()) {
> +    aError = NS_ERROR_DOM_SYNTAX_ERR;

aError.Thorws(NS_ERROR_DOM_SYNTAX_ERR);

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Attachment: Patch with review comments addressed

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92a78fabeb8d

Comment 17

[Carsten Book [:Tomcat]]

Hi, had to back this out - could you take a look at https://treeherder.mozilla.org/logviewer.html#?job_id=25702974&repo=mozilla-inbound

Comment 18

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98048beb73c9

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Ohh cool, unexpected test passes!

Will update the test expectations :)

Comment 20

[Emilio Cobos Álvarez [:emilio]]

Attachment: Patch with expectations updated

Comment 21

[Boris Zbarsky [:bzbarsky]]

Wait, so that last patch still has some wpt failure annotations around classList.replace.  Are those tests wrong, or is this implementation wrong or incomplete?

Comment 22

[Emilio Cobos Álvarez [:emilio]]

The failure is due to other more general DOMTokenList problems, not inherent to the replace() implementatoon: We don't treat \n, \t, ... as space, and we don't deduplicate tokens when they are added.

I don't know if there is any bug filled for that, but I can certainly fix it. Even though is maybe worth fixing it as a follow-up, since this patch has been lying around for a while.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Yeah, if they're general issues not specific to replace() followup is definitely better.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0eb8cf4e5b92

Comment 25

[Carsten Book [:Tomcat]]

backed out seems there are still issues like https://treeherder.mozilla.org/logviewer.html#?job_id=25810767&repo=mozilla-inbound

Comment 26

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9cfac4ce926

Comment 27

[Emilio Cobos Álvarez [:emilio]]

Attachment: 0001-Bug-1224186-Implement-DOMTokenlist.replace-r-baku-Ms.patch

Comment 28

[Emilio Cobos Álvarez [:emilio]]

So sorry! I forgot to re-run the interfaces test again :(

There it is with the expectations updated.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/030b17d4947a

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/030b17d4947a

Comment 31

[Jean-Yves Perrier [:teoli]]

Created: https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/replace
Updated: https://developer.mozilla.org/en-US/Firefox/Releases/49#DOM_HTML_DOM

Comment 32

[Arkadiusz Michalski (Spirit)]

In Example: document.documentElement.replas << not "replas" but "replace"