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
Nate Chapin
2011-10-10 15:03:17 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 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? |