Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1833653002:
Move LayoutObject::nextOffset/previousOffset() to editing utility (Closed)
|
Created:
4 years, 9 months ago by Seigo Nonaka Modified:
4 years, 9 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove LayoutObject::nextOffset/previousOffset() to editing utility
This is preparation of introducing our own grapheme cluster boundary
breaker to the Blink. To prevent regressions, added unit tests for
known rules.
There are few differences from original Unicode grapheme cluster
boundary spec.
- Break even after Prepend class character.
- Break after Tamil virama sign.
The cursorMovementIterator function also addresses following things
but not necessary for latest Unicode spec.
- Extend class is excluded from SpacingMark.
- Japanese half-width katakana voiced marks has Extend property.
BUG=594923
Committed: https://crrev.com/5caf6fd59d6f35157a3e6ba4728434ac3d1b0886
Cr-Commit-Position: refs/heads/master@{#383265}
Patch Set 1 : #
Total comments: 24
Patch Set 2 : WIP: addressed comments. #Patch Set 3 : added test cases. #Messages
Total messages: 24 (9 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Remove LayoutObject::previous/nextOffset. This is preparation of introducing our own grapheme cluster boundary breaker to the Blink. To prevent regressions, added unit tests for known rules. There are few differences from original Unicode grapheme cluster boundary spec. - Break even after Prepend class character. - Break after Tamil virama sign. cursorMovementIterator also addresses following things but not necessary for latest Unicode spec. - Already Extend class is excluded from SpacingMark. - Japanese half-width katakana voiced marks are in Extend class. BUG=594923 ========== to ========== Remove LayoutObject::previous/nextOffset. This is preparation of introducing our own grapheme cluster boundary breaker to the Blink. To prevent regressions, added unit tests for known rules. There are few differences from original Unicode grapheme cluster boundary spec. - Break even after Prepend class character. - Break after Tamil virama sign. The cursorMovementIterator function also addresses following things but not necessary for latest Unicode spec. - Extend class is excluded from SpacingMark. - Japanese half-width katakana voiced marks has Extend property. BUG=594923 ==========
nona@chromium.org changed reviewers: + yosin@chromium.org
Hi Yoshi-san, Could you kindly take a look? Thank you.
yosin@chromium.org changed reviewers: + tkent@chromium.org
lgtm w/ small nits +tkent@ for core/layout changes for moving code from core/layout to core/editing https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingTestBase.h (right): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingTestBase.h:29: void setBodyContent(const std::string&); Good! (^_^)b BTW, I'm not sure why PRESUBMIT complain about having |#include <string>| https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:545: int uncheckedPreviousOffset(const Node* n, int current) Could you avoid to use one letter variable name? https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:555: long result = iterator->preceding(current); |int| is better since |TextBreakIterator::preceding()| returns |int|. |const int result = iterator->preceding(current)| https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:556: if (result == TextBreakDone) Use ternary or early return: return reuslt == TextBreakDone ? current - 1 : result; or if (result == TextBreakDone) return current - 1; return result; https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:576: long result = iterator->following(current); |int| is better since |TextBreakIterator::following()| returns |int|. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:577: if (result == TextBreakDone) Use ternary or early return: return reuslt == TextBreakDone ? current - 1 : result; or if (result == TextBreakDone) return current - 1; return result;
It is better to use summary as Move LayoutObject::nextOffset/previousOffset() to editing utility
https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (left): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:547: return n->layoutObject() ? n->layoutObject()->previousOffset(current) : current - 1; Is it ok to remove n->layoutObject() check in the original code? If |n| is a Text node without LayoutObject, this CL has behavior difference. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2854: offset = box->isLeftToRightDirection() ? uncheckedPreviousOffset(lineLayoutItem.node(), offset) : uncheckedNextOffset(lineLayoutItem.node(), offset); nit: no need to add an extra space before |box|. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:3029: offset = box->isLeftToRightDirection() ? uncheckedNextOffset(layoutObject->node(), offset) : uncheckedPreviousOffset(layoutObject->node(), offset); nit: no need to add an extra space before |uncheckedNextOffset|.
https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (left): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:547: return n->layoutObject() ? n->layoutObject()->previousOffset(current) : current - 1; On 2016/03/25 at 04:34:08, tkent wrote: > Is it ok to remove n->layoutObject() check in the original code? If |n| is a Text node without LayoutObject, this CL has behavior difference. Yes. Using |LayoutObject::previousOffset()| is wrong regarding - first-letter: LayoutText contains non-first-letter part, e.g. <div::first-letter>foo</div> yields two LayoutTextFragment, one holds "f" and another holds "oo". TextNode("foo").layoutObject() = LayoutText("oo"). This is source of first-letter related bug - text-transform: text-transform:uppercase yield TextNode("ß"), eszett => LayoutText("SS"). This is source of unbound index in various places. From above, we change not to use LayoutObject here.
https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (left): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:547: return n->layoutObject() ? n->layoutObject()->previousOffset(current) : current - 1; On 2016/03/25 at 04:57:53, Yosi_UTC9 wrote: > On 2016/03/25 at 04:34:08, tkent wrote: > > Is it ok to remove n->layoutObject() check in the original code? If |n| is a Text node without LayoutObject, this CL has behavior difference. > Yes. Using |LayoutObject::previousOffset()| is wrong regarding > - first-letter: LayoutText contains non-first-letter part, e.g. <div::first-letter>foo</div> yields two LayoutTextFragment, one holds "f" and another holds "oo". TextNode("foo").layoutObject() = LayoutText("oo"). This is source of first-letter related bug > - text-transform: text-transform:uppercase yield TextNode("ß"), eszett => LayoutText("SS"). This is source of unbound index in various places. > From above, we change not to use LayoutObject here. I asked about a case of no LayoutObject. Anyway, we need more tests: - |n| is Text without LayoutObject - correctly work even if first-letter is specified - correctly work even if text-transform is specified
Thank you for your review. I need extra work for unit test but let me reply at this moment. I'm going to ping you once the unit tests are ready to review. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingTestBase.h (right): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingTestBase.h:29: void setBodyContent(const std::string&); On 2016/03/25 04:13:30, Yosi_UTC9 wrote: > Good! (^_^)b > > BTW, I'm not sure why PRESUBMIT complain about having |#include <string>| Yeah, it doesn't warn about it. Fixed by adding include. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (left): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:547: return n->layoutObject() ? n->layoutObject()->previousOffset(current) : current - 1; On 2016/03/25 05:05:25, tkent wrote: > On 2016/03/25 at 04:57:53, Yosi_UTC9 wrote: > > On 2016/03/25 at 04:34:08, tkent wrote: > > > Is it ok to remove n->layoutObject() check in the original code? If |n| is > a Text node without LayoutObject, this CL has behavior difference. > > Yes. Using |LayoutObject::previousOffset()| is wrong regarding > > - first-letter: LayoutText contains non-first-letter part, e.g. > <div::first-letter>foo</div> yields two LayoutTextFragment, one holds "f" and > another holds "oo". TextNode("foo").layoutObject() = LayoutText("oo"). This is > source of first-letter related bug > > - text-transform: text-transform:uppercase yield TextNode("ß"), eszett => > LayoutText("SS"). This is source of unbound index in various places. > > From above, we change not to use LayoutObject here. > > I asked about a case of no LayoutObject. Anyway, we need more tests: > - |n| is Text without LayoutObject > - correctly work even if first-letter is specified > - correctly work even if text-transform is specified Sure, I'm happy to add these test case but let me give extra time. In unit test context, looks like LayoutObject is not available. I need extra work for unit test. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:547: return n->layoutObject() ? n->layoutObject()->previousOffset(current) : current - 1; On 2016/03/25 04:57:53, Yosi_UTC9 wrote: > On 2016/03/25 at 04:34:08, tkent wrote: > > Is it ok to remove n->layoutObject() check in the original code? If |n| is a > Text node without LayoutObject, this CL has behavior difference. > Yes. Using |LayoutObject::previousOffset()| is wrong regarding > - first-letter: LayoutText contains non-first-letter part, e.g. > <div::first-letter>foo</div> yields two LayoutTextFragment, one holds "f" and > another holds "oo". TextNode("foo").layoutObject() = LayoutText("oo"). This is > source of first-letter related bug > - text-transform: text-transform:uppercase yield TextNode("ß"), eszett => > LayoutText("SS"). This is source of unbound index in various places. > From above, we change not to use LayoutObject here. On 2016/03/25 04:57:53, Yosi_UTC9 wrote: > On 2016/03/25 at 04:34:08, tkent wrote: > > Is it ok to remove n->layoutObject() check in the original code? If |n| is a > Text node without LayoutObject, this CL has behavior difference. > Yes. Using |LayoutObject::previousOffset()| is wrong regarding > - first-letter: LayoutText contains non-first-letter part, e.g. > <div::first-letter>foo</div> yields two LayoutTextFragment, one holds "f" and > another holds "oo". TextNode("foo").layoutObject() = LayoutText("oo"). This is > source of first-letter related bug > - text-transform: text-transform:uppercase yield TextNode("ß"), eszett => > LayoutText("SS"). This is source of unbound index in various places. > From above, we change not to use LayoutObject here. Thank you for your follow-up comment. Let me add one note for those issues. Even if this CL and http://codereview.chromium.org/1824143003 are submitted, backspace problem for first-letter/text-transform is not fixed. I'm now investigating the root cause. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:545: int uncheckedPreviousOffset(const Node* n, int current) On 2016/03/25 04:13:30, Yosi_UTC9 wrote: > Could you avoid to use one letter variable name? Done. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:555: long result = iterator->preceding(current); On 2016/03/25 04:13:30, Yosi_UTC9 wrote: > |int| is better since |TextBreakIterator::preceding()| returns |int|. > |const int result = iterator->preceding(current)| Done. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:556: if (result == TextBreakDone) On 2016/03/25 04:13:30, Yosi_UTC9 wrote: > Use ternary or early return: > > return reuslt == TextBreakDone ? current - 1 : result; > > or > if (result == TextBreakDone) > return current - 1; > return result; Done. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:576: long result = iterator->following(current); On 2016/03/25 04:13:30, Yosi_UTC9 wrote: > |int| is better since |TextBreakIterator::following()| returns |int|. Done. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:577: if (result == TextBreakDone) On 2016/03/25 04:13:30, Yosi_UTC9 wrote: > Use ternary or early return: > > return reuslt == TextBreakDone ? current - 1 : result; > > or > if (result == TextBreakDone) > return current - 1; > return result; Done. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2854: offset = box->isLeftToRightDirection() ? uncheckedPreviousOffset(lineLayoutItem.node(), offset) : uncheckedNextOffset(lineLayoutItem.node(), offset); On 2016/03/25 04:34:08, tkent wrote: > nit: no need to add an extra space before |box|. Done. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:3029: offset = box->isLeftToRightDirection() ? uncheckedNextOffset(layoutObject->node(), offset) : uncheckedPreviousOffset(layoutObject->node(), offset); On 2016/03/25 04:34:08, tkent wrote: > nit: no need to add an extra space before |uncheckedNextOffset|. Done.
Thank you for your review. I need extra work for unit test but let me reply at this moment. I'm going to ping you once the unit tests are ready to review. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingTestBase.h (right): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingTestBase.h:29: void setBodyContent(const std::string&); On 2016/03/25 04:13:30, Yosi_UTC9 wrote: > Good! (^_^)b > > BTW, I'm not sure why PRESUBMIT complain about having |#include <string>| Yeah, it doesn't warn about it. Fixed by adding include. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (left): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:547: return n->layoutObject() ? n->layoutObject()->previousOffset(current) : current - 1; On 2016/03/25 05:05:25, tkent wrote: > On 2016/03/25 at 04:57:53, Yosi_UTC9 wrote: > > On 2016/03/25 at 04:34:08, tkent wrote: > > > Is it ok to remove n->layoutObject() check in the original code? If |n| is > a Text node without LayoutObject, this CL has behavior difference. > > Yes. Using |LayoutObject::previousOffset()| is wrong regarding > > - first-letter: LayoutText contains non-first-letter part, e.g. > <div::first-letter>foo</div> yields two LayoutTextFragment, one holds "f" and > another holds "oo". TextNode("foo").layoutObject() = LayoutText("oo"). This is > source of first-letter related bug > > - text-transform: text-transform:uppercase yield TextNode("ß"), eszett => > LayoutText("SS"). This is source of unbound index in various places. > > From above, we change not to use LayoutObject here. > > I asked about a case of no LayoutObject. Anyway, we need more tests: > - |n| is Text without LayoutObject > - correctly work even if first-letter is specified > - correctly work even if text-transform is specified Sure, I'm happy to add these test case but let me give extra time. In unit test context, looks like LayoutObject is not available. I need extra work for unit test. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:547: return n->layoutObject() ? n->layoutObject()->previousOffset(current) : current - 1; On 2016/03/25 04:57:53, Yosi_UTC9 wrote: > On 2016/03/25 at 04:34:08, tkent wrote: > > Is it ok to remove n->layoutObject() check in the original code? If |n| is a > Text node without LayoutObject, this CL has behavior difference. > Yes. Using |LayoutObject::previousOffset()| is wrong regarding > - first-letter: LayoutText contains non-first-letter part, e.g. > <div::first-letter>foo</div> yields two LayoutTextFragment, one holds "f" and > another holds "oo". TextNode("foo").layoutObject() = LayoutText("oo"). This is > source of first-letter related bug > - text-transform: text-transform:uppercase yield TextNode("ß"), eszett => > LayoutText("SS"). This is source of unbound index in various places. > From above, we change not to use LayoutObject here. On 2016/03/25 04:57:53, Yosi_UTC9 wrote: > On 2016/03/25 at 04:34:08, tkent wrote: > > Is it ok to remove n->layoutObject() check in the original code? If |n| is a > Text node without LayoutObject, this CL has behavior difference. > Yes. Using |LayoutObject::previousOffset()| is wrong regarding > - first-letter: LayoutText contains non-first-letter part, e.g. > <div::first-letter>foo</div> yields two LayoutTextFragment, one holds "f" and > another holds "oo". TextNode("foo").layoutObject() = LayoutText("oo"). This is > source of first-letter related bug > - text-transform: text-transform:uppercase yield TextNode("ß"), eszett => > LayoutText("SS"). This is source of unbound index in various places. > From above, we change not to use LayoutObject here. Thank you for your follow-up comment. Let me add one note for those issues. Even if this CL and http://codereview.chromium.org/1824143003 are submitted, backspace problem for first-letter/text-transform is not fixed. I'm now investigating the root cause. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:545: int uncheckedPreviousOffset(const Node* n, int current) On 2016/03/25 04:13:30, Yosi_UTC9 wrote: > Could you avoid to use one letter variable name? Done. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:555: long result = iterator->preceding(current); On 2016/03/25 04:13:30, Yosi_UTC9 wrote: > |int| is better since |TextBreakIterator::preceding()| returns |int|. > |const int result = iterator->preceding(current)| Done. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:556: if (result == TextBreakDone) On 2016/03/25 04:13:30, Yosi_UTC9 wrote: > Use ternary or early return: > > return reuslt == TextBreakDone ? current - 1 : result; > > or > if (result == TextBreakDone) > return current - 1; > return result; Done. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:576: long result = iterator->following(current); On 2016/03/25 04:13:30, Yosi_UTC9 wrote: > |int| is better since |TextBreakIterator::following()| returns |int|. Done. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:577: if (result == TextBreakDone) On 2016/03/25 04:13:30, Yosi_UTC9 wrote: > Use ternary or early return: > > return reuslt == TextBreakDone ? current - 1 : result; > > or > if (result == TextBreakDone) > return current - 1; > return result; Done. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2854: offset = box->isLeftToRightDirection() ? uncheckedPreviousOffset(lineLayoutItem.node(), offset) : uncheckedNextOffset(lineLayoutItem.node(), offset); On 2016/03/25 04:34:08, tkent wrote: > nit: no need to add an extra space before |box|. Done. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleUnits.cpp:3029: offset = box->isLeftToRightDirection() ? uncheckedNextOffset(layoutObject->node(), offset) : uncheckedPreviousOffset(layoutObject->node(), offset); On 2016/03/25 04:34:08, tkent wrote: > nit: no need to add an extra space before |uncheckedNextOffset|. Done.
https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (left): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:547: return n->layoutObject() ? n->layoutObject()->previousOffset(current) : current - 1; On 2016/03/25 at 05:11:22, Seigo Nonaka wrote: > On 2016/03/25 04:57:53, Yosi_UTC9 wrote: > > On 2016/03/25 at 04:34:08, tkent wrote: > > > Is it ok to remove n->layoutObject() check in the original code? If |n| is a > > Text node without LayoutObject, this CL has behavior difference. > > Yes. Using |LayoutObject::previousOffset()| is wrong regarding > > - first-letter: LayoutText contains non-first-letter part, e.g. > > <div::first-letter>foo</div> yields two LayoutTextFragment, one holds "f" and > > another holds "oo". TextNode("foo").layoutObject() = LayoutText("oo"). This is > > source of first-letter related bug > > - text-transform: text-transform:uppercase yield TextNode("ß"), eszett => > > LayoutText("SS"). This is source of unbound index in various places. > > From above, we change not to use LayoutObject here. > > On 2016/03/25 04:57:53, Yosi_UTC9 wrote: > > On 2016/03/25 at 04:34:08, tkent wrote: > > > Is it ok to remove n->layoutObject() check in the original code? If |n| is a > > Text node without LayoutObject, this CL has behavior difference. > > Yes. Using |LayoutObject::previousOffset()| is wrong regarding > > - first-letter: LayoutText contains non-first-letter part, e.g. > > <div::first-letter>foo</div> yields two LayoutTextFragment, one holds "f" and > > another holds "oo". TextNode("foo").layoutObject() = LayoutText("oo"). This is > > source of first-letter related bug > > - text-transform: text-transform:uppercase yield TextNode("ß"), eszett => > > LayoutText("SS"). This is source of unbound index in various places. > > From above, we change not to use LayoutObject here. > > Thank you for your follow-up comment. > Let me add one note for those issues. Even if this CL and http://codereview.chromium.org/1824143003 are submitted, backspace problem for first-letter/text-transform is not fixed. I'm now investigating the root cause. Oh, sorry for confusion. |return n->layoutObject() ? n->layoutObject()->previousOffset(current) : current - 1;| Original code does wrong thing when |n->layoutObject()| is |nullptr|. It can return middle of surrogate pair. This patch fixes this too.
Description was changed from ========== Remove LayoutObject::previous/nextOffset. This is preparation of introducing our own grapheme cluster boundary breaker to the Blink. To prevent regressions, added unit tests for known rules. There are few differences from original Unicode grapheme cluster boundary spec. - Break even after Prepend class character. - Break after Tamil virama sign. The cursorMovementIterator function also addresses following things but not necessary for latest Unicode spec. - Extend class is excluded from SpacingMark. - Japanese half-width katakana voiced marks has Extend property. BUG=594923 ========== to ========== Move LayoutObject::nextOffset/previousOffset() to editing utility This is preparation of introducing our own grapheme cluster boundary breaker to the Blink. To prevent regressions, added unit tests for known rules. There are few differences from original Unicode grapheme cluster boundary spec. - Break even after Prepend class character. - Break after Tamil virama sign. The cursorMovementIterator function also addresses following things but not necessary for latest Unicode spec. - Extend class is excluded from SpacingMark. - Japanese half-width katakana voiced marks has Extend property. BUG=594923 ==========
Patchset #3 (id:60001) has been deleted
Yoshi, thank you for your support. I added test cases. Could you resume the review? Thank you. https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (left): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:547: return n->layoutObject() ? n->layoutObject()->previousOffset(current) : current - 1; On 2016/03/25 05:50:04, Yosi_UTC9 wrote: > On 2016/03/25 at 05:11:22, Seigo Nonaka wrote: > > On 2016/03/25 04:57:53, Yosi_UTC9 wrote: > > > On 2016/03/25 at 04:34:08, tkent wrote: > > > > Is it ok to remove n->layoutObject() check in the original code? If |n| > is a > > > Text node without LayoutObject, this CL has behavior difference. > > > Yes. Using |LayoutObject::previousOffset()| is wrong regarding > > > - first-letter: LayoutText contains non-first-letter part, e.g. > > > <div::first-letter>foo</div> yields two LayoutTextFragment, one holds "f" > and > > > another holds "oo". TextNode("foo").layoutObject() = LayoutText("oo"). This > is > > > source of first-letter related bug > > > - text-transform: text-transform:uppercase yield TextNode("ß"), eszett => > > > LayoutText("SS"). This is source of unbound index in various places. > > > From above, we change not to use LayoutObject here. > > > > On 2016/03/25 04:57:53, Yosi_UTC9 wrote: > > > On 2016/03/25 at 04:34:08, tkent wrote: > > > > Is it ok to remove n->layoutObject() check in the original code? If |n| > is a > > > Text node without LayoutObject, this CL has behavior difference. > > > Yes. Using |LayoutObject::previousOffset()| is wrong regarding > > > - first-letter: LayoutText contains non-first-letter part, e.g. > > > <div::first-letter>foo</div> yields two LayoutTextFragment, one holds "f" > and > > > another holds "oo". TextNode("foo").layoutObject() = LayoutText("oo"). This > is > > > source of first-letter related bug > > > - text-transform: text-transform:uppercase yield TextNode("ß"), eszett => > > > LayoutText("SS"). This is source of unbound index in various places. > > > From above, we change not to use LayoutObject here. > > > > Thank you for your follow-up comment. > > Let me add one note for those issues. Even if this CL and > http://codereview.chromium.org/1824143003 are submitted, backspace problem for > first-letter/text-transform is not fixed. I'm now investigating the root cause. > > Oh, sorry for confusion. > |return n->layoutObject() ? n->layoutObject()->previousOffset(current) : > current - 1;| > Original code does wrong thing when |n->layoutObject()| is |nullptr|. It can > return middle of surrogate pair. > This patch fixes this too. > Added test cases for first-letter and text-transform with/without LayoutObject.
lgtm
lgtm Thanks a lot! https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (left): https://codereview.chromium.org/1833653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:547: return n->layoutObject() ? n->layoutObject()->previousOffset(current) : current - 1; On 2016/03/25 at 05:11:21, Seigo Nonaka wrote: > On 2016/03/25 05:05:25, tkent wrote: > > On 2016/03/25 at 04:57:53, Yosi_UTC9 wrote: > > > On 2016/03/25 at 04:34:08, tkent wrote: > > > > Is it ok to remove n->layoutObject() check in the original code? If |n| is > > a Text node without LayoutObject, this CL has behavior difference. > > > Yes. Using |LayoutObject::previousOffset()| is wrong regarding > > > - first-letter: LayoutText contains non-first-letter part, e.g. > > <div::first-letter>foo</div> yields two LayoutTextFragment, one holds "f" and > > another holds "oo". TextNode("foo").layoutObject() = LayoutText("oo"). This is > > source of first-letter related bug > > > - text-transform: text-transform:uppercase yield TextNode("ß"), eszett => > > LayoutText("SS"). This is source of unbound index in various places. > > > From above, we change not to use LayoutObject here. > > > > I asked about a case of no LayoutObject. Anyway, we need more tests: > > - |n| is Text without LayoutObject > > - correctly work even if first-letter is specified > > - correctly work even if text-transform is specified > > Sure, I'm happy to add these test case but let me give extra time. > In unit test context, looks like LayoutObject is not available. > I need extra work for unit test. You need to call |updateLayoutAndStyleForPainting()| after |setBodyContent()| to get layout objects.
The CQ bit was checked by nona@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1833653002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1833653002/80001
Message was sent while issue was closed.
Description was changed from ========== Move LayoutObject::nextOffset/previousOffset() to editing utility This is preparation of introducing our own grapheme cluster boundary breaker to the Blink. To prevent regressions, added unit tests for known rules. There are few differences from original Unicode grapheme cluster boundary spec. - Break even after Prepend class character. - Break after Tamil virama sign. The cursorMovementIterator function also addresses following things but not necessary for latest Unicode spec. - Extend class is excluded from SpacingMark. - Japanese half-width katakana voiced marks has Extend property. BUG=594923 ========== to ========== Move LayoutObject::nextOffset/previousOffset() to editing utility This is preparation of introducing our own grapheme cluster boundary breaker to the Blink. To prevent regressions, added unit tests for known rules. There are few differences from original Unicode grapheme cluster boundary spec. - Break even after Prepend class character. - Break after Tamil virama sign. The cursorMovementIterator function also addresses following things but not necessary for latest Unicode spec. - Extend class is excluded from SpacingMark. - Japanese half-width katakana voiced marks has Extend property. BUG=594923 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Move LayoutObject::nextOffset/previousOffset() to editing utility This is preparation of introducing our own grapheme cluster boundary breaker to the Blink. To prevent regressions, added unit tests for known rules. There are few differences from original Unicode grapheme cluster boundary spec. - Break even after Prepend class character. - Break after Tamil virama sign. The cursorMovementIterator function also addresses following things but not necessary for latest Unicode spec. - Extend class is excluded from SpacingMark. - Japanese half-width katakana voiced marks has Extend property. BUG=594923 ========== to ========== Move LayoutObject::nextOffset/previousOffset() to editing utility This is preparation of introducing our own grapheme cluster boundary breaker to the Blink. To prevent regressions, added unit tests for known rules. There are few differences from original Unicode grapheme cluster boundary spec. - Break even after Prepend class character. - Break after Tamil virama sign. The cursorMovementIterator function also addresses following things but not necessary for latest Unicode spec. - Extend class is excluded from SpacingMark. - Japanese half-width katakana voiced marks has Extend property. BUG=594923 Committed: https://crrev.com/5caf6fd59d6f35157a3e6ba4728434ac3d1b0886 Cr-Commit-Position: refs/heads/master@{#383265} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5caf6fd59d6f35157a3e6ba4728434ac3d1b0886 Cr-Commit-Position: refs/heads/master@{#383265} |
