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

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

This patch introduces |Find{Left,Right}BoundaryOfBidiRun()| and variations
in |InlineBoxTraversal| class to simplify common pattern in
|AdjustInlineBoxPositionForTextDirection()| for improving code health.

Pattern:
InlineBox* result = start;
while (InlineBox* runner = runner->Forward()) {
  if (ShouldBreak(runner->BidiLevel(), bidi_level))
    break;
  result = runner;
}

- Forward can be {Next,Prev}LeafChild{,IgnoreLineBreak}()
- ShouldBreak can be operator<() or operator<=()

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

Review-Url: https://codereview.chromium.org/2942523003
Cr-Commit-Position: refs/heads/master@{#479919}
Committed: https://chromium.googlesource.com/chromium/src/+/415ac0eadcdf44479c216c06445e84c7556b718c

[yosin_UTC9, 2017-06-14 07:27:59]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 07:28:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yosin_UTC9, 2017-06-14 08:10:51]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 08:11:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yosin_UTC9, 2017-06-14 08:24:41]

Description was changed from

==========
2017-06-14T16:26:23

BUG=707656

2017-06-14T16:23:54
==========

to

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

This patch introduces |Find{Left,Right}BoundaryOfBidiRun()| and variations
in |InlineBoxTraversal| class to simplify common pattern in
|AdjustInlineBoxPositionForTextDirection()| for improving code health.

Pattern:
InlineBox* result = start;
while (InlineBox* runner = runner->Forward()) {
  if (ShouldBreak(runner->BidiLevel(), start->level))
    break;
  result = runner;
}

- Forward can be {Next,Prev}LeafChild{,IgnoreLineBreak}()
- ShouldBreak can be operator<() or operator<=()

Note: To avoid pass both |InlineBox*| and BiDi level, we use |InlineBox*|
to set |level| for |Find{Left,Right}BoundaryOfEntireBidiRun()|.

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

[yosin_UTC9, 2017-06-14 09:41:32]

yosin@chromium.org changed reviewers:
+ xiaochengh@chromium.org, yoichio@chromium.org

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

PTAL

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right):

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:886:
InlineBoxTraversal::FindRightBoundaryOfEntireBidiRun(*next_box);
Because |level = next_box->BidiLevel()| at L874, we use |next_box| to
avoid pass both |InlineBox*| and BiDi level to
|indRightBoundaryOfEntireBidiRun|.

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:903:
*inline_box->PrevLeafChild());
To avoid pass both |InlineBox*| and BiDi level to
|FindLeftBoundaryOfEntireBidiRun()|,
we use |inline_box->PrevLeafChild()| since |level =
inline_box->PrevLeafChild()->BidiLevel()| at L893.

[commit-bot: I haz the power, 2017-06-14 10:45:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 10:45:22]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

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

It seems very tricky not to pass in the bidi level explicitly...

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (left):

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:884: while (InlineBox*
next_box = inline_box->NextLeafChild()) {
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.

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right):

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:886:
InlineBoxTraversal::FindRightBoundaryOfEntireBidiRun(*next_box);
On 2017/06/14 at 09:41:34, yosin_UTC9 wrote:
> Because |level = next_box->BidiLevel()| at L874, we use |next_box| to
> avoid pass both |InlineBox*| and BiDi level to
|indRightBoundaryOfEntireBidiRun|.

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?

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:903:
*inline_box->PrevLeafChild());
On 2017/06/14 at 09:41:34, yosin_UTC9 wrote:
> To avoid pass both |InlineBox*| and BiDi level to
|FindLeftBoundaryOfEntireBidiRun()|,
> we use |inline_box->PrevLeafChild()| since |level =
inline_box->PrevLeafChild()->BidiLevel()| at L893.

Ditto.

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:914: *inline_box);
Do we pass in the correct bidi level here?

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:921:
InlineBoxTraversal::FindLeftBoundaryOfBidiRunIgnoringLineBreak(
Do we pass in the correct bidi level here?

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:939:
InlineBoxTraversal::FindLeftBoundaryOfEntireBidiRunIgnoringLineBreak(
Do we pass in the correct bidi level here?

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:948: inline_box =
InlineBoxTraversal::FindRightBoundaryOfBidiRunIgnoringLineBreak(
Do we pass in the correct bidi level here?

[yosin_UTC9, 2017-06-15 03:31:24]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 03:31:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yosin_UTC9, 2017-06-15 03:37:06]

Description was changed from

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

This patch introduces |Find{Left,Right}BoundaryOfBidiRun()| and variations
in |InlineBoxTraversal| class to simplify common pattern in
|AdjustInlineBoxPositionForTextDirection()| for improving code health.

Pattern:
InlineBox* result = start;
while (InlineBox* runner = runner->Forward()) {
  if (ShouldBreak(runner->BidiLevel(), start->level))
    break;
  result = runner;
}

- Forward can be {Next,Prev}LeafChild{,IgnoreLineBreak}()
- ShouldBreak can be operator<() or operator<=()

Note: To avoid pass both |InlineBox*| and BiDi level, we use |InlineBox*|
to set |level| for |Find{Left,Right}BoundaryOfEntireBidiRun()|.

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

to

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

This patch introduces |Find{Left,Right}BoundaryOfBidiRun()| and variations
in |InlineBoxTraversal| class to simplify common pattern in
|AdjustInlineBoxPositionForTextDirection()| for improving code health.

Pattern:
InlineBox* result = start;
while (InlineBox* runner = runner->Forward()) {
  if (ShouldBreak(runner->BidiLevel(), bidi_level))
    break;
  result = runner;
}

- Forward can be {Next,Prev}LeafChild{,IgnoreLineBreak}()
- ShouldBreak can be operator<() or operator<=()

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

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

PTAL

Changed Find{Left,Right}BoundaryOfBidiRun() to take |bidi_level| parameter to
avoid
using assumption |bidi_level == inline_box->BidiLevel()|.

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (left):

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:884: while (InlineBox*
next_box = inline_box->NextLeafChild()) {
On 2017/06/15 at 01:11:48, Xiaocheng wrote:
> 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.

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.

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right):

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:886:
InlineBoxTraversal::FindRightBoundaryOfEntireBidiRun(*next_box);
On 2017/06/15 at 01:11:48, Xiaocheng wrote:
> On 2017/06/14 at 09:41:34, yosin_UTC9 wrote:
> > Because |level = next_box->BidiLevel()| at L874, we use |next_box| to
> > avoid pass both |InlineBox*| and BiDi level to
|indRightBoundaryOfEntireBidiRun|.
> 
> 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?

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

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:903:
*inline_box->PrevLeafChild());
On 2017/06/15 at 01:11:48, Xiaocheng wrote:
> On 2017/06/14 at 09:41:34, yosin_UTC9 wrote:
> > To avoid pass both |InlineBox*| and BiDi level to
|FindLeftBoundaryOfEntireBidiRun()|,
> > we use |inline_box->PrevLeafChild()| since |level =
inline_box->PrevLeafChild()->BidiLevel()| at L893.
> 
> Ditto.

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

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:914: *inline_box);
On 2017/06/15 at 01:11:48, Xiaocheng wrote:
> Do we pass in the correct bidi level here?
Done.

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:921:
InlineBoxTraversal::FindLeftBoundaryOfBidiRunIgnoringLineBreak(
On 2017/06/15 at 01:11:48, Xiaocheng wrote:
> Do we pass in the correct bidi level here?

Done.

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:939:
InlineBoxTraversal::FindLeftBoundaryOfEntireBidiRunIgnoringLineBreak(
On 2017/06/15 at 01:11:48, Xiaocheng wrote:
> Do we pass in the correct bidi level here?

Done.

https://codereview.chromium.org/2942523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:948: inline_box =
InlineBoxTraversal::FindRightBoundaryOfBidiRunIgnoringLineBreak(
On 2017/06/15 at 01:11:48, Xiaocheng wrote:
> Do we pass in the correct bidi level here?

Done.

[commit-bot: I haz the power, 2017-06-15 05:29:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-15 05:29:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xiaocheng, 2017-06-15 18:10:52]

lgtm

[yosin_UTC9, 2017-06-15 23:29:07]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-06-15 23:29:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 02:03:18]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497569347004610,
"parent_rev": "ab6c2f591d66ceca9ff7901de4f632a6e1a3260b", "commit_rev":
"415ac0eadcdf44479c216c06445e84c7556b718c"}

[commit-bot: I haz the power, 2017-06-16 02:03:26]

Description was changed from

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

This patch introduces |Find{Left,Right}BoundaryOfBidiRun()| and variations
in |InlineBoxTraversal| class to simplify common pattern in
|AdjustInlineBoxPositionForTextDirection()| for improving code health.

Pattern:
InlineBox* result = start;
while (InlineBox* runner = runner->Forward()) {
  if (ShouldBreak(runner->BidiLevel(), bidi_level))
    break;
  result = runner;
}

- Forward can be {Next,Prev}LeafChild{,IgnoreLineBreak}()
- ShouldBreak can be operator<() or operator<=()

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

to

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

This patch introduces |Find{Left,Right}BoundaryOfBidiRun()| and variations
in |InlineBoxTraversal| class to simplify common pattern in
|AdjustInlineBoxPositionForTextDirection()| for improving code health.

Pattern:
InlineBox* result = start;
while (InlineBox* runner = runner->Forward()) {
  if (ShouldBreak(runner->BidiLevel(), bidi_level))
    break;
  result = runner;
}

- Forward can be {Next,Prev}LeafChild{,IgnoreLineBreak}()
- ShouldBreak can be operator<() or operator<=()

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

Review-Url: https://codereview.chromium.org/2942523003
Cr-Commit-Position: refs/heads/master@{#479919}
Committed:
https://chromium.googlesource.com/chromium/src/+/415ac0eadcdf44479c216c06445e...
==========

[commit-bot: I haz the power, 2017-06-16 02:03:27]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/415ac0eadcdf44479c216c06445e...