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

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> 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 211229: Patch
https://bugs.webkit.org/attachment.cgi?id=211229&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=211229&action=review


This looks great. Some nitpick bellow.

Let's iterate on this quickly so you can move on.

> Source/WebCore/ChangeLog:7
> +

You also updated the behaviors to follow the spec regarding data URLs. You
should mention the behavior change in the description.

> Source/WebCore/ChangeLog:8
> +	   No new tests, covered by existing tests.

This is no longer true.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:323
> +//
http://www.w3.org/html/wg/drafts/srcset/w3c-srcset/Overview.html#processing-the
-image-candidates

Draft change by definition.
IMHO, it is good to link the published version with a date:
http://www.w3.org/TR/2013/WD-html-srcset-20130228/#processing-the-image-candida
tes

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:324
> +// Note: Some items in comments below make reference to the spec (prefixed
by numbers).

You don't need this line, that is common enough in WebKit.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:334
> +	   // 5. Collect a sequence of characters that are not space
characters, and let that be url.

This should be moved to #344 now.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:336
> +	   size_t imageUrlStart = srcSetAttribute.find(isNotHTMLSpace,
imageCandidateStart); // 4. Splitting loop: Skip whitespace.

Move the comment above the block.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:343
> +	   // If The current candidate is either totally empty or only contains
space, skipping.
> +	   if (srcSetAttribute[imageUrlStart] == ',') {
> +	       imageCandidateStart = imageUrlStart + 1;
>	       continue;
> +	   }

I kind of agree with your reasoning here. It is probably worth mailing the
standard group to have this clarified in the spec.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:344
> +	   size_t imageUrlEnd = srcSetAttribute.find(isHTMLSpace, imageUrlStart
+ 1);

The comment 5. would be good here.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:346
> +	   if (imageUrlEnd == notFound)
> +	       break;

Not sure I agree with this.
if you have srcset="   foobar.jpg", you would fail here.

Is there a test covering such case?

(*)

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:349
> +	       imageUrlEnd--;

--imageUrlEnd; usually in WebKit.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:354
> +
> +	       // 7. Collect a sequence of characters that are not "," (U+002C)
characters, and let that be descriptors.
> +

You can remove the blank lines above and above and bellow the comment.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:357
> +	       size_t imageScaleStart = srcSetAttribute.find(isNotHTMLSpace,
imageUrlEnd + 1);
> +	       if (imageScaleStart == notFound)
> +		   break;

Same as for (*), I am not sure about this.
You could have srcset="   foobar.jpg   "
Again, do we have tests for this?

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:365
> +		   // Be sure that there are no other descriptors.

Be sure that -> Make sure there are no...

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:367
> +		   while ((separator < srcSetLength - 1) &&
isHTMLSpace(srcSetAttribute[separator]))
> +		       separator++;

I think this may be abusing the separator variable. I would use an other
variable and assign it to separator once you found a comma.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:368
> +		   if ((separator < srcSetLength - 1) &&
srcSetAttribute[separator] != ',') {

I would add a comment explaining this failure case.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:371
> +			   separator = srcSetLength;

break?

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:381
> +		   imgScaleFactor =
charactersToFloat(srcSetAttribute.characters() + imageScaleStart, imageScaleEnd
- imageScaleStart - 1, &validScaleFactor);

The -1 is not obvious here.
To explain it, you can just add more temporary variables. E.g.:
size_t scaleFactorStringLength = imageScaleEnd - imageScaleStart;
size_t scaleFactorStringLengthWithoutUnit = scaleFactorStringLength - 1;
imgScaleFactor = charactersToFloat(srcSetAttribute.characters() +
imageScaleStart, scaleFactorStringLengthWithoutUnit, &validScaleFactor);

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:390
> -	   image.imageURL = decodeURLEscapeSequences(data[0]);
> +	   image.imageURL =
decodeURLEscapeSequences(StringImpl::createWithoutCopying(srcSetAttribute.chara
cters() + imageUrlStart, imageUrlEnd - imageUrlStart));

I feel like you mentioned why createWithoutCopying is safe here but I forgot.
Could you explain again?