Bug 178061 – Fix bugs related to setting reflected floating point DOM attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Darin Adler]

Fix bugs related to setting reflected floating point DOM attributes

[Darin Adler]

Created attachment 323129 [details]
Patch

[Darin Adler]

Created attachment 323130 [details]
Patch

[Darin Adler]

Created attachment 323131 [details]
Patch

[Chris Dumez]

Comment on attachment 323131 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        (WebCore::HTMLProgressElement::setMax): Changed the semantics to match what

May want to mention:
https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#limited-to-numbers-greater-than-zero

> Source/WebCore/html/HTMLProgressElement.cpp:101
> +    setAttributeWithoutSynchronization(valueAttr, AtomicString::number(value));

The specification [1[ says to use the "best representation of the number as a floating-point number" [2]. It basically says to use Ecma's ToString(), so shouldn't it call String::numberToStringECMAScript() ?
Same for meter.value.

- https://html.spec.whatwg.org/multipage/form-elements.html#dom-progress-value
- https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#best-representation-of-the-number-as-a-floating-point-number

[Darin Adler]

Comment on attachment 323131 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        (WebCore::HTMLProgressElement::setMax): Changed the semantics to match what
> 
> May want to mention:
> https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#limited-to-numbers-greater-than-zero

OK

>> Source/WebCore/html/HTMLProgressElement.cpp:101
>> +    setAttributeWithoutSynchronization(valueAttr, AtomicString::number(value));
> 
> The specification [1[ says to use the "best representation of the number as a floating-point number" [2]. It basically says to use Ecma's ToString(), so shouldn't it call String::numberToStringECMAScript() ?
> Same for meter.value.
> 
> - https://html.spec.whatwg.org/multipage/form-elements.html#dom-progress-value
> - https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#best-representation-of-the-number-as-a-floating-point-number

Yes, exactly.

AtomicSstring::number does exactly the same thing as String::numberToStringECMAScript. The String::number function needs to be renamed to include the words “fixed precision” and we will not use it that much any more going forward. I am trying to make the ECMAScript behavior, which is a great default anyway, the default stringifying for doubles throughout WebKit over time.

So yes, this is what this patch does and that’s why the tests are passing now.

I could have renamed the AtomicStriing::number function to add the word ECMAScript, but then I would’ve had to change all the call sites, and all of them want the ECMAScript semantic. I chose to do it this way: it’s a bit more subtle but I think it’s good forward-looking.

[Darin Adler]

Maybe I should add more tests; not sure every last behavior change is sufficiently covered.

[Darin Adler]

Now that EWS has run: review anyone?

[WebKit Commit Bot]

Comment on attachment 323131 [details]
Patch

Clearing flags on attachment: 323131

Committed r223035: <http://trac.webkit.org/changeset/223035>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34877132>