Introduce Find{Left,Right}BoundaryOfBidiRun() and variations

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

Index: third_party/WebKit/Source/core/editing/VisibleUnits.cpp
diff --git a/third_party/WebKit/Source/core/editing/VisibleUnits.cpp b/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
index 5def96008bcf39ce3f192892caeeb93e0db0ea4e..5d8f79fdcbcb22fd182fe01c75b37f2b213cd4be 100644
--- a/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
+++ b/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
@@ -34,6 +34,7 @@
 #include "core/dom/Text.h"
 #include "core/editing/EditingUtilities.h"
 #include "core/editing/FrameSelection.h"
+#include "core/editing/InlineBoxTraversal.h"
 #include "core/editing/Position.h"
 #include "core/editing/PositionIterator.h"
 #include "core/editing/RenderedPosition.h"
@@ -881,11 +882,8 @@ static InlineBoxPosition AdjustInlineBoxPositionForTextDirection(
         return InlineBoxPosition(inline_box, caret_offset);
 
       // For example, abc 123 ^ CBA
-      while (InlineBox* next_box = inline_box->NextLeafChild()) {

[Xiaocheng, 2017/06/15 01:11:48]

The existing code is so bad that a loop variable and a local variable have the
same name |next_box|...

It's good that we caught that.

[yosin_UTC9, 2017/06/15 03:42:14]

FYI: MSVC has a compile option to detect multiple occurrences of variable names
in function.
C# doesn't allow name confliction in same scope in the spec.

-        if (next_box->BidiLevel() < level)
-          break;
-        inline_box = next_box;
-      }
+      inline_box =
+          InlineBoxTraversal::FindRightBoundaryOfEntireBidiRun(*next_box);

[yosin_UTC9, 2017/06/14 09:41:34]

Because |level = next_box->BidiLevel()| at L874, we use |next_box| to
avoid pass both |InlineBox*| and BiDi level to
|indRightBoundaryOfEntireBidiRun|.

[Xiaocheng, 2017/06/15 01:11:48]

We should pass in |inline_box|, otherwise |next_box| is skipped in
FindBoudnaryOfEntireBidiRun().

However, do we still pass in the correct Bidi level if |inline_box| is passed
in?

[yosin_UTC9, 2017/06/15 03:42:14]

Add |level| parameter to |FindXXX()|. Once code cleaner, we'll remove |level|
parameter.

       return InlineBoxPosition(inline_box, inline_box->CaretRightmostOffset());
     }
 
@@ -901,11 +899,8 @@ static InlineBoxPosition AdjustInlineBoxPositionForTextDirection(
     if (next_box && next_box->BidiLevel() == level)
       return InlineBoxPosition(inline_box, caret_offset);
 
-    while (InlineBox* prev_box = inline_box->PrevLeafChild()) {
-      if (prev_box->BidiLevel() < level)
-        break;
-      inline_box = prev_box;
-    }
+    inline_box = InlineBoxTraversal::FindLeftBoundaryOfEntireBidiRun(
+        *inline_box->PrevLeafChild());

[yosin_UTC9, 2017/06/14 09:41:34]

To avoid pass both |InlineBox*| and BiDi level to
|FindLeftBoundaryOfEntireBidiRun()|,
we use |inline_box->PrevLeafChild()| since |level =
inline_box->PrevLeafChild()->BidiLevel()| at L893.

[Xiaocheng, 2017/06/15 01:11:48]

Ditto.

[yosin_UTC9, 2017/06/15 03:42:14]

Add |level| parameter to |FindXXX()|. Once code cleaner, we'll remove |level|
parameter.

     return InlineBoxPosition(inline_box, inline_box->CaretLeftmostOffset());
   }
 
@@ -914,23 +909,17 @@ static InlineBoxPosition AdjustInlineBoxPositionForTextDirection(
     if (!prev_box || prev_box->BidiLevel() < level) {
       // Left edge of a secondary run. Set to the right edge of the entire
       // run.
-      while (InlineBox* next_box =
-                 inline_box->NextLeafChildIgnoringLineBreak()) {
-        if (next_box->BidiLevel() < level)
-          break;
-        inline_box = next_box;
-      }
+      inline_box =
+          InlineBoxTraversal::FindRightBoundaryOfEntireBidiRunIgnoringLineBreak(
+              *inline_box);

[Xiaocheng, 2017/06/15 01:11:48]

Do we pass in the correct bidi level here?

[yosin_UTC9, 2017/06/15 03:42:14]

Done.

       return InlineBoxPosition(inline_box, inline_box->CaretRightmostOffset());
     }
 
     if (prev_box->BidiLevel() > level) {
       // Right edge of a "tertiary" run. Set to the left edge of that run.
-      while (InlineBox* tertiary_box =
-                 inline_box->PrevLeafChildIgnoringLineBreak()) {
-        if (tertiary_box->BidiLevel() <= level)
-          break;
-        inline_box = tertiary_box;
-      }
+      inline_box =
+          InlineBoxTraversal::FindLeftBoundaryOfBidiRunIgnoringLineBreak(

[Xiaocheng, 2017/06/15 01:11:48]

Do we pass in the correct bidi level here?

[yosin_UTC9, 2017/06/15 03:42:14]

Done.

+              *inline_box);
       return InlineBoxPosition(inline_box, inline_box->CaretLeftmostOffset());
     }
     return InlineBoxPosition(inline_box, caret_offset);
@@ -946,11 +935,9 @@ static InlineBoxPosition AdjustInlineBoxPositionForTextDirection(
   if (!next_box || next_box->BidiLevel() < level) {
     // Right edge of a secondary run. Set to the left edge of the entire
     // run.
-    while (InlineBox* prev_box = inline_box->PrevLeafChildIgnoringLineBreak()) {
-      if (prev_box->BidiLevel() < level)
-        break;
-      inline_box = prev_box;
-    }
+    inline_box =
+        InlineBoxTraversal::FindLeftBoundaryOfEntireBidiRunIgnoringLineBreak(

[Xiaocheng, 2017/06/15 01:11:48]

Do we pass in the correct bidi level here?

[yosin_UTC9, 2017/06/15 03:42:14]

Done.

+            *inline_box);
     return InlineBoxPosition(inline_box, inline_box->CaretLeftmostOffset());
   }
 
@@ -958,12 +945,8 @@ static InlineBoxPosition AdjustInlineBoxPositionForTextDirection(
     return InlineBoxPosition(inline_box, caret_offset);
 
   // Left edge of a "tertiary" run. Set to the right edge of that run.
-  while (InlineBox* tertiary_box =
-             inline_box->NextLeafChildIgnoringLineBreak()) {
-    if (tertiary_box->BidiLevel() <= level)
-      break;
-    inline_box = tertiary_box;
-  }
+  inline_box = InlineBoxTraversal::FindRightBoundaryOfBidiRunIgnoringLineBreak(

[Xiaocheng, 2017/06/15 01:11:48]

Do we pass in the correct bidi level here?

[yosin_UTC9, 2017/06/15 03:42:14]

Done.

+      *inline_box);
   return InlineBoxPosition(inline_box, inline_box->CaretRightmostOffset());
 }