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).
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 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?
(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.
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 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 on attachment 110433 [details] patch without diamond inheritance Clearing flags on attachment: 110433 Committed r97113: <http://trac.webkit.org/changeset/97113>
All reviewed patches have been landed. Closing bug.
CachedResourceClientWalker needs some templating. Lots of other nasty casting still left too.
(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?