[webkit-reviews] review granted: [Bug 119360] Update HTMLPreloadScanner to handle img srcset : [Attachment 208062] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Dean Jackson <dino at apple.com>'s request
for review:
Bug 119360: Update HTMLPreloadScanner to handle img srcset
https://bugs.webkit.org/show_bug.cgi?id=119360

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=208062&action=review


> Source/WebCore/html/HTMLImageElement.cpp:131
> +    for (size_t i = 1; i < imageCandidates.size(); ++i) {
> +	   if (imageCandidates[i-1].scaleFactor ==
imageCandidates[i].scaleFactor) {
> +	       imageCandidates.remove(i);
>	       i--;
>	   }
>      }

Is this removal really needed if we are just going to pick the first one below?
 If it is, please either add a comment, or move into its own function with a
clear name.

> Source/WebCore/html/HTMLImageElement.cpp:142
> +    const String& src =  attributeValue.simplifyWhiteSpace(isHTMLSpace);

I don't think using a const String& gets you anything here, but I might be
wrong.

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:176
>	       if (match(attributeName, srcAttr))
>		   setUrlToLoad(attributeValue);
> +	       else if (match(attributeName, srcsetAttr)) {

Since we process all attributes in a loop (see
TokenPreloadScanner::StartTagScanner::processAttributes()), could we make this
more like the normal case and process srcAttr and srcsetAttr all at once?  This
is not a hard block, but I think it would be nice if the logic could be the
same.

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:179
> +		   String srcMatchingScale =
HTMLImageElement::bestFitImageSource(m_deviceScaleFactor, m_urlToLoad,
attributeValue);
> +		   if (!srcMatchingScale.isEmpty())
> +		       setUrlToLoad(srcMatchingScale, true);

Seems like this doesn't really need to check !isEmpty(), as that will be done
in setUrlToLoad, but it doesn't really hurt that much either.  Also, perhaps
bestFitImageSource should go in either its own file or something like
HTMLParserIdioms

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:221
> +	   String url = stripLeadingAndTrailingHTMLSpaces(value);
> +	   if (url.isEmpty())
> +	       return;

This does an allocation, so it should move below the early return below.