Followup to https://bugs.webkit.org/show_bug.cgi?id=69790, which introduced a bunch of unchecked casts that rely on CachedResource subtypes and CachedResourceClient subtypes matching each other correctly. As written, it will fail dangerously in the event of a programmer error.
Created attachment 110597 [details] patch
Comment on attachment 110597 [details] patch Clearing flags on attachment: 110597 Committed r97199: <http://trac.webkit.org/changeset/97199>
All reviewed patches have been landed. Closing bug.
Comment on attachment 110597 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=110597&action=review > Source/WebCore/loader/cache/CachedResourceClientWalker.h:54 > + if (m_clientSet.contains(next) && (T::expectedType() == CachedResourceClient::expectedType() || next->type() == T::expectedType())) > + return static_cast<T*>(next); What is the purpose of T::expectedType() == CachedResourceClient::expectedType()? Could this assert the correct type instead?
(In reply to comment #4) > (From update of attachment 110597 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110597&action=review > > > Source/WebCore/loader/cache/CachedResourceClientWalker.h:54 > > + if (m_clientSet.contains(next) && (T::expectedType() == CachedResourceClient::expectedType() || next->type() == T::expectedType())) > > + return static_cast<T*>(next); > > What is the purpose of T::expectedType() == CachedResourceClient::expectedType()? > > Could this assert the correct type instead? The purpose is to handle the notifyFinished() callback correctly. notifyFinished() is called for all CachedResourceClient types, and is templated with the base class rather than a subclass. I suppose we could assert instead of just skipping clients of the wrong type. I don't have a strong opinion.
I suspect that all cases where there are unexpected types on the list are bugs so assert would be the right choice.