Closed
Bug 669580
Opened 13 years ago
Closed 13 years ago
Add a reflectBool method to reflect.js
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: mounir, Assigned: mounir)
Details
Attachments
(1 file, 2 obsolete files)
11.90 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•13 years ago
|
||
I wrote a patch this week-end. Will attach it later (when I will have access to my laptop).
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•13 years ago
|
||
Hope you will enjoy :)
Attachment #545337 -
Flags: review?(Ms2ger)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [needs review]
Comment 3•13 years ago
|
||
Comment on attachment 545337 [details] [diff] [review] Patch v1 --- a/content/html/content/test/reflect.js +++ b/content/html/content/test/reflect.js +/** + * Checks that a given attribute is correctly reflected as limited to known + * values enumerated attribute. as what exactly? +function reflectBoolean(aParameters) +{ + // Tests various values. + var valuesToTest = [ Some documentation for these objects would be nice. Also, I don't think the nested object is necessary, in particular because setContent is always true. + { value: +0, stringified: 0, result: { setContent: true, setIdl: false } }, + { value: -0, stringified: 0, result: { setContent: true, setIdl: false } }, stringified: "0" Could you test +-Infinity as well? + // ES5, verse 9.2. + { value: { toString: function() { return "foo" } }, stringified: "foo", + result: { setContent: true, setIdl: true } }, + { value: { valueOf: function() { return "foo" } }, + stringified: "[object Object]", + result: { setContent: true, setIdl: true } }, + { value: { valueOf: function() { return "quux" }, toString: undefined }, + stringified: "quux", result: { setContent: true, setIdl: true } }, + { value: { valueOf: function() { return "foo" }, + toString: function() { return "bar" } }, stringified: "bar", + result: { setContent: true, setIdl: true } }, + { value: { foo: false, bar: false }, stringified: "[object Object]", + result: { setContent: true, setIdl: true } }, These don't seem particularly interesting. The stringification is already tested in reflectString, and I don't think these would catch other bugs. Maybe a couple of variations on { valueOf: function() { return false; } }? + ]; + + valuesToTest.forEach(function(v) { + if (v.value === null) { + todo(element.getAttribute(contentAttr), v.stringified, + "Content attribute should return the stringified value it has been set to."); Indentation Maybe note bug 667856 here? +} + No need for the empty line. --- a/content/html/content/test/forms/test_formnovalidate_attribute.html +++ b/content/html/content/test/forms/test_formnovalidate_attribute.html -checkFormNoValidateAttribute('input'); checkFormNoValidateAttribute('button'); Please move that one to test_button_attributes_reflection.html. --- a/content/html/content/test/test_bug546995.html +++ b/content/html/content/test/test_bug546995.html // TODO: keygen should be added when correctly implemented, see bug 101019. -checkAutofocusIDLAttribute(document.getElementById('i')); checkAutofocusIDLAttribute(document.getElementById('t')); checkAutofocusIDLAttribute(document.getElementById('s')); checkAutofocusIDLAttribute(document.getElementById('b')); Please move these as well, so this file can go entirely. (Followup is fine for both.) This looks good generally, but I'd like to see it once more.
Attachment #545337 -
Flags: review?(Ms2ger) → review-
Assignee | ||
Comment 4•13 years ago
|
||
(In reply to comment #3) > Comment on attachment 545337 [details] [diff] [review] [review] > Patch v1 > > --- a/content/html/content/test/reflect.js > +++ b/content/html/content/test/reflect.js > +/** > + * Checks that a given attribute is correctly reflected as a boolean. > > as what exactly? "as a boolean". Did you learn how to read? :) > +function reflectBoolean(aParameters) > +{ > + // Tests various values. > + var valuesToTest = [ > > Some documentation for these objects would be nice. Also, I don't think the > nested object is necessary, in particular because setContent is always true. Indeed, setContent is always true. Though, I find it more readable like that. > + // ES5, verse 9.2. > + { value: { toString: function() { return "foo" } }, stringified: "foo", > + result: { setContent: true, setIdl: true } }, > + { value: { valueOf: function() { return "foo" } }, > + stringified: "[object Object]", > + result: { setContent: true, setIdl: true } }, > + { value: { valueOf: function() { return "quux" }, toString: undefined }, > + stringified: "quux", result: { setContent: true, setIdl: true } }, > + { value: { valueOf: function() { return "foo" }, > + toString: function() { return "bar" } }, stringified: "bar", > + result: { setContent: true, setIdl: true } }, > + { value: { foo: false, bar: false }, stringified: "[object Object]", > + result: { setContent: true, setIdl: true } }, > > These don't seem particularly interesting. The stringification is already > tested in reflectString, and I don't think these would catch other bugs. > Maybe a couple of variations on { valueOf: function() { return false; } }? For tests, I tend to think that more is better. Added a test with "{ valueOf: function() { return false; } }". > --- a/content/html/content/test/test_bug546995.html > +++ b/content/html/content/test/test_bug546995.html > // TODO: keygen should be added when correctly implemented, see bug 101019. > -checkAutofocusIDLAttribute(document.getElementById('i')); > checkAutofocusIDLAttribute(document.getElementById('t')); > checkAutofocusIDLAttribute(document.getElementById('s')); > checkAutofocusIDLAttribute(document.getElementById('b')); > > Please move these as well, so this file can go entirely. (Followup is fine > for both.) Will do that, like I did for textarea.
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #545337 -
Attachment is obsolete: true
Attachment #546670 -
Flags: review?(Ms2ger)
Comment 6•13 years ago
|
||
Comment on attachment 546670 [details] [diff] [review] Patch v1.1 I really want to get rid of setContent.
Attachment #546670 -
Flags: review?(Ms2ger) → review-
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #546670 -
Attachment is obsolete: true
Attachment #546820 -
Flags: review?(Ms2ger)
Comment 8•13 years ago
|
||
Comment on attachment 546820 [details] [diff] [review] Patch v2 >--- a/content/html/content/test/reflect.js >+++ b/content/html/content/test/reflect.js >+ is(element[idlAttr], true, "IDL attribute should return always return " + >+ "'true' if the content attribute has been set"); Start the message on a new line. r=me
Updated•13 years ago
|
Attachment #546820 -
Flags: review?(Ms2ger) → review+
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite+
Whiteboard: [needs review] → [inbound]
Comment 9•13 years ago
|
||
this has been backed out by ehsan due to bustage with all the other changesets in the same push
Whiteboard: [inbound]
Assignee | ||
Updated•13 years ago
|
Whiteboard: [inbound]
Comment 10•13 years ago
|
||
(In reply to comment #4) > Will do that, like I did for textarea. Please don't forget to file.
Comment 11•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/ed2b2c4b453a
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Assignee | ||
Comment 12•13 years ago
|
||
(In reply to comment #10) > (In reply to comment #4) > > Will do that, like I did for textarea. > > Please don't forget to file. bug 673553, Sir!
Comment 13•13 years ago
|
||
Thanks, and select?
Assignee | ||
Comment 14•13 years ago
|
||
bug 673796, your Majesty!
You need to log in
before you can comment on or make changes to this bug.
Description
•