Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902683005:
Ensure single entry into TextIterator::HandleTextNode for each node (Closed)
|
Created:
3 years, 7 months ago by Xiaocheng Modified:
3 years, 7 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure single entry into TextIterator::HandleTextNode for each node
This patch ensures that TextIterator::HandleTextNode is called at most
once on each text node. This allows us to maintain text node related
states in this function instead of spreading out in the entire
TextIterator, which will be done by a followup patch.
BUG=721957
TEST=n/a; no behavioral changes
Review-Url: https://codereview.chromium.org/2902683005
Cr-Commit-Position: refs/heads/master@{#475186}
Committed: https://chromium.googlesource.com/chromium/src/+/ae7cd2d737112818ef88c1167c816e171e2e4623
Patch Set 1 #Patch Set 2 : Tue May 23 14:56:35 PDT 2017 #Patch Set 3 : Tue May 23 14:56:35 PDT 2017 #Patch Set 4 : Tue May 23 22:17:17 PDT 2017 #
Total comments: 4
Patch Set 5 : Wed May 24 09:59:19 PDT 2017 #
Dependent Patchsets: Messages
Total messages: 31 (26 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 ========== Ensure single entry to TextIterator::HandleTextNode This patch ensures that TextIterator::HandleTextNode is called at most once on each text node. This allows us to maintain text node related states in this function instead of spreading out in the entire TextIterator, which will be done by a followup patch. BUG=721957 TEST=n/a; no behavioral changes ========== to ========== Ensure single entry into TextIterator::HandleTextNode for each node This patch ensures that TextIterator::HandleTextNode is called at most once on each text node. This allows us to maintain text node related states in this function instead of spreading out in the entire TextIterator, which will be done by a followup patch. BUG=721957 TEST=n/a; no behavioral changes ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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 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...
xiaochengh@chromium.org changed reviewers: + yosin@chromium.org
PTAL.
lgtm w/ nits https://codereview.chromium.org/2902683005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIterator.h (right): https://codereview.chromium.org/2902683005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:147: void HandlePreFormattedTextNode(); I think we don't need to move this function here, since we'll make all of |bool HandleXXX()| to change |void| and blame tool friendly. https://codereview.chromium.org/2902683005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:254: bool needs_handle_pre_formatted_text_node_; Let's initialize here instead of ctor.
Thanks for reviewing. I'll land this patch after M60 branch in case this patch causes any regression, since this is a non-trivial refactoring patch. https://codereview.chromium.org/2902683005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIterator.h (right): https://codereview.chromium.org/2902683005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:147: void HandlePreFormattedTextNode(); On 2017/05/24 at 08:48:39, yosin_UTC9 wrote: > I think we don't need to move this function here, > since we'll make all of |bool HandleXXX()| to change |void| > and blame tool friendly. Done. https://codereview.chromium.org/2902683005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:254: bool needs_handle_pre_formatted_text_node_; On 2017/05/24 at 08:48:39, yosin_UTC9 wrote: > Let's initialize here instead of ctor. I'll do it in followup patches. Rebasing stacked patches is troublesome...
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/2902683005/#ps80001 (title: "Wed May 24 09:59:19 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": 80001, "attempt_start_ts": 1495834887251990,
"parent_rev": "756189476715f0585dc70426f734a46be74d585e", "commit_rev":
"ae7cd2d737112818ef88c1167c816e171e2e4623"}
Message was sent while issue was closed.
Description was changed from ========== Ensure single entry into TextIterator::HandleTextNode for each node This patch ensures that TextIterator::HandleTextNode is called at most once on each text node. This allows us to maintain text node related states in this function instead of spreading out in the entire TextIterator, which will be done by a followup patch. BUG=721957 TEST=n/a; no behavioral changes ========== to ========== Ensure single entry into TextIterator::HandleTextNode for each node This patch ensures that TextIterator::HandleTextNode is called at most once on each text node. This allows us to maintain text node related states in this function instead of spreading out in the entire TextIterator, which will be done by a followup patch. BUG=721957 TEST=n/a; no behavioral changes Review-Url: https://codereview.chromium.org/2902683005 Cr-Commit-Position: refs/heads/master@{#475186} Committed: https://chromium.googlesource.com/chromium/src/+/ae7cd2d737112818ef88c1167c81... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/ae7cd2d737112818ef88c1167c81... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
