Clean up virtual functions in rendering/ by: - Making virtual functions final when possible - Making classes final when possible - Using 'override' when appropriate - Explicitly marking functions / destructors as virtual when they are inherently virtual - Dropping virtual destructors when the class does not have subclasses and mark the class as final, to get rid of unnecessary vtables - Making isXXX() virtual functions private on XXX classes to avoid unnecessary type checks - De-virtualizing some functions that do not need to be virtual - Dropping final for virtual functions in classes already marked as final
Created attachment 240299 [details] Patch
Created attachment 240304 [details] Patch
Created attachment 240306 [details] Patch
Created attachment 240312 [details] Patch
Comment on attachment 240312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240312&action=review Are you sure that none of the functions that you converted to non-virtual are overridden in derived classes? I see that they are not overriding things in base classes, but I am not 100% sure none of them are overridden in derived ones. > Source/WebCore/rendering/RenderScrollbarPart.h:60 > + virtual void imageChanged(WrappedImagePtr, const IntRect* = 0) override; nullptr > Source/WebCore/rendering/RenderThemeSafari.h:100 > + // FIXME: Shouldn't this take a const FloatRect&, and override the parent's paintTextField()? > + bool paintTextField(const RenderObject&, const PaintInfo&, const IntRect&); Yes. But I think this entire class is dead code and should be deleted. > Source/WebCore/rendering/RenderThemeSafari.h:104 > + // FIXME: Shouldn't this take a const FloatRect&, and override the parent's paintTextArea()? > + bool paintTextArea(const RenderObject&, const PaintInfo&, const IntRect&); Yes. But I think this entire class is dead code and should be deleted. > Source/WebCore/rendering/RenderThemeSafari.h:108 > + // FIXME: Shouldn't this take a const FloatRect&, and override the parent's paintMenuList()? > + bool paintMenuList(const RenderObject&, const PaintInfo&, const IntRect&); Yes. But I think this entire class is dead code and should be deleted.
Comment on attachment 240312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240312&action=review I went through each devirtualization one-by-one and double-checked. Please see comments below. > Source/WebCore/rendering/ClipPathOperation.h:55 > + OperationType type() const { return m_type; } I double-checked that these were not overridden in subclasses. > Source/WebCore/rendering/EllipsisBox.h:43 > + int height() const { return m_height; } The class is final so there is no way someone was overriding this one. > Source/WebCore/rendering/RenderLayer.h:792 > + void filterNeedsRepaint(); The class is final so there is no way this was overridden. >> Source/WebCore/rendering/RenderThemeSafari.h:100 >> + bool paintTextField(const RenderObject&, const PaintInfo&, const IntRect&); > > Yes. But I think this entire class is dead code and should be deleted. It is being built by the win-ews at least as I got errors on this file in earlier revisions. > Source/WebCore/rendering/RenderVideo.h:53 > + bool shouldDisplayVideo() const; The class is final so there is no way someone was overriding this. > Source/WebCore/rendering/svg/RenderSVGRoot.h:93 > + FloatRect repaintRectInLocalCoordinatesExcludingSVGShadow() const { return m_repaintBoundingBoxExcludingShadow; } The class is final so no one could override this. > Source/WebCore/rendering/svg/SVGInlineTextBox.h:43 > + int selectionTop() { return top(); } The class is final so no one could override this. > Source/WebCore/rendering/svg/SVGInlineTextBox.h:44 > + int selectionHeight() { return static_cast<int>(ceilf(m_logicalHeight)); } The class is final so no one could override this.
Created attachment 240320 [details] Patch
I fixed the nit and killed the dead class. It was being built on Windows but does not seem to be used.
Comment on attachment 240320 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240320&action=review > Source/WebCore/rendering/RenderThemeSafari.cpp:-81 > -PassRefPtr<RenderTheme> RenderTheme::themeForPage(Page* page) Hmm, This doesn't appear to be dead code after all. This function is being used. USE(SAFARI_THEME) appears to be true on win-ews. #if PLATFORM(WIN) && USE(CG) #define WTF_USE_SAFARI_THEME 1 #endif
Comment on attachment 240320 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240320&action=review > Source/WebCore/rendering/RenderThemeSafari.cpp:-94 > -PassRefPtr<RenderTheme> RenderTheme::themeForPage(Page* page) > -{ > - static RenderTheme* safariTheme = RenderThemeSafari::create().leakRef(); > - static RenderTheme* windowsTheme = RenderThemeWin::create().leakRef(); > - > - // FIXME: This is called before Settings has been initialized by WebKit, so will return a > - // potentially wrong answer the very first time it's called (see > - // <https://bugs.webkit.org/show_bug.cgi?id=26493>). > - if (Settings::shouldPaintNativeControls()) { > - RenderTheme::setCustomFocusRingColor(safariTheme->platformFocusRingColor()); > - return windowsTheme; // keep the reference of one. > - } > - return safariTheme; // keep the reference of one. > -} Looks like we still need this function, but it can move into another source file, probably RenderThemeWin. It would be roughly like this: PassRefPtr<RenderTheme> RenderTheme::themeForPage(Page*) { static RenderTheme* windowsTheme = RenderThemeWin::create().leakRef(); return windowsTheme; } Not sure what we need to do about focus ring color. Someone should check what iTunes needs/uses.
(In reply to comment #9) > This doesn't appear to be dead code after all. This function is being > used. USE(SAFARI_THEME) appears to be true on win-ews. > > #if PLATFORM(WIN) && USE(CG) > #define WTF_USE_SAFARI_THEME 1 > #endif I think the issue is whether Settings::shouldPaintNativeControls is true or false with our actual Windows clients. If itβs always true in practice, then we can delete RenderThemeSafari. Sorry to push you to do it prematurely, though.
Created attachment 240322 [details] Patch
Since Settings::shouldPaintNativeControls() might return false in practice (causing RenderThemeSafari to be used on Windows), I propose to keep the changes to RenderThemeSafari out of this patch until we figure out if it can safely be removed. The patch is rather large and I'd like to land it relatively quickly to avoid conflicts.
Created attachment 240323 [details] Patch
Comment on attachment 240323 [details] Patch Clearing flags on attachment: 240323 Committed r175084: <http://trac.webkit.org/changeset/175084>
All reviewed patches have been landed. Closing bug.