Implement DOMTokenList's new replace() method

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: annevk, Assigned: emilio)

Tracking

({dev-doc-complete})

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: dom-triaged btpp-active)

Attachments

(1 attachment, 7 obsolete attachments)

Reporter

Description

4 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.
Posted patch Proposed patch (obsolete) — Splinter Review
Anne, are there existing tests for this? Do you know who should I ask for review?
Flags: needinfo?(annevk)
Reporter

Comment 2

3 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.
Flags: needinfo?(annevk)

Comment 3

3 years ago
We have some tests in this open PR: https://github.com/servo/servo/pull/9353
Posted patch Proposed patch with tests (obsolete) — Splinter Review
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.
Attachment #8724384 - Attachment is obsolete: true
Attachment #8724414 - Attachment is obsolete: true
Attachment #8724415 - Flags: review?(Ms2ger)
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.
Attachment #8724415 - Flags: review?(amarchesini)
Attachment #8724415 - Flags: review?(Ms2ger)
Attachment #8724415 - Flags: feedback+
Assignee: nobody → ecoal95
Whiteboard: dom-triaged btpp-active
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
Attachment #8724415 - Flags: review?(amarchesini) → review+
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.
(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 :)
Flags: needinfo?(amarchesini)
It's ok. Thanks for the comment. r+
Flags: needinfo?(amarchesini)
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
Flags: needinfo?(ecoal95)
Keywords: checkin-needed
Flags: needinfo?(ecoal95)
Sorry, got distracted and didn't took the time to unbitrot it.

There it goes again :)
Attachment #8724415 - Attachment is obsolete: true
Attachment #8731484 - Attachment is obsolete: true
Attachment #8731485 - Flags: review?(amarchesini)
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);
Attachment #8731485 - Flags: review?(amarchesini) → review+
Attachment #8731485 - Attachment is obsolete: true
Attachment #8732826 - Flags: review?(amarchesini)
Attachment #8732826 - Flags: review?(amarchesini) → review+
Hi, had to back this out - could you take a look at https://treeherder.mozilla.org/logviewer.html#?job_id=25702974&repo=mozilla-inbound
Flags: needinfo?(ecoal95)
Ohh cool, unexpected test passes!

Will update the test expectations :)
Flags: needinfo?(ecoal95)
Attachment #8740983 - Flags: review?(amarchesini)
Assignee

Updated

3 years ago
Attachment #8732826 - Attachment is obsolete: true
Attachment #8740983 - Flags: review?(amarchesini) → review+
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?
Flags: needinfo?(ecoal95)
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.
Flags: needinfo?(ecoal95)
Yeah, if they're general issues not specific to replace() followup is definitely better.
backed out seems there are still issues like https://treeherder.mozilla.org/logviewer.html#?job_id=25810767&repo=mozilla-inbound
Flags: needinfo?(ecoal95)
Assignee

Updated

3 years ago
Attachment #8740983 - Attachment is obsolete: true
So sorry! I forgot to re-run the interfaces test again :(

There it is with the expectations updated.

Comment 30

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/030b17d4947a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
In Example: document.documentElement.replas << not "replas" but "replace"
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.