Anne (:annevk) Reporter
	Description • 9 years ago

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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 1 • 8 years ago

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

	Anne (:annevk) Reporter
	Comment 2 • 8 years ago

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.

	n.oxyde
	Comment 3 • 8 years ago

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 5 • 8 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 6 • 8 years ago

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.

	Andrea Marchesini [:baku]
	Comment 7 • 8 years ago

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

	Andrea Marchesini [:baku]
	Comment 8 • 8 years ago

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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 9 • 8 years ago

(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 :)

	Andrea Marchesini [:baku]
	Comment 10 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 11 • 8 years ago

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 13 • 8 years ago

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

There it goes again :)

	Andrea Marchesini [:baku]
	Comment 14 • 8 years ago

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);

	Pulsebot
	Comment 16 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 17 • 8 years ago

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

	Pulsebot
	Comment 18 • 8 years ago

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 19 • 8 years ago

Ohh cool, unexpected test passes!

Will update the test expectations :)

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 8 years ago

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?

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 22 • 8 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 8 years ago

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

	Pulsebot
	Comment 24 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 25 • 8 years ago

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

	Pulsebot
	Comment 26 • 8 years ago

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

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 28 • 8 years ago

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

There it is with the expectations updated.

	Pulsebot
	Comment 29 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 30 • 8 years ago
bugherder

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

	Jean-Yves Perrier [:teoli]
	Comment 31 • 8 years ago

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

	Arkadiusz Michalski (Spirit)
	Comment 32 • 8 years ago

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