Bug 119360 – Update HTMLPreloadScanner to handle img srcset

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Dean Jackson]

Once img srcset lands (110252), the preload scanner should load the value from srcset rather than src (if present). Also, HTMLDocumentParser will have to be aware of the page scale in order to tell the preloader.

[Radar WebKit Bug Importer]

<rdar://problem/14613238>

[Dean Jackson]

Created attachment 207975 [details]
Patch

[Darin Adler]

Comment on attachment 207975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207975&action=review

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:568
> +            m_preloadScanner = adoptPtr(new HTMLPreloadScanner(m_options, document()->url(), document()->page() ? document()->page()->deviceScaleFactor() : 1));

Seems like we need a helper function for this somewhere. There are so many places where we do this scale factor computation!

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:177
> +                if (srcMatchingScale != WTF::nullAtom)

This is not the right way to check if a string is null. Instead it should be !srcMatchingScale.isNull().

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:201
> +        // FIXME: This parser is suboptimal and should be shared
> +        // with HTMLImageElement.
> +        // https://bugs.webkit.org/show_bug.cgi?id=119423

Suboptimal is not a big deal. Sharing with HTMLImageElement, though, is. Why isn't it shared in this patch!? Given the role of the preload scanner, I would suggest we export the function in HTMLImageElement.h and have it be there and use it here. It’s kind of “pre-doing” what the image element will do later.

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:227
> +        return WTF::nullAtom;

Should just return String() to create a null string. No reason to use nullAtom.

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:252
> +        if (attributeValue.isEmpty())
> +            return;

Don't we want to strip leading and trailing HTML spaces before checking if it's empty?

[Dean Jackson]

*** Bug 119464 has been marked as a duplicate of this bug. ***

[Dean Jackson]

Created attachment 208062 [details]
Patch

[Dean Jackson]

New patch. It's different enough that I don't think Darin's r+ should carry over.

I've updated for Darin's comments, in particular that the parser is now shared via static methods on HTMLImageElement.

By coincidence  Yoav Weiss was doing similar work in a duplicate Bug 119464, so I've merged the patches and attributed Yoav.

[Sam Weinig]

Comment on attachment 208062 [details]
Patch

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.

[Dean Jackson]

Comment on attachment 208062 [details]
Patch

Thanks! I made all the requested changes. Code is now in HTMLParserIdioms, and we only process the attributes once in the preloader.

[Dean Jackson]

Committed r153733: <http://trac.webkit.org/changeset/153733>