Bug 117289 – Make CachedResource virtual methods overridden in derived classes private

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Carlos Garcia Campos]

There's some inconsistencies, some of them are private in raw resources and public and all other cached resources.

[Carlos Garcia Campos]

Created attachment 203926 [details]
Patch

This patch applies on top of patch attached to bug #117288

[Darin Adler]

Comment on attachment 203926 [details]
Patch

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.

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 203926 [details])
> 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?

You mean making all of them FINAL or just the public ones? because the private ones will always do a a virtual function dispatch.

> > 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.

[Carlos Garcia Campos]

Created attachment 203948 [details]
Updated patch

Leave data as public in CachedImage as it's used directly and make it FINAL too.

[Darin Adler]

(In reply to comment #3)
> You mean making all of them FINAL or just the public ones? because the private ones will always do a a virtual function dispatch.

They should all be FINAL.

Even the private ones might be called from inside the class. Making them private doesn’t make them uncallable. We want all virtual functions to be FINAL unless we override them. This creates opportunities for the compiler to do optimizations it otherwise couldn’t do.

One shortcut is that we can get the same optimization without marking any virtual functions FINAL if the entire class is marked FINAL.

But we should be in the habit of marking almost every virtual function FINAL.

[Carlos Garcia Campos]

Created attachment 204011 [details]
Patch for landing

Mark the classes as FINAL as suggested by Darin.

[Carlos Garcia Campos]

Committed r151310: <http://trac.webkit.org/changeset/151310>