[webkit-reviews] review denied: [Bug 6523] Extract data from SelectionController : [Attachment 5642] patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Duncan Wilcox <duncan at mclink.it>'s
request for review:
Bug 6523: Extract data from SelectionController
http://bugzilla.opendarwin.org/show_bug.cgi?id=6523

Attachment 5642: patch
http://bugzilla.opendarwin.org/attachment.cgi?id=5642&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Please don't landed commented out lines like these:

+//#include <qrect.h>
+//#include "editing/visible_text.h"

I don't think Selection.h needs to include "misc/shared.h".

For files like Selection.cpp we now are back to "using namespace DOM" rather
than individual "using" lines.

The copy constructor and assignment operator that are implemented for Selection
do exactly what the defaults would do. In those cases it's better to not
declare or define them and let the compiler take care of it.

Since Selection.h includes things like PassRefPtr.h and dom_position.h, those
includes can be removed from the top of SelectionController.

-	 case SelectionController::RANGE:
+	 case khtml::Selection::RANGE:

Why do we need a khtml:: prefix in the above code and other lines like it?

This seems like a nice next step. Over time we might refine and adjust what
Selection has vs. SelectionController, and change various clients that hold
SelectionController to hold Selection instead.

The above are minor quibbles but lets get answers to them before we do review+.


Did you run layout tests? Do they all give identical results?