Bug 22433 - Make selection rect-related methods |const|, disambiguate RenderView::selectionRect()
Summary: Make selection rect-related methods |const|, disambiguate RenderView::selecti...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-22 22:42 PST by Simon Fraser (smfr)
Modified: 2008-11-24 12:24 PST (History)
3 users (show)

See Also:


Attachments
Perl script that looks for possible const mismatches (2.26 KB, text/plain)
2008-11-22 22:43 PST, Simon Fraser (smfr)
no flags Details
Output of above perl script (56.82 KB, text/plain)
2008-11-22 22:44 PST, Simon Fraser (smfr)
no flags Details
Patch to make lots of methods const, changelog (85.52 KB, patch)
2008-11-23 19:54 PST, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff
Path to rename selectionRect on RenderView to avoid ambiguity. (6.73 KB, patch)
2008-11-23 22:48 PST, Simon Fraser (smfr)
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2008-11-22 22:42:49 PST
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.
Comment 1 Simon Fraser (smfr) 2008-11-22 22:43:25 PST
Created attachment 25392 [details]
Perl script that looks for possible const mismatches
Comment 2 Simon Fraser (smfr) 2008-11-22 22:44:18 PST
Created attachment 25393 [details]
Output of above perl script
Comment 3 Simon Fraser (smfr) 2008-11-23 19:54:09 PST
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
Comment 4 Darin Adler 2008-11-23 20:04:44 PST
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.
Comment 5 Simon Fraser (smfr) 2008-11-23 22:27:23 PST
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.
Comment 6 Simon Fraser (smfr) 2008-11-23 22:48:15 PST
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.
Comment 7 Simon Fraser (smfr) 2008-11-24 10:37:07 PST
Script committed in 
r38711 = e149b8957b22cda7753ce635057267527cee0532 (trunk)
	M	WebKitTools/ChangeLog
	A	WebKitTools/Scripts/detect-mismatched-virtual-const
Comment 8 Simon Fraser (smfr) 2008-11-24 11:27:16 PST
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 9 mitz 2008-11-24 11:27:22 PST
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 10 mitz 2008-11-24 11:32:46 PST
Comment on attachment 25413 [details]
Path to rename selectionRect on RenderView to avoid ambiguity.

r=me
Comment 11 Simon Fraser (smfr) 2008-11-24 11:58:23 PST
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)
Comment 12 Simon Fraser (smfr) 2008-11-24 12:24:00 PST
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.