Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1844663002:
Implement backspace state machine for complex emoji sequence. (Closed)
|
Created:
4 years, 8 months ago by Seigo Nonaka Modified:
4 years, 8 months ago CC:
blink-reviews, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement backspace state machine for complex emoji sequence.
There are five types of complex emoji sequence as of Unicode 9.0
- Emoji + Variatino selector
- Emoji + Emoji modifier
- Keycap base + Keycap
- Regional indicator symbols
- ZWJ sequence
By this CL, BackspaceStateMachine can delete above emoji sequence
correctly.
BUG=594920
Committed: https://crrev.com/808548a16239bb307eae1d369595792f2d8cf277
Cr-Commit-Position: refs/heads/master@{#384511}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : rebase and addresses comments #Patch Set 3 : rebase #
Total comments: 22
Patch Set 4 : Addressing comments and formatting 80-chars. #Patch Set 5 : moved iosfwd #
Total comments: 14
Patch Set 6 : rebased to head and addresses the comments #
Total comments: 16
Patch Set 7 : Move char definitions to CharacterNames.h #
Messages
Total messages: 26 (11 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
nona@chromium.org changed reviewers: + yosin@chromium.org
Hi Yosi-san Could you kindly take a look? Thank you.
https://codereview.chromium.org/1844663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp (right): https://codereview.chromium.org/1844663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp:34: return State::Finished; // Already finished. I think attempting update state for finished state machine is error of caller. It is better to have |DCHECK_NE(State::Finished, m_state)|. Note: we need |operator<<(std::ostream, BackspaceStateMachine::State)| for |DCHECK()| https://codereview.chromium.org/1844663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp:64: m_state = BackspaceState::BeforeLF; I suggest to introduce member functions |State needMoreCodeUnit(BackspaceState)| and |State finish()| Then we could write: case BackspaceState::Start: if (codePoint == kLineFeed) return udpateState(BackspaceState::BeforeLF); if (u_hasBinaryProperty(codePoint, UCHAR_VARIATION_SELECTOR)) return needMoreCodeUnit(BackspaceState::BeforeVS); } // end of switch NOTREACHED() << "Invalid state " << m_state; } // end of function State moveNextState(BackspaceState newState) { DCHECK_NE(BackspaceState::Finished, newState); // Below |DCHECK_NE()| allows us to infinite loop in state machine. DCHECK_NE(m_state, newState); // Is this true? If not, it is better to have |keepState()| or something better name. return State::NeedMoreCodeUnit; } State finish() { DCHECK_NE(BackspaceState::Finished, m_state); m_state = BackspaceState::Finished; return State::Finished; }
Thank you for your review and I'm sorry for mixing rebased change and latest change. I'm going to split them from the next time. Please take an another look. https://codereview.chromium.org/1844663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp (right): https://codereview.chromium.org/1844663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp:34: return State::Finished; // Already finished. On 2016/03/30 04:05:57, Yosi_UTC9 wrote: > I think attempting update state for finished state machine is error of caller. > It is better to have |DCHECK_NE(State::Finished, m_state)|. > Note: we need |operator<<(std::ostream, BackspaceStateMachine::State)| for > |DCHECK()| Sure added DCHECK_NE and operator function. https://codereview.chromium.org/1844663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/BackspaceStateMachine.cpp:64: m_state = BackspaceState::BeforeLF; On 2016/03/30 04:05:57, Yosi_UTC9 wrote: > I suggest to introduce member functions |State needMoreCodeUnit(BackspaceState)| > and |State finish()| > Then we could write: > > case BackspaceState::Start: > if (codePoint == kLineFeed) > return udpateState(BackspaceState::BeforeLF); > if (u_hasBinaryProperty(codePoint, UCHAR_VARIATION_SELECTOR)) > return needMoreCodeUnit(BackspaceState::BeforeVS); > > } // end of switch > > NOTREACHED() << "Invalid state " << m_state; > } // end of function > > State moveNextState(BackspaceState newState) { > DCHECK_NE(BackspaceState::Finished, newState); > // Below |DCHECK_NE()| allows us to infinite loop in state machine. > DCHECK_NE(m_state, newState); // Is this true? If not, it is better to have > |keepState()| or something better name. > return State::NeedMoreCodeUnit; > } > > State finish() { > DCHECK_NE(BackspaceState::Finished, m_state); > m_state = BackspaceState::Finished; > return State::Finished; > } > I applied your suggestions. Thank you.
Thank you for your review and I'm sorry for mixing rebased change and latest change. I'm going to split them from the next time. Please take an another look.
https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp (right): https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:34: case BackspaceStateMachine::BackspaceState::Start: FYI: There are technique using #define-macro used in V8. #define FOR_EACH_BACKSPACE_MACHINE_STATE(V) \ V(Start) V(BeforeLF) ... enum class State { #define V(name) name, FOR_EACH_BACKSPACE_MACHINE_STATE(V) } switch (state) { #define V(name) case BackspaceStateMachine::BackspaceState::name: return os << #name; FOR_EACH_BACKSPACE_MACHINE_STATE(V) } https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:35: os << "Start"; |return os << "Start";| is better. https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:203: default: Add |// fallthroguh| or |NOTREACHED() << "You should not call feedPrecedingCodeUnit() once it finished."| https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:206: NOTREACHED() << "TextSegmentationTextSegmentationMachineState::Invalid state: " << m_state; |NOTREACHED() << "Unhandled state " << m_state;|? |m_state| can be other than |Invalid|, e.g. forget add |case| or machine is corrupted. https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:250: DCHECK_NE(BackspaceState::Finished, newState) << "Use finish() instead."; Just in case, we may want to have DCHECK_NE(BackspaceState::Invalid, newState) << "You can't update invalid state" https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:252: // Below |DCHECK_NE()| allows us to infinite loop in state machine. s/allow/prevent/ https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:260: DCHECK_NE(BackspaceState::Finished, m_state); Just in case, we may want to have DCHECK_NE(BackspaceState::Invalid, m_state) << "You can't reset invalid state." https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.h (right): https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.h:46: BeforeVSAndEmojiModifier, // Indicates the current offset is just before variation selector and emoji modifier. nit: could you format comment within 80-chars? It seems we don't need to have "Indicates" https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.h:55: friend std::ostream& operator << (std::ostream&, BackspaceState); nit: s/ << /<</ nit: It is better to have |#include <iosfwd>| https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.h:57: // Updates the internal state to the |newState| then return InternalState::NeedMoreCodeUnit. nit: could you format comment within 80-chars? https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.h:60: // Update the internal state to BackspaceState::Finished, then return MachineState::Finished. nit: could you format comment within 80-chars?
Addressed comments. Could you kindly take an another look? Thank you. https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp (right): https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:34: case BackspaceStateMachine::BackspaceState::Start: On 2016/03/30 10:00:48, Yosi_UTC9 wrote: > FYI: There are technique using #define-macro used in V8. > > #define FOR_EACH_BACKSPACE_MACHINE_STATE(V) \ > V(Start) V(BeforeLF) ... > > enum class State { > #define V(name) name, > FOR_EACH_BACKSPACE_MACHINE_STATE(V) > } > > switch (state) { > #define V(name) case BackspaceStateMachine::BackspaceState::name: return os << > #name; > FOR_EACH_BACKSPACE_MACHINE_STATE(V) > } Done. https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:35: os << "Start"; On 2016/03/30 10:00:48, Yosi_UTC9 wrote: > |return os << "Start";| is better. Done. https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:203: default: On 2016/03/30 10:00:48, Yosi_UTC9 wrote: > Add |// fallthroguh| or |NOTREACHED() << "You should not call > feedPrecedingCodeUnit() once it finished."| Done. https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:206: NOTREACHED() << "TextSegmentationTextSegmentationMachineState::Invalid state: " << m_state; On 2016/03/30 10:00:48, Yosi_UTC9 wrote: > |NOTREACHED() << "Unhandled state " << m_state;|? > > |m_state| can be other than |Invalid|, e.g. forget add |case| or machine is > corrupted. Ugh, sorry this is due to my bad editor replacement... Fixed the error message. https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:250: DCHECK_NE(BackspaceState::Finished, newState) << "Use finish() instead."; On 2016/03/30 10:00:48, Yosi_UTC9 wrote: > Just in case, we may want to have > DCHECK_NE(BackspaceState::Invalid, newState) << "You can't update invalid state" BackspaceState doesn't have invalid state. I'm sorry maybe I confused you with MachineState::Invalid. https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:252: // Below |DCHECK_NE()| allows us to infinite loop in state machine. On 2016/03/30 10:00:48, Yosi_UTC9 wrote: > s/allow/prevent/ Done. https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:260: DCHECK_NE(BackspaceState::Finished, m_state); On 2016/03/30 10:00:48, Yosi_UTC9 wrote: > Just in case, we may want to have > DCHECK_NE(BackspaceState::Invalid, m_state) << "You can't reset invalid state." Same as above. https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.h (right): https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.h:46: BeforeVSAndEmojiModifier, // Indicates the current offset is just before variation selector and emoji modifier. On 2016/03/30 10:00:48, Yosi_UTC9 wrote: > nit: could you format comment within 80-chars? > It seems we don't need to have "Indicates" Done. https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.h:55: friend std::ostream& operator << (std::ostream&, BackspaceState); On 2016/03/30 10:00:48, Yosi_UTC9 wrote: > nit: s/ << /<</ > nit: It is better to have |#include <iosfwd>| Done. https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.h:57: // Updates the internal state to the |newState| then return InternalState::NeedMoreCodeUnit. On 2016/03/30 10:00:48, Yosi_UTC9 wrote: > nit: could you format comment within 80-chars? Done. https://codereview.chromium.org/1844663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.h:60: // Update the internal state to BackspaceState::Finished, then return MachineState::Finished. On 2016/03/30 10:00:48, Yosi_UTC9 wrote: > nit: could you format comment within 80-chars? Done.
https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp (right): https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:9: #include <iosfwd> s/iosfwd/ostream/ <iosfwd> declares |std::ostream|, etc by |using|. It is usually include in ".h". In ".cpp", <ostream> brings full implementation of |std::ostream|. https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:33: #define FOR_EACH_BACKSPACE_STATE_MACHINE_STATE(V) \ Just in case, could you run "git cl format" or "clang-format -i" for this file? clang-format aligns backslash to maximum line of continuous lines. We'll see. https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:68: std::ostream& operator << (std::ostream& os, BackspaceState state) s/ << /<</ https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:80: : m_state(BackspaceState::Start) {} BackspaceStateMachine::BackspaceStateMachine() : m_state(BackspaceState::Start) { } Running "git cl format" for "clang-format -i" seems to be good idea. https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:147: m_lastSeenVSCodeUnits = 0; Should we have |DCHECK_EQ(m_lastSeenVSCodeUnit, 0)| in |finish()|? If not, we don't need to reset |m_lastSeenVSCodeUnits = 0| before calling |finish()|. https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:160: DCHECK_NE(m_lastSeenVSCodeUnits, 0); DCHECK_GT(m_lastSeenVSCodeUnits, 0) ? https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.h (right): https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.h:43: enum class BackspaceState : uint8_t m_state; Why |uint8_t|? Member variable declaration should be bottom. You can declare |enum class BackspaceState| here and having |BackspaceState m_state| at bottom.
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
Thank you for your review and I'm sorry I misunderstand some of C++ features. Could you kindly take a look again? Thank you. https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp (right): https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:9: #include <iosfwd> On 2016/03/31 01:44:45, Yosi_UTC9 wrote: > s/iosfwd/ostream/ > > <iosfwd> declares |std::ostream|, etc by |using|. It is usually include in ".h". > In ".cpp", <ostream> brings full implementation of |std::ostream|. Done. https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:33: #define FOR_EACH_BACKSPACE_STATE_MACHINE_STATE(V) \ On 2016/03/31 01:44:44, Yosi_UTC9 wrote: > Just in case, could you run "git cl format" or "clang-format -i" for this file? > clang-format aligns backslash to maximum line of continuous lines. We'll see. Oh! I didn't know such a useful tool, but unfortunately presubmit complains about the new format. I uploaded by using --bypass-hooks. Thank you for letting me know https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:68: std::ostream& operator << (std::ostream& os, BackspaceState state) On 2016/03/31 01:44:44, Yosi_UTC9 wrote: > s/ << /<</ Done. https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:80: : m_state(BackspaceState::Start) {} On 2016/03/31 01:44:44, Yosi_UTC9 wrote: > BackspaceStateMachine::BackspaceStateMachine() > : m_state(BackspaceState::Start) > { > } > > > Running "git cl format" for "clang-format -i" seems to be good idea. Done. https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:147: m_lastSeenVSCodeUnits = 0; On 2016/03/31 01:44:44, Yosi_UTC9 wrote: > Should we have |DCHECK_EQ(m_lastSeenVSCodeUnit, 0)| in |finish()|? > If not, we don't need to reset |m_lastSeenVSCodeUnits = 0| before calling > |finish()|. No need to check m_lastSeenVSCodeUnit since backspace state machine may abort with unexpected sequence. In that case, m_lastSeenVSCodeUnit can be non zero as expected. Removed m_lastSeenVSCodeUnit = 0. https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:160: DCHECK_NE(m_lastSeenVSCodeUnits, 0); On 2016/03/31 01:44:44, Yosi_UTC9 wrote: > DCHECK_GT(m_lastSeenVSCodeUnits, 0) ? Done. https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.h (right): https://codereview.chromium.org/1844663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.h:43: enum class BackspaceState : uint8_t m_state; On 2016/03/31 01:44:45, Yosi_UTC9 wrote: > Why |uint8_t|? > > Member variable declaration should be bottom. You can declare |enum class > BackspaceState| here and > having |BackspaceState m_state| at bottom. I'm sorry I misunderstand the enum spec. I thought I need to specify the size to declare here. Fixed.
yosin@chromium.org changed reviewers: + tkent@chromium.org
lgtm +tkent@ for second opinion, since I'm deeply involved implementation. Fresh look may find something. Thanks! https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp (right): https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:71: switch (state) { Let's use |static const char* const text[] = {...}| with |std::begin()| and |std::end()|. Following implementation seems to be simpler: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:80: } clang bug? This function should return |std::ostream|.
https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp (right): https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:7: #include "platform/fonts/Character.h" Using platform/fonts/ in core/editing looks weird. This include is ok as is in this CL, but we should move platform/fonts/Characters.h to platform/text/. https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:15: const uint32_t kCarriageReturn = 0x000D; Please remove this. Use carriageReturnCharacter defined in wtf/text/CharacterNames.h. https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:16: const uint32_t kLineFeed = 0x000A; Use newlineCharacter defined in wtf/text/CharacterNames.h. https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:17: const uint32_t kZWJ = 0x200D; Use zeroWidthJoinerCharacter in wtf/text/CharacterNames.h. https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:18: const uint32_t kCombiningEnclosingKeycap = 0x20E3; Use combiningEnclosingKeycapCharacter. https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:23: return codePoint == 0x2764 // HEAVY BLACK HEART Let's add these code points to wtf/text/CharacterNames.h.
Patchset #7 (id:220001) has been deleted
Thank you for your review. Please take an another look. https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp (right): https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:7: #include "platform/fonts/Character.h" On 2016/04/01 01:50:20, tkent wrote: > Using platform/fonts/ in core/editing looks weird. This include is ok as is in > this CL, but we should move platform/fonts/Characters.h to platform/text/. Sure, left TODO. https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:15: const uint32_t kCarriageReturn = 0x000D; On 2016/04/01 01:50:20, tkent wrote: > Please remove this. Use carriageReturnCharacter defined in > wtf/text/CharacterNames.h. Done. https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:16: const uint32_t kLineFeed = 0x000A; On 2016/04/01 01:50:19, tkent wrote: > Use newlineCharacter defined in wtf/text/CharacterNames.h. Done. https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:17: const uint32_t kZWJ = 0x200D; On 2016/04/01 01:50:19, tkent wrote: > Use zeroWidthJoinerCharacter in wtf/text/CharacterNames.h. Done. https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:18: const uint32_t kCombiningEnclosingKeycap = 0x20E3; On 2016/04/01 01:50:20, tkent wrote: > Use combiningEnclosingKeycapCharacter. Done. https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:23: return codePoint == 0x2764 // HEAVY BLACK HEART On 2016/04/01 01:50:20, tkent wrote: > Let's add these code points to wtf/text/CharacterNames.h. Done. https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:71: switch (state) { On 2016/04/01 01:27:03, Yosi_UTC9 wrote: > Let's use |static const char* const text[] = {...}| with |std::begin()| and > |std::end()|. > > Following implementation seems to be simpler: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Done. https://codereview.chromium.org/1844663002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/state_machines/BackspaceStateMachine.cpp:80: } On 2016/04/01 01:27:02, Yosi_UTC9 wrote: > clang bug? This function should return |std::ostream|. Done.
lgtm
The CQ bit was checked by nona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/1844663002/#ps240001 (title: "Move char definitions to CharacterNames.h")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1844663002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1844663002/240001
Message was sent while issue was closed.
Committed patchset #7 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Implement backspace state machine for complex emoji sequence. There are five types of complex emoji sequence as of Unicode 9.0 - Emoji + Variatino selector - Emoji + Emoji modifier - Keycap base + Keycap - Regional indicator symbols - ZWJ sequence By this CL, BackspaceStateMachine can delete above emoji sequence correctly. BUG=594920 ========== to ========== Implement backspace state machine for complex emoji sequence. There are five types of complex emoji sequence as of Unicode 9.0 - Emoji + Variatino selector - Emoji + Emoji modifier - Keycap base + Keycap - Regional indicator symbols - ZWJ sequence By this CL, BackspaceStateMachine can delete above emoji sequence correctly. BUG=594920 Committed: https://crrev.com/808548a16239bb307eae1d369595792f2d8cf277 Cr-Commit-Position: refs/heads/master@{#384511} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/808548a16239bb307eae1d369595792f2d8cf277 Cr-Commit-Position: refs/heads/master@{#384511} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
