[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 #45 from Yoav Weiss <yoav at yoav.ws> ---
(In reply to comment #42)
> Comment on attachment 280903 [details]
> 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.

done

> 
> > 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.

done

> 
> > 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.

done

> 
> > 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?

I'll add one

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

done

> 
> > 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?

Yeah, that's just here to keep the preloadResourceClient alive, in order to get onload/onerror events. The Resource will always go in the memcache.

> 
> > 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.

That's a pure virtual because clearResource() is only defined for descendants of CachedResourceClient, not for LinkPreloadResourceClient. So, I didn't find a way to centralize the implementation of that function.

-- 
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/20160628/1f805714/attachment-0001.html>