Add support for read-only attributes in reflect.js reflectUnsignedInt()

RESOLVED WONTFIX

Status

()

--
enhancement
RESOLVED WONTFIX
7 years ago
7 years ago

People

(Reporter: mats, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
Add support for read-only attributes in reflect.js reflectUnsignedInt()

This is needed for the test in bug 648910.
(Reporter)

Comment 1

7 years ago
Created attachment 545378 [details] [diff] [review]
patch v1
Attachment #545378 - Flags: review?(mounir)
Comment on attachment 545378 [details] [diff] [review]
patch v1

Review of attachment 545378 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the nits fixed.

::: content/html/content/test/reflect.js
@@ +122,5 @@
>   *  - element           Element   node to test on
>   *  - attribute         String    name of the attribute
>   *  - nonZero           Boolean   whether the attribute should be non-null
>   *  - defaultValue      Integer   [optional] default value, if different from the default one
> + *  - readOnly          Boolean   whether the attribute is read-only

Could you add [optional] in the description.

@@ +130,5 @@
>    var element = aParameters.element;
>    var attr = aParameters.attribute;
>    var nonZero = aParameters.nonZero;
>    var defaultValue = aParameters.defaultValue;
> +  var readOnly = aParameters.readOnly;

Could you do:
var readOnly = aParameters.readOnly === true;
Attachment #545378 - Flags: review?(mounir) → review+
Comment on attachment 545378 [details] [diff] [review]
patch v1

Review of attachment 545378 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the nits fixed.

::: content/html/content/test/reflect.js
@@ +130,5 @@
>    var element = aParameters.element;
>    var attr = aParameters.attribute;
>    var nonZero = aParameters.nonZero;
>    var defaultValue = aParameters.defaultValue;
> +  var readOnly = aParameters.readOnly;

I've been too fast here: you are misusing defaultValue. You should probably pass the value instead of assuming it's the defaultValue. For that, you can pass an object to readOnly containing a 'value' attribut. Like:

var readOnly = aParameters.readOnly === Object;
if (readOnly) {
  defaultValue = aParameters.readOnly.value;
}
Attachment #545378 - Flags: review+ → review-
(Reporter)

Comment 4

7 years ago
> you are misusing defaultValue

I don't understand; on (the existing) line 148 we have:

  is(element[attr], defaultValue, "default value should be " + defaultValue);

so the test already makes that assumption.  Why introduce a second mechanism
of passing in defaultValue when we already have one?  Are you saying that the
assumption on line 148 is wrong?
(Reporter)

Comment 5

7 years ago
It seems there isn't any interest in this...
Assignee: matspal → nobody
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.