DOMTokenList methods handle null incorrectly

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
DOM
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla1.9.3a1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

9 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20091119 Minefield/3.7a1pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20091119 Minefield/3.7a1pre

The contains(), add(), remove() & toggle() methods on DOMTokenList should
convert a null argument to "null". [1] I'll attach a patch.

[1] <http://www.whatwg.org/html5/#domtokenlist>

Reproducible: Always
(Assignee)

Comment 1

9 years ago
Created attachment 413687 [details] [diff] [review]
Patch

This patch also updates comments that that still refers to dom/public/idl, which
was moved to dom/interfaces in bug 481027.
Attachment #413687 - Flags: review?(Olli.Pettay)

Updated

9 years ago
Attachment #413687 - Flags: review?(Olli.Pettay) → review?(sylvain.pasche)

Comment 2

9 years ago
I'll sr, but syp wrote this code.

Comment 3

9 years ago
Ok, so the relevant draft spec is http://www.w3.org/TR/2008/WD-WebIDL-20081219/#Null

(I'm not sure I agree with HTML5 here, but still, seems like we don't
handle the case like it is currently specified.)

Ms2ger, you need to update/add some tests.

Updated

9 years ago
Assignee: nobody → Ms2ger
Blocks: 501257
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk

Comment 4

9 years ago
(In reply to comment #3)
> Ok, so the relevant draft spec is
> http://www.w3.org/TR/2008/WD-WebIDL-20081219/#Null

More specifically, that might be http://dev.w3.org/2006/webapi/WebIDL/#es-DOMString, because the current version of the DOMTokenList interface (http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#domtokenlist) has no [Null] annotation.

My understanding from reading bug 478251 is that we might make [Null(Stringify)] the default for DOMString one day, thus removing the annotations that are added in this bug.

Comment 5

9 years ago
Comment on attachment 413687 [details] [diff] [review]
Patch


>   DOMString                        item(in unsigned long index);
>-  boolean                          contains(in DOMString token);
>-  void                             add(in DOMString token);
>-  void                             remove(in DOMString token);
>-  boolean                          toggle(in DOMString token);
>+  boolean                          contains([Null(Stringify)] in DOMString token);
>+  void                             add([Null(Stringify)] in DOMString token);
>+  void                             remove([Null(Stringify)] in DOMString token);
>+  boolean                          toggle([Null(Stringify)] in DOMString token);


nit: I aligned the method names on column 36 to match some other .idl I saw.
But with this change code is getting past the 80th column, which should be avoided.
I suggest you simply move the method names to the left to avoid that.

I'll let someone more familiar with the quickstubs stuff review that part. Although the change seems pretty straightforward.

It would be great if you could add some tests for this. The main test file is:
http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_classList.html

Looking at the test, I don't think that it tests passing non string arguments to the DOMTokenList methods yet (so your patch shouldn't regress it).

Note: to run just the test, you can use:
cd /path/to/your/objdir
TEST_PATH=content/base/test/test_classList.html make mochitest-plain

Marking review- so that I can have a look at the tests. Thanks!

Note: this has nothing to do with this bug but if you write some other classList testcases you may also encounter bug 529328 (there are some undefined or inconsistent behavior in the spec which should be discussed in the appropriate lists).
Attachment #413687 - Flags: review?(sylvain.pasche) → review-
(Assignee)

Comment 6

9 years ago
Created attachment 414528 [details] [diff] [review]
v2

I fixed the indentation and added some tests.
Attachment #413687 - Attachment is obsolete: true
Attachment #414528 - Flags: review?(sylvain.pasche)

Comment 7

9 years ago
Comment on attachment 414528 [details] [diff] [review]
v2

>diff -r 21e65793a3cc content/base/test/test_classList.html
>--- a/content/base/test/test_classList.html	Tue Nov 24 16:59:53 2009 +0100
>+++ b/content/base/test/test_classList.html	Wed Nov 25 19:14:39 2009 +0100
>@@ -1,12 +1,13 @@
> <!DOCTYPE HTML>
> <html>
> <!--
> https://bugzilla.mozilla.org/show_bug.cgi?id=501257
>+https://bugzilla.mozilla.org/show_bug.cgi?id=530171
> -->

You don't need to add that line here, we can get to the follow-up bugs from the main bug and you mention bug 530171 in the comments below.

> interface nsIDOMDOMTokenList : nsISupports
> {
>   readonly attribute unsigned long length;
> 
>-  DOMString                        item(in unsigned long index);
>-  boolean                          contains(in DOMString token);
>-  void                             add(in DOMString token);
>-  void                             remove(in DOMString token);
>-  boolean                          toggle(in DOMString token);
>+  DOMString item(in unsigned long index);
>+  boolean contains([Null(Stringify)] in DOMString token);
>+  void add([Null(Stringify)] in DOMString token);
>+  void remove([Null(Stringify)] in DOMString token);
>+  boolean toggle([Null(Stringify)] in DOMString token);
> 
>-  DOMString                        toString();
>+  DOMString toString();

Could you still keep the method names aligned on the same column? I think it helps for readability. For instance: http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/core/nsIDOMDOMStringList.idl.

Looks good otherwise, thanks.
Attachment #414528 - Flags: review?(sylvain.pasche) → review+
(Assignee)

Comment 8

9 years ago
Created attachment 414544 [details] [diff] [review]
v3

Updated to comments. Thanks for the review.
Attachment #414528 - Attachment is obsolete: true
Attachment #414544 - Flags: superreview?(Olli.Pettay)

Comment 9

9 years ago
Comment on attachment 414544 [details] [diff] [review]
v3

Argh, sorry for the delay.

>@@ -197,16 +197,23 @@ members = [
>     'nsIDOM3Node.isDefaultNamespace',
>     'nsIDOM3Node.isEqualNode',
>     'nsIDOM3Text.isElementContentWhitespace',
>     'nsIDOM3Text.replaceWholeText',
>     'nsIDOM3Text.wholeText',
>     'nsIDOMDOMStringList.item',
>     'nsIDOMDOMStringList.length',
>     'nsIDOMDOMStringList.contains',
>+    'nsIDOMDOMTokenList.length',
>+    'nsIDOMDOMTokenList.item',
>+    'nsIDOMDOMTokenList.contains',
>+    'nsIDOMDOMTokenList.add',
>+    'nsIDOMDOMTokenList.remove',
>+    'nsIDOMDOMTokenList.toggle',
>+    'nsIDOMDOMTokenList.toString',
nsIDOMDOMTokenList.* should work.
Attachment #414544 - Flags: superreview?(Olli.Pettay) → superreview+
(Assignee)

Comment 10

9 years ago
Created attachment 418383 [details] [diff] [review]
Patch for checkin

No problem. Thanks for the sr.
Attachment #414544 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/797a19033c01.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.