Bug 69567 – Make IconLoader a CachedResourceClient

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

This, combined with bugs.webkit.org/show_bug.cgi?id=61225, would allow us to remove SubresourceLoaderClient and simplify the layering in the loader.  Unlike bugs.webkit.org/show_bug.cgi?id=61225, this patch is relatively small.

[Nate Chapin]

Created attachment 110037 [details]
patch

This is plumbing only, the CachedResource for an icon load is never reused.  I'd be happy to change that policy in a later patch, if others think it's a good idea.

[Adam Barth]

Comment on attachment 110037 [details]
patch

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

This looks great.  One question: Why "raw" resource instead of "icon" resource?  Are you planning to use this kind of cached resource for other purposes?

> Source/WebCore/loader/cache/CachedResource.h:139
> +    virtual bool ignoreForRequestCount() const

Why virtual?

[Nate Chapin]

(In reply to comment #2)
> (From update of attachment 110037 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110037&action=review
> 
> This looks great.  One question: Why "raw" resource instead of "icon" resource?  Are you planning to use this kind of cached resource for other purposes?

The behavior of the CachedResource for icons will be pretty much exactly the behavior of the CachedResource for DocumentThreadableLoader (see the naming in https://bugs.webkit.org/show_bug.cgi?id=61225)

Also, I was trying to indicate that this CachedResource type has no decoded version of the data, unlike all of the other types.

> 
> > Source/WebCore/loader/cache/CachedResource.h:139
> > +    virtual bool ignoreForRequestCount() const
> 
> Why virtual?

I had been thinking that it might make sense to allow CachedResource subtypes to override this, but I don't think it's actually necessary.  Will remove before landing.

[Nate Chapin]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 110037 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=110037&action=review
> > 
> > This looks great.  One question: Why "raw" resource instead of "icon" resource?  Are you planning to use this kind of cached resource for other purposes?

> 
> The behavior of the CachedResource for icons will be pretty much exactly the behavior of the CachedResource for DocumentThreadableLoader (see the naming in https://bugs.webkit.org/show_bug.cgi?id=61225)

I hit submit too soon.  The icon behavior should be a strict subset of the behavior for DocumentThreadableLoader

[Adam Barth]

Ok.  We might end up with empty subclasses so that we can continue to do security checks using semantically meaningful type names, but that's a super minor issue and something we can worry about in the future.

[Nate Chapin]

Created attachment 110167 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 110167 [details]
Patch for landing

Clearing flags on attachment: 110167

Committed r96961: <http://trac.webkit.org/changeset/96961>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.