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

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

Index: third_party/WebKit/Source/core/editing/EditingUtilities.cpp
diff --git a/third_party/WebKit/Source/core/editing/EditingUtilities.cpp b/third_party/WebKit/Source/core/editing/EditingUtilities.cpp
index fed03353f55ed4fa03b8f0b67ffc02d2b62934ef..ce35912d30cecc6e279ef5b555d56c7ae8fd0168 100644
--- a/third_party/WebKit/Source/core/editing/EditingUtilities.cpp
+++ b/third_party/WebKit/Source/core/editing/EditingUtilities.cpp
@@ -544,7 +544,18 @@ PositionInFlatTree lastEditablePositionBeforePositionInRoot(const PositionInFlat
 
 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)
@@ -554,7 +565,18 @@ static int uncheckedPreviousOffsetForBackwardDeletion(const Node* n, int current
 
 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>