Bug 119360

Summary: Update HTMLPreloadScanner to handle img srcset
Product: WebKit Reporter: Dean Jackson <dino>
Component: ImagesAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, webkit-bug-importer, yoav
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 110252    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch sam: review+

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
<rdar://problem/14613238>
Comment 2 Dean Jackson 2013-08-01 19:17:52 PDT
Created attachment 207975 [details]
Patch
Comment 3 Darin Adler 2013-08-01 20:32:29 PDT
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?
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]
Patch
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]
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.
Comment 8 Dean Jackson 2013-08-05 17:01:10 PDT
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.
Comment 9 Dean Jackson 2013-08-05 18:13:10 PDT
Committed r153733: <http://trac.webkit.org/changeset/153733>