669580 - Add a reflectBool method to reflect.js

Status: RESOLVED FIXED

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

Comment 1

[Mounir Lamouri (:mounir)]

I wrote a patch this week-end. Will attach it later (when I will have access to my laptop).

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Hope you will enjoy :)

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 4

[Mounir Lamouri (:mounir)]

(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.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 546670 [details] [diff] [review]
Patch v1.1

I really want to get rid of setContent.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 9

[Marco Bonardo [:mak]]

this has been backed out by ehsan due to bustage with all the other changesets in the same push

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to comment #4)
> Will do that, like I did for textarea.

Please don't forget to file.

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/ed2b2c4b453a

Comment 12

[Mounir Lamouri (:mounir)]

(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

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thanks, and select?

Comment 14

[Mounir Lamouri (:mounir)]

bug 673796, your Majesty!