Bug 162364 – Setting HTMLMeterElement's attributes to non-finite values throws wrong exception type

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Setter HTMLMeterElement's attributes to non-finite values throws wrong exception type. It should throw a TypeError but we throw a NOT_SUPPORTED_ERR.

[Chris Dumez]

Created attachment 289486 [details]
Patch

[Darin Adler]

Comment on attachment 289486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289486&action=review

That iOS Simulator error looks like it might be a real one. Please investigate and fix.

> Source/WebCore/html/HTMLMeterElement.cpp:88
>      setAttributeWithoutSynchronization(minAttr, AtomicString::number(min));

This looks the same as "[Reflect]" but for the setter only; is there some way to use that here? Maybe we can add a way to state "reflect for setter, but call C++ for getter" with extended attributes? Maybe "[ReflectSetterOnly]"?

> Source/WebCore/html/HTMLMeterElement.cpp:98
>      setAttributeWithoutSynchronization(maxAttr, AtomicString::number(max));

Ditto.

> Source/WebCore/html/HTMLMeterElement.cpp:109
>      setAttributeWithoutSynchronization(valueAttr, AtomicString::number(value));

Ditto.

> Source/WebCore/html/HTMLMeterElement.cpp:120
>      setAttributeWithoutSynchronization(lowAttr, AtomicString::number(low));

Ditto.

> Source/WebCore/html/HTMLMeterElement.cpp:131
>      setAttributeWithoutSynchronization(highAttr, AtomicString::number(high));

Ditto.

> Source/WebCore/html/HTMLMeterElement.cpp:142
>      setAttributeWithoutSynchronization(optimumAttr, AtomicString::number(optimum));

Ditto.

[Chris Dumez]

(In reply to comment #2)
> Comment on attachment 289486 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289486&action=review
> 
> That iOS Simulator error looks like it might be a real one. Please
> investigate and fix.
> 
> > Source/WebCore/html/HTMLMeterElement.cpp:88
> >      setAttributeWithoutSynchronization(minAttr, AtomicString::number(min));
> 
> This looks the same as "[Reflect]" but for the setter only; is there some
> way to use that here? Maybe we can add a way to state "reflect for setter,
> but call C++ for getter" with extended attributes? Maybe
> "[ReflectSetterOnly]"?
> 
> > Source/WebCore/html/HTMLMeterElement.cpp:98
> >      setAttributeWithoutSynchronization(maxAttr, AtomicString::number(max));
> 
> Ditto.
> 
> > Source/WebCore/html/HTMLMeterElement.cpp:109
> >      setAttributeWithoutSynchronization(valueAttr, AtomicString::number(value));
> 
> Ditto.
> 
> > Source/WebCore/html/HTMLMeterElement.cpp:120
> >      setAttributeWithoutSynchronization(lowAttr, AtomicString::number(low));
> 
> Ditto.
> 
> > Source/WebCore/html/HTMLMeterElement.cpp:131
> >      setAttributeWithoutSynchronization(highAttr, AtomicString::number(high));
> 
> Ditto.
> 
> > Source/WebCore/html/HTMLMeterElement.cpp:142
> >      setAttributeWithoutSynchronization(optimumAttr, AtomicString::number(optimum));
> 
> Ditto.

We would probably need a [ReflectSetterOnly] as you suggest. Although, I have not checked yet if the behavior of our getters is actually consistent with other browsers (it could be that [Reflect] is the right thing to do).

Note however that our bindings generator currently does not even seem to have support for [Reflect] for floating point attributes. This should probably be added.

[Darin Adler]

Comment on attachment 289486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289486&action=review

>>> Source/WebCore/html/HTMLMeterElement.cpp:88
>>>      setAttributeWithoutSynchronization(minAttr, AtomicString::number(min));
>> 
>> This looks the same as "[Reflect]" but for the setter only; is there some way to use that here? Maybe we can add a way to state "reflect for setter, but call C++ for getter" with extended attributes? Maybe "[ReflectSetterOnly]"?
> 
> We would probably need a [ReflectSetterOnly] as you suggest. Although, I have not checked yet if the behavior of our getters is actually consistent with other browsers (it could be that [Reflect] is the right thing to do).
> 
> Note however that our bindings generator currently does not even seem to have support for [Reflect] for floating point attributes. This should probably be added.

To state the obvious, I am guessing that code generation for [Reflect] for floating point attributes is almost identical to [Reflect] for integer attributes.

[Build Bot]

Comment on attachment 289486 [details]
Patch

Attachment 289486 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2120982

New failing tests:
imported/w3c/web-platform-tests/html/semantics/forms/the-meter-element/meter.html

[Build Bot]

Created attachment 289490 [details]
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Committed r206243: <http://trac.webkit.org/changeset/206243>

[Chris Dumez]

(In reply to comment #7)
> Committed r206243: <http://trac.webkit.org/changeset/206243>

iOS just needed a rebaseline for the new PASS.

[Chris Dumez]

(In reply to comment #2)
> Comment on attachment 289486 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289486&action=review
> 
> That iOS Simulator error looks like it might be a real one. Please
> investigate and fix.
> 
> > Source/WebCore/html/HTMLMeterElement.cpp:88
> >      setAttributeWithoutSynchronization(minAttr, AtomicString::number(min));
> 
> This looks the same as "[Reflect]" but for the setter only; is there some
> way to use that here? Maybe we can add a way to state "reflect for setter,
> but call C++ for getter" with extended attributes? Maybe
> "[ReflectSetterOnly]"?

I checked that the behavior of our getters is spec-compliant and matches Chrome at least. Therefore, this is not really reflection and [Reflect] is not suitable. As you say, we could add [ReflectSetterOnly] but I am not sure it is worth it. I would also find it a little bit confusing because it is not really a reflection if only the setter is "transparent".