Bug 69790

Summary: Make CachedResourceClient a base class, with specialized callbacks in subclasses
Product: WebKit Reporter: Nate Chapin <japhet>
Component: WebCore Misc.Assignee: Nate Chapin <japhet>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, koivisto, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61225    
Attachments:
Description Flags
patch
none
patch without diamond inheritance none

Description Nate Chapin 2011-10-10 15:03:17 PDT
See https://bugs.webkit.org/show_bug.cgi?id=61225#c17

notifyFinished() is the only callback which is implemented in CachedResource.cpp, so it makes sense to leave that in CachedResourceClient.  However, all the other callbacks are specific to a given type of CachedResource.

This patch will make each subtype of CachedResource use a different CachedResourceClient (e.g., CachedImage will expect a CachedImageClient).  The exceptions to this will be Scripts (since they only use notifyFinished()) and Links (since they use CachedResource directly).
Comment 1 Nate Chapin 2011-10-10 15:11:53 PDT
Created attachment 110411 [details]
patch

I tried very hard to find all the platform-specific CachedResourceClients, but I would be surprised if I succeeded.  EWS will probably explode.
Comment 2 Adam Barth 2011-10-10 15:22:00 PDT
Comment on attachment 110411 [details]
patch

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

This looks like a nice separation-of-concerns win.  One question below.  I wonder if we should keep the two stylesheet interfaces together to avoid diamond inheritance.

> Source/WebCore/dom/ProcessingInstruction.h:35
> -class ProcessingInstruction : public ContainerNode, private CachedResourceClient {
> +class ProcessingInstruction : public ContainerNode, private CachedXSLStyleSheetClient, private CachedCSSStyleSheetClient {

Are we going to have a diamond inheritance problem here?
Comment 3 Nate Chapin 2011-10-10 15:24:06 PDT
(In reply to comment #2)
> (From update of attachment 110411 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110411&action=review
> 
> This looks like a nice separation-of-concerns win.  One question below.  I wonder if we should keep the two stylesheet interfaces together to avoid diamond inheritance.
> 
> > Source/WebCore/dom/ProcessingInstruction.h:35
> > -class ProcessingInstruction : public ContainerNode, private CachedResourceClient {
> > +class ProcessingInstruction : public ContainerNode, private CachedXSLStyleSheetClient, private CachedCSSStyleSheetClient {
> 
> Are we going to have a diamond inheritance problem here?

Yes.

See ProcessingInstruction.cpp for how I handled it in the one place it currently exists.
Comment 4 Nate Chapin 2011-10-10 16:35:47 PDT
Created attachment 110433 [details]
patch without diamond inheritance

I talked with abarth some more off-bugzilla, and we concluded that the diamond inheritance is a disaster waiting to happen.  This new patch has a single CachedResourceClient subclass for both XSL and CSS stylesheets (CachedStyleSheetClient).
Comment 5 Adam Barth 2011-10-10 17:06:09 PDT
Comment on attachment 110433 [details]
patch without diamond inheritance

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

> Source/WebCore/loader/cache/CachedFont.cpp:83
> -        c->fontLoaded(this);
> +        static_cast<CachedFontClient*>(c)->fontLoaded(this);

I wish there was a way to check this cast.
Comment 6 WebKit Review Bot 2011-10-10 17:40:27 PDT
Comment on attachment 110433 [details]
patch without diamond inheritance

Clearing flags on attachment: 110433

Committed r97113: <http://trac.webkit.org/changeset/97113>
Comment 7 WebKit Review Bot 2011-10-10 17:40:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Antti Koivisto 2011-10-11 11:47:34 PDT
CachedResourceClientWalker needs some templating.

Lots of other nasty casting still left too.
Comment 9 Nate Chapin 2011-10-11 12:40:23 PDT
(In reply to comment #8)
> CachedResourceClientWalker needs some templating.
> 
> Lots of other nasty casting still left too.

My current thought is to:
* Add a type enum to CachedResourceClient, a la CachedResource::type().
* Template CachedResourceClientWalker and have it do the filtering and downcasting.

Does that seem reasonable?