[webkit-reviews] review denied: [Bug 117289] Make CachedResource virtual methods overridden in derived classes private : [Attachment 203926] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 117289: Make CachedResource virtual methods overridden in derived classes
private
https://bugs.webkit.org/show_bug.cgi?id=117289

Attachment 203926: Patch
https://bugs.webkit.org/attachment.cgi?id=203926&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=203926&action=review


Good to make functions as private as possible. But if someone does have a
reason to call a function directly, then I think it’s OK to have some of these
be public. But then it’s helpful to have them be FINAL so we don’t do a virtual
function dispatch for no good reason.

But why no FINAL?

> Source/WebCore/html/ImageDocument.cpp:137
> -    CachedImage* cachedImage = document()->cachedImage();
> +    CachedResource* cachedImage = document()->cachedImage();

This change isn’t good even though it allows us to make overrides private. I
suggest leaving the overrides public instead if they are called like this, but
marking either the function or the class or both FINAL if they are never
overridden further. That way we get a non-virtual function call, slightly more
efficient.

> Source/WebCore/html/ImageDocument.cpp:147
> -	   CachedImage* cachedImage = document()->cachedImage();
> +	   CachedResource* cachedImage = document()->cachedImage();

Same comment as above.

> Source/WebCore/html/ImageDocument.cpp:162
> -	   IntSize size =
flooredIntSize(cachedImage->imageSizeForRenderer(document()->imageElement()->re
nderer(), 1.0f));
> +	   IntSize size =
flooredIntSize(static_cast<CachedImage*>(cachedImage)->imageSizeForRenderer(doc
ument()->imageElement()->renderer(), 1.0f));

Lets not do this.

> Source/WebCore/inspector/InspectorPageAgent.cpp:551
>	       // Skip images that were not auto loaded (images disabled in the
user agent).
> -	       if (static_cast<CachedImage*>(cachedResource)->stillNeedsLoad())

> -		   continue;
> -	       break;
>	   case CachedResource::FontResource:
>	       // Skip fonts that were referenced in CSS but never
used/downloaded.
> -	       if (static_cast<CachedFont*>(cachedResource)->stillNeedsLoad())
> +	       if (cachedResource->stillNeedsLoad())
>		   continue;

This one, I think, is OK, because its nice to not have the casts.