Bug 25500

Summary: Remove Range::compareBoundaryPoints(Position, Position) per Darin's suggestion in bug 25056
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: HTML EditingAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 25056    
Attachments:
Description Flags
Remove Range::compareBoundaryPoints(Position, Position) per Darin's suggestion in bug 25056
mjs: review+
Add a compareBoundaryPoints which takes RangeBoundaryPoints mjs: review+

Description Eric Seidel (no email) 2009-04-30 16:58:42 PDT
Remove Range::compareBoundaryPoints(Position, Position) per Darin's suggestion in bug 25056

Darin indicated the Range should deal only with primitive DOM node/offset pairs, and that Position (which is a more robust editing construct) should have its own comparison functions and that Range.h should not mention Position at all.

Turns out that Position already has a comparePositions() function (which knows how to additionally handled positions in shadow trees).  So I've just changed all callers of compareBoundaryPoints(Position, Position) to use the existing comparePositions() function.  I've also added a comparePositions which takes VisiblePositions for convenience.
Comment 1 Eric Seidel (no email) 2009-04-30 17:01:20 PDT
Created attachment 29930 [details]
Remove Range::compareBoundaryPoints(Position, Position) per Darin's suggestion in bug 25056

 11 files changed, 77 insertions(+), 34 deletions(-)
Comment 2 Eric Seidel (no email) 2009-04-30 17:14:14 PDT
Created attachment 29931 [details]
Add a compareBoundaryPoints which takes RangeBoundaryPoints

 3 files changed, 32 insertions(+), 11 deletions(-)
Comment 3 Maciej Stachowiak 2009-05-21 20:26:09 PDT
Comment on attachment 29930 [details]
Remove Range::compareBoundaryPoints(Position, Position) per Darin's suggestion in bug 25056

Is the VisiblePosition version of comparePositions ever used? I couldn't tell from the diff. If not, I suggest removing it.
Comment 4 Maciej Stachowiak 2009-05-22 01:06:54 PDT
Comment on attachment 29931 [details]
Add a compareBoundaryPoints which takes RangeBoundaryPoints

r=me
Comment 5 Maciej Stachowiak 2009-05-22 01:09:49 PDT
Comment on attachment 29930 [details]
Remove Range::compareBoundaryPoints(Position, Position) per Darin's suggestion in bug 25056

Mostly looks ok. But please remove this function if it's not actually used. Would rather not add dead code.

+int comparePositions(const VisiblePosition& a, const VisiblePosition& b)
+{
+    return comparePositions(a.deepEquivalent(), b.deepEquivalent());
+}
+
Comment 6 Eric Seidel (no email) 2009-06-02 14:31:27 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/dom/Range.cpp
	M	WebCore/dom/Range.h
	M	WebCore/editing/ApplyStyleCommand.cpp
	M	WebCore/editing/CompositeEditCommand.cpp
	M	WebCore/editing/DeleteSelectionCommand.cpp
	M	WebCore/editing/SelectionController.cpp
	M	WebCore/editing/VisibleSelection.cpp
	M	WebCore/editing/htmlediting.cpp
	M	WebCore/editing/htmlediting.h
	M	WebCore/page/EventHandler.cpp
Committed r44367
	M	WebCore/ChangeLog
	M	WebCore/dom/Range.cpp
	M	WebCore/dom/Range.h
Committed r44368