Bug 22433 – Make selection rect-related methods |const|, disambiguate RenderView::selectionRect()

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

[Simon Fraser (smfr)]

While spelunking in the selection code, I noticed a bunch of methods that should be |const|, but were not. I also noticed some instances of virtual methods of the same name, but differing constness (which changes the signature, meaning that they don't override correctly).

I'll attach a patch, and a script that helps with finding methods that intend to override virtual methods, but have mismatched constness.

[Simon Fraser (smfr)]

Created attachment 25392 [details]
Perl script that looks for possible const mismatches

[Simon Fraser (smfr)]

Created attachment 25393 [details]
Output of above perl script

[Simon Fraser (smfr)]

Created attachment 25408 [details]
Patch to make lots of methods const, changelog

Issue caught by the script that this patch fixes:

Both const and non-const versions of selectionRect(bool):
	RenderObject::selectionRect(bool)
...
	RenderView::selectionRect(bool) const

[Darin Adler]

Comment on attachment 25408 [details]
Patch to make lots of methods const, changelog

My general take on this is that const doesn't work well for objects in trees such as RenderObject* and Node* and InlineBox*. Since you're walking the tree so often, it's not so useful to make a distinction and say you have a "const" pointer, but then have a non-const pointer to the children and parent of the node. And it's equally non-useful to treat the entire tree as const.

I was going to suggest eliminating const entirely from those classes.

Making const consistent and "fan out" correctly is also OK. But next time around we might want to consider eliminating the use of const from these tree node families of classes.

I suspect that there were no callers of the virtual RenderObject::selectionRect that actually wanted the RenderView::selectionRect function, and in fact by making the RenderView one override the RenderObject one we might even have introduced some sort of problem. Maybe these two functions should have different names. If the old behavior was wrong there must have been a symptom. I'd really like to know what it was and maybe even make a regression test for it.

Normally we prefer to declare virtual functions virtual explicitly and it's telling that RenderView's definition doesn't declare selectionRect virtual explicitly. Maybe it should now that it's overriding a virtual function -- it wasn't before.

Would you consider checking in the perl script too?

I'm going to say r=me despite these comments.

[Simon Fraser (smfr)]

RenderView's selectionRect() has been const back since it was RenderCanvas, and has mismatched RenderObject's since as far as I could dig (thwarted by file moves at some point).

The only place I can see that may call RenderView::selectionRect() through a base class pointer is the SelectionInfo(RenderObject*, bool) ctor. I added a printf, but never hit it during the layout tests, and I doubt that it can ever get called with a RenderView.

This would argue for renaming RenderView's (and Frame's) selectionRect() to avoid the ambiguity.

[Simon Fraser (smfr)]

Created attachment 25413 [details]
Path to rename selectionRect on RenderView to avoid ambiguity.

Here's an option for removing the selectionRect() ambiguity: rename RenderView::selectionRect() to RenderView::selectionBounds(), and same on Frame. We could then implement RenderView::selectionRect() and ASSERT if it gets called.

[Simon Fraser (smfr)]

Script committed in 
r38711 = e149b8957b22cda7753ce635057267527cee0532 (trunk)
	M	WebKitTools/ChangeLog
	A	WebKitTools/Scripts/detect-mismatched-virtual-const

[Simon Fraser (smfr)]

Comment on attachment 25408 [details]
Patch to make lots of methods const, changelog

I think we've decided that we don't want const creep here. Will address the RenderView::selectionRect() issue in an upcoming patch.

Comment on attachment 25413 [details]
Path to rename selectionRect on RenderView to avoid ambiguity.

I think this is the right thing to do, but I wish there was a better name for the RenderView method. You can change this to r? if you can't think of one, or otherwise upload a new patch with the new name.

Comment on attachment 25413 [details]
Path to rename selectionRect on RenderView to avoid ambiguity.

r=me

[Simon Fraser (smfr)]

Comment on attachment 25413 [details]
Path to rename selectionRect on RenderView to avoid ambiguity.

Committed r38716
	M	WebKit/mac/ChangeLog
	M	WebKit/mac/WebView/WebHTMLView.mm
	M	WebCore/editing/SelectionController.cpp
	M	WebCore/ChangeLog
	M	WebCore/WebCore.base.exp
	M	WebCore/page/mac/FrameMac.mm
	M	WebCore/page/Frame.h
	M	WebCore/page/DragController.cpp
	M	WebCore/page/Frame.cpp
	M	WebCore/rendering/RenderView.cpp
	M	WebCore/rendering/RenderView.h
r38716 = c3a23de1c179af0d729bc78c9ca513713aa4aed6 (trunk)

[Simon Fraser (smfr)]

Filed bug 22461 on the one remaining genuine 'const' mismatch.
Filed bug 22462 general 'const' cleanup unrelated to virtual method mismatch.

Work for this bug is done.