[All-In-One] Introduce BackspaceStateMachine

Introduce BackspaceStateMachine

BackspaceStateMachine is for deleting complex unicode sequence by
backspace key event.  This CL only introduces surrogate pair handling.

BUG=594920
R=yosin

[Seigo Nonaka, 2016-03-23 13:47:19]

Patchset #1 (id:1) has been deleted

[Seigo Nonaka, 2016-03-23 14:03:38]

Patchset #1 (id:20001) has been deleted

[Seigo Nonaka, 2016-03-24 01:35:26]

Patchset #1 (id:40001) has been deleted

[Seigo Nonaka, 2016-03-25 02:15:55]

Patchset #1 (id:60001) has been deleted

[Seigo Nonaka, 2016-03-25 02:16:20]

Description was changed from

==========
Introduce BackspaceStateMachine

BackspaceStateMachine is for deleting complex unicode sequence by
backspace key event.  This CL only introduces surrogate pair handling.

Note that broken surrogate pairs won't arrive to BackspaceStateMachine
since
Selection::modify(AlterationExtend, DirectionBackward, CharacterGranularity)
breaks before and after the broken surrogates. As the result,
TypingCommand::deleteKeyPressed doesn't call previousPositionOf for
broken surrogates.

BUG=594920
R=yosin
==========

to

==========
Introduce BackspaceStateMachine

BackspaceStateMachine is for deleting complex unicode sequence by
backspace key event.  This CL only introduces surrogate pair handling.

BUG=594920
R=yosin
==========

[Seigo Nonaka, 2016-03-25 02:16:43]

Hi Yoshi-san,

could you kindly take a look?

Thank you.

[yosin_UTC9, 2016-03-25 04:37:22]

yosin@chromium.org changed reviewers:
+ yosin@chromium.org

[yosin_UTC9, 2016-03-25 04:37:23]

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp (right):

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp:21:
++mCodeUnitsToBeDeleted;
It seems we should reset |mTrailSurogate| too.

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp:26: bool
BackspaceStateMachine::ComputeNextState(uint16_t codeUnit)
|updateState()| or |moveToNextState()| or another?

I feel |compute| is const function and returning |bool| doesn't not seem to be
"next state".

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp:28: uint32_t cp
= codeUnit;
s/cp/codePoint/

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/BackspaceStateMachine.h (right):

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:12: class
BackspaceStateMachine {
Could you write unit test for |BackspaceStateMachine| class?
Note: we need to have |CORE_EXPORT| for class when using class in unit test.

Once we have unit test for |BackspaceStateMachine|, we can commit itself rather
than including changes of existing files.

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:13: public:
Please insert a following line before |public:|
   WTF_MAKE_NONCOPYABLE(BackspaceStateMachine);


You need to add #include "wtf/Noncopyable.h"

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:17: bool
ComputeNextState(uint16_t codeUnit);
Could you use |camelCase| for member function?
We've not moved to Chromium style yet.

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:25: int
mCodeUnitsToBeDeleted;
Blink uses |m_camelCase| for member variable.

Let's use Non-Static Class Member Initializers
(http://chromium-cpp.appspot.com/)

s/;/= 0/

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:28: uint16_t
mTrailSurrogate;
s/;/=0/

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:551: static int
uncheckedPreviousOffsetForBackwardDeletion(const Node* n, int current)
Please avoid to use one letter variable name.

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:562: if
(machine.ComputeNextState(text[i])) {
nit: no need to have braces for single line statement.

[Seigo Nonaka, 2016-03-25 06:59:06]

Thank you for your review.
I addressed your comments.
Please take another look.

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp (right):

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp:21:
++mCodeUnitsToBeDeleted;
On 2016/03/25 04:37:23, Yosi_UTC9 wrote:
> It seems we should reset |mTrailSurogate| too.

Done.

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp:26: bool
BackspaceStateMachine::ComputeNextState(uint16_t codeUnit)
On 2016/03/25 04:37:22, Yosi_UTC9 wrote:
> |updateState()| or |moveToNextState()| or another?
> 
> I feel |compute| is const function and returning |bool| doesn't not seem to be
> "next state".

Sure, updated.

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp:28: uint32_t cp
= codeUnit;
On 2016/03/25 04:37:23, Yosi_UTC9 wrote:
> s/cp/codePoint/

Done.

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/BackspaceStateMachine.h (right):

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:12: class
BackspaceStateMachine {
On 2016/03/25 04:37:23, Yosi_UTC9 wrote:
> Could you write unit test for |BackspaceStateMachine| class?
> Note: we need to have |CORE_EXPORT| for class when using class in unit test.
> 
> Once we have unit test for |BackspaceStateMachine|, we can commit itself
rather
> than including changes of existing files.

Done.

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:13: public:
On 2016/03/25 04:37:23, Yosi_UTC9 wrote:
> Please insert a following line before |public:|
>    WTF_MAKE_NONCOPYABLE(BackspaceStateMachine);
> 
> 
> You need to add #include "wtf/Noncopyable.h"

Done.

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:17: bool
ComputeNextState(uint16_t codeUnit);
On 2016/03/25 04:37:23, Yosi_UTC9 wrote:
> Could you use |camelCase| for member function?
> We've not moved to Chromium style yet.

Done.

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:25: int
mCodeUnitsToBeDeleted;
On 2016/03/25 04:37:23, Yosi_UTC9 wrote:
> Blink uses |m_camelCase| for member variable.
> 
> Let's use Non-Static Class Member Initializers
> (http://chromium-cpp.appspot.com/)
> 
> s/;/= 0/

Done.

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:28: uint16_t
mTrailSurrogate;
On 2016/03/25 04:37:23, Yosi_UTC9 wrote:
> s/;/=0/

Done.

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:551: static int
uncheckedPreviousOffsetForBackwardDeletion(const Node* n, int current)
On 2016/03/25 04:37:23, Yosi_UTC9 wrote:
> Please avoid to use one letter variable name.

Done.

https://codereview.chromium.org/1824143003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:562: if
(machine.ComputeNextState(text[i])) {
On 2016/03/25 04:37:23, Yosi_UTC9 wrote:
> nit: no need to have braces for single line statement.

Done.

[Seigo Nonaka, 2016-03-25 06:59:09]

Thank you for your review.
I addressed your comments.
Please take another look.

[yosin_UTC9, 2016-03-25 10:01:05]

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp (right):

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp:7: #include
<unicode/utf16.h>
nit: s/<unicode/utf16.h>/"third_party/icu/source/common/unicode/utf16.h"/

We'll see what PRESUBMIT says. If PRESUBMIT complains, we need to update DEPS.

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/BackspaceStateMachine.h (right):

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:15:
WTF_MAKE_NONCOPYABLE(BackspaceStateMachine);
Oops, one more, please add |STACK_ALLOCATED();|

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:31: int
m_CodeUnitsToBeDeleted = 0;
s/m_CodeUnitsToBeDeleted/m_codeUnitsToBeDeleted/

We use "camelCase" instead of "UpperCamelCase"

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:34: uint16_t
m_TrailSurrogate = 0;
s/m_TrailSurrogate/m_trailSurrogate/

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/BackspaceStateMachineTest.cpp
(right):

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/BackspaceStateMachineTest.cpp:7: #include
<gtest/gtest.h>
s/<gtest/gtest.h>/"testing/gtest/include/gtest/gtest.h"/

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/BackspaceStateMachineTest.cpp:10: 
Could you add test for no call for |udpateState()|?

e.g. Is below expected? Should we assert when calling finalizeXXX without
updateState?
BackspaceStateMachine machine;
EXPECT_EQ(0, machine.finalizeAndGetCodeUnitCountToBeDeleted());


And, test calling finalizeAndGetCodeUnitCountToBeDeleted() twice().
Or, assert for repeated finalizeXXX?

[Seigo Nonaka, 2016-03-25 10:25:18]

Patchset #4 (id:140001) has been deleted

[Seigo Nonaka, 2016-03-25 10:26:33]

Thank you for your review.
I addressed and rebased to head.

Please take an another look.

Thanks.

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp (right):

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp:7: #include
<unicode/utf16.h>
On 2016/03/25 10:01:05, Yosi_UTC9 wrote:
> nit: s/<unicode/utf16.h>/"third_party/icu/source/common/unicode/utf16.h"/
> 
> We'll see what PRESUBMIT says. If PRESUBMIT complains, we need to update DEPS.

Done and also updated DEPS.

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/BackspaceStateMachine.h (right):

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:15:
WTF_MAKE_NONCOPYABLE(BackspaceStateMachine);
On 2016/03/25 10:01:05, Yosi_UTC9 wrote:
> Oops, one more, please add |STACK_ALLOCATED();|

Thank you very much. I'm sorry I'm not familiar with Blink.

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:31: int
m_CodeUnitsToBeDeleted = 0;
On 2016/03/25 10:01:05, Yosi_UTC9 wrote:
> s/m_CodeUnitsToBeDeleted/m_codeUnitsToBeDeleted/
> 
> We use "camelCase" instead of "UpperCamelCase"

Done.

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.h:34: uint16_t
m_TrailSurrogate = 0;
On 2016/03/25 10:01:05, Yosi_UTC9 wrote:
> s/m_TrailSurrogate/m_trailSurrogate/

Done.

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/BackspaceStateMachineTest.cpp
(right):

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/BackspaceStateMachineTest.cpp:7: #include
<gtest/gtest.h>
On 2016/03/25 10:01:05, Yosi_UTC9 wrote:
> s/<gtest/gtest.h>/"testing/gtest/include/gtest/gtest.h"/

Done.

https://codereview.chromium.org/1824143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/BackspaceStateMachineTest.cpp:10: 
On 2016/03/25 10:01:05, Yosi_UTC9 wrote:
> Could you add test for no call for |udpateState()|?
> 
> e.g. Is below expected? Should we assert when calling finalizeXXX without
> updateState?
> BackspaceStateMachine machine;
> EXPECT_EQ(0, machine.finalizeAndGetCodeUnitCountToBeDeleted());
> 
> 
> And, test calling finalizeAndGetCodeUnitCountToBeDeleted() twice().
> Or, assert for repeated finalizeXXX?

Done.

[yosin_UTC9, 2016-03-28 05:05:47]

yosin@chromium.org changed reviewers:
- yosin@google.com

[yosin_UTC9, 2016-03-28 05:05:47]

Could split this patch into three and make this patch is "All-In-One" patch for
reference split patches?

1. Move LayoutObject::previousOffsetForBackwardDeletion() to
EditingUtilities.cpp
2. Introduce BackspaceMachine
3. Utilize BacksapceMachine in previousOffsetForBackwardDeletion()

1 and 2 can be review and commit in parallel.

Thanks!
-yosi

https://codereview.chromium.org/1824143003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp (right):

https://codereview.chromium.org/1824143003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp:11:
BackspaceStateMachine::BackspaceStateMachine()
nit: s/{}/= default/

https://codereview.chromium.org/1824143003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/1824143003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:561: {
Could you add DCHECK_GE(current, 0)?

https://codereview.chromium.org/1824143003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:574: return current
- machine.finalizeAndGetCodeUnitCountToBeDeleted();
Could you check |current| >= 0?

e.g.
int amount = machine.finalizeAndGetCodeUnitCountToBeDeleted();
DCHECK_GE(amount, 1);
DCHECK_GE(current, amount);
return current - amount;

[Seigo Nonaka, 2016-03-28 07:28:12]

Thank you for your review. I'm going to upload separated CLs.

https://codereview.chromium.org/1824143003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp (right):

https://codereview.chromium.org/1824143003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp:11:
BackspaceStateMachine::BackspaceStateMachine()
On 2016/03/28 05:05:47, Yosi_UTC9 wrote:
> nit: s/{}/= default/

Done.

https://codereview.chromium.org/1824143003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/1824143003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:561: {
On 2016/03/28 05:05:47, Yosi_UTC9 wrote:
> Could you add DCHECK_GE(current, 0)?

Done.

https://codereview.chromium.org/1824143003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:574: return current
- machine.finalizeAndGetCodeUnitCountToBeDeleted();
On 2016/03/28 05:05:47, Yosi_UTC9 wrote:
> Could you check |current| >= 0?
> 
> e.g.
> int amount = machine.finalizeAndGetCodeUnitCountToBeDeleted();
> DCHECK_GE(amount, 1);
> DCHECK_GE(current, amount);
> return current - amount;

Done.