Summary: | [Reflected] IDL attributes of integer types should use HTML rules for parsing integers | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||
Component: | Bindings | Assignee: | 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
Chris Dumez
2016-02-22 22:31:41 PST
Created attachment 271990 [details]
Patch
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 on attachment 271990 [details] Patch Clearing flags on attachment: 271990 Committed r197014: <http://trac.webkit.org/changeset/197014> All reviewed patches have been landed. Closing bug. |