Last Comment Bug 669580 - Add a reflectBool method to reflect.js
: Add a reflectBool method to reflect.js
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-06 03:05 PDT by Mounir Lamouri (:mounir)
Modified: 2011-07-24 10:56 PDT (History)
3 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (12.13 KB, patch)
2011-07-12 01:44 PDT, Mounir Lamouri (:mounir)
Ms2ger: review-
Details | Diff | Splinter Review
Patch v1.1 (12.87 KB, patch)
2011-07-18 16:55 PDT, Mounir Lamouri (:mounir)
Ms2ger: review-
Details | Diff | Splinter Review
Patch v2 (11.90 KB, patch)
2011-07-19 10:21 PDT, Mounir Lamouri (:mounir)
Ms2ger: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-07-06 03:05:28 PDT

    
Comment 1 Mounir Lamouri (:mounir) 2011-07-11 03:32:46 PDT
I wrote a patch this week-end. Will attach it later (when I will have access to my laptop).
Comment 2 Mounir Lamouri (:mounir) 2011-07-12 01:44:48 PDT
Created attachment 545337 [details] [diff] [review]
Patch v1

Hope you will enjoy :)
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2011-07-13 08:12:01 PDT
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) 2011-07-18 16:54:36 PDT
(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) 2011-07-18 16:55:11 PDT
Created attachment 546670 [details] [diff] [review]
Patch v1.1
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2011-07-19 07:28:46 PDT
Comment on attachment 546670 [details] [diff] [review]
Patch v1.1

I really want to get rid of setContent.
Comment 7 Mounir Lamouri (:mounir) 2011-07-19 10:21:17 PDT
Created attachment 546820 [details] [diff] [review]
Patch v2
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2011-07-19 10:52:39 PDT
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] 2011-07-20 06:55:24 PDT
this has been backed out by ehsan due to bustage with all the other changesets in the same push
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2011-07-22 13:33:25 PDT
(In reply to comment #4)
> Will do that, like I did for textarea.

Please don't forget to file.
Comment 12 Mounir Lamouri (:mounir) 2011-07-22 14:29:19 PDT
(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 (⌚ UTC+1/+2) 2011-07-23 05:25:30 PDT
Thanks, and select?
Comment 14 Mounir Lamouri (:mounir) 2011-07-24 10:56:38 PDT
bug 673796, your Majesty!

Note You need to log in before you can comment on or make changes to this bug.