<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_REOPENED "
title="REOPENED - Async loading of image resources"
href="https://bugs.webkit.org/show_bug.cgi?id=134488#c89">Comment # 89</a>
on <a class="bz_bug_link
bz_status_REOPENED "
title="REOPENED - Async loading of image resources"
href="https://bugs.webkit.org/show_bug.cgi?id=134488">bug 134488</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=266613&action=diff" name="attach_266613" title="Patch">attachment 266613</a> <a href="attachment.cgi?id=266613&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=266613&action=review">https://bugs.webkit.org/attachment.cgi?id=266613&action=review</a>
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:70
> +class ImageLoader::ImageLoaderTask : public Microtask {</span >
Should be marked final.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:72
> + ImageLoaderTask(WeakPtr<ImageLoader> loader, CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy shouldBypassMainWorldContentSecurityPolicy)</span >
This should take a WeakPtr&& and use WTF::move, and we should add a move constructor and move assignment operator to WeakPtr.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:192
> AtomicString attr = element().imageSourceURL();</span >
I don’t see any reason to put this into a local variable now that we are only calling a single function on it.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:193
> + String srcURI = sourceURI(attr);</span >
I would name this local variable sourceURLString. In fact, I think the virtual function sourceURI should be renamed to sourceURLString in the future. Then this line of code would be:
String sourceURLString = this->sourceURLString(element().imageSourceURL());
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:194
> + URL url;</span >
I would name this local variable sourceURL.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:201
> CachedResourceHandle<CachedImage> newImage = nullptr;</span >
No need for the "= nullptr" here.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:210
> + AtomicString crossOriginMode = m_element.fastGetAttribute(HTMLNames::crossoriginAttr);</span >
The type here should be auto& or const AtomicString&; there is no need to do a bit of reference count churn on the attribute value just to call equalIgnoringCase on it.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:211
> if (!crossOriginMode.isNull()) {</span >
Is behavior here correct? crossorigin="" means do not allow stored credentials? Do we have test cases covering this?
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:312
> + CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy shouldBypassMainWorldContentSecurityPolicy = CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy::No;</span >
It would be better to use auto here so we didn’t have such a breathtakingly long line of code:
auto shouldBypassMainWorldContentSecurityPolicy = CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy::No;
Also, for a local variable with small scope like this, I think we can use a bit of shorthand:
auto shouldBypassPolicy = CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy::No
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:326
> +bool ImageLoader::shouldLoadImmediately(const AtomicString& attribute) const</span >
This function needs a why comment explaining its policies. Why are each of these valid reasons we should load immediately? I’m not even sure these are all correct or all tested.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:328
> + String srcURI = sourceURI(attribute);</span >
Please don’t use the name URI for any new code. I would call this sourceURLString. Also, I think early return would make this easier to read.
auto sourceURLString = sourceURI(attribute);
if (sourceURLString.isEmpty())
return true;
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:329
> + URL url = element().document().completeURL(srcURI);</span >
I would call this sourceURL.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:330
> + ResourceRequest resourceRequest(url);</span >
I suggest constructing this only where it’s used and not putting it into a local variable.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:331
> + return (srcURI.isEmpty()</span >
No need for the parentheses here.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:333
> + || m_loadManually</span >
This should be an early exit before we do any other work.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:334
> + || !is<HTMLImageElement>(m_element)</span >
This should be an early exit before we do work on the attribute value.
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:336
> + || MemoryCache::singleton().resourceForRequest(resourceRequest, element().document().page()->sessionID()));</span >
What guarantees page() is non-null?
<span class="quote">> Source/WebCore/loader/ImageLoader.cpp:349
> + ASSERT(resource);
> ASSERT(resource == m_image.get());</span >
Not sure I understand the need for a separate assertion here.
Further, we should simply change notifyFinished to take a CachedResource& instead of a CachedResource* in the future. There’s no chance this is going to be null.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>