<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Add event support for link preload."
href="https://bugs.webkit.org/show_bug.cgi?id=158466#c42">Comment # 42</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Add event support for link preload."
href="https://bugs.webkit.org/show_bug.cgi?id=158466">bug 158466</a>
from <span class="vcard"><a class="email" href="mailto:achristensen@apple.com" title="Alex Christensen <achristensen@apple.com>"> <span class="fn">Alex Christensen</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=280903&action=diff" name="attach_280903" title="Patch">attachment 280903</a> <a href="attachment.cgi?id=280903&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=280903&action=review">https://bugs.webkit.org/attachment.cgi?id=280903&action=review</a>
<span class="quote">> Source/WebCore/loader/LinkLoader.cpp:108
> + ASSERT(loader);</span >
make loader a LinkLoader& and call it with *this.
<span class="quote">> Source/WebCore/loader/LinkLoader.cpp:110
> + if (!resource)
> + return std::unique_ptr<LinkPreloadResourceClient>(nullptr);</span >
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.
<span class="quote">> Source/WebCore/loader/LinkLoader.cpp:126
> + default:</span >
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.
<span class="quote">> Source/WebCore/loader/LinkLoader.cpp:127
> + ASSERT_NOT_REACHED();</span >
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?
<span class="quote">> Source/WebCore/loader/LinkLoader.cpp:128
> + return std::unique_ptr<LinkPreloadResourceClient>(nullptr);</span >
ditto. Just nullptr.
<span class="quote">> Source/WebCore/loader/LinkLoader.h:67
> + std::unique_ptr<LinkPreloadResourceClient> m_preloadResourceClient;</span >
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?
<span class="quote">> Source/WebCore/loader/LinkPreloadResourceClients.h:51
> + virtual void clear() = 0;</span >
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.</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>