[Webkit-unassigned] [Bug 158466] Add event support for link preload.

[bugzilla-daemon at webkit.org]

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

--- Comment #42 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 280903
  --> https://bugs.webkit.org/attachment.cgi?id=280903
Patch

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

> Source/WebCore/loader/LinkLoader.cpp:108
> +    ASSERT(loader);

make loader a LinkLoader& and call it with *this.

> Source/WebCore/loader/LinkLoader.cpp:110
> +    if (!resource)
> +        return std::unique_ptr<LinkPreloadResourceClient>(nullptr);

I think resource should be a CachedResource&.  As it stands right now, this can never be hit because we return early if resource is null in the one callsite.

> Source/WebCore/loader/LinkLoader.cpp:126
> +    default:

I think it would be better to list all the CachedResource types so we don't forget to update this if we change the enum, and so we can verify that we're doing the right thing in all cases.

> Source/WebCore/loader/LinkLoader.cpp:127
> +        ASSERT_NOT_REACHED();

It would be nice to have a comment explaining why this cannot be reached, possibly with a link to the relevant part of the spec.  I glance at this and wonder why there are only 7 of the 14 cases handled.  Is this from section 2.2.1 of the spec?  I see as=foobarxmlthing in the test, but what about as=somethingvalidbutunexpected?

> Source/WebCore/loader/LinkLoader.cpp:128
> +        return std::unique_ptr<LinkPreloadResourceClient>(nullptr);

ditto.  Just nullptr.

> Source/WebCore/loader/LinkLoader.h:67
> +    std::unique_ptr<LinkPreloadResourceClient> m_preloadResourceClient;

So this is just a CachedResourceClient that doesn't do anything, right?  It just puts the resource in the cache and that's it.  Makes sense.  Will the result always be cached?

> Source/WebCore/loader/LinkPreloadResourceClients.h:51
> +    virtual void clear() = 0;

Why is this purely virtual?  All the implementations do the exact same thing.  This feels excessive unless you plan to do something different in any of the clients.

-- 
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/20160622/f4510069/attachment-0001.html>