[Webkit-unassigned] [Bug 117289] Make CachedResource virtual methods overridden in derived classes private

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=117289


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #203926|review?                     |review-
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2013-06-06 09:28:15 PST ---
(From update of attachment 203926)
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()->renderer(), 1.0f));
> +        IntSize size = flooredIntSize(static_cast<CachedImage*>(cachedImage)->imageSizeForRenderer(document()->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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.