Bug 119360 - Update HTMLPreloadScanner to handle img srcset
Summary: Update HTMLPreloadScanner to handle img srcset
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
Keywords: InRadar
: 119464 (view as bug list)
Depends on: 110252
  Show dependency treegraph
Reported: 2013-07-31 17:25 PDT by Dean Jackson
Modified: 2013-08-05 18:13 PDT (History)
4 users (show)

See Also:

Patch (11.81 KB, patch)
2013-08-01 19:17 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (17.79 KB, patch)
2013-08-03 05:34 PDT, Dean Jackson
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2013-07-31 17:25:50 PDT
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.
Comment 1 Radar WebKit Bug Importer 2013-07-31 17:26:29 PDT
Comment 2 Dean Jackson 2013-08-01 19:17:52 PDT
Created attachment 207975 [details]
Comment 3 Darin Adler 2013-08-01 20:32:29 PDT
Comment on attachment 207975 [details]

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?
Comment 4 Dean Jackson 2013-08-03 04:00:27 PDT
*** Bug 119464 has been marked as a duplicate of this bug. ***
Comment 5 Dean Jackson 2013-08-03 05:34:41 PDT
Created attachment 208062 [details]
Comment 6 Dean Jackson 2013-08-03 05:37:15 PDT
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.
Comment 7 Sam Weinig 2013-08-04 18:18:24 PDT
Comment on attachment 208062 [details]

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.
Comment 8 Dean Jackson 2013-08-05 17:01:10 PDT
Comment on attachment 208062 [details]

Thanks! I made all the requested changes. Code is now in HTMLParserIdioms, and we only process the attributes once in the preloader.
Comment 9 Dean Jackson 2013-08-05 18:13:10 PDT
Committed r153733: <http://trac.webkit.org/changeset/153733>