Add number() static methods to AtomicString class

Add number() static methods to AtomicString class

Add number() static methods to AtomicString class as a lot of callers need it and
previously had to use String::number() then convert the String to an AtomicString.
Calling AtomicString::number() directly when the caller needs an AtomicString is
~2% faster.

Also refactored HTML code to use the Element helper methods for Number attributes.

R=eseidel

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162593

[tkent, 2013-11-24 21:20:03]

lgtm with a nit.

It seems V8StringResource still uses String::number().

https://codereview.chromium.org/84713002/diff/1/Source/wtf/text/AtomicStringT...
File Source/wtf/text/AtomicStringTest.cpp (right):

https://codereview.chromium.org/84713002/diff/1/Source/wtf/text/AtomicStringT...
Source/wtf/text/AtomicStringTest.cpp:38: TEST(WTF,
AtomicStringCreationFromNumber)
We usually specify <test target name> + "Test" for the first argument, and <test
case name> for the second argument.  So this line should be:
    TEST(AtomicStringTest, Number)

[eseidel, 2013-11-24 21:37:43]

lgtm.  We can fix all the various usage nits in a later patch.

https://codereview.chromium.org/84713002/diff/1/Source/core/html/HTMLBodyElem...
File Source/core/html/HTMLBodyElement.cpp (right):

https://codereview.chromium.org/84713002/diff/1/Source/core/html/HTMLBodyElem...
Source/core/html/HTMLBodyElement.cpp:172: setAttribute(marginheightAttr,
AtomicString::number(marginHeight));
Looks like some of these could use the helpers on Element.  But that can be a
later change.

https://codereview.chromium.org/84713002/diff/1/Source/core/html/HTMLProgress...
File Source/core/html/HTMLProgressElement.cpp (right):

https://codereview.chromium.org/84713002/diff/1/Source/core/html/HTMLProgress...
Source/core/html/HTMLProgressElement.cpp:107: setAttribute(valueAttr,
AtomicString::number(value >= 0 ? value : 0));
std::max(0, value)?

https://codereview.chromium.org/84713002/diff/1/Source/core/html/HTMLProgress...
Source/core/html/HTMLProgressElement.cpp:122: setAttribute(maxAttr,
AtomicString::number(max > 0 ? max : 1));
std::max(1, value)?

[Inactive, 2013-11-24 21:48:13]

https://codereview.chromium.org/84713002/diff/1/Source/bindings/v8/V8StringRe...
File Source/bindings/v8/V8StringResource.cpp (right):

https://codereview.chromium.org/84713002/diff/1/Source/bindings/v8/V8StringRe...
Source/bindings/v8/V8StringResource.cpp:195: webCoreString =
String::number(value);
tkent, are you referring to this String::number() call? If so, it seems it
really does require a String, not an AtomicString.

https://codereview.chromium.org/84713002/diff/1/Source/wtf/text/AtomicStringT...
File Source/wtf/text/AtomicStringTest.cpp (right):

https://codereview.chromium.org/84713002/diff/1/Source/wtf/text/AtomicStringT...
Source/wtf/text/AtomicStringTest.cpp:38: TEST(WTF,
AtomicStringCreationFromNumber)
On 2013/11/24 21:20:04, tkent wrote:
> We usually specify <test target name> + "Test" for the first argument, and
<test
> case name> for the second argument.  So this line should be:
>     TEST(AtomicStringTest, Number)

I see, I used WTFStringTest.cpp as reference but I guess this was a bad example.
I'll make the change.

[tkent, 2013-11-24 22:03:25]

https://codereview.chromium.org/84713002/diff/1/Source/bindings/v8/V8StringRe...
File Source/bindings/v8/V8StringResource.cpp (right):

https://codereview.chromium.org/84713002/diff/1/Source/bindings/v8/V8StringRe...
Source/bindings/v8/V8StringResource.cpp:195: webCoreString =
String::number(value);
On 2013/11/24 21:48:13, Chris Dumez wrote:
> tkent, are you referring to this String::number() call? If so, it seems it
> really does require a String, not an AtomicString.

I meant this one and another in int32ToWebCoreString() below.
I thought using String had some overhead in a case that the destination is a
reflected IDL attribute. However It's not easy to resolve it, and you don't need
to address it in this CL.

[Inactive, 2013-11-24 22:06:19]

https://codereview.chromium.org/84713002/diff/1/Source/core/html/HTMLProgress...
File Source/core/html/HTMLProgressElement.cpp (right):

https://codereview.chromium.org/84713002/diff/1/Source/core/html/HTMLProgress...
Source/core/html/HTMLProgressElement.cpp:122: setAttribute(maxAttr,
AtomicString::number(max > 0 ? max : 1));
On 2013/11/24 21:37:44, eseidel wrote:
> std::max(1, value)?

This is not equivalent. 0.5 is a valid input value but would be converted to 1
std::max(1, value).

By the way, this code is not according to specification. The specification says
that if the input value is inferior or equal to 0, it should be ignored.
Currently, we reset the max to 1.0 in this case:
http://www.whatwg.org/specs/web-apps/current-work/#dom-progress-max
http://www.whatwg.org/specs/web-apps/current-work/#limited-to-numbers-greater...

[Inactive, 2013-11-24 22:30:00]

https://codereview.chromium.org/84713002/diff/1/Source/core/html/HTMLBodyElem...
File Source/core/html/HTMLBodyElement.cpp (right):

https://codereview.chromium.org/84713002/diff/1/Source/core/html/HTMLBodyElem...
Source/core/html/HTMLBodyElement.cpp:172: setAttribute(marginheightAttr,
AtomicString::number(marginHeight));
On 2013/11/24 21:37:44, eseidel wrote:
> Looks like some of these could use the helpers on Element.  But that can be a
> later change.

Excellent point, I'll update everywhere to use the Element helpers.

[Inactive, 2013-11-24 23:19:16]

I updated the patch according to feedback, thanks. Please take another look.

I'm getting AppEngine errors when uploading (error 500) but it looks like the
patch is reviewable.

https://codereview.chromium.org/84713002/diff/1/Source/core/html/HTMLBodyElem...
File Source/core/html/HTMLBodyElement.cpp (right):

https://codereview.chromium.org/84713002/diff/1/Source/core/html/HTMLBodyElem...
Source/core/html/HTMLBodyElement.cpp:172: setAttribute(marginheightAttr,
AtomicString::number(marginHeight));
On 2013/11/24 21:37:44, eseidel wrote:
> Looks like some of these could use the helpers on Element.  But that can be a
> later change.

Done.

https://codereview.chromium.org/84713002/diff/1/Source/wtf/text/AtomicStringT...
File Source/wtf/text/AtomicStringTest.cpp (right):

https://codereview.chromium.org/84713002/diff/1/Source/wtf/text/AtomicStringT...
Source/wtf/text/AtomicStringTest.cpp:38: TEST(WTF,
AtomicStringCreationFromNumber)
On 2013/11/24 21:20:04, tkent wrote:
> We usually specify <test target name> + "Test" for the first argument, and
<test
> case name> for the second argument.  So this line should be:
>     TEST(AtomicStringTest, Number)

Done.

[eseidel, 2013-11-25 00:46:41]

lgtm

https://codereview.chromium.org/84713002/diff/210003/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/84713002/diff/210003/Source/core/dom/Element....
Source/core/dom/Element.cpp:2768: double
Element::getFloatingPointAttribute(const QualifiedName& attributeName, double
fallbackValue) const
This is the same as the next function.  We just need to figure out what
"parseToDoubleForNumberType" returns on failure and make that the default
fallbackValue.

getFloatingPointAttribute(QualifiedName, double fallback = 0); for instance.  I
presume all of these getters will want a fallback value and can just pass it
along to parseToDoubleForNumberType.

[Inactive, 2013-11-25 01:19:37]

https://codereview.chromium.org/84713002/diff/210003/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/84713002/diff/210003/Source/core/dom/Element....
Source/core/dom/Element.cpp:2768: double
Element::getFloatingPointAttribute(const QualifiedName& attributeName, double
fallbackValue) const
On 2013/11/25 00:46:43, eseidel wrote:
> This is the same as the next function.  We just need to figure out what
> "parseToDoubleForNumberType" returns on failure and make that the default
> fallbackValue.
> 
> getFloatingPointAttribute(QualifiedName, double fallback = 0); for instance. 
I
> presume all of these getters will want a fallback value and can just pass it
> along to parseToDoubleForNumberType.

parseToDoubleForNumberType() uses std::numeric_limits<double>::quiet_NaN() as
default fallback value.

I followed the pattern used by parseToDoubleForNumberType():
double parseToDoubleForNumberType(const String&);
double parseToDoubleForNumberType(const String&, double fallbackValue);

I can update it to:
double parseToDoubleForNumberType(const String&, double fallbackValue =
std::numeric_limits<double>::quiet_NaN());

and them do the same for the Element helper method. I'll do that.

[Inactive, 2013-11-25 01:28:12]

https://codereview.chromium.org/84713002/diff/210003/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/84713002/diff/210003/Source/core/dom/Element....
Source/core/dom/Element.cpp:2768: double
Element::getFloatingPointAttribute(const QualifiedName& attributeName, double
fallbackValue) const
On 2013/11/25 01:19:38, Chris Dumez wrote:
> On 2013/11/25 00:46:43, eseidel wrote:
> > This is the same as the next function.  We just need to figure out what
> > "parseToDoubleForNumberType" returns on failure and make that the default
> > fallbackValue.
> > 
> > getFloatingPointAttribute(QualifiedName, double fallback = 0); for instance.

> I
> > presume all of these getters will want a fallback value and can just pass it
> > along to parseToDoubleForNumberType.
> 
> parseToDoubleForNumberType() uses std::numeric_limits<double>::quiet_NaN() as
> default fallback value.
> 
> I followed the pattern used by parseToDoubleForNumberType():
> double parseToDoubleForNumberType(const String&);
> double parseToDoubleForNumberType(const String&, double fallbackValue);
> 
> I can update it to:
> double parseToDoubleForNumberType(const String&, double fallbackValue =
> std::numeric_limits<double>::quiet_NaN());
> 
> and them do the same for the Element helper method. I'll do that.

Done.

[eseidel, 2013-11-25 01:47:46]

lgtm

Thanks.  This is now very explicit to the callers of this function what will
happen if they don't provide a fallback.

[eseidel, 2013-11-25 01:48:27]

You really went above and beyond the call of duty with this change.  Thank you
again!

[commit-bot: I haz the power, 2013-11-25 01:52:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/84713002/70002

[commit-bot: I haz the power, 2013-11-25 03:04:59]

Change committed as 162593