Bug 23774 – Selection::validate() should be broken into smaller pieces to be less confusing

• Component: HTML Editing
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Selection::validate() should be broken into smaller pieces to be less confusing

I've done so.  I think it's a lot easier to read. :)

I also changed EState to SelectionState to match the coding style guidelines.

[Eric Seidel (no email)]

Created attachment 27367 [details]
Minor refactoring and cleanup to Selection code

 WebCore/ChangeLog                     |   36 +++++++++++
 WebCore/editing/Editor.cpp            |   16 +++---
 WebCore/editing/Selection.cpp         |  104 +++++++++++++++++---------------
 WebCore/editing/Selection.h           |   36 ++++++-----
 WebCore/editing/SelectionController.h |    2 +-
 WebCore/editing/TypingCommand.cpp     |   16 +++---
 WebCore/page/Frame.cpp                |   16 ++---
 7 files changed, 134 insertions(+), 92 deletions(-)

[Eric Seidel (no email)]

Created attachment 27368 [details]
Slightly clearer diff, and added documentation of ASSERTS ojan and I kept hitting in this code.

 WebCore/ChangeLog                     |   36 ++++++++++++++++++++++
 WebCore/editing/Editor.cpp            |   16 +++++-----
 WebCore/editing/Selection.cpp         |   54 +++++++++++++++++++-------------
 WebCore/editing/Selection.h           |   36 ++++++++++++----------
 WebCore/editing/SelectionController.h |    2 +-
 WebCore/editing/TypingCommand.cpp     |   16 +++++-----
 WebCore/page/Frame.cpp                |   16 ++++------
 7 files changed, 111 insertions(+), 65 deletions(-)

[Eric Seidel (no email)]

Adding folks who might like to review this (tiny) piece of wonderment!

[Ojan Vafai]

Comment on attachment 27368 [details]
Slightly clearer diff, and added documentation of ASSERTS ojan and I kept hitting in this code.

A nit. (not that I'm a reviewer)

> +++ b/WebCore/editing/Selection.cpp
> +void Selection::setBaseAndExtentToDeepEquivilants()

Typo: Equivalents. Here and elsewhere.

[Eric Seidel (no email)]

Comment on attachment 27368 [details]
Slightly clearer diff, and added documentation of ASSERTS ojan and I kept hitting in this code.

Will post update with spelling correction and fix of ASSERT

[Eric Seidel (no email)]

Created attachment 27419 [details]
updated after ojan and mark's comments

 WebCore/ChangeLog                     |   36 ++++++++++++++++++++++
 WebCore/editing/Editor.cpp            |   16 +++++-----
 WebCore/editing/Selection.cpp         |   52 +++++++++++++++++++-------------
 WebCore/editing/Selection.h           |   36 ++++++++++++----------
 WebCore/editing/SelectionController.h |    2 +-
 WebCore/editing/TypingCommand.cpp     |   16 +++++-----
 WebCore/page/Frame.cpp                |   16 ++++------
 7 files changed, 110 insertions(+), 64 deletions(-)

[Eric Seidel (no email)]

I've of course run the layout tests and they pass. :)

[Justin Garcia]

-    enum EDirection { FORWARD, BACKWARD, RIGHT, LEFT };

Where did it go?

[Eric Seidel (no email)]

(In reply to comment #8)
> -    enum EDirection { FORWARD, BACKWARD, RIGHT, LEFT };
> 
> Where did it go?

It's not needed in this file.  It's already declared in SelectionController as well iirc.

[Eric Seidel (no email)]

Thanks!

VisibleSelection here we come!

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/editing/Editor.cpp
	M	WebCore/editing/Selection.cpp
	M	WebCore/editing/Selection.h
	M	WebCore/editing/SelectionController.h
	M	WebCore/editing/TypingCommand.cpp
	M	WebCore/page/Frame.cpp
Committed r40738