[Webkit-unassigned] [Bug 134488] Async loading of image resources

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=134488

--- Comment #89 from Darin Adler <darin at apple.com> ---
Comment on attachment 266613
  --> https://bugs.webkit.org/attachment.cgi?id=266613
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266613&action=review

> Source/WebCore/loader/ImageLoader.cpp:70
> +class ImageLoader::ImageLoaderTask : public Microtask {

Should be marked final.

> Source/WebCore/loader/ImageLoader.cpp:72
> +    ImageLoaderTask(WeakPtr<ImageLoader> loader, CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy shouldBypassMainWorldContentSecurityPolicy)

This should take a WeakPtr&& and use WTF::move, and we should add a move constructor and move assignment operator to WeakPtr.

> Source/WebCore/loader/ImageLoader.cpp:192
>      AtomicString attr = element().imageSourceURL();

I don’t see any reason to put this into a local variable now that we are only calling a single function on it.

> Source/WebCore/loader/ImageLoader.cpp:193
> +    String srcURI = sourceURI(attr);

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());

> Source/WebCore/loader/ImageLoader.cpp:194
> +    URL url;

I would name this local variable sourceURL.

> Source/WebCore/loader/ImageLoader.cpp:201
>      CachedResourceHandle<CachedImage> newImage = nullptr;

No need for the "= nullptr" here.

> Source/WebCore/loader/ImageLoader.cpp:210
> +        AtomicString crossOriginMode = m_element.fastGetAttribute(HTMLNames::crossoriginAttr);

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.

> Source/WebCore/loader/ImageLoader.cpp:211
>          if (!crossOriginMode.isNull()) {

Is behavior here correct? crossorigin="" means do not allow stored credentials? Do we have test cases covering this?

> Source/WebCore/loader/ImageLoader.cpp:312
> +    CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy shouldBypassMainWorldContentSecurityPolicy = CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy::No;

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

> Source/WebCore/loader/ImageLoader.cpp:326
> +bool ImageLoader::shouldLoadImmediately(const AtomicString& attribute) const

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.

> Source/WebCore/loader/ImageLoader.cpp:328
> +    String srcURI = sourceURI(attribute);

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;

> Source/WebCore/loader/ImageLoader.cpp:329
> +    URL url = element().document().completeURL(srcURI);

I would call this sourceURL.

> Source/WebCore/loader/ImageLoader.cpp:330
> +    ResourceRequest resourceRequest(url);

I suggest constructing this only where it’s used and not putting it into a local variable.

> Source/WebCore/loader/ImageLoader.cpp:331
> +    return (srcURI.isEmpty()

No need for the parentheses here.

> Source/WebCore/loader/ImageLoader.cpp:333
> +        || m_loadManually

This should be an early exit before we do any other work.

> Source/WebCore/loader/ImageLoader.cpp:334
> +        || !is<HTMLImageElement>(m_element)

This should be an early exit before we do work on the attribute value.

> Source/WebCore/loader/ImageLoader.cpp:336
> +        || MemoryCache::singleton().resourceForRequest(resourceRequest, element().document().page()->sessionID()));

What guarantees page() is non-null?

> Source/WebCore/loader/ImageLoader.cpp:349
> +    ASSERT(resource);
>      ASSERT(resource == m_image.get());

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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151211/fdbccb3d/attachment.html>