[Webkit-unassigned] [Bug 121899] Optimize strings copies in srcset parser

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=121899


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #212732|review?                     |review-
               Flag|                            |




--- Comment #6 from Benjamin Poulain <benjamin at webkit.org>  2013-09-26 16:08:31 PST ---
(From update of attachment 212732)
View in context: https://bugs.webkit.org/attachment.cgi?id=212732&action=review

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:303
> +    size_t imageURLStart;

String length are unsigned. You should have imageURLStart unsigned .

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:309
> +        return !imageURLStart && !imageURLLength;

Isn't !imageURLLength enough?

The method should probably be hasImageURL() and the caller use "!hasImage()".

>> Source/WebCore/html/parser/HTMLParserIdioms.cpp:433
>> -            return String(imageCandidates[i].imageURL);
>> +            return imageCandidates[i].hasNoImage() ? srcAttribute : String(srcsetAttribute.characters() + imageCandidates[i].imageURLStart, imageCandidates[i].imageURLLength);
>>      }
>> -    return String(imageCandidates.last().imageURL);
>> +    return imageCandidates.last().hasNoImage() ? srcAttribute : String(srcsetAttribute.characters() + imageCandidates.last().imageURLStart, imageCandidates.last().imageURLLength);
> 
> Please use srcsetAttribute.substring() instead. That's more readable, and could help future optimizations.
> 
> Also, I'd use a local variable for imageCandidates.last().

substring() would be better indeed. StringImpl::substring takes into account the 8/16 bits strings.
You could even use String::substringSharingImpl() here since srcsetAttribute will stay alive anyway.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.