Bug 23774

Summary: Selection::validate() should be broken into smaller pieces to be less confusing
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: hyatt, justin.garcia
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
Minor refactoring and cleanup to Selection code
none
Slightly clearer diff, and added documentation of ASSERTS ojan and I kept hitting in this code.
none
updated after ojan and mark's comments justin.garcia: review+

Description Eric Seidel (no email) 2009-02-05 15:44:41 PST
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.
Comment 1 Eric Seidel (no email) 2009-02-05 15:45:25 PST
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(-)
Comment 2 Eric Seidel (no email) 2009-02-05 16:04:55 PST
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(-)
Comment 3 Eric Seidel (no email) 2009-02-05 16:18:57 PST
Adding folks who might like to review this (tiny) piece of wonderment!
Comment 4 Ojan Vafai 2009-02-05 17:34:15 PST
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.
Comment 5 Eric Seidel (no email) 2009-02-06 14:37:18 PST
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
Comment 6 Eric Seidel (no email) 2009-02-06 14:37:34 PST
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(-)
Comment 7 Eric Seidel (no email) 2009-02-06 14:38:15 PST
I've of course run the layout tests and they pass. :)
Comment 8 Justin Garcia 2009-02-06 15:22:01 PST
-    enum EDirection { FORWARD, BACKWARD, RIGHT, LEFT };

Where did it go?
Comment 9 Eric Seidel (no email) 2009-02-06 15:33:22 PST
(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.
Comment 10 Eric Seidel (no email) 2009-02-06 16:09:03 PST
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