Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2706033007:
Add SetCharacterDataCommand (Closed)
DescriptionAdd SetCharacterDataCommand
Spun off from https://codereview.chromium.org/2692093003/ with the changes
requested there made.
One weird thing I noticed: doUnapply() is declared as public in
SimpleEditCommand, but all the subclasses I've looked at declare the subclass
implementation as private. I've declared my implementation in
SetCharacterDataCommand as public so we can call doUnapply() from the test case.
Review-Url: https://codereview.chromium.org/2706033007
Cr-Commit-Position: refs/heads/master@{#452766}
Committed: https://chromium.googlesource.com/chromium/src/+/b2b632ce4c039452f4c3158131c50e92381b586d
Patch Set 1 #
Total comments: 3
Patch Set 2 : Make doUnapply() private #
Total comments: 1
Patch Set 3 : Fix DCHECK #
Total comments: 3
Patch Set 4 : Remove updateStyleAndLayout(), add DCHECK, add more tests #
Dependent Patchsets: Messages
Total messages: 39 (23 generated)
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
rlanday@chromium.org changed reviewers: + yosin@chromium.org
rlanday@chromium.org changed required reviewers: + yosin@chromium.org
Spun off from https://codereview.chromium.org/2692093003/ as requested
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xiaochengh@chromium.org changed reviewers: + xiaochengh@chromium.org
https://codereview.chromium.org/2706033007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp (right): https://codereview.chromium.org/2706033007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:53: document().updateStyleAndLayout(); This call of Document::updateSAL is not good... It shouldn't be here, as there's no clear client requiring a clean layout. It also worsens the performance of undo and redo, which do not require clean layout. Though this patch is just following the current implementation of InsertInto/DeleteFromTextNodeCommand, we should improve those two commands instead of copy existing bad approaches. I'll make a patch to clean them up. https://codereview.chromium.org/2706033007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.h (right): https://codereview.chromium.org/2706033007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.h:22: void doUnapply() final; Hiding doUnapply() as private ensures that only virtual calls via SimpleEditCommand are allowed. In the unit test, the command should be stored as a |SimpleEditCommand*| so that doUnapply() can be called.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2706033007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp (right): https://codereview.chromium.org/2706033007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:53: document().updateStyleAndLayout(); On 2017/02/21 at 23:37:32, Xiaocheng wrote: > This call of Document::updateSAL is not good... It shouldn't be here, as there's no clear client requiring a clean layout. It also worsens the performance of undo and redo, which do not require clean layout. > > Though this patch is just following the current implementation of InsertInto/DeleteFromTextNodeCommand, we should improve those two commands instead of copy existing bad approaches. > > I'll make a patch to clean them up. So should I remove this? Or is the fix more involved than that?
On 2017/02/22 at 01:54:38, rlanday wrote: > https://codereview.chromium.org/2706033007/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp (right): > > https://codereview.chromium.org/2706033007/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:53: document().updateStyleAndLayout(); > On 2017/02/21 at 23:37:32, Xiaocheng wrote: > > This call of Document::updateSAL is not good... It shouldn't be here, as there's no clear client requiring a clean layout. It also worsens the performance of undo and redo, which do not require clean layout. > > > > Though this patch is just following the current implementation of InsertInto/DeleteFromTextNodeCommand, we should improve those two commands instead of copy existing bad approaches. > > > > I'll make a patch to clean them up. > > So should I remove this? Or is the fix more involved than that? I'm trying to remove them with https://codereview.chromium.org/2705373002. It seems more involved than a simple removal. Please wait until that patch is landed. After that, you can remove the layout update call.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2706033007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp (right): https://codereview.chromium.org/2706033007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:23: DCHECK_LT(m_offset, m_node->length()); @yosin, you said this should be DCHECK_LT: https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... I think it should actually be DCHECK_LE. For example, if we have a zero-length text node, you can insert at offset 0.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/22 at 02:58:23, rlanday wrote: > https://codereview.chromium.org/2706033007/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp (right): > > https://codereview.chromium.org/2706033007/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:23: DCHECK_LT(m_offset, m_node->length()); > @yosin, you said this should be DCHECK_LT: > https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... > > I think it should actually be DCHECK_LE. For example, if we have a zero-length text node, you can insert at offset 0. Do we allow to m_offset = m_node->length() && m_count == 0? To call this command with m_count == 0 is useless and callers should care. DCHECK_LE(m_offset, m_node->length()); DCHECK_LE(m_offset + m_count, m_node->length());
On 2017/02/22 at 03:15:33, yosin wrote: > On 2017/02/22 at 02:58:23, rlanday wrote: > > https://codereview.chromium.org/2706033007/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp (right): > > > > https://codereview.chromium.org/2706033007/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:23: DCHECK_LT(m_offset, m_node->length()); > > @yosin, you said this should be DCHECK_LT: > > https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... > > > > I think it should actually be DCHECK_LE. For example, if we have a zero-length text node, you can insert at offset 0. > > Do we allow to m_offset = m_node->length() && m_count == 0? To call this command with m_count == 0 is useless and callers should care. > DCHECK_LE(m_offset, m_node->length()); > DCHECK_LE(m_offset + m_count, m_node->length()); We can add a DCHECK for that particular case if you want, but I think the case where m_node->length() == 0 && m_offset == 0 && m_count != 0 (inserting a non-zero number of characters into an empty string) and the case where m_node->length() != 0 && m_offset == m_node->length() (inserting characters at the end of a non-empty string) are perfectly valid.
On 2017/02/22 at 04:38:23, rlanday wrote: > On 2017/02/22 at 03:15:33, yosin wrote: > > On 2017/02/22 at 02:58:23, rlanday wrote: > > > https://codereview.chromium.org/2706033007/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp (right): > > > > > > https://codereview.chromium.org/2706033007/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:23: DCHECK_LT(m_offset, m_node->length()); > > > @yosin, you said this should be DCHECK_LT: > > > https://codereview.chromium.org/2692093003/diff/80001/third_party/WebKit/Sour... > > > > > > I think it should actually be DCHECK_LE. For example, if we have a zero-length text node, you can insert at offset 0. > > > > Do we allow to m_offset = m_node->length() && m_count == 0? To call this command with m_count == 0 is useless and callers should care. > > DCHECK_LE(m_offset, m_node->length()); > > DCHECK_LE(m_offset + m_count, m_node->length()); > > We can add a DCHECK for that particular case if you want, but I think the case where m_node->length() == 0 && m_offset == 0 && m_count != 0 (inserting a non-zero number of characters into an empty string) and the case where m_node->length() != 0 && m_offset == m_node->length() (inserting characters at the end of a non-empty string) are perfectly valid. I see. I'm usually use start and end. offset, count/length makes me confusing.
https://codereview.chromium.org/2706033007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp (right): https://codereview.chromium.org/2706033007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:46: layoutText->momentarilyRevealLastTypedCharacter(m_offset + Wow, we need to find out alternative way in layout NG. https://codereview.chromium.org/2706033007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommand.cpp:53: document().updateStyleAndLayout(); We need to wait xiaochengh@'s patch. https://codereview.chromium.org/2706033007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommandTest.cpp (right): https://codereview.chromium.org/2706033007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/SetCharacterDataCommandTest.cpp:27: } Could you add edge cases? - node.length=0 offset=count=0 - node.length>0 offset=node.length, count=0 - node.length>0 offset=0, count=node.length Cases: count == newText.length count < newText.length count > newText.length
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@yosin, I've made the requested changes
lgtm My patch is already landed, so you can proceed now
The CQ bit was checked by rlanday@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by yosin@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487909365336280,
"parent_rev": "f667030fb4c6bb84a55332a5335d451e541d2c02", "commit_rev":
"b2b632ce4c039452f4c3158131c50e92381b586d"}
Message was sent while issue was closed.
Description was changed from ========== Add SetCharacterDataCommand Spun off from https://codereview.chromium.org/2692093003/ with the changes requested there made. One weird thing I noticed: doUnapply() is declared as public in SimpleEditCommand, but all the subclasses I've looked at declare the subclass implementation as private. I've declared my implementation in SetCharacterDataCommand as public so we can call doUnapply() from the test case. ========== to ========== Add SetCharacterDataCommand Spun off from https://codereview.chromium.org/2692093003/ with the changes requested there made. One weird thing I noticed: doUnapply() is declared as public in SimpleEditCommand, but all the subclasses I've looked at declare the subclass implementation as private. I've declared my implementation in SetCharacterDataCommand as public so we can call doUnapply() from the test case. Review-Url: https://codereview.chromium.org/2706033007 Cr-Commit-Position: refs/heads/master@{#452766} Committed: https://chromium.googlesource.com/chromium/src/+/b2b632ce4c039452f4c3158131c5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b2b632ce4c039452f4c3158131c5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
