Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2901713002:
Add TextIterator::HandlePreFormattedTextNode to wrap relevant logic (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. |
DescriptionAdd TextIterator::HandlePreFormattedTextNode to wrap relevant logic
This patch wraps code in TextIterator for extracting text from pre-formatted
text node into a new function to improve code health and prepare for follow-up
refactoring.
BUG=721957
TEST=n/a; no behavioral changes
Review-Url: https://codereview.chromium.org/2901713002
Cr-Commit-Position: refs/heads/master@{#474205}
Committed: https://chromium.googlesource.com/chromium/src/+/c49c1c508631745d9be1bdeea115c63038f8cdf0
Patch Set 1 #
Total comments: 14
Patch Set 2 : Fix nits #Patch Set 3 : Tue May 23 14:25:01 PDT 2017 #
Total comments: 4
Patch Set 4 : Tue May 23 22:08:24 PDT 2017 #
Dependent Patchsets: Messages
Total messages: 26 (19 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 ========== Add TextIterator::HandlePreFormattedTextNode to wrap relevant logic BUG=721957 ========== to ========== Add TextIterator::HandlePreFormattedTextNode to wrap relevant logic This patch wraps code in TextIterator for extracting text from pre-formatted text node into a new function to improve code health and prepare for follow-up refactoring. BUG=721957 TEST=n/a; no behavioral changes ==========
xiaochengh@chromium.org changed reviewers: + yosin@chromium.org
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:519: bool TextIteratorAlgorithm<Strategy>::HandlePreFormattedTextNode() { Could you add a comment about |bool| return value? It seems all |HandleXXX()| return |bool| return value. It is OK to one comment to explain meaning of |bool| return value in class declaration. https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:521: Text* text_node = ToText(node_); nit: s/Text*/Text* const/ Can we use |const Text*|? If so, it should be |const Text* const| https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:522: LayoutText* layout_object = text_node->GetLayoutObject(); nit: s/LayoutText*/LayoutText* const/ https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:523: String str = layout_object->GetText(); nit: s/String/const String&/ https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:532: } else { How about switching then and else clause to decrease indentation with utilizing early-return style? https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:540: String first_letter = first_letter_text_->GetText(); nit: s/String/const String&/ https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:554: NOTREACHED(); We should have |return false;| after |NOTREACHED()| to help compiler optimization.
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Updated. PTAL. https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:519: bool TextIteratorAlgorithm<Strategy>::HandlePreFormattedTextNode() { On 2017/05/23 at 09:27:45, yosin_UTC9 wrote: > Could you add a comment about |bool| return value? > > It seems all |HandleXXX()| return |bool| return value. > It is OK to one comment to explain meaning of |bool| return value in > class declaration. Done. In fact, their return values are not very meaningful and are not always used. And in the next patch I'll remove the return value of HandleTextNode and HandlePreFormattedTextNode... I guess, when they were first designed, the principles were: 1. Non-empty text will be emitted as long as a |HandleXXX()| is called 2. The return value indicates whether the handling of the node is complete As a result, whenever we call Advance(), we keep calling HandleXXX on the same node to emit text progressively from the node, until HandleXXX returns true. After that, the next time we call Advance(), we will move on to the next element. However, the two principles are violated at a lot of places: - In some cases, HandleXXX returns false without emitting any text. As a result, we move on to the next node directly without using the return value of HandleXXX - HandleTextNode can return true without fully handling the node I may completely remove the return values of all HandleXXXNode in the future. That won't be earlier than finishing crbug.com/721957, though. https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:521: Text* text_node = ToText(node_); On 2017/05/23 at 09:27:45, yosin_UTC9 wrote: > nit: s/Text*/Text* const/ Done. > Can we use |const Text*|? If so, it should be |const Text* const| Can't do that now because it's passed to EmitText(). https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:522: LayoutText* layout_object = text_node->GetLayoutObject(); On 2017/05/23 at 09:27:45, yosin_UTC9 wrote: > nit: s/LayoutText*/LayoutText* const/ Done. https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:523: String str = layout_object->GetText(); On 2017/05/23 at 09:27:45, yosin_UTC9 wrote: > nit: s/String/const String&/ Done. https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:532: } else { On 2017/05/23 at 09:27:45, yosin_UTC9 wrote: > How about switching then and else clause to decrease indentation > with utilizing early-return style? Done. https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:540: String first_letter = first_letter_text_->GetText(); On 2017/05/23 at 09:27:45, yosin_UTC9 wrote: > nit: s/String/const String&/ Done. https://codereview.chromium.org/2901713002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:554: NOTREACHED(); On 2017/05/23 at 09:27:45, yosin_UTC9 wrote: > We should have |return false;| after |NOTREACHED()| to help compiler optimization. Done.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ nits https://codereview.chromium.org/2901713002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2901713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:557: const unsigned run_start = offset_ - layout_object->TextStartOffset(); Could you add |DCHECK_GE(offset_, layout_object->TextStartOffset())|? https://codereview.chromium.org/2901713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:561: : INT_MAX; nit: s/INT_MAX/str_length/ Because |end| is used only for |std::min(str_length, end)|. It is unclear why we use |INT_MAX| == static_cast<int>(1 << 31 - 1) and assign |int| to |unsigned int|.
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 xiaochengh@chromium.org
The CQ bit was checked by xiaochengh@chromium.org
Thanks for reviewing. https://codereview.chromium.org/2901713002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2901713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:557: const unsigned run_start = offset_ - layout_object->TextStartOffset(); On 2017/05/24 at 04:41:58, yosin_UTC9 wrote: > Could you add |DCHECK_GE(offset_, layout_object->TextStartOffset())|? Done. https://codereview.chromium.org/2901713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:561: : INT_MAX; On 2017/05/24 at 04:41:58, yosin_UTC9 wrote: > nit: s/INT_MAX/str_length/ > Because |end| is used only for |std::min(str_length, end)|. > It is unclear why we use |INT_MAX| == static_cast<int>(1 << 31 - 1) and > assign |int| to |unsigned int|. Done. There's too much sloppy code in TextIterator and it's tedious to fix all of them. I'd rather put them into a wrapper class first.
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/2901713002/#ps60001 (title: "Tue May 23 22:08:24 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": 1495602639511800,
"parent_rev": "0086814cbe7eb55cd96ff214489e2e4867422cb5", "commit_rev":
"c49c1c508631745d9be1bdeea115c63038f8cdf0"}
Message was sent while issue was closed.
Description was changed from ========== Add TextIterator::HandlePreFormattedTextNode to wrap relevant logic This patch wraps code in TextIterator for extracting text from pre-formatted text node into a new function to improve code health and prepare for follow-up refactoring. BUG=721957 TEST=n/a; no behavioral changes ========== to ========== Add TextIterator::HandlePreFormattedTextNode to wrap relevant logic This patch wraps code in TextIterator for extracting text from pre-formatted text node into a new function to improve code health and prepare for follow-up refactoring. BUG=721957 TEST=n/a; no behavioral changes Review-Url: https://codereview.chromium.org/2901713002 Cr-Commit-Position: refs/heads/master@{#474205} Committed: https://chromium.googlesource.com/chromium/src/+/c49c1c508631745d9be1bdeea115... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c49c1c508631745d9be1bdeea115... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
