[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