Bug 63040 - [meta] positionIsOffsetInAnchor should only accept text nodes
Summary: [meta] positionIsOffsetInAnchor should only accept text nodes
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 63037 63100 63181 63384
Blocks: 52098
  Show dependency treegraph
 
Reported: 2011-06-20 20:15 PDT by Ryosuke Niwa
Modified: 2017-07-18 08:29 PDT (History)
7 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-06-20 20:15:25 PDT
During the contributor's meeting, we have concluded that we should not have positions with offsets in non-text nodes for performance reasons and for comparison.  We should refactor the code to enforce this.
Comment 1 Ryosuke Niwa 2011-06-22 11:33:56 PDT
The current plan is follows:
1. Fix the bug 63100 - Add BeforeChildren/AfterChildren anchor types.
2. Add Position(PassRefPtr<Text>, unsigned offset)
3. Remove instantes of Position(..., PositionIsOffsetInAnchor)
Comment 2 Ryosuke Niwa 2011-06-25 12:53:19 PDT
There are few files in WebKit that instantiates Position directly:
Source/WebKit/chromium/src/WebViewImpl.cpp:1302:                                      Position::PositionIsOffsetInAnchor);
Source/WebKit/mac/WebView/WebFrame.mm:743:    Position start = Position(startContainer, [proposedRange startOffset], Position::PositionIsOffsetInAnchor);
Source/WebKit/mac/WebView/WebFrame.mm:744:    Position end = Position(endContainer, [proposedRange endOffset], Position::PositionIsOffsetInAnchor);
Source/WebKit/mac/WebView/WebFrame.mm:1107:    Position startPos(startContainer, [rangeToReplace startOffset], Position::PositionIsOffsetInAnchor);
Source/WebKit/mac/WebView/WebFrame.mm:1108:    Position endPos(endContainer, [rangeToReplace endOffset], Position::PositionIsOffsetInAnchor);
Source/WebKit/mac/WebView/WebTextCompletionController.mm:208:            NSRect wordRect = [frame _caretRectAtPosition:Position(core([wholeWord startContainer]), [wholeWord startOffset], Position::PositionIsOffsetInAnchor) affinity:NSSelectionAffinityDownstream];
Comment 3 yosin 2015-06-28 19:13:50 PDT
It seems there are no alternative way to represent child node other than PositionIsOffsetInAnchor.

What do you mind to represent child node w/o PositionIsOffsetInAnchor?
Comment 4 Levi Weintraub 2015-06-28 19:34:22 PDT
(In reply to comment #3)
> It seems there are no alternative way to represent child node other than
> PositionIsOffsetInAnchor.
> 
> What do you mind to represent child node w/o PositionIsOffsetInAnchor?

For non-text nodes, we want to use the other types: 

PositionIsBeforeAnchor,
PositionIsAfterAnchor,
PositionIsBeforeChildren,
PositionIsAfterChildren,

This avoids having to walk the child list.
Comment 5 Ryosuke Niwa 2015-06-28 19:34:49 PDT
(In reply to comment #3)
> It seems there are no alternative way to represent child node other than
> PositionIsOffsetInAnchor.
> 
> What do you mind to represent child node w/o PositionIsOffsetInAnchor?

Performance.
Comment 6 yosin 2015-06-28 20:50:18 PDT
>> What do you mind to represent child node w/o PositionIsOffsetInAnchor?
> Performance.

Oops, my question is unclear.
How do we represent a position at child node w/o PositionIsOffsetInAnchor?
Use LegacyEditingPosition?
Comment 7 Ryosuke Niwa 2015-06-28 21:52:56 PDT
(In reply to comment #6)
> >> What do you mind to represent child node w/o PositionIsOffsetInAnchor?
> > Performance.
> 
> Oops, my question is unclear.
> How do we represent a position at child node w/o PositionIsOffsetInAnchor?
> Use LegacyEditingPosition?

Use BeforeChildren, AfterChildren, BeforeAnchor, or AfterAnchor (for the latter two, you need to change the anchor node since we're no longer anchoring at the parent)
Comment 8 yosin 2015-06-28 22:52:57 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > >> What do you mind to represent child node w/o PositionIsOffsetInAnchor?
> > > Performance.
> > 
> > Oops, my question is unclear.
> > How do we represent a position at child node w/o PositionIsOffsetInAnchor?
> > Use LegacyEditingPosition?
> 
> Use BeforeChildren, AfterChildren, BeforeAnchor, or AfterAnchor (for the
> latter two, you need to change the anchor node since we're no longer
> anchoring at the parent)

Thanks for explanation. So, you want to avoid computing node index. But, why not use RangeBoundaryPoint?
It seems AnchorType introduces complexity in Position class.
Comment 9 Levi Weintraub 2015-06-28 23:46:23 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > >> What do you mind to represent child node w/o PositionIsOffsetInAnchor?
> > > > Performance.
> > > 
> > > Oops, my question is unclear.
> > > How do we represent a position at child node w/o PositionIsOffsetInAnchor?
> > > Use LegacyEditingPosition?
> > 
> > Use BeforeChildren, AfterChildren, BeforeAnchor, or AfterAnchor (for the
> > latter two, you need to change the anchor node since we're no longer
> > anchoring at the parent)
> 
> Thanks for explanation. So, you want to avoid computing node index. But, why
> not use RangeBoundaryPoint?

It's not only about *computing* the node index, it's about finding the child nodes when you just have a pointer to the parent node and index. If you have a position between two non-text nodes in a node list with 1000 siblings, just having [parent, 500] will be very expensive...

> It seems AnchorType introduces complexity in Position class.