Bug 137984 - Clean up virtual functions in rendering/
Summary: Clean up virtual functions in rendering/
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-22 15:33 PDT by Chris Dumez
Modified: 2014-10-22 21:32 PDT (History)
5 users (show)

See Also:


Attachments
Patch (115.25 KB, patch)
2014-10-22 15:41 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (117.66 KB, patch)
2014-10-22 16:19 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (115.39 KB, patch)
2014-10-22 16:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (115.35 KB, patch)
2014-10-22 17:33 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (163.33 KB, patch)
2014-10-22 19:09 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (108.23 KB, patch)
2014-10-22 20:10 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (105.73 KB, patch)
2014-10-22 20:14 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2014-10-22 15:33:34 PDT
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
Comment 1 Chris Dumez 2014-10-22 15:41:07 PDT
Created attachment 240299 [details]
Patch
Comment 2 Chris Dumez 2014-10-22 16:19:21 PDT
Created attachment 240304 [details]
Patch
Comment 3 Chris Dumez 2014-10-22 16:57:07 PDT
Created attachment 240306 [details]
Patch
Comment 4 Chris Dumez 2014-10-22 17:33:29 PDT
Created attachment 240312 [details]
Patch
Comment 5 Darin Adler 2014-10-22 18:43:45 PDT
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 6 Chris Dumez 2014-10-22 18:59:30 PDT
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.
Comment 7 Chris Dumez 2014-10-22 19:09:03 PDT
Created attachment 240320 [details]
Patch
Comment 8 Chris Dumez 2014-10-22 19:09:59 PDT
I fixed the nit and killed the dead class. It was being built on Windows but does not seem to be used.
Comment 9 Chris Dumez 2014-10-22 20:07:13 PDT
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 10 Darin Adler 2014-10-22 20:07:21 PDT
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.
Comment 11 Darin Adler 2014-10-22 20:08:45 PDT
(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.
Comment 12 Chris Dumez 2014-10-22 20:10:12 PDT
Created attachment 240322 [details]
Patch
Comment 13 Chris Dumez 2014-10-22 20:12:29 PDT
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.
Comment 14 Chris Dumez 2014-10-22 20:14:08 PDT
Created attachment 240323 [details]
Patch
Comment 15 WebKit Commit Bot 2014-10-22 21:32:27 PDT
Comment on attachment 240323 [details]
Patch

Clearing flags on attachment: 240323

Committed r175084: <http://trac.webkit.org/changeset/175084>
Comment 16 WebKit Commit Bot 2014-10-22 21:32:32 PDT
All reviewed patches have been landed.  Closing bug.