Make CharacterIterator::GetPosition{Before,After}() not to crash with block content [chromium/src : master]

Yoshifumi Inoue (Gerrit)

unread,

Apr 23, 2018, 6:19:55 AM4/23/18

to Xiaocheng Hu, blink-...@chromium.org

Yoshifumi Inoue would like Xiaocheng Hu and Yoichi Osato to review this change.

Make CharacterIterator::GetPosition{Before,After}() not to crash with block content

This patch makes |CharacterIterator::GetPosition{Before,After}() not to crash
with block content, e.g. "a<div>b</div>c".

This is follow-up of the patch[1].

[1] http://crrev.com/c/1021430 Make CharacterIterator::GetPosition{After,Before}()
to work with BR

Change-Id: I0e5aae4cef69d6c1d902cbeb86e8e3552968c280
---
M third_party/blink/renderer/core/editing/iterators/character_iterator.cc
M third_party/blink/renderer/core/editing/iterators/character_iterator_test.cc
2 files changed, 75 insertions(+), 12 deletions(-)

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoshifumi Inoue (Gerrit)

unread,

Apr 23, 2018, 6:19:56 AM4/23/18

to blink-...@chromium.org, Xiaocheng Hu, Commit Bot, chromium...@chromium.org

PTAL

Patch set 3:-Code-Review

View Change

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Xiaocheng Hu (Gerrit)

unread,

Apr 23, 2018, 2:47:34 PM4/23/18

to blink-...@chromium.org, Commit Bot, chromium...@chromium.org

CTI::GetPositionBefore/After are becoming hackier and hackier...

I think a better approach is move all complex logic into TI, by introducing "proper" position getters in TI (that don't use EditingPositionOf):

TI::GetPositionBeforeTextRun
TI::GetPositionAfterTextRun
TI::GetPositionBeforeCharacter(offset)
TI::GetPositionAfterCharacter(offset)

And then CTI can simply wrap the last two

View Change

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoichi Osato (Gerrit)

unread,

Apr 23, 2018, 9:48:57 PM4/23/18

to blink-...@chromium.org, Xiaocheng Hu, Commit Bot, chromium...@chromium.org

Patch Set 3:

CTI::GetPositionBefore/After are becoming hackier and hackier...
I think a better approach is move all complex logic into TI, by introducing "proper" position getters in TI (that don't use EditingPositionOf):
TI::GetPositionBeforeTextRun
TI::GetPositionAfterTextRun
TI::GetPositionBeforeCharacter(offset)
TI::GetPositionAfterCharacter(offset)

And then CTI can simply wrap the last two

+1
I also think TextIterator is already hacky and complex then it is better to
refactor the class by splitting functionality,,,

View Change

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoshifumi Inoue (Gerrit)

unread,

Apr 24, 2018, 6:22:16 AM4/24/18

to Xiaocheng Hu, blink-...@chromium.org, chromium...@chromium.org, Commit Bot

Yoshifumi Inoue uploaded patch set #6 to this change.

View Change

Make CharacterIterator::GetPosition{Before,After}() not to crash with block content

This patch introduces |TextIterator::GetPosition{Before,After}()| as
implementation of |CharacterIterator::GetPosition{Before,After}()| not to crash
with block content.

To implement before/after position, this patch introduces
|position_anchor_node_| and |position_node_type_| in |TextIteratorTextState| to
remember what |SpliceBuffer()| used for emitting characters.



This is follow-up of the patch[1].

[1] http://crrev.com/c/1021430 Make CharacterIterator::GetPosition{After,Before}()
to work with BR

Change-Id: I0e5aae4cef69d6c1d902cbeb86e8e3552968c280
---
M third_party/blink/renderer/core/editing/iterators/character_iterator.cc
M third_party/blink/renderer/core/editing/iterators/character_iterator_test.cc

M third_party/blink/renderer/core/editing/iterators/text_iterator.cc
M third_party/blink/renderer/core/editing/iterators/text_iterator.h
M third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.cc
M third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.h
6 files changed, 281 insertions(+), 33 deletions(-)

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoshifumi Inoue (Gerrit)

unread,

Apr 24, 2018, 6:23:35 AM4/24/18

to Xiaocheng Hu, blink-...@chromium.org, chromium...@chromium.org, Commit Bot

Yoshifumi Inoue uploaded patch set #7 to this change.

View Change

Make CharacterIterator::GetPosition{Before,After}() not to crash with block content

This patch introduces |TextIterator::GetPosition{Before,After}()| as
implementation of |CharacterIterator::GetPosition{Before,After}()| not to crash
with block content.

To implement before/after position, this patch introduces
|position_anchor_node_| and |position_node_type_| in |TextIteratorTextState| to

remember the purpose of emitted characters.



This is follow-up of the patch[1].

[1] http://crrev.com/c/1021430 Make CharacterIterator::GetPosition{After,Before}()
to work with BR

Change-Id: I0e5aae4cef69d6c1d902cbeb86e8e3552968c280
---
M third_party/blink/renderer/core/editing/iterators/character_iterator.cc
M third_party/blink/renderer/core/editing/iterators/character_iterator_test.cc

M third_party/blink/renderer/core/editing/iterators/text_iterator.cc
M third_party/blink/renderer/core/editing/iterators/text_iterator.h
M third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.cc
M third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.h

6 files changed, 281 insertions(+), 33 deletions(-)

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoshifumi Inoue (Gerrit)

unread,

Apr 24, 2018, 11:25:29 AM4/24/18

to blink-...@chromium.org, Xiaocheng Hu, Commit Bot, chromium...@chromium.org

PTAL

View Change

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Xiaocheng Hu (Gerrit)

unread,

Apr 24, 2018, 2:51:15 PM4/24/18

to blink-...@chromium.org, Commit Bot, chromium...@chromium.org

There seem to be too many merge conflicts with the other refactoring patches.

I'll wait until those patches are settled.

View Change

1 comment:

File third_party/blink/renderer/core/editing/iterators/text_iterator.h:
- Patch Set #9, Line 83: PositionTemplate<Strategy> GetPositionBefore(int run_offset) const;
  Could you document these two functions? In particular, meaning of |run_offset|?

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoshifumi Inoue (Gerrit)

unread,

Apr 24, 2018, 9:57:18 PM4/24/18

to blink-...@chromium.org, Xiaocheng Hu, Commit Bot, chromium...@chromium.org

PTAL

The dependent patch is still in CQ due by android try bot flakiness...

View Change

1 comment:

File third_party/blink/renderer/core/editing/iterators/text_iterator.h:

- Patch Set #9, Line 83:

- Could you document these two functions? In particular, meaning of |run_offset|?
  Done

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoichi Osato (Gerrit)

unread,

Apr 24, 2018, 10:02:21 PM4/24/18

to blink-...@chromium.org, Xiaocheng Hu, Commit Bot, chromium...@chromium.org

This patch looks rearchitecture/refactoring rather than fixing a crash bug.
Could you split them to each other?

View Change

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoshifumi Inoue (Gerrit)

unread,

Apr 24, 2018, 10:25:00 PM4/24/18

to blink-...@chromium.org, Xiaocheng Hu, Commit Bot, chromium...@chromium.org

PTAL

No more dependent patches and merge conflicts!

View Change

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoshifumi Inoue (Gerrit)

unread,

Apr 24, 2018, 10:52:54 PM4/24/18

to blink-...@chromium.org, Xiaocheng Hu, Commit Bot, chromium...@chromium.org

Patch Set 12:

This patch looks rearchitecture/refactoring rather than fixing a crash bug.
Could you split them to each other?

No, the PS#1 fixed the crash bug.

But, we decide to move implementation into TextIterator and I found PS#1 is not enough.
This is reason why I change TextIteratorTextState.

I don't see no good reason to split this patch more.

Could you tell me your idea of splitting this patch?

View Change

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoichi Osato (Gerrit)

unread,

Apr 25, 2018, 12:58:56 AM4/25/18

to blink-...@chromium.org, Xiaocheng Hu, Commit Bot, chromium...@chromium.org

Patch Set 13:

Patch Set 12:
This patch looks rearchitecture/refactoring rather than fixing a crash bug.
Could you split them to each other?
No, the PS#1 fixed the crash bug.
But, we decide to move implementation into TextIterator and I found PS#1 is not enough.
This is reason why I change TextIteratorTextState.
I don't see no good reason to split this patch more.
Could you tell me your idea of splitting this patch?

1. Move each CIA::GetPositionBefore/After function body to TIA::GetBepositioBefore/After.
This is pure refactoring.

2. Fix the issue by modifying the TIA::GetBepositioBefore/After.

View Change

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Xiaocheng Hu (Gerrit)

unread,

Apr 25, 2018, 1:24:50 AM4/25/18

to blink-...@chromium.org, Commit Bot, chromium...@chromium.org

View Change

5 comments:

File third_party/blink/renderer/core/editing/iterators/text_iterator.h:
- Patch Set #13, Line 84: Returns a |Position| generating current text run before |run_offset|
  nit: This can be improved.
  How about "Returns the position before |run_offset| in the current text run"?
- Patch Set #13, Line 95: a
  nit: the
- Patch Set #13, Line 98: a
  nit: the
- Patch Set #13, Line 100: with
  nit: remove "with"
File third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.h:
- Patch Set #13, Line 126: Member<const Node> position_anchor_node_;
  I haven't seen why we need to introduce |position_anchor_node_| and |position_anchor_kind_|. The previous four variables already represent the DOM position of the current text run clearly.
  Or, do we want to have a different representation when the current text run's container node isn't a text node?

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoshifumi Inoue (Gerrit)

unread,

Apr 25, 2018, 2:26:14 AM4/25/18

to Xiaocheng Hu, blink-...@chromium.org, chromium...@chromium.org, Commit Bot

Yoshifumi Inoue uploaded patch set #14 to this change.

View Change

Make CharacterIterator::GetPosition{Before,After}() not to crash with block content

This patch introduces |TextIterator::GetPosition{Before,After}()| as
implementation of |CharacterIterator::GetPosition{Before,After}()| not to crash
with block content.

To implement before/after position, this patch introduces
|position_anchor_node_| and |position_node_type_| in |TextIteratorTextState| to

remember the relative position of node of emitted characters since
|position_node_| and |position_offset_base_node_| are changed by
|FlushPositionOffsets()| via |{Start,End}OffsetInCurrentContainer()| in
|TextIterator|.

In current implementation, relative node position is encoded by start and end
offsets of |SpliceBuffer()|.



This is follow-up of the patch[1].

[1] http://crrev.com/c/1021430 Make CharacterIterator::GetPosition{After,Before}()
to work with BR

Change-Id: I0e5aae4cef69d6c1d902cbeb86e8e3552968c280
---
M third_party/blink/renderer/core/editing/iterators/character_iterator.cc
M third_party/blink/renderer/core/editing/iterators/character_iterator_test.cc

M third_party/blink/renderer/core/editing/iterators/text_iterator.cc
M third_party/blink/renderer/core/editing/iterators/text_iterator.h
M third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.cc
M third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.h

6 files changed, 314 insertions(+), 35 deletions(-)

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoshifumi Inoue (Gerrit)

unread,

Apr 25, 2018, 2:31:34 AM4/25/18

to blink-...@chromium.org, Xiaocheng Hu, Commit Bot, chromium...@chromium.org

PTAL

Update to follow review comments.

View Change

5 comments:

File third_party/blink/renderer/core/editing/iterators/text_iterator.h:
- Patch Set #13, Line 84: Returns the position before |run_offset| in the current text run
  nit: This can be improved. […]
  Done
- Patch Set #13, Line 95: t
  nit: the
  Done
- Patch Set #13, Line 98: t
  nit: the
  Done
- Patch Set #13, Line 100: |Get
  nit: remove "with"
  Done
File third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.h:

- Patch Set #13, Line 126: Member<const Node> position_anchor_node_;

- I haven't seen why we need to introduce |position_anchor_node_| and |position_anchor_kind_|. […]
  Yes, I would like to get rid of |position_node_| and |position_offset_base_node|.
  The reason I introduce |position_anchor_node_| is to keep it after calling
  |FlushPositionOffsets()| which is called by |TextIterator::StartOffsetInCurrentContainer()| and |TextIterator::EndOffsetInCurrentContainer()|.
  The reason of introducing |AnchorNodeKind| is avoid to decode start/end
  offset pair in |SpliceBuffer()|. See TODO for |SpliceBuffer()|.
  I would like to introduce |SpliceBufferBefore()|, |SpliceBufferAfter()| etc, to set |position_anchor_kind_| directly instead of checking start/end offsets.
  I updated commit message about |position_anchor_node_| and |position_anchor_kind_|.

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Xiaocheng Hu (Gerrit)

unread,

Apr 25, 2018, 2:09:31 PM4/25/18

to blink-...@chromium.org, Commit Bot, chromium...@chromium.org

View Change

5 comments:

File third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.h:
- Patch Set #13, Line 95: AnchorNodeKind
  nit: |AnchorType| seems better.
- Patch Set #13, Line 97:
```
    kAfter,
    kBefore,
    kReplaced,
    kText,
```
  |kAfterNode, kBeforeNode, kReplacesNode, kInText| seem better

- Patch Set #13, Line 126: Member<const Node> position_anchor_node_;

- Yes, I would like to get rid of |position_node_| and |position_offset_base_node|. […]
  OK, that makes sense.
  I think the refactoring to TextIteratorTextState is complex enough to deserve its own patch. Could you move them to another patch with more details of the motivation and plan of the refactoring?
File third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.cc:
- Patch Set #13, Line 109: position_anchor_kind_ = AnchorNodeKind::kReplaced;
  The only call site of this function is in HandleReplacedElement() as a preparation step for emitting alt text.
  We probably want another anchor type |kAltText| for it.
- Patch Set #13, Line 141:
```
  // TODO(editing-dev): We should introduce
  //  - SpliceBufferBefore(const Node&, UChar);
  //  - SpliceBufferAfter(const Node&, UChar);
  //  - SpliceBufferReplacer(const Node&, UChar);
  //  - SpliceBuffer(const Text&, UChar, start, end)
  //  instead of checking |start_offset| and |end_offset| for non-|Text| node.
```
  Let's finish this part first in a refactoring patch.

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoichi Osato (Gerrit)

unread,

Apr 25, 2018, 9:53:42 PM4/25/18

to blink-...@chromium.org, Xiaocheng Hu, Commit Bot, chromium...@chromium.org

View Change

1 comment:

File third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.h:
- Patch Set #13, Line 126: Member<const Node> position_anchor_node_;
  OK, that makes sense. […]
  +1 to make another patch for refactoring.

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoshifumi Inoue (Gerrit)

unread,

Apr 27, 2018, 4:34:07 AM4/27/18

to Xiaocheng Hu, blink-...@chromium.org, chromium...@chromium.org, Commit Bot

Yoshifumi Inoue uploaded patch set #17 to this change.

View Change

Make CharacterIterator::GetPosition{Before,After}() not to crash with block content

This patch introduces |TextIterator::GetPosition{Before,After}()| as

implementation of |CharacterIterator::GetPosition{Before,After}()| not to crash
with block content.



This is follow-up of the patch[1].

[1] http://crrev.com/c/1021430 Make CharacterIterator::GetPosition{After,Before}()
to work with BR

Change-Id: I0e5aae4cef69d6c1d902cbeb86e8e3552968c280
---
M third_party/blink/renderer/core/editing/iterators/character_iterator.cc
M third_party/blink/renderer/core/editing/iterators/character_iterator_test.cc

M third_party/blink/renderer/core/editing/iterators/text_iterator.cc
M third_party/blink/renderer/core/editing/iterators/text_iterator.h
M third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.h
5 files changed, 220 insertions(+), 26 deletions(-)

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoshifumi Inoue (Gerrit)

unread,

Apr 27, 2018, 4:35:24 AM4/27/18

to blink-...@chromium.org, Xiaocheng Hu, Commit Bot, chromium...@chromium.org

PTAL

View Change

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Xiaocheng Hu (Gerrit)

unread,

Apr 27, 2018, 3:29:41 PM4/27/18

to blink-...@chromium.org, Commit Bot, chromium...@chromium.org

View Change

7 comments:

File third_party/blink/renderer/core/editing/iterators/text_iterator.cc:
- Patch Set #17, Line 887: LE
  LT?
- Patch Set #17, Line 894: nod
  nit: node
- Patch Set #17, Line 906: return PositionTemplate<Strategy>::BeforeNode(nod);
  BeforeChildren anchor type not handled. Maybe we would like to fix SBTI to eliminate this type first.
  Also could you add a comment that this line also handles AsNode and AltText types, instead of just BeforeNode?
- Patch Set #17, Line 913: LE
  LT?
- Patch Set #17, Line 920: nod
  nit: node
- Patch Set #17, Line 932: return PositionTemplate<Strategy>::AfterNode(nod);
  ditto.
File third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.h:
- Patch Set #17, Line 100: IsPositionNodeText
  |IsInTextNode| may be better.
  |IsPositionNodeText| sounds like checking |position_node_->IsText()|.

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoshifumi Inoue (Gerrit)

unread,

May 7, 2018, 2:29:47 AM5/7/18

to blink-...@chromium.org, Xiaocheng Hu, Commit Bot, chromium...@chromium.org

PTAL

View Change

7 comments:

File third_party/blink/renderer/core/editing/iterators/text_iterator.cc:
- Patch Set #17, Line 887: em
  LT?
  This should be LT, clients at end of text, e.g. "abc|".

- Patch Set #17, Line 894:

- nit: node
  Done

- Patch Set #17, Line 906: }

- BeforeChildren anchor type not handled. […]
  Just Add DCHECK(!text_state_.IsBeforeChildren()).
  I think we don't need to list |PositionNodeType| members, since it is
  implementation details.
- Patch Set #17, Line 913: <t
  LT?
  This should be LT, clients at end of text, e.g. "abc|".
- Patch Set #17, Line 920: iti
  nit: node
  Done
- Patch Set #17, Line 932: return PositionTemplate<Strategy>(node, ToText(node).length());
  ditto.
  Just Add DCHECK(!text_state_.IsBeforeChildren()).
  I think we don't need to list |PositionNodeType| members, since it is
  implementation details.
File third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.h:
- Patch Set #17, Line 100: IsBeforeChildren()
  |IsInTextNode| may be better. […]
  Renamed to |IsInTextNode()|.

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Xiaocheng Hu (Gerrit)

unread,

May 7, 2018, 2:27:52 PM5/7/18

to blink-...@chromium.org, Commit Bot, chromium...@chromium.org

Since TextIteratorTextState already stores all information about the DOM position of the text run, we should move most of TI::GetPositionBefore/After (when not AtEnd()) to TITextState.

In this way, we also avoid manipulating text offsets in TI, which is error prone.

View Change

1 comment:

File third_party/blink/renderer/core/editing/iterators/text_iterator.cc:
- Patch Set #18, Line 927: run_offset + 1
  This is incorrect if the current text run is set with an |EmitChar16Before()| call.

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoshifumi Inoue (Gerrit)

unread,

May 8, 2018, 6:08:37 AM5/8/18

to blink-...@chromium.org, Xiaocheng Hu, Commit Bot, chromium...@chromium.org

PTAL

View Change

1 comment:

File third_party/blink/renderer/core/editing/iterators/text_iterator.cc:

- Patch Set #18, Line 927:

This is incorrect if the current text run is set with an |EmitChar16Before()| call.

Discussed offline and get three options:
 1. templatize TITextState, so that it can create position, and we can also merge UpdatePositionOffsets() calls from TI and SBTI to TITextState
 2. Add a new anchor type for EmitChar16Before
 3. Ignore

We take option 2 since option 1 increase code size due by template and
some callers may hit this (not sure now).

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Xiaocheng Hu (Gerrit)

unread,

May 8, 2018, 6:01:15 PM5/8/18

to blink-...@chromium.org, Commit Bot, chromium...@chromium.org

LGTM with some nits regarding comments and var names.

Patch set 19:Code-Review +1

View Change

5 comments:

File third_party/blink/renderer/core/editing/iterators/text_iterator.h:
- Patch Set #19, Line 83:

  PositionTemplate<Strategy> GetPositionBefore(int run_offset) const;

- ```
  PositionTemplate<Strategy> GetPositionAfter(int run_offset) const;
```
  |char_offset| seems a better name than |run_offset|.
  Could you also add documentation to these functions, like:
  // Returns the position before/after the character at the given offset in the text run.
File third_party/blink/renderer/core/editing/iterators/text_iterator.cc:
- Patch Set #19, Line 890: run_offset
  nit: |char_offset| seems a better name.
- Patch Set #19, Line 898: DCHECK_GE(length(), 1);
  We should also DCHECK_LT(char_offset, length()) when not at end.
- Patch Set #19, Line 917: run_offset
  nit: |char_offset| seems a better name.
- Patch Set #19, Line 925: DCHECK_GE(length(), 1);
  We should also DCHECK_LT(char_offset, length()) when not at end.

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Xiaocheng Hu (Gerrit)

unread,

May 8, 2018, 6:04:55 PM5/8/18

to blink-...@chromium.org, Commit Bot, chromium...@chromium.org

View Change

1 comment:

File third_party/blink/renderer/core/editing/iterators/character_iterator_test.cc:
- Patch Set #19, Line 247:
```
 EXPECT_EQ(Position(text_b, 3), it.GetPositionBefore());
  EXPECT_EQ(Position(text_b, 3), it.GetPositionAfter());
```
  Wait... Shouldn't this be DIV@AfterNode?
  Can we add a TODO to track?

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Yoshifumi Inoue (Gerrit)

unread,

May 9, 2018, 12:28:45 AM5/9/18

to blink-...@chromium.org, Xiaocheng Hu, Commit Bot, chromium...@chromium.org

Thanks for reviewing!

Committing...

Patch set 20:Commit-Queue +2

View Change

6 comments:

File third_party/blink/renderer/core/editing/iterators/character_iterator_test.cc:

Patch Set #19, Line 247:

 EXPECT_EQ(Position(text_b, 3), it.GetPositionBefore());
  EXPECT_EQ(Position(text_b, 3), it.GetPositionAfter());

- Wait... Shouldn't this be DIV@AfterNode? […]
  " b "@3 (== AfterChildren(DIV) == DIV@1) is correct value.
  We prefer a position on Text node.
File third_party/blink/renderer/core/editing/iterators/text_iterator.h:

- Patch Set #19, Line 83:


  // Returns the position before |char16_offset| in current text run

|char_offset| seems a better name than |run_offset|. […]

Done

File third_party/blink/renderer/core/editing/iterators/text_iterator.cc:
- Patch Set #19, Line 890: char16_off

- nit: |char_offset| seems a better name.

- Done

- Patch Set #19, Line 898: DCHECK_GE(length(), 1);
  We should also DCHECK_LT(char_offset, length()) when not at end.

- Done
- Patch Set #19, Line 917: char16_off

- nit: |char_offset| seems a better name.

- Done

- Patch Set #19, Line 925: DCHECK_GE(length(), 1);
  We should also DCHECK_LT(char_offset, length()) when not at end.

- Done

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

May 9, 2018, 12:28:48 AM5/9/18

to blink-...@chromium.org, Xiaocheng Hu, chromium...@chromium.org

CQ is trying the patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.
"2018-05-09T13:26:29" https://chromium-review.googlesource.com/c/1023502/20

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1023502/20

Bot data: {"action": "start", "triggered_at": "2018-05-09T04:28:41.0Z", "cq_cfg_revision": "ffcb66b8cd25853c8032d1c584f85512bf031ffc", "revision": "219988ea3de5db41409a2aec3e31261ace94190c"}

View Change

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

May 9, 2018, 1:34:34 AM5/9/18

to blink-...@chromium.org, Xiaocheng Hu, chromium...@chromium.org

Commit Bot merged this change.

View Change

Approvals:
  Xiaocheng Hu: Looks good to me
  Yoshifumi Inoue: Commit

Make CharacterIterator::GetPosition{Before,After}() not to crash with block content

This patch introduces |TextIterator::GetPosition{Before,After}()| as
implementation of |CharacterIterator::GetPosition{Before,After}()| not to crash
with block content.

This is follow-up of the patch[1].

[1] http://crrev.com/c/1021430 Make CharacterIterator::GetPosition{After,Before}()
to work with BR

Change-Id: I0e5aae4cef69d6c1d902cbeb86e8e3552968c280

Reviewed-on: https://chromium-review.googlesource.com/1023502
Commit-Queue: Yoshifumi Inoue <yo...@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaoc...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557103}


---
M third_party/blink/renderer/core/editing/iterators/character_iterator.cc
M third_party/blink/renderer/core/editing/iterators/character_iterator_test.cc
M third_party/blink/renderer/core/editing/iterators/text_iterator.cc
M third_party/blink/renderer/core/editing/iterators/text_iterator.h

M third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.cc
M third_party/blink/renderer/core/editing/iterators/text_iterator_text_state.h
6 files changed, 304 insertions(+), 28 deletions(-)

To view, visit change 1023502. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward