|
|
Created:
3 years, 7 months ago by kojii Modified:
3 years, 7 months ago Reviewers:
eae CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, cbiesinger, chromium-reviews, dgrogan+ng_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, eae+blinkwatch, fmalita+watch_chromium.org, glebl+reviews_chromium.org, jchaffraix+rendering, Justin Novosad, kinuko+watch, leviw+renderwatch, ojan+watch_chromium.org, pdr+graphicswatchlist_chromium.org, pdr+renderingwatchlist_chromium.org, rwlbuis, Stephen Chennney, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[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
Patch Set 1 #Patch Set 2 : clang build fix #
Total comments: 7
Patch Set 3 : eae review + RTL snapping fixes #
Dependent Patchsets: Messages
Total messages: 40 (33 generated)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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 ==========
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 ==========
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 ==========
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kojii@chromium.org changed reviewers: + eae@chromium.org
PTAL. I hope they're well related but if I'm putting too many into one CL, please let me know.
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 ==========
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 ==========
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 ==========
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!
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!
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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!
The CQ bit was checked by kojii@chromium.org
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")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494784415233470, "parent_rev": "df78633101a43642053c681b8530c88bf7032e06", "commit_rev": "51cc75bd66227dafea735ffa7ea1672b51a72e20"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/51cc75bd66227dafea735ffa7ea1... |