Bug 55645 - DatasetDOMStringMap::item and ::contains copies attribute name string
Summary: DatasetDOMStringMap::item and ::contains copies attribute name string
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Emil A Eklund
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-02 19:02 PST by Emil A Eklund
Modified: 2011-03-29 15:10 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.42 KB, patch)
2011-03-02 19:05 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2011-03-02 19:02:59 PST
DatasetDOMStringMap::item and DatasetDOMStringMap::contains both use propertyNameMatchesAttributeName to match dataset property names with attribute names. The current implementation does this matching by creating a new string that's converted from a attribute name to a property name. It also loops over the attribute string twice (first to check if it's valid and then to create the copy).

This is unnecessary and rather inefficient.
Comment 1 Emil A Eklund 2011-03-02 19:05:38 PST
Created attachment 84510 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2011-03-29 11:03:23 PDT
Comment on attachment 84510 [details]
Patch

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

Ask abarth to take a gander, I am not very well-versed in character encodings and the challenges sorrounding them. I am just scared of them :)

> Source/WebCore/dom/DatasetDOMStringMap.cpp:-80
> -    String convertedName = convertAttributeNameToPropertyName(attributeName);

Do we still need this helper?

> Source/WebCore/dom/DatasetDOMStringMap.cpp:95
> +        if (attribute[a] == '-' && a + 1 < attributeLength && attribute[a + 1] != '-')
> +            wordBoundary = true;
> +        else {
> +            if ((wordBoundary ? toASCIIUpper(attribute[a]) : attribute[a]) != property[p])
> +                return false;
> +            p++;
> +            wordBoundary = false;
> +        }
> +        a++;

Will this still work with non-ASCII characters?
Comment 3 Emil A Eklund 2011-03-29 11:31:23 PDT
Comment on attachment 84510 [details]
Patch

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

>> Source/WebCore/dom/DatasetDOMStringMap.cpp:-80
>> -    String convertedName = convertAttributeNameToPropertyName(attributeName);
> 
> Do we still need this helper?

Yeah, at least for now. It's used in one more place.

>> Source/WebCore/dom/DatasetDOMStringMap.cpp:95
>> +        a++;
> 
> Will this still work with non-ASCII characters?

No, but then again neither will convertAttributeNameToPropertyName.
Comment 4 Dimitri Glazkov (Google) 2011-03-29 11:33:47 PDT
Comment on attachment 84510 [details]
Patch

alrighty.
Comment 5 WebKit Commit Bot 2011-03-29 15:07:37 PDT
The commit-queue encountered the following flaky tests while processing attachment 84510 [details]:

http/tests/inspector/console-websocket-error.html bug 57392 (authors: pfeldman@chromium.org and yutak@chromium.org)
The commit-queue is continuing to process your patch.
Comment 6 WebKit Commit Bot 2011-03-29 15:10:28 PDT
Comment on attachment 84510 [details]
Patch

Clearing flags on attachment: 84510

Committed r82332: <http://trac.webkit.org/changeset/82332>
Comment 7 WebKit Commit Bot 2011-03-29 15:10:33 PDT
All reviewed patches have been landed.  Closing bug.