Bug 154573

Summary: [Reflected] IDL attributes of integer types should use HTML rules for parsing integers
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: BindingsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, commit-queue, darin, esprehn+autocc, ggaren, gyuyoung.kim, kangil.han, rniwa, sam
Priority: P2 Keywords: WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://html.spec.whatwg.org/#reflecting-content-attributes-in-idl-attributes
Attachments:
Description Flags
Patch none

Description Chris Dumez 2016-02-22 22:31:41 PST
[Reflected] IDL attributes of integer types should use HTML rules for parsing integers:
- https://html.spec.whatwg.org/#reflecting-content-attributes-in-idl-attributes

Those rules are defined here:
- https://html.spec.whatwg.org/#rules-for-parsing-integers
- https://html.spec.whatwg.org/#rules-for-parsing-non-negative-integers

We already have an implementation for parsing HTML integers but our reflected attributes currently use WTFString::toInt() / toUint() instead.
Comment 1 Chris Dumez 2016-02-22 22:39:01 PST
Created attachment 271990 [details]
Patch
Comment 2 Darin Adler 2016-02-23 22:14:58 PST
Comment on attachment 271990 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:2769
> +    parseHTMLInteger(getAttribute(attributeName), value);

Kind of sad that this doesn’t use fastGetAttribute.

> Source/WebCore/dom/Element.cpp:2781
> +    parseHTMLNonNegativeInteger(getAttribute(attributeName), value);

Kind of sad that this doesn’t use fastGetAttribute.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:156
>  static bool parseHTMLIntegerInternal(const CharacterType* position, const CharacterType* end, int& value)

Do we have tests for parseHTMLInteger and parseHTMLNonNegativeInteger?

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:195
> +    StringBuilder cleanCharacters;
> +    if (!isPositive)
> +        cleanCharacters.append('-');
>      while (position < end) {
>          if (!isASCIIDigit(*position))
>              break;
> -        digits.append(*position++);
> +        cleanCharacters.append(*position++);
>      }

This is needlessly inefficient, doing heap access for no reason. Given that we are only looking at ASCII digits with a single possible sign prefix, why use a StringBuilder? Why not just use a Vector<LChar, 32> or something like that? We could also use reserveInitialCapacity and uncheckedAppend. And there would be no need for an 8-bit and 16-bit branch below.
Comment 3 WebKit Commit Bot 2016-02-23 23:03:58 PST
Comment on attachment 271990 [details]
Patch

Clearing flags on attachment: 271990

Committed r197014: <http://trac.webkit.org/changeset/197014>
Comment 4 WebKit Commit Bot 2016-02-23 23:04:03 PST
All reviewed patches have been landed.  Closing bug.