Bug 55650 – Give Position's upstream and downstream more descriptive names

• Component: HTML Editing
• Status: REOPENED

[Ryosuke Niwa]

While I was working on the bug 55098, I realized that VisiblePosition can be upstream even if its affinity claims to be downstream.  This bug is due to the following lines in http://trac.webkit.org/browser/trunk/Source/WebCore/editing/VisiblePosition.cpp.

456	    Position candidate = position.upstream();
457	    if (candidate.isCandidate())
458	        return candidate;
459	    candidate = position.downstream();
460	    if (candidate.isCandidate())
461	        return candidate;

It returns an upstream position but it doesn't modify its affinity so we end up with a VisiblePosition that stores an upstream position but claims to be a downstream position.

[Levi Weintraub]

I believe you're mixing your concepts here. Position's upstream/downstream concepts are not the same as VisiblePosition's; Upstream and downstream Positions are meant to be visually equivalent while upstream and downstream VisiblePositions explicitly are not.

In other words, in the following scenario:

<div>foo<br/>
<span>bar</span>
</div>

A Position at the beginning of the second line would upstream to [span, 0] and downstream to ["bar", 0], but a VisiblePosition would jump from a downstream of [span, 0] to [br, 0].

[Ryosuke Niwa]

(In reply to comment #1)
> I believe you're mixing your concepts here. Position's upstream/downstream concepts are not the same as VisiblePosition's; Upstream and downstream Positions are meant to be visually equivalent while upstream and downstream VisiblePositions explicitly are not.

:( I wasn't aware of this.  Thanks for the clarification.

[Levi Weintraub]

No problem. The naming is confusing, I know :(

[Justin Garcia]

(In reply to comment #3)
> No problem. The naming is confusing, I know :(

Maybe someone should do some renaming. Names that are descriptive for upstream()/ downstream()  would be really verbose though I think.

[Levi Weintraub]

(In reply to comment #4)
> Maybe someone should do some renaming. Names that are descriptive for upstream()/ downstream()  would be really verbose though I think.

I think renaming the Position methods would be do-able. Something like next/previousVisuallyEquivalentPosition?

[Ryosuke Niwa]

Okay, let's rename these two member functions.

[Ryosuke Niwa]

Created attachment 85034 [details]
work in progress

[Levi Weintraub]

(In reply to comment #7)
> Created an attachment (id=85034) [details]
> work in progress

I'm glad you went with renaming the Position, and not VisiblePosition methods. This is a good change except I'm slightly concerned with the names containing "next" and "previous" (despite that being my own proposal!). They aren't strictly next/previous, as calling the function again won't yield a further position that is also visually equivalent. They're really more like higher and lower visually equivalent boundary positions.