Bug 117289

Created attachment 203926 [details] Patch This patch applies on top of patch attached to bug #117288

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.

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

Created attachment 203948 [details] Updated patch Leave data as public in CachedImage as it's used directly and make it FINAL too.

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

Created attachment 204011 [details] Patch for landing Mark the classes as FINAL as suggested by Darin.

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