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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 6 09:29:41 PDT 2013


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.

More information about the webkit-unassigned mailing list