[Webkit-unassigned] [Bug 25056] Make editing stop using Position::m_offset directly

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=25056





------- Comment #4 from darin at apple.com  2009-04-27 15:56 PDT -------
(From update of attachment 29273)
I think you should first make a patch that changes all use of m_offset to
deprecatedEditingOffset(), then a separate patch that moves to using the other
functions such as computeOffsetInContainerNode. The renaming should be separate
from the behavior change.

> +    // This silently converts the position to being parent relative before setting it on the Range

That's a strange comment. What else could the function do? What do you mean by
"silently"?

>  short Range::compareBoundaryPoints(const Position& a, const Position& b)
>  {
> -    return compareBoundaryPoints(a.node(), a.m_offset, b.node(), b.m_offset);
> +    // FIXME: Some callers pass in sibling-anchored positions.
> +    // Those callers should move to a new comparePositions function, then
> +    // compareBoundaryPoints need only care about parent-anchored node/offset pairs.
> +    return compareBoundaryPoints(a.anchorNode(), a.deprecatedEditingOffset(), b.anchorNode(), b.deprecatedEditingOffset());
>  }

The change above seems wrong. Shouldn't this be using contaonerNode and
computeOffsetInContainerNode?

> +    void setStart(const Position&, ExceptionCode&);
> +    void setEnd(const Position&, ExceptionCode&);

Your change makes Range lower level than Position. Range is based on raw DOM,
and Position is coming into its own as a higher level editing building block.

Accordingly, I don't think these functions should be added to the Range class.
We should remove any use of Position from Range.h and put helper functions for
using Position with Range into Position.h or some other header.

> -    return Range::compareBoundaryPoints(node, 0, start.node(), start.m_offset) >= 0 &&
> +    return Range::compareBoundaryPoints(node, 0, start.anchorNode(), start.deprecatedEditingOffset()) >= 0 &&

> -    bool isFullyAfterEnd = Range::compareBoundaryPoints(node, 0, end.node(), end.m_offset) > 0;
> +    bool isFullyAfterEnd = Range::compareBoundaryPoints(node, 0, end.containerNode(), end.offsetInContainerNode()) > 0;

This seems strange to me. Two nearly identical cases, but in one you used
anchorNode/deprecatedEditingOffset and in the other
containerNode/offsetInContainerNode.

The patch is *way* too big for me to review.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.