Bug 154573 - [Reflected] IDL attributes of integer types should use HTML rules for parsing integers
Summary: [Reflected] IDL attributes of integer types should use HTML rules for parsing...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://html.spec.whatwg.org/#reflect...
Keywords: WebExposed
Depends on:
Reported: 2016-02-22 22:31 PST by Chris Dumez
Modified: 2016-02-23 23:04 PST (History)
9 users (show)

See Also:

Patch (242.65 KB, patch)
2016-02-22 22:39 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Comment 2 Darin Adler 2016-02-23 22:14:58 PST
Comment on attachment 271990 [details]

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]

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.