[LayoutNG] Misc fixes in ShapingLineBreaker

[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Makes ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Fixed ShapeResult::OffsetForPosition() to return start/end offset
   when the position is outside of the ShapeResult. Before this change,
   OffsetForPosition() returns visual right, which produces incorrect
   results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

These fixes make at least a few simple cases to layout correctly with
NGLineBreaker[2] when locally enabled.

[1] https://codereview.chromium.org/2871173002
[2] https://codereview.chromium.org/2871733003

BUG=636993
Review-Url: https://codereview.chromium.org/2875933006
Cr-Commit-Position: refs/heads/master@{#471636}
Committed: https://chromium.googlesource.com/chromium/src/+/51cc75bd66227dafea735ffa7ea1672b51a72e20

[kojii, 2017-05-12 17:39:39]

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

[commit-bot: I haz the power, 2017-05-12 17:41:01]

Dry run: CQ is trying da patch.

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

[kojii, 2017-05-12 18:02:31]

Description was changed from

==========
slb

BUG=
==========

to

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
3. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
4. Add ShapeResult::kOutsideAsStartOrEnd for OffsetForPosition() to
   return start/end offset when the position is outside of the
   ShapeResult. Without this, OffsetForPosition() returns visual right,
   which produces incorrect results for RTL.
5. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
6. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
7. Fixes start position to floor and end position to ceil. This fixes
  an issue when available width is zero.
8. Fixes when shape range ends at the mid of a word.

[1] https://codereview.chromium.org/2871173002

BUG=636993
==========

[commit-bot: I haz the power, 2017-05-12 18:10:15]

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

[commit-bot: I haz the power, 2017-05-12 18:10:16]

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

[kojii, 2017-05-12 18:26:05]

Description was changed from

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
3. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
4. Add ShapeResult::kOutsideAsStartOrEnd for OffsetForPosition() to
   return start/end offset when the position is outside of the
   ShapeResult. Without this, OffsetForPosition() returns visual right,
   which produces incorrect results for RTL.
5. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
6. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
7. Fixes start position to floor and end position to ceil. This fixes
  an issue when available width is zero.
8. Fixes when shape range ends at the mid of a word.

[1] https://codereview.chromium.org/2871173002

BUG=636993
==========

to

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Made ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Add ShapeResult::kOutsideAsStartOrEnd for OffsetForPosition() to
   return start/end offset when the position is outside of the
   ShapeResult. Without this, OffsetForPosition() returns visual right,
   which produces incorrect results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

[1] https://codereview.chromium.org/2871173002

BUG=636993
==========

[kojii, 2017-05-12 18:26:21]

Description was changed from

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Made ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Add ShapeResult::kOutsideAsStartOrEnd for OffsetForPosition() to
   return start/end offset when the position is outside of the
   ShapeResult. Without this, OffsetForPosition() returns visual right,
   which produces incorrect results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

[1] https://codereview.chromium.org/2871173002

BUG=636993
==========

to

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Makes ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Add ShapeResult::kOutsideAsStartOrEnd for OffsetForPosition() to
   return start/end offset when the position is outside of the
   ShapeResult. Without this, OffsetForPosition() returns visual right,
   which produces incorrect results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

[1] https://codereview.chromium.org/2871173002

BUG=636993
==========

[kojii, 2017-05-12 18:28:04]

Description was changed from

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Makes ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Add ShapeResult::kOutsideAsStartOrEnd for OffsetForPosition() to
   return start/end offset when the position is outside of the
   ShapeResult. Without this, OffsetForPosition() returns visual right,
   which produces incorrect results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

[1] https://codereview.chromium.org/2871173002

BUG=636993
==========

to

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Makes ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Add ShapeResult::kOutsideAsStartOrEnd for OffsetForPosition() to
   return start/end offset when the position is outside of the
   ShapeResult. Without this, OffsetForPosition() returns visual right,
   which produces incorrect results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

These fixes makes at least a few simple cases to layout with
NGLineBreaker[2], 

[1] https://codereview.chromium.org/2871173002
[2] https://codereview.chromium.org/2871733003

BUG=636993
==========

[kojii, 2017-05-12 18:29:14]

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

[commit-bot: I haz the power, 2017-05-12 18:30:37]

Dry run: CQ is trying da patch.

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

[kojii, 2017-05-12 18:33:38]

kojii@chromium.org changed reviewers:
+ eae@chromium.org

[kojii, 2017-05-12 18:33:39]

PTAL.

I hope they're well related but if I'm putting too many into one CL, please let
me know.

[kojii, 2017-05-12 18:34:07]

Description was changed from

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Makes ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Add ShapeResult::kOutsideAsStartOrEnd for OffsetForPosition() to
   return start/end offset when the position is outside of the
   ShapeResult. Without this, OffsetForPosition() returns visual right,
   which produces incorrect results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

These fixes makes at least a few simple cases to layout with
NGLineBreaker[2], 

[1] https://codereview.chromium.org/2871173002
[2] https://codereview.chromium.org/2871733003

BUG=636993
==========

to

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Makes ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Add ShapeResult::kOutsideAsStartOrEnd for OffsetForPosition() to
   return start/end offset when the position is outside of the
   ShapeResult. Without this, OffsetForPosition() returns visual right,
   which produces incorrect results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

These fixes make at least a few simple cases to layout with
NGLineBreaker[2], 

[1] https://codereview.chromium.org/2871173002
[2] https://codereview.chromium.org/2871733003

BUG=636993
==========

[kojii, 2017-05-12 18:34:17]

Description was changed from

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Makes ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Add ShapeResult::kOutsideAsStartOrEnd for OffsetForPosition() to
   return start/end offset when the position is outside of the
   ShapeResult. Without this, OffsetForPosition() returns visual right,
   which produces incorrect results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

These fixes make at least a few simple cases to layout with
NGLineBreaker[2], 

[1] https://codereview.chromium.org/2871173002
[2] https://codereview.chromium.org/2871733003

BUG=636993
==========

to

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Makes ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Add ShapeResult::kOutsideAsStartOrEnd for OffsetForPosition() to
   return start/end offset when the position is outside of the
   ShapeResult. Without this, OffsetForPosition() returns visual right,
   which produces incorrect results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

These fixes make at least a few simple cases to layout correctly with
NGLineBreaker[2], 

[1] https://codereview.chromium.org/2871173002
[2] https://codereview.chromium.org/2871733003

BUG=636993
==========

[kojii, 2017-05-12 18:34:43]

Description was changed from

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Makes ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Add ShapeResult::kOutsideAsStartOrEnd for OffsetForPosition() to
   return start/end offset when the position is outside of the
   ShapeResult. Without this, OffsetForPosition() returns visual right,
   which produces incorrect results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

These fixes make at least a few simple cases to layout correctly with
NGLineBreaker[2], 

[1] https://codereview.chromium.org/2871173002
[2] https://codereview.chromium.org/2871733003

BUG=636993
==========

to

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Makes ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Add ShapeResult::kOutsideAsStartOrEnd for OffsetForPosition() to
   return start/end offset when the position is outside of the
   ShapeResult. Without this, OffsetForPosition() returns visual right,
   which produces incorrect results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

These fixes make at least a few simple cases to layout correctly with
NGLineBreaker[2] when locally enabled.

[1] https://codereview.chromium.org/2871173002
[2] https://codereview.chromium.org/2871733003

BUG=636993
==========

[eae, 2017-05-12 18:45:49]

https://codereview.chromium.org/2875933006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.h (right):

https://codereview.chromium.org/2875933006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.h:71: unsigned
CharacterStartIndex() const;
CharacterOffset perhaps and CharacterLength for (NumCharacters) to be more
consistent?

or CharacterStartIndex and CharacterEndIndex

https://codereview.chromium.org/2875933006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.h:90: unsigned
OffsetForPosition(float target_x, OffsetOptions) const;
Isn't kOutsideAsStartOrEnd always the desirable behavior?

https://codereview.chromium.org/2875933006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/shaping/ShapingLineBreaker.cpp
(right):

https://codereview.chromium.org/2875933006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/shaping/ShapingLineBreaker.cpp:127: //
Reshape takes place only when first_safe is before the break opportunity.
I considered this case and wanted to fix it but failed to create a test case
where it would fail which is why I left it out of the original version. Did you
manage to come up with a test for it? If so, yay!

[kojii, 2017-05-12 19:08:13]

https://codereview.chromium.org/2875933006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.h (right):

https://codereview.chromium.org/2875933006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.h:71: unsigned
CharacterStartIndex() const;
On 2017/05/12 at 18:45:49, eae wrote:
> CharacterOffset perhaps and CharacterLength for (NumCharacters) to be more
consistent?
> 
> or CharacterStartIndex and CharacterEndIndex

Existing functions such as OffsetForPosition() use "Offset" meaning offset from
the first shape range, so I avoided "Offset". ...or should all these function
changed to use offset from the beginning of a full string?

If not, I might add CharacterEndIndex(). Or if you get better suggestions after
reading my reply.

https://codereview.chromium.org/2875933006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.h:90: unsigned
OffsetForPosition(float target_x, OffsetOptions) const;
On 2017/05/12 at 18:45:49, eae wrote:
> Isn't kOutsideAsStartOrEnd always the desirable behavior?

I think so, but there are bunch of existing callers and hard to determine if
anyone is relying on kOutsideAsRight behavior.

I'll try to change and see if any tests break.

https://codereview.chromium.org/2875933006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/shaping/ShapingLineBreaker.cpp
(right):

https://codereview.chromium.org/2875933006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/shaping/ShapingLineBreaker.cpp:127: //
Reshape takes place only when first_safe is before the break opportunity.
On 2017/05/12 at 18:45:49, eae wrote:
> I considered this case and wanted to fix it but failed to create a test case
where it would fail which is why I left it out of the original version. Did you
manage to come up with a test for it? If so, yay!

Yes, the Han character in your ShapeLineArabicThaiHanLatinBreakAll() hit this
when I added some new cases. Thank you for the test!

[commit-bot: I haz the power, 2017-05-12 19:52:43]

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

[commit-bot: I haz the power, 2017-05-12 19:52:44]

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_...)

[eae, 2017-05-12 21:24:05]

LGTM

https://codereview.chromium.org/2875933006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.h (right):

https://codereview.chromium.org/2875933006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.h:71: unsigned
CharacterStartIndex() const;
On 2017/05/12 19:08:13, kojii wrote:
> On 2017/05/12 at 18:45:49, eae wrote:
> > CharacterOffset perhaps and CharacterLength for (NumCharacters) to be more
> consistent?
> > 
> > or CharacterStartIndex and CharacterEndIndex
> 
> Existing functions such as OffsetForPosition() use "Offset" meaning offset
from
> the first shape range, so I avoided "Offset". ...or should all these function
> changed to use offset from the beginning of a full string?
> 
> If not, I might add CharacterEndIndex(). Or if you get better suggestions
after
> reading my reply.

Ah, fair enough. Perhaps we could be even more explicit and call it
StartIndexForResult and EndIndexForResult?

Either way, as long as the comment is accurate (which it is) the name is fine as
is. Thanks for the explanation.

[kojii, 2017-05-14 04:52:43]

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

[commit-bot: I haz the power, 2017-05-14 04:52:55]

Dry run: 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-05-14 06:18:59]

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

[commit-bot: I haz the power, 2017-05-14 06:19:00]

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

[kojii, 2017-05-14 07:34:46]

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

[commit-bot: I haz the power, 2017-05-14 07:34:56]

Dry run: CQ is trying da patch.

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

[kojii, 2017-05-14 07:59:36]

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

[commit-bot: I haz the power, 2017-05-14 07:59:50]

Dry run: CQ is trying da patch.

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

[kojii, 2017-05-14 08:00:29]

Patchset #3 (id:40001) has been deleted

[kojii, 2017-05-14 08:00:39]

Patchset #3 (id:60001) has been deleted

[kojii, 2017-05-14 08:02:26]

Description was changed from

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Makes ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Add ShapeResult::kOutsideAsStartOrEnd for OffsetForPosition() to
   return start/end offset when the position is outside of the
   ShapeResult. Without this, OffsetForPosition() returns visual right,
   which produces incorrect results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

These fixes make at least a few simple cases to layout correctly with
NGLineBreaker[2] when locally enabled.

[1] https://codereview.chromium.org/2871173002
[2] https://codereview.chromium.org/2871733003

BUG=636993
==========

to

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Makes ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Fixed ShapeResult::OffsetForPosition() to return start/end offset
   when the position is outside of the ShapeResult. Before this change,
   OffsetForPosition() returns visual right, which produces incorrect
   results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

These fixes make at least a few simple cases to layout correctly with
NGLineBreaker[2] when locally enabled.

[1] https://codereview.chromium.org/2871173002
[2] https://codereview.chromium.org/2871733003

BUG=636993
==========

[commit-bot: I haz the power, 2017-05-14 11:44:37]

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

[commit-bot: I haz the power, 2017-05-14 11:44:38]

Dry run: This issue passed the CQ dry run.

[kojii, 2017-05-14 17:53:31]

On 2017/05/12 at 21:24:05, eae wrote:
> Perhaps we could be even more explicit and call it StartIndexForResult and
EndIndexForResult?
> 
> Either way, as long as the comment is accurate (which it is) the name is fine
as is. Thanks for the explanation.

I like more explicit ones, so took your idea, but please feel free to rename if
you come up with better one!

[kojii, 2017-05-14 17:53:35]

The CQ bit was checked by kojii@chromium.org

[kojii, 2017-05-14 17:53:35]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2875933006/#ps80001
(title: "eae review + RTL snapping fixes")

[commit-bot: I haz the power, 2017-05-14 17:53:40]

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-05-14 17:57:31]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494784415233470,
"parent_rev": "df78633101a43642053c681b8530c88bf7032e06", "commit_rev":
"51cc75bd66227dafea735ffa7ea1672b51a72e20"}

[commit-bot: I haz the power, 2017-05-14 17:57:48]

Description was changed from

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Makes ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Fixed ShapeResult::OffsetForPosition() to return start/end offset
   when the position is outside of the ShapeResult. Before this change,
   OffsetForPosition() returns visual right, which produces incorrect
   results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

These fixes make at least a few simple cases to layout correctly with
NGLineBreaker[2] when locally enabled.

[1] https://codereview.chromium.org/2871173002
[2] https://codereview.chromium.org/2871733003

BUG=636993
==========

to

==========
[LayoutNG] Misc fixes in ShapingLineBreaker

This patch:
1. Changes ShapingLineBreaker constructor to take LazyLineBreakIterator
   instead of locale and LineBreakType. LineBreakType was added to
   LazyLineBreakIterator in [1].
2. Makes ShapingLineBreaker STACK_ALLOCATED to keep a pointer to
   LazyLineBreakIterator.
3. Fixes ShapingLineBreaker to take the range shaping into account.
   Measuring outside of the shaped range results in zero, which lead to
   incorrect results.
4. To fix 2 above, adds ShapeResult::CharacterStartIndex(), that returns
   the start index of range shaping.
5. Fixed ShapeResult::OffsetForPosition() to return start/end offset
   when the position is outside of the ShapeResult. Before this change,
   OffsetForPosition() returns visual right, which produces incorrect
   results for RTL.
6. Fixes RTL and its tests. Since ShapingLineBreaker depends on
   conversions between offsets and positions, and since positions in
   ShapeResult is visual, adds FlipRtl() function.
7. Fixes when first safe is beyond the break opportunity. CJK, Thai,
   and some other scripts do not use spaces for word boundaries. This
   occurs often in these scripts with current mock safe-to-break, but
   could happen in real scenario too.
   To do this, changed to compute break opportunity before first safe.
8. Fixes start position to floor and end position to ceil. This fixes
   an issue when available width is zero.
9. Fixes when shape range ends at the mid of a word.

These fixes make at least a few simple cases to layout correctly with
NGLineBreaker[2] when locally enabled.

[1] https://codereview.chromium.org/2871173002
[2] https://codereview.chromium.org/2871733003

BUG=636993

Review-Url: https://codereview.chromium.org/2875933006
Cr-Commit-Position: refs/heads/master@{#471636}
Committed:
https://chromium.googlesource.com/chromium/src/+/51cc75bd66227dafea735ffa7ea1...
==========

[commit-bot: I haz the power, 2017-05-14 17:57:49]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/51cc75bd66227dafea735ffa7ea1...