Bug 137984 – Clean up virtual functions in rendering/

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Chris Dumez]

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

[Chris Dumez]

Created attachment 240299 [details]
Patch

[Chris Dumez]

Created attachment 240304 [details]
Patch

[Chris Dumez]

Created attachment 240306 [details]
Patch

[Chris Dumez]

Created attachment 240312 [details]
Patch

[Darin Adler]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 240320 [details]
Patch

[Chris Dumez]

I fixed the nit and killed the dead class. It was being built on Windows but does not seem to be used.

[Chris Dumez]

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

[Darin Adler]

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.

[Darin Adler]

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

[Chris Dumez]

Created attachment 240322 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 240323 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 240323 [details]
Patch

Clearing flags on attachment: 240323

Committed r175084: <http://trac.webkit.org/changeset/175084>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.