1293563 - Fix some DOMTokenlist.replace bugs

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez [:emilio]]

Bug 1224186 was my first non-totally-trivial contribution to Gecko, yet it's rid of bugs, so... I had to clean it up a bit :)

Didn't implement the functionality related to "replace any duplicate", just because it felt a bit slow-ish. That being said, given most of our other related bugs are around duplicates and white-space collapsing I don't think it's a big issue.

I guess the correct thing to do would be using an atom array as an attribute back-end, and clear duplicates and white-space upfront when the tokenlist is instantiated/set directly, but I thought that someone should already have thought about it. Let me know if that should change so I can fill a followup and work on it if I find time.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1293563: Fix some DOMTokenList.replace bugs.

Review commit: https://reviewboard.mozilla.org/r/70228/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70228/

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8779191 [details]
Bug 1293563: Fix some DOMTokenList.replace bugs.

https://reviewboard.mozilla.org/r/70228/#review71366

The code is ok, but I prefer to use common components in gecko platform such as tokenizer.

::: dom/base/nsDOMTokenList.cpp:343
(Diff revision 1)
>  
> -  tokens.AppendElement(aToken);
> -  RemoveInternal(attr, tokens);
> +void
> +nsDOMTokenList::ReplaceInternal(const nsAttrValue* aAttr,
> +                                const nsAString& aToken,
> +                                const nsAString& aNewToken)
> +{

What about if you use 
nsCharSeparatedTokenizerTemplate instead?
You create then a list of token, store them in an array and use Contains() instead oldTokenFound/newTokenFound/isFirstToken.

::: dom/base/nsDOMTokenList.cpp:356
(Diff revision 1)
> +
> +  nsAString::const_iterator iter, end, tokenStart;
> +  attribute.BeginReading(iter);
> +  attribute.EndReading(end);
>  
> -  tokens[0] = aNewToken;
> +  while (iter != end) {

while (true)

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1293563: Simplify DOMTokenlist.remove using a tokenizer.

Review commit: https://reviewboard.mozilla.org/r/74722/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74722/

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8779191 [details]
Bug 1293563: Fix some DOMTokenList.replace bugs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70228/diff/1-2/

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1293563: Update expectations in dom/base/test/test_classList.html.

Review commit: https://reviewboard.mozilla.org/r/74778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74778/

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8779191 [details]
Bug 1293563: Fix some DOMTokenList.replace bugs.

https://reviewboard.mozilla.org/r/70228/#review72614

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8785544 [details]
Bug 1293563: Simplify DOMTokenlist.remove using a tokenizer.

https://reviewboard.mozilla.org/r/74722/#review72616

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8785640 [details]
Bug 1293563: Update expectations in dom/base/test/test_classList.html.

https://reviewboard.mozilla.org/r/74778/#review72618

perfect! Thanks for this!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3faaecb8dfcb
Fix some DOMTokenList.replace bugs. r=baku
https://hg.mozilla.org/integration/autoland/rev/e8ada2555f83
Simplify DOMTokenlist.remove using a tokenizer. r=baku
https://hg.mozilla.org/integration/autoland/rev/854dbb84bdbb
Update expectations in dom/base/test/test_classList.html. r=baku

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3faaecb8dfcb
https://hg.mozilla.org/mozilla-central/rev/e8ada2555f83
https://hg.mozilla.org/mozilla-central/rev/854dbb84bdbb