Move LayoutObject::nextOffset/previousOffset() to editing utility

third_party/WebKit/Source/core/editing/EditingUtilities.cpp

 /*
  * Copyright (C) 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 524 matching lines @@
 Position lastEditablePositionBeforePositionInRoot(const Position& position, Node& highestRoot)
 {
     return lastEditablePositionBeforePositionInRootAlgorithm<EditingStrategy>(position, highestRoot);
 }
 
 PositionInFlatTree lastEditablePositionBeforePositionInRoot(const PositionInFlatTree& position, Node& highestRoot)
 {
     return lastEditablePositionBeforePositionInRootAlgorithm<EditingInFlatTreeStrategy>(position, highestRoot);
 }
 
 int uncheckedPreviousOffset(const Node* n, int current)

[yosin_UTC9, 2016/03/25 04:13:30]

Could you avoid to use one letter variable name?

[Seigo Nonaka, 2016/03/25 05:11:22]

Done.

 {
-    return n->layoutObject() ? n->layoutObject()->previousOffset(current) : current - 1;

[tkent, 2016/03/25 04:34:08]

Is it ok to remove n->layoutObject() check in the original code?  If |n| is a
Text node without LayoutObject, this CL has behavior difference.

[yosin_UTC9, 2016/03/25 04:57:53]

Yes. Using |LayoutObject::previousOffset()| is wrong regarding
 - first-letter: LayoutText contains non-first-letter part, e.g.
<div::first-letter>foo</div> yields two LayoutTextFragment, one holds "f" and
another holds "oo". TextNode("foo").layoutObject() = LayoutText("oo"). This is
source of first-letter related bug
 - text-transform: text-transform:uppercase yield TextNode("ß"), eszett =>
LayoutText("SS"). This is source of unbound index in various places.
From above, we change not to use LayoutObject here.

[tkent, 2016/03/25 05:05:25]

I asked about a case of no LayoutObject. Anyway, we need more tests:
 - |n| is Text without LayoutObject
 - correctly work even if first-letter is specified
 - correctly work even if text-transform is specified

[Seigo Nonaka, 2016/03/25 05:11:21]

Sure, I'm happy to add these test case but let me give extra time.
In unit test context, looks like LayoutObject is not available.
I need extra work for unit test.

[Seigo Nonaka, 2016/03/25 05:11:22]

Thank you for your follow-up comment.
Let me add one note for those issues. Even if this CL and
http://codereview.chromium.org/1824143003 are submitted, backspace problem for
first-letter/text-transform is not fixed. I'm now investigating the root cause.

[yosin_UTC9, 2016/03/25 05:50:04]

Oh, sorry for confusion.
 |return n->layoutObject() ? n->layoutObject()->previousOffset(current) :
current - 1;|
Original code does wrong thing when |n->layoutObject()| is |nullptr|. It can
return middle of surrogate pair.
This patch fixes this too.

[Seigo Nonaka, 2016/03/25 08:02:41]

Added test cases for first-letter and text-transform with/without LayoutObject.

[yosin_UTC9, 2016/03/25 08:18:55]

You need to call |updateLayoutAndStyleForPainting()| after |setBodyContent()| to
get layout objects.

+    if (!n->isTextNode())
+        return current - 1;
+    const String& text = toText(n)->data();
+    if (text.is8Bit())
+        return current - 1; // TODO(nona): Good to support CR x LF.
+    TextBreakIterator* iterator = cursorMovementIterator(text.characters16(), text.length());
+    if (!iterator)
+        return current - 1;
+    long result = iterator->preceding(current);

[yosin_UTC9, 2016/03/25 04:13:30]

|int| is better since |TextBreakIterator::preceding()| returns |int|.
|const int result = iterator->preceding(current)|

[Seigo Nonaka, 2016/03/25 05:11:22]

Done.

+    if (result == TextBreakDone)

[yosin_UTC9, 2016/03/25 04:13:30]

Use ternary or early return:

return reuslt == TextBreakDone ? current - 1 : result;

or
if (result == TextBreakDone)
  return current - 1;
return result;

[Seigo Nonaka, 2016/03/25 05:11:21]

Done.

+        result = current - 1;
+    return result;
 }
 
 static int uncheckedPreviousOffsetForBackwardDeletion(const Node* n, int current)
 {
     return n->layoutObject() ? n->layoutObject()->previousOffsetForBackwardDeletion(current) : current - 1;
 }
 
 int uncheckedNextOffset(const Node* n, int current)
 {
-    return n->layoutObject() ? n->layoutObject()->nextOffset(current) : current + 1;
+    if (!n->isTextNode())
+        return current + 1;
+    const String& text = toText(n)->data();
+    if (text.is8Bit())
+        return current + 1; // TODO(nona): Good to support CR x LF.
+    TextBreakIterator* iterator = cursorMovementIterator(text.characters16(), text.length());
+    if (!iterator)
+        return current + 1;
+    long result = iterator->following(current);

[yosin_UTC9, 2016/03/25 04:13:30]

|int| is better since |TextBreakIterator::following()| returns |int|.

[Seigo Nonaka, 2016/03/25 05:11:21]

Done.

+    if (result == TextBreakDone)

[yosin_UTC9, 2016/03/25 04:13:30]

Use ternary or early return:

return reuslt == TextBreakDone ? current - 1 : result;

or
if (result == TextBreakDone)
  return current - 1;
return result;

[Seigo Nonaka, 2016/03/25 05:11:22]

Done.

+        result = current + 1;
+    return result;
 }
 
 template <typename Strategy>
 PositionTemplate<Strategy> previousPositionOfAlgorithm(const PositionTemplate<Strategy>& position, PositionMoveType moveType)
 {
     Node* const node = position.anchorNode();
     if (!node)
         return position;
 
     const int offset = position.computeEditingOffset();
@@ skipping 1078 matching lines @@
     // instead of possibly at the end of the last node before the selection
     return mostForwardCaretPosition(visiblePosition.deepEquivalent());
 }
 
 bool isTextSecurityNode(const Node* node)
 {
     return node && node->layoutObject() && node->layoutObject()->style()->textSecurity() != TSNONE;
 }
 
 } // namespace blink