Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2921483002:
Reorganize member initialization code of TextIterator (Closed)
|
Created:
3 years, 6 months ago by Xiaocheng Modified:
3 years, 6 months ago Reviewers:
yosin_UTC9 CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReorganize member initialization code of TextIterator
This patch reorganizes the initialization of TextIterator for better code
health.
1. Members originally initialized in the function body of TextIterator::ctor
are changed to be initialized in the initialization list. Some wrapper
functions are created for members with non-trivial initialization.
- start_container_
- start_offset_
- end_container_
- end_offset_
- end_node_
- past_end_node_
- node_
- iteration_progress_
- shadow_depth_
2. Members with trivial initialization are initialized in class declaration
instead of ctor:
- needs_another_new_line_
- needs_handle_replaced_element_
- should_stop_
- handle_shadow_root_
BUG=721957
TEST=n/a; no behavioral change
Review-Url: https://codereview.chromium.org/2921483002
Cr-Commit-Position: refs/heads/master@{#476393}
Committed: https://chromium.googlesource.com/chromium/src/+/c8070f461957f5f2aaaf41147538ebe811b4abce
Patch Set 1 #
Total comments: 6
Patch Set 2 : Thu Jun 1 10:44:24 PDT 2017 #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 19 (12 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 member initialization code of TextIterator BUG= ========== to ========== Reorganize member initialization code of TextIterator This patch reorganizes the initialization of TextIterator that: 1. Members originally initialized in the function body of TextIterator::ctor are changed to be initialized in the initialization list. Some wrapper functions are created for members with non-trivial initialization. - start_container_ - start_offset_ - end_container_ - end_offset_ - end_node_ - past_end_node_ - node_ - iteration_progress_ - shadow_depth_ 2. Members with trivial initialization are initialized in class declaration instead of ctor: - needs_another_new_line_ - needs_handle_replaced_element_ - should_stop_ - handle_shadow_root_ BUG= ==========
Description was changed from ========== Reorganize member initialization code of TextIterator This patch reorganizes the initialization of TextIterator that: 1. Members originally initialized in the function body of TextIterator::ctor are changed to be initialized in the initialization list. Some wrapper functions are created for members with non-trivial initialization. - start_container_ - start_offset_ - end_container_ - end_offset_ - end_node_ - past_end_node_ - node_ - iteration_progress_ - shadow_depth_ 2. Members with trivial initialization are initialized in class declaration instead of ctor: - needs_another_new_line_ - needs_handle_replaced_element_ - should_stop_ - handle_shadow_root_ BUG= ========== to ========== Reorganize member initialization code of TextIterator This patch reorganizes the initialization of TextIterator for better code health. 1. Members originally initialized in the function body of TextIterator::ctor are changed to be initialized in the initialization list. Some wrapper functions are created for members with non-trivial initialization. - start_container_ - start_offset_ - end_container_ - end_offset_ - end_node_ - past_end_node_ - node_ - iteration_progress_ - shadow_depth_ 2. Members with trivial initialization are initialized in class declaration instead of ctor: - needs_another_new_line_ - needs_handle_replaced_element_ - should_stop_ - handle_shadow_root_ BUG= ==========
Description was changed from ========== Reorganize member initialization code of TextIterator This patch reorganizes the initialization of TextIterator for better code health. 1. Members originally initialized in the function body of TextIterator::ctor are changed to be initialized in the initialization list. Some wrapper functions are created for members with non-trivial initialization. - start_container_ - start_offset_ - end_container_ - end_offset_ - end_node_ - past_end_node_ - node_ - iteration_progress_ - shadow_depth_ 2. Members with trivial initialization are initialized in class declaration instead of ctor: - needs_another_new_line_ - needs_handle_replaced_element_ - should_stop_ - handle_shadow_root_ BUG= ========== to ========== Reorganize member initialization code of TextIterator This patch reorganizes the initialization of TextIterator for better code health. 1. Members originally initialized in the function body of TextIterator::ctor are changed to be initialized in the initialization list. Some wrapper functions are created for members with non-trivial initialization. - start_container_ - start_offset_ - end_container_ - end_offset_ - end_node_ - past_end_node_ - node_ - iteration_progress_ - shadow_depth_ 2. Members with trivial initialization are initialized in class declaration instead of ctor: - needs_another_new_line_ - needs_handle_replaced_element_ - should_stop_ - handle_shadow_root_ BUG=721957 TEST=n/a; no behavioral change ==========
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.
lgtm w/ nits https://codereview.chromium.org/2921483002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2921483002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:89: Node* StartNode(Node* start_container, int start_offset) { FYI: I used to try to use |Position::NodeAsFirstNode()| and |Position::NodeAsRangePastLastNode()|. Some layout tests failed. But, it is worth to try again. https://codereview.chromium.org/2921483002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:90: DCHECK(start_container); nit: Since we have |start_container->XXX| in next line, we don't need to have |DCHECK(start_container)| https://codereview.chromium.org/2921483002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:102: DCHECK(&end_container); nit: Since we have |end_container->XXX| in next line, we don't need to have |DCHECK(end_container)| Note: null-reference is undefined behavior in C++. In some compiler, |DCHECK(&end_container)| always false.
BTW, another idea of using Position instead of {start,end}_{container, offset}.
When we change passed Position to OffsetInAnchory type, it is as same as
container/offset pair.
As you know ContainerNode::ChildAt(index) is O(n). It is better to use to Next
position and
using BeforeAnchor position.
In this way, we expect following steps:
1. Replace {start,end}_{container,offset} by Position OffsetInAnchor, no logic
change
2. Use BeforeAnchor with Next function used in EphemeralRange::iterator
On 2017/06/01 at 02:30:08, yosin wrote:
> BTW, another idea of using Position instead of {start,end}_{container,
offset}.
> When we change passed Position to OffsetInAnchory type, it is as same as
container/offset pair.
> As you know ContainerNode::ChildAt(index) is O(n). It is better to use to Next
position and
> using BeforeAnchor position.
>
> In this way, we expect following steps:
>
> 1. Replace {start,end}_{container,offset} by Position OffsetInAnchor, no logic
change
> 2. Use BeforeAnchor with Next function used in EphemeralRange::iterator
I'll try in follow-up patches.
The CQ bit was checked by xiaochengh@chromium.org
Thanks for the ideas! https://codereview.chromium.org/2921483002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2921483002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:89: Node* StartNode(Node* start_container, int start_offset) { On 2017/06/01 at 01:43:33, yosin_UTC9 wrote: > FYI: I used to try to use |Position::NodeAsFirstNode()| and |Position::NodeAsRangePastLastNode()|. > Some layout tests failed. But, it is worth to try again. I'll try, but not in this patch. https://codereview.chromium.org/2921483002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:90: DCHECK(start_container); On 2017/06/01 at 01:43:33, yosin_UTC9 wrote: > nit: Since we have |start_container->XXX| in next line, we don't need to have |DCHECK(start_container)| Done. https://codereview.chromium.org/2921483002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:102: DCHECK(&end_container); On 2017/06/01 at 01:43:33, yosin_UTC9 wrote: > nit: Since we have |end_container->XXX| in next line, we don't need to have |DCHECK(end_container)| > > Note: null-reference is undefined behavior in C++. In some compiler, |DCHECK(&end_container)| always false. Done.
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/2921483002/#ps20001 (title: "Thu Jun 1 10:44: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": 20001, "attempt_start_ts": 1496339121572380,
"parent_rev": "9bf01e4dcb9fce62221d4e3ac0c959f2f5f26f84", "commit_rev":
"c8070f461957f5f2aaaf41147538ebe811b4abce"}
Message was sent while issue was closed.
Description was changed from ========== Reorganize member initialization code of TextIterator This patch reorganizes the initialization of TextIterator for better code health. 1. Members originally initialized in the function body of TextIterator::ctor are changed to be initialized in the initialization list. Some wrapper functions are created for members with non-trivial initialization. - start_container_ - start_offset_ - end_container_ - end_offset_ - end_node_ - past_end_node_ - node_ - iteration_progress_ - shadow_depth_ 2. Members with trivial initialization are initialized in class declaration instead of ctor: - needs_another_new_line_ - needs_handle_replaced_element_ - should_stop_ - handle_shadow_root_ BUG=721957 TEST=n/a; no behavioral change ========== to ========== Reorganize member initialization code of TextIterator This patch reorganizes the initialization of TextIterator for better code health. 1. Members originally initialized in the function body of TextIterator::ctor are changed to be initialized in the initialization list. Some wrapper functions are created for members with non-trivial initialization. - start_container_ - start_offset_ - end_container_ - end_offset_ - end_node_ - past_end_node_ - node_ - iteration_progress_ - shadow_depth_ 2. Members with trivial initialization are initialized in class declaration instead of ctor: - needs_another_new_line_ - needs_handle_replaced_element_ - should_stop_ - handle_shadow_root_ BUG=721957 TEST=n/a; no behavioral change Review-Url: https://codereview.chromium.org/2921483002 Cr-Commit-Position: refs/heads/master@{#476393} Committed: https://chromium.googlesource.com/chromium/src/+/c8070f461957f5f2aaaf41147538... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/c8070f461957f5f2aaaf41147538... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
