[webkit-reviews] review granted: [Bug 22433] Make selection rect-related methods |const| : [Attachment 25408] Patch to make lots of methods const, changelog

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 22433: Make selection rect-related methods |const|
https://bugs.webkit.org/show_bug.cgi?id=22433

Attachment 25408: Patch to make lots of methods const, changelog
https://bugs.webkit.org/attachment.cgi?id=25408&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.