Bug 6523 – Extract data from SelectionController

• Component: HTML Editing
• Status: RESOLVED FIXED

[Duncan Wilcox]

The attached patch refactors the data portions of SelectionController into its own class, "Selection".

The extracted data are the base/extent/start/end positions and cursor affinity. Also a couple cached 
values generated by validation are kept around in the new class.

This patch does away with all the different affinities (start/end/base/extent) that were never implemented 
properly, since affinity only makes sense when the selection is a caret (as per discussion with mjs and 
darin on #webkit).

This is a step in the development of bug 4582.

[Duncan Wilcox]

Created attachment 5642 [details]
patch

[Darin Adler]

Comment on attachment 5642 [details]
patch

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?

[Duncan Wilcox]

Created attachment 5649 [details]
patch

As mentioned in #webkit, without the khtml:: prefix it doesn't compile, not
sure why.

Layout tests run fine.

All other issues should be addressed in this patch.

[Darin Adler]

Comment on attachment 5649 [details]
patch

r=me