[webkit-reviews] review granted: [Bug 119413] srcset algorithm breaks base64 src attributes : [Attachment 207963] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Aug 1 16:27:30 PDT 2013
Darin Adler <darin at apple.com> has granted Dean Jackson <dino at apple.com>'s
request for review:
Bug 119413: srcset algorithm breaks base64 src attributes
https://bugs.webkit.org/show_bug.cgi?id=119413
Attachment 207963: Patch
https://bugs.webkit.org/attachment.cgi?id=207963&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=207963&action=review
EWS couldn't apply the patch
> Source/WebCore/html/HTMLImageElement.cpp:115
> + return m_bestFitImageURL.isEmpty() ? getAttribute(srcAttr) :
m_bestFitImageURL;
Should use fastGetAttribute, I think.
> Source/WebCore/html/HTMLImageElement.cpp:148
> + const String& srcSetAttributeValue =
getAttribute(srcsetAttr).string().simplifyWhiteSpace(isHTMLSpace);
Should use fastGetAttribute. Calling simplifyWhiteSpace on the entire string
seems a bit like overkill to me, but I guess it’s OK.
> Source/WebCore/html/HTMLImageElement.cpp:151
> + srcSetAttributeValue.split(',', srcSetTokens);
Sure seems like a shame to allocate all these separate strings for the tokens.
There must be some nicer way to write the parser. Ideally for good performance
we wouldn't construct substrings at all, but that would require a version of
toFloat and decodeURLEscapeSequences that operated on a substring.
> Source/WebCore/html/HTMLImageElement.cpp:172
> + image.imageURL = decodeURLEscapeSequences(data[0]);
This seems like a strange thing to do. Why would we decode URL escape sequences
here? Is there a test case that demonstrates the benefit of this?
> Source/WebCore/html/HTMLImageElement.cpp:196
> + determineBestImageForScaleFactor();
It seems a shame to do this twice during the initial parse of an <img> that has
both src and srcset attributes.
More information about the webkit-reviews
mailing list