Introduce BackwardGraphemeBoundaryStateMachine

Introduce BackwardGraphemeBoundaryStateMachine

BackwardGraphemeBoundaryStateMachine provides a way to obtain the preceding grapheme boundary.

BackwardGraphemeBoundaryStateMachine is compiled and tested
but not yet used in production with this CL.

BUG=594923
Committed: https://crrev.com/d619c552a3a328470be10bf9135003365c88fb03
Cr-Commit-Position: refs/heads/master@{#385423}

[Seigo Nonaka, 2016-04-03 19:22:04]

Description was changed from

==========
Introduce BackwardGraphemeBoundaryStateMachine

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

to

==========
Introduce BackwardGraphemeBoundaryStateMachine

BackwardGraphemeBoundaryStateMachine provides movement
with grapheme cluster units.

BackwardGraphemeBoundaryStateMachine is compiled and tested
but not yet used in production with this CL.

BUG=594923
==========

[Seigo Nonaka, 2016-04-03 19:22:04]

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

[Seigo Nonaka, 2016-04-03 19:26:47]

Description was changed from

==========
Introduce BackwardGraphemeBoundaryStateMachine

BackwardGraphemeBoundaryStateMachine provides movement
with grapheme cluster units.

BackwardGraphemeBoundaryStateMachine is compiled and tested
but not yet used in production with this CL.

BUG=594923
==========

to

==========
Introduce BackwardGraphemeBoundaryStateMachine

BackwardGraphemeBoundaryStateMachine provides a way to obtain the preceding
grapheme boundary.

BackwardGraphemeBoundaryStateMachine is compiled and tested
but not yet used in production with this CL.

BUG=594923
==========

[yosin_UTC9, 2016-04-04 04:11:24]

https://codereview.chromium.org/1855813002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp
(right):

https://codereview.chromium.org/1855813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:16:
SearchBoundary, // Searching grapheme boundary.
It seems integrating surrogate pair handling into |InternalState| can make code
simpler with switch-statement.

...::feedPrecedingCodeUnit(...) {
  switch (m_state) {
    case InternalState::Neutral:
      DCHECK_EQ(m_trailSurroage, 0);
      if (U16_IS_LEAD(codeUnit))
         return finishWithBrokenSurrogatePair();
      if (U16_IS_TRAIL(codeUnit)) {
         m_trailSurroage = codeUnit;
         return TextSegmentationMachineState::NeedMoreCodeUnit;
      }
      return ...;
    case InteranlState::SurrogatePairTrail:
      if (!U16_IS_LEAD(codeUnit))
         return finishWithBrokenSurrogatePair();
      ...

  }
  NOTREACHED() << "Unknown state " << m_state;
  return ...;
    
}

https://codereview.chromium.org/1855813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:53:
} else {
You don't need to have |else| here, since previous then-clause ends with
return-statement.
Once, we check |U16_IS_TRAIL()| before |U16_IS_LEAD()|, this code fragment can
be:

if (U16_IS_TRAIL(codeUnit)) {
  ... blah ..
  return TextSegmentationMachineState::NeedMoreCodeUnit;
}

if (U16_IS_TRAIL(codeUnit)) {
  codePoint = U16_GET_SUPPLEMENTARY(codeUnit, m_trailSurrogate);
  m_trailSurrogate = 0;
} else if (m_trailSurrogate != 0) {
  return finishWithBrokenSurrogatePair();
}

Or, we could write code base on current state:

if (m_trailSurrogate == 0) {
  if (U16_IS_LEAD(codeUnit))
    return finishWithBrokenSurrogatePair();
  if (U16_IS_TRAIL(codeUnit)) {
    m_trailSurroage = codeUnit;
    return TextSegmentationMachineState::NeedMoreCodeUnit;
  }
} else if (U16_IS_LEAD(codeUnit) {
  codePoint = U16_GET_SUPPLEMENTARY(codeUnit, m_trailSurrogate);
  m_trailSurrogate = 0;
} else {
  return finishWithBrokenSurrogatePair();
}

https://codereview.chromium.org/1855813002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachineTest.cpp
(right):

https://codereview.chromium.org/1855813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachineTest.cpp:700:
for (int i = 0; i < 1024; ++i) {
Why do we need to loop 1024 times? 2 times enough?

[Seigo Nonaka, 2016-04-04 10:50:53]

Patchset #2 (id:20001) has been deleted

[Seigo Nonaka, 2016-04-04 10:52:56]

Hi Yosi-san,

as we chatted locally, I'd like to introduce test helper.

Could you kindly take an another look?

Thank you.

https://codereview.chromium.org/1855813002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp
(right):

https://codereview.chromium.org/1855813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:16:
SearchBoundary, // Searching grapheme boundary.
On 2016/04/04 04:11:24, Yosi_UTC9 wrote:
> It seems integrating surrogate pair handling into |InternalState| can make
code
> simpler with switch-statement.
> 
> ...::feedPrecedingCodeUnit(...) {
>   switch (m_state) {
>     case InternalState::Neutral:
>       DCHECK_EQ(m_trailSurroage, 0);
>       if (U16_IS_LEAD(codeUnit))
>          return finishWithBrokenSurrogatePair();
>       if (U16_IS_TRAIL(codeUnit)) {
>          m_trailSurroage = codeUnit;
>          return TextSegmentationMachineState::NeedMoreCodeUnit;
>       }
>       return ...;
>     case InteranlState::SurrogatePairTrail:
>       if (!U16_IS_LEAD(codeUnit))
>          return finishWithBrokenSurrogatePair();
>       ...
> 
>   }
>   NOTREACHED() << "Unknown state " << m_state;
>   return ...;
>     
> }

Sure, I introduced *_WaitLeadSurrogate internal state.

https://codereview.chromium.org/1855813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:53:
} else {
On 2016/04/04 04:11:24, Yosi_UTC9 wrote:
> You don't need to have |else| here, since previous then-clause ends with
> return-statement.
> Once, we check |U16_IS_TRAIL()| before |U16_IS_LEAD()|, this code fragment can
> be:
> 
> if (U16_IS_TRAIL(codeUnit)) {
>   ... blah ..
>   return TextSegmentationMachineState::NeedMoreCodeUnit;
> }
> 
> if (U16_IS_TRAIL(codeUnit)) {
>   codePoint = U16_GET_SUPPLEMENTARY(codeUnit, m_trailSurrogate);
>   m_trailSurrogate = 0;
> } else if (m_trailSurrogate != 0) {
>   return finishWithBrokenSurrogatePair();
> }
> 
> Or, we could write code base on current state:
> 
> if (m_trailSurrogate == 0) {
>   if (U16_IS_LEAD(codeUnit))
>     return finishWithBrokenSurrogatePair();
>   if (U16_IS_TRAIL(codeUnit)) {
>     m_trailSurroage = codeUnit;
>     return TextSegmentationMachineState::NeedMoreCodeUnit;
>   }
> } else if (U16_IS_LEAD(codeUnit) {
>   codePoint = U16_GET_SUPPLEMENTARY(codeUnit, m_trailSurrogate);
>   m_trailSurrogate = 0;
> } else {
>   return finishWithBrokenSurrogatePair();
> }
> 
> 

Sure, done, but I removed this code. I keep in mind your comments. Thank you.

https://codereview.chromium.org/1855813002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachineTest.cpp
(right):

https://codereview.chromium.org/1855813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachineTest.cpp:700:
for (int i = 0; i < 1024; ++i) {
On 2016/04/04 04:11:24, Yosi_UTC9 wrote:
> Why do we need to loop 1024 times? 2 times enough?

I haven't thought deeply. Yes, two flags are enough.
Removed.

[yosin_UTC9, 2016-04-05 02:22:43]

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

[yosin_UTC9, 2016-04-05 02:22:44]

+kojii@ as Unicode expert. ;-)

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp
(right):

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:16:
// Initial state
Since, we have seven members, is it time to use FOR_EACH_XXX?

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:19:
Start_WaitLeadSurrogate,
No need for "_". It is violate naming convention.

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:155:
NOTREACHED();
Let's add explanation why |NOTREACHED()|, e.g.
NOTREACHED() << "You should not call state machine once it finished.";

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:157:
NOTREACHED();
Let's print |m_internalState| here for ease of debugging, e.g.
NOTREACHED() << "case exhausted, state=" << m_internalState

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:184:
NOTREACHED();
Make NOTREACHED() to have a reason.

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:186:
NOTREACHED();
Make NOTREACHED() to have a reason.

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:201:
return m_boundaryOffset;
DCHECK_LT(m_boundaryOffset, 0)?

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.cpp
(right):

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.cpp:32:
for (size_t i = 0; i < codePoints.size(); ++i) {
Use range-for, e.g.

for (const auto& codePoint : codePoints) {
 ...
}

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.cpp:52:
for (size_t i = precedingCodeUnits.size(); i != 0; --i) {
Use |.rbegin()| and |.rend()| or use |std::reverse()| with range-for.
Once, we can use base::Reversed(),  it should be a choice:
https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/ad...

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.cpp:73:
for (size_t i = 0; i < followingCodeUnits.size(); ++i) {
use range-for

[Seigo Nonaka, 2016-04-05 03:29:57]

Yosi-san, thank you for your quick review!
I addressed your comments, could you kindly take an another look?

Koji-san, could you kindly take a look?

Thank you.

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp
(right):

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:16:
// Initial state
On 2016/04/05 02:22:44, Yosi_UTC9 wrote:
> Since, we have seven members, is it time to use FOR_EACH_XXX?

Sure, Done.

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:19:
Start_WaitLeadSurrogate,
On 2016/04/05 02:22:44, Yosi_UTC9 wrote:
> No need for "_". It is violate naming convention.

Done.

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:155:
NOTREACHED();
On 2016/04/05 02:22:43, Yosi_UTC9 wrote:
> Let's add explanation why |NOTREACHED()|, e.g.
> NOTREACHED() << "You should not call state machine once it finished.";

Done.

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:157:
NOTREACHED();
On 2016/04/05 02:22:43, Yosi_UTC9 wrote:
> Let's print |m_internalState| here for ease of debugging, e.g.
> NOTREACHED() << "case exhausted, state=" << m_internalState

Done.

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:184:
NOTREACHED();
On 2016/04/05 02:22:44, Yosi_UTC9 wrote:
> Make NOTREACHED() to have a reason.

Done.

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:186:
NOTREACHED();
On 2016/04/05 02:22:44, Yosi_UTC9 wrote:
> Make NOTREACHED() to have a reason.

Done.

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/BackwardGraphemeBoundaryStateMachine.cpp:201:
return m_boundaryOffset;
On 2016/04/05 02:22:44, Yosi_UTC9 wrote:
> DCHECK_LT(m_boundaryOffset, 0)?

Done.
Used DCHECK_LE instead since m_boundaryOffset can be 0 if any code units are
provided.

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.cpp
(right):

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.cpp:32:
for (size_t i = 0; i < codePoints.size(); ++i) {
On 2016/04/05 02:22:44, Yosi_UTC9 wrote:
> Use range-for, e.g.
> 
> for (const auto& codePoint : codePoints) {
>  ...
> }

Done.

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.cpp:52:
for (size_t i = precedingCodeUnits.size(); i != 0; --i) {
On 2016/04/05 02:22:44, Yosi_UTC9 wrote:
> Use |.rbegin()| and |.rend()| or use |std::reverse()| with range-for.
> Once, we can use base::Reversed(),  it should be a choice:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/ad...

Sure, left TODO.

https://codereview.chromium.org/1855813002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.cpp:73:
for (size_t i = 0; i < followingCodeUnits.size(); ++i) {
On 2016/04/05 02:22:44, Yosi_UTC9 wrote:
> use range-for

Done.

[yosin_UTC9, 2016-04-05 05:23:59]

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

[yosin_UTC9, 2016-04-05 05:23:59]

lgtm w/ small nits

+keishi@ for core.gypi change.

https://codereview.chromium.org/1855813002/diff/70001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.cpp
(right):

https://codereview.chromium.org/1855813002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.cpp:33:
for (const auto & codePoint : codePoints) {
s/const auto &/const auto&/

https://codereview.chromium.org/1855813002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.cpp:96:
std::string out =
Use |const std::string&| to avoid necessary copy.

https://codereview.chromium.org/1855813002/diff/70001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.h
(right):

https://codereview.chromium.org/1855813002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.h:24:
std::string processSequenceBackward(
We should have |#include <string>|

[Seigo Nonaka, 2016-04-05 05:28:18]

Thank you for your quick review, yosi!

Keishi-san, could you kindly take a look?

Thank you.

https://codereview.chromium.org/1855813002/diff/70001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.cpp
(right):

https://codereview.chromium.org/1855813002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.cpp:33:
for (const auto & codePoint : codePoints) {
On 2016/04/05 05:23:59, Yosi_UTC9 wrote:
> s/const auto &/const auto&/

Done.

https://codereview.chromium.org/1855813002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.cpp:96:
std::string out =
On 2016/04/05 05:23:59, Yosi_UTC9 wrote:
> Use |const std::string&| to avoid necessary copy.

Done.

https://codereview.chromium.org/1855813002/diff/70001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.h
(right):

https://codereview.chromium.org/1855813002/diff/70001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/state_machines/StateMachineTestUtil.h:24:
std::string processSequenceBackward(
On 2016/04/05 05:23:59, Yosi_UTC9 wrote:
> We should have |#include <string>|

Done.

[keishi, 2016-04-05 05:30:38]

LGTM

[Seigo Nonaka, 2016-04-05 05:41:44]

Thank you for your quick review, Keishi-san!

[kojii, 2016-04-06 07:36:23]

lgtm

[Seigo Nonaka, 2016-04-06 07:41:21]

The CQ bit was checked by nona@chromium.org

[Seigo Nonaka, 2016-04-06 07:41:22]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org,
keishi@chromium.org
Link to the patchset: https://codereview.chromium.org/1855813002/#ps130001
(title: "Fix typo")

[commit-bot: I haz the power, 2016-04-06 07:41:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1855813002/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1855813002/130001

[commit-bot: I haz the power, 2016-04-06 09:44:09]

Description was changed from

==========
Introduce BackwardGraphemeBoundaryStateMachine

BackwardGraphemeBoundaryStateMachine provides a way to obtain the preceding
grapheme boundary.

BackwardGraphemeBoundaryStateMachine is compiled and tested
but not yet used in production with this CL.

BUG=594923
==========

to

==========
Introduce BackwardGraphemeBoundaryStateMachine

BackwardGraphemeBoundaryStateMachine provides a way to obtain the preceding
grapheme boundary.

BackwardGraphemeBoundaryStateMachine is compiled and tested
but not yet used in production with this CL.

BUG=594923
==========

[commit-bot: I haz the power, 2016-04-06 09:44:10]

Committed patchset #7 (id:130001)

[commit-bot: I haz the power, 2016-04-06 09:45:48]

Description was changed from

==========
Introduce BackwardGraphemeBoundaryStateMachine

BackwardGraphemeBoundaryStateMachine provides a way to obtain the preceding
grapheme boundary.

BackwardGraphemeBoundaryStateMachine is compiled and tested
but not yet used in production with this CL.

BUG=594923
==========

to

==========
Introduce BackwardGraphemeBoundaryStateMachine

BackwardGraphemeBoundaryStateMachine provides a way to obtain the preceding
grapheme boundary.

BackwardGraphemeBoundaryStateMachine is compiled and tested
but not yet used in production with this CL.

BUG=594923

Committed: https://crrev.com/d619c552a3a328470be10bf9135003365c88fb03
Cr-Commit-Position: refs/heads/master@{#385423}
==========

[commit-bot: I haz the power, 2016-04-06 09:45:49]

Patchset 7 (id:??) landed as
https://crrev.com/d619c552a3a328470be10bf9135003365c88fb03
Cr-Commit-Position: refs/heads/master@{#385423}