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.
<rdar://problem/14613238>
Created attachment 207975 [details] Patch
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?
*** Bug 119464 has been marked as a duplicate of this bug. ***
Created attachment 208062 [details] Patch
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 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 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.
Committed r153733: <http://trac.webkit.org/changeset/153733>