Bug 55650

Summary: Give Position's upstream and downstream more descriptive names
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: REOPENED    
Severity: Enhancement CC: enrica, eric, justin.garcia, kalman, leviw, xji
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 52098    
Attachments:
Description Flags
work in progress none

Ryosuke Niwa
Reported 2011-03-02 22:46:51 PST
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.
Attachments
work in progress (56.80 KB, patch)
2011-03-08 01:21 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Levi Weintraub
Comment 1 2011-03-03 11:00:33 PST
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
Comment 2 2011-03-03 14:56:23 PST
(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
Comment 3 2011-03-03 14:57:47 PST
No problem. The naming is confusing, I know :(
Justin Garcia
Comment 4 2011-03-03 18:18:05 PST
(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
Comment 5 2011-03-03 22:52:22 PST
(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
Comment 6 2011-03-07 22:49:34 PST
Okay, let's rename these two member functions.
Ryosuke Niwa
Comment 7 2011-03-08 01:21:56 PST
Created attachment 85034 [details] work in progress
Levi Weintraub
Comment 8 2011-03-08 14:06:51 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.