WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
137984
Clean up virtual functions in rendering/
https://bugs.webkit.org/show_bug.cgi?id=137984
Summary
Clean up virtual functions in rendering/
Chris Dumez
Reported
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
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
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Chris Dumez
Comment 1
2014-10-22 15:41:07 PDT
Created
attachment 240299
[details]
Patch
Chris Dumez
Comment 2
2014-10-22 16:19:21 PDT
Created
attachment 240304
[details]
Patch
Chris Dumez
Comment 3
2014-10-22 16:57:07 PDT
Created
attachment 240306
[details]
Patch
Chris Dumez
Comment 4
2014-10-22 17:33:29 PDT
Created
attachment 240312
[details]
Patch
Darin Adler
Comment 5
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.
Chris Dumez
Comment 6
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.
Chris Dumez
Comment 7
2014-10-22 19:09:03 PDT
Created
attachment 240320
[details]
Patch
Chris Dumez
Comment 8
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.
Chris Dumez
Comment 9
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
Darin Adler
Comment 10
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.
Darin Adler
Comment 11
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.
Chris Dumez
Comment 12
2014-10-22 20:10:12 PDT
Created
attachment 240322
[details]
Patch
Chris Dumez
Comment 13
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.
Chris Dumez
Comment 14
2014-10-22 20:14:08 PDT
Created
attachment 240323
[details]
Patch
WebKit Commit Bot
Comment 15
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
>
WebKit Commit Bot
Comment 16
2014-10-22 21:32:32 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug