[webkit-reviews] review denied: [Bug 121899] Optimize strings copies in srcset parser : [Attachment 212732] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Sep 26 16:09:30 PDT 2013
Benjamin Poulain <benjamin at webkit.org> has denied Romain Perier
<romain.perier at gmail.com>'s request for review:
Bug 121899: Optimize strings copies in srcset parser
https://bugs.webkit.org/show_bug.cgi?id=121899
Attachment 212732: Patch
https://bugs.webkit.org/attachment.cgi?id=212732&action=review
------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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.
More information about the webkit-reviews
mailing list