Bug 69790 – Make CachedResourceClient a base class, with specialized callbacks in subclasses

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

[Nate Chapin]

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

[Nate Chapin]

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.

[Adam Barth]

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?

[Nate Chapin]

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

[Nate Chapin]

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

[Adam Barth]

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.

[WebKit Review Bot]

Comment on attachment 110433 [details]
patch without diamond inheritance

Clearing flags on attachment: 110433

Committed r97113: <http://trac.webkit.org/changeset/97113>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

CachedResourceClientWalker needs some templating.

Lots of other nasty casting still left too.

[Nate Chapin]

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