[webkit-reviews] review denied: [Bug 119423] Improve srcset parser : [Attachment 210285] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Romain Perier
<romain.perier at gmail.com>'s request for review:
Bug 119423: Improve srcset parser
https://bugs.webkit.org/show_bug.cgi?id=119423

Attachment 210285: Patch
https://bugs.webkit.org/attachment.cgi?id=210285&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=210285&action=review


I think this looks almost ready - just some minor comments.

However, the fact that this is custom parsing code makes me nervous. How
confident are we that our tests cover all ranges of correct and incorrect
input? It would be nice if we had some stress tests for really bad input (e.g.
malformed data:, encoded commas, etc).

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:322
> +static void parseImagesWithScaleFromSrcsetAttribute(const String&
srcSetAttribute, ImageCandidates& imageCandidates)

I guess for consistency "FromSrcsetAttribute" should be "FromSrcSetAttribute"

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:329
> +	   // Collect a sequence of characters that are not space, it's the
url.

I don't think we need this comment. Or it should be "Find the next non-space
character in the attribute."

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:350
> +	   // Collect a sequence of characters that are neither ',' nor space,
it's the descriptor.
> +	   size_t imageScaleStart = srcSetAttribute.find(isNotHTMLSpace,
imageUrlEnd + 1);

The comment doesn't match the code. isNotHTMLSpace doesn't check for ','.