Make VisiblePosition::characterAfter() not to use Position::containerText() (issue 1259243007 by yosin@chromium.org)

[yo...@chromium.org]

Reviewers: tkent, yoichio, hajimehoshi,

Message:
PTAL

Description:
Make VisiblePosition::characterAfter() not to use Position::containerText()

This patch changes |VisiblePosition::characterAfter()| not to use
|Position::containerText()| by using |toText()| to avoid redundant call
of |Position::containerNode()| and |Position::containerText()| for improving
code health.

This patch is a preparation of templatizing |VisiblePosition| and getting
rid
of |Position::containerText()|.

BUG=513568
TEST=n/a; no behavior changes


Please review this at https://codereview.chromium.org/1259243007/

Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Affected files (+6, -4 lines):
M Source/core/editing/VisiblePosition.cpp


Index: Source/core/editing/VisiblePosition.cpp
diff --git a/Source/core/editing/VisiblePosition.cpp
b/Source/core/editing/VisiblePosition.cpp
index
d9ede35db03812aa76fccd16fb876e473a6bf4dc..f618ee45c326755ebc6ab6fbf43aae47735801f9
100644
--- a/Source/core/editing/VisiblePosition.cpp
+++ b/Source/core/editing/VisiblePosition.cpp
@@ -659,13 +659,15 @@ void VisiblePosition::init(const
PositionAlgorithm<Strategy>& position, EAffinit

UChar32 VisiblePosition::characterAfter() const
{
- // We canonicalize to the first of two equivalent candidates, but the
second of the two candidates
- // is the one that will be inside the text node containing the
character after this visible position.
+ // We canonicalize to the first of two equivalent candidates, but the
second
+ // of the two candidates is the one that will be inside the text node
+ // containing the character after this visible position.
Position pos = m_deepPosition.downstream();
- if (!pos.containerNode() || !pos.containerNode()->isTextNode() |
| !pos.isOffsetInAnchor())
+ Node* containerNode = pos.containerNode();
+ if (!containerNode || !containerNode->isTextNode() |
| !pos.isOffsetInAnchor())
return 0;
unsigned offset = static_cast<unsigned>(pos.offsetInContainerNode());
- Text* textNode = pos.containerText();
+ Text* textNode = toText(containerNode);
unsigned length = textNode->length();
if (offset >= length)
return 0;

[yoi...@chromium.org]

https://codereview.chromium.org/1259243007/diff/1/Source/core/editing/VisiblePosition.cpp
File Source/core/editing/VisiblePosition.cpp (right):

https://codereview.chromium.org/1259243007/diff/1/Source/core/editing/VisiblePosition.cpp#newcode667
Source/core/editing/VisiblePosition.cpp:667: if (!containerNode ||
!containerNode->isTextNode() || !pos.isOffsetInAnchor())
Could you early return if |!pos.isOffsetInAnchor()| not to call
pos.containerNode?

https://codereview.chromium.org/1259243007/

[yo...@chromium.org]

PTAL

https://codereview.chromium.org/1259243007/diff/1/Source/core/editing/VisiblePosition.cpp
File Source/core/editing/VisiblePosition.cpp (right):

https://codereview.chromium.org/1259243007/diff/1/Source/core/editing/VisiblePosition.cpp#newcode667
Source/core/editing/VisiblePosition.cpp:667: if (!containerNode ||
!containerNode->isTextNode() || !pos.isOffsetInAnchor())

On 2015/08/04 01:36:07, yoichio wrote:
> Could you early return if |!pos.isOffsetInAnchor()| not to call
> pos.containerNode?

Done.

https://codereview.chromium.org/1259243007/

[yoi...@chromium.org]

lgtm



https://codereview.chromium.org/1259243007/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1259243007/20001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1259243007/20001


https://codereview.chromium.org/1259243007/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199932

https://codereview.chromium.org/1259243007/