Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903203002:
Reorganize the maintenance and usage of TextIterator::last_text_node_ (Closed)
|
Created:
3 years, 7 months ago by Xiaocheng Modified:
3 years, 6 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReorganize the maintenance and usage of TextIterator::last_text_node_
This patch reorganizes the maintenance and usage of the data member, so that
it is only used at non-text-node handling places, so that text node handling
code can be moved into a wrapper class.
BUG=721957
TEST=n/a; shouldn't be any behavioral change...
Review-Url: https://codereview.chromium.org/2903203002
Cr-Commit-Position: refs/heads/master@{#475242}
Committed: https://chromium.googlesource.com/chromium/src/+/d1b64ea824a10da503d0ee572442cb3119c3d2ed
Patch Set 1 #
Total comments: 5
Patch Set 2 : Thu May 25 15:30:19 PDT 2017 #Patch Set 3 : Thu May 25 15:30:19 PDT 2017 #Patch Set 4 : Thu May 25 16:01:04 PDT 2017 #Messages
Total messages: 30 (24 generated)
The CQ bit was checked by xiaochengh@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...
Description was changed from ========== Reorganize the maintenance and usage of TextIterator::last_text_node_ BUG= ========== to ========== Reorganize the maintenance and usage of TextIterator::last_text_node_ BUG=721957 ==========
Description was changed from ========== Reorganize the maintenance and usage of TextIterator::last_text_node_ BUG=721957 ========== to ========== Reorganize the maintenance and usage of TextIterator::last_text_node_ This patch reorganizes the maintenance and usage of the data member, so that it is only used at non-text-node handling places, so that text node handling code can be moved into a wrapper class. BUG=721957 ==========
Description was changed from ========== Reorganize the maintenance and usage of TextIterator::last_text_node_ This patch reorganizes the maintenance and usage of the data member, so that it is only used at non-text-node handling places, so that text node handling code can be moved into a wrapper class. BUG=721957 ========== to ========== Reorganize the maintenance and usage of TextIterator::last_text_node_ This patch reorganizes the maintenance and usage of the data member, so that it is only used at non-text-node handling places, so that text node handling code can be moved into a wrapper class. BUG=721957 TEST=n/a; shouldn't be any behavioral change... ==========
xiaochengh@chromium.org changed reviewers: + yosin@chromium.org
PTAL. https://codereview.chromium.org/2903203002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (left): https://codereview.chromium.org/2903203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:712: if (last_text_node_ == text_node_ && run_start > 0 && |last_text_node == text_node_| must be true, since |last_text_node_| was just set in HandleTextNode(). Same holds for all the revisions below. The purpose is to wrap the relevant code by a wrapper class that doesn't use last_text_node_. https://codereview.chromium.org/2903203002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2903203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:592: last_text_node_ = ToText(node_); Moved this assignment so that the remaining part can be wrapped by another function.
rs lgtm
https://codereview.chromium.org/2903203002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (left): https://codereview.chromium.org/2903203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:712: if (last_text_node_ == text_node_ && run_start > 0 && On 2017/05/25 at 05:55:25, Xiaocheng wrote: > |last_text_node == text_node_| must be true, since |last_text_node_| was just set in HandleTextNode(). Same holds for all the revisions below. > > The purpose is to wrap the relevant code by a wrapper class that doesn't use last_text_node_. Please add DCHECK_EQ(last_text_node, text_node_)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2903203002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (left): https://codereview.chromium.org/2903203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:712: if (last_text_node_ == text_node_ && run_start > 0 && On 2017/05/25 at 06:36:15, yosin_UTC9 wrote: > On 2017/05/25 at 05:55:25, Xiaocheng wrote: > > |last_text_node == text_node_| must be true, since |last_text_node_| was just set in HandleTextNode(). Same holds for all the revisions below. > > > > The purpose is to wrap the relevant code by a wrapper class that doesn't use last_text_node_. > > Please add DCHECK_EQ(last_text_node, text_node_) This part will be wrapped in to a wrapper class, so we can't use |last_text_node_| here. https://codereview.chromium.org/2903203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:896: SpliceBuffer(kSpaceCharacter, Strategy::Parent(*last_text_node_), But we can add a DCHECK here.
The CQ bit was checked by xiaochengh@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.
The CQ bit was checked by xiaochengh@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.
The CQ bit was checked by xiaochengh@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.
The CQ bit was checked by xiaochengh@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/2903203002/#ps60001 (title: "Thu May 25 16:01:04 PDT 2017")
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": 1495865383656640,
"parent_rev": "dad3f187406314cfa60dbb7d48e8295aca9fd7a0", "commit_rev":
"d1b64ea824a10da503d0ee572442cb3119c3d2ed"}
Message was sent while issue was closed.
Description was changed from ========== Reorganize the maintenance and usage of TextIterator::last_text_node_ This patch reorganizes the maintenance and usage of the data member, so that it is only used at non-text-node handling places, so that text node handling code can be moved into a wrapper class. BUG=721957 TEST=n/a; shouldn't be any behavioral change... ========== to ========== Reorganize the maintenance and usage of TextIterator::last_text_node_ This patch reorganizes the maintenance and usage of the data member, so that it is only used at non-text-node handling places, so that text node handling code can be moved into a wrapper class. BUG=721957 TEST=n/a; shouldn't be any behavioral change... Review-Url: https://codereview.chromium.org/2903203002 Cr-Commit-Position: refs/heads/master@{#475242} Committed: https://chromium.googlesource.com/chromium/src/+/d1b64ea824a10da503d0ee572442... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d1b64ea824a10da503d0ee572442... |
