Closed Bug 530171 Opened 16 years ago Closed 16 years ago

DOMTokenList methods handle null incorrectly

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a1

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

(
URL
)

Details

Attachments

(1 file, 3 obsolete files)

Patch for checkin
12.90 KB, patch
Details | Diff | Splinter Review
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
Attached patch Patch (obsolete) — — Splinter Review
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)
Attachment #413687 - Flags: review?(Olli.Pettay) → review?(sylvain.pasche)
I'll sr, but syp wrote this code.
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.
Assignee: nobody → Ms2ger
Blocks: 501257
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
(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 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-
Attached patch v2 (obsolete) — — Splinter Review
I fixed the indentation and added some tests.
Attachment #413687 - Attachment is obsolete: true
Attachment #414528 - Flags: review?(sylvain.pasche)
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+
Attached patch v3 (obsolete) — — Splinter Review
Updated to comments. Thanks for the review.
Attachment #414528 - Attachment is obsolete: true
Attachment #414544 - Flags: superreview?(Olli.Pettay)
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+
No problem. Thanks for the sr.
Attachment #414544 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/797a19033c01.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: