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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Aug 1 20:32:44 PDT 2013


Darin Adler <darin at apple.com> 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 207975: Patch
https://bugs.webkit.org/attachment.cgi?id=207975&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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?


More information about the webkit-reviews mailing list