|
|
Created:
3 years, 6 months ago by yoichio Modified:
3 years, 6 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce range-based for loop in LayoutSelection.
LayoutSelection traverses Layout tree in the way here and there.
This CL introduces LayoutSelection::Iterator which lists
only 'valid' LayoutObjects( see SelectionPaintRange::Iterator::operator++);
BUG=708453
Review-Url: https://codereview.chromium.org/2921863002
Cr-Commit-Position: refs/heads/master@{#478562}
Committed: https://chromium.googlesource.com/chromium/src/+/0a7a89f4a4f300968dd600ad428a45b01541e3e0
Patch Set 1 #
Total comments: 10
Patch Set 2 : update #
Total comments: 2
Messages
Total messages: 20 (12 generated)
The CQ bit was checked by yoichio@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 ========== Introduce range-based for loop in LayoutSelection. BUG=708453 ========== to ========== Introduce range-based for loop in LayoutSelection. LayoutSelection traverses Layout tree in the way here and there. This CL introduces LayoutSelection::Iterator which lists only 'valid' LayoutObjects( see SelectionPaintRange::Iterator::operator++); BUG=708453 ==========
yoichio@chromium.org changed reviewers: + yosin@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2921863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2921863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:85: if (!stop_) nit: prefer early-retrun if (stop_) return; stop_ = range->EndLayoutObject()->NextInPreOrderAfterChildren(); https://codereview.chromium.org/2921863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:91: current_ = current_->NextInPreOrder(); Let's use for-statement for(current_ = current_->NextInPreOrder(); current_ && current_ != stop; current_ = current_->NextInPreOrder()) { if (current_ == included_end_ || current_->CanBeSelectionLeaf()) return *this; } https://codereview.chromium.org/2921863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:418: const LayoutBlock* cb = runner->ContainingBlock(); Let't use for-statement for (const LayoutBlock* cb = runner->ContainingBlock(); cb && !cb->IsLayoutView(); cb = cb->ContainingBlock()) { sel_rect.Unite(SelectionRectForLayoutObject(cb)); VisitedContainingBlockSet::AddResult add_result = visited_containing_blocks.insert(cb); if (!add_result.is_new_entry) break; } Could you use descriptive name instead of |cb|? Blink doesn't use abbreviation. https://codereview.chromium.org/2921863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/LayoutSelection.h (right): https://codereview.chromium.org/2921863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.h:56: return current_ != other.current_; |return !operator==(other)| is better. https://codereview.chromium.org/2921863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.h:59: LayoutObject* operator*() const { return current_; } We should have DCHECK(current_);
The CQ bit was checked by yoichio@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/2921863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2921863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:85: if (!stop_) On 2017/06/02 08:35:44, yosin_UTC9 wrote: > nit: prefer early-retrun > > if (stop_) > return; > stop_ = range->EndLayoutObject()->NextInPreOrderAfterChildren(); Done. https://codereview.chromium.org/2921863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:91: current_ = current_->NextInPreOrder(); On 2017/06/02 08:35:44, yosin_UTC9 wrote: > Let's use for-statement > > for(current_ = current_->NextInPreOrder(); > current_ && current_ != stop; > current_ = current_->NextInPreOrder()) { > if (current_ == included_end_ || current_->CanBeSelectionLeaf()) > return *this; > } Done. https://codereview.chromium.org/2921863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:418: const LayoutBlock* cb = runner->ContainingBlock(); I will use CollectSelectedMap and remove these lines. https://codereview.chromium.org/2921863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/LayoutSelection.h (right): https://codereview.chromium.org/2921863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.h:56: return current_ != other.current_; On 2017/06/02 08:35:44, yosin_UTC9 wrote: > |return !operator==(other)| is better. Done. https://codereview.chromium.org/2921863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.h:59: LayoutObject* operator*() const { return current_; } On 2017/06/02 08:35:44, yosin_UTC9 wrote: > We should have DCHECK(current_); Done.
https://codereview.chromium.org/2921863002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2921863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:437: while (cb && !cb->IsLayoutView()) { Let's use for-statement. s/cb/block/ or s/cb/block/ for (const LayoutBlock* block = runner->ContainingBlock(); block && !block->IsLayoutView(); block = block->ContainingBlock()) { ... }
https://codereview.chromium.org/2921863002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2921863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:437: while (cb && !cb->IsLayoutView()) { I will use CollectSelectedMap and remove these lines.
The CQ bit was unchecked by yosin@chromium.org
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": 20001, "attempt_start_ts": 1497250535566660, "parent_rev": "3c433e7dca949e4df3ce288475d2cc5a6d134787", "commit_rev": "0a7a89f4a4f300968dd600ad428a45b01541e3e0"}
Message was sent while issue was closed.
Description was changed from ========== Introduce range-based for loop in LayoutSelection. LayoutSelection traverses Layout tree in the way here and there. This CL introduces LayoutSelection::Iterator which lists only 'valid' LayoutObjects( see SelectionPaintRange::Iterator::operator++); BUG=708453 ========== to ========== Introduce range-based for loop in LayoutSelection. LayoutSelection traverses Layout tree in the way here and there. This CL introduces LayoutSelection::Iterator which lists only 'valid' LayoutObjects( see SelectionPaintRange::Iterator::operator++); BUG=708453 Review-Url: https://codereview.chromium.org/2921863002 Cr-Commit-Position: refs/heads/master@{#478562} Committed: https://chromium.googlesource.com/chromium/src/+/0a7a89f4a4f300968dd600ad428a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/0a7a89f4a4f300968dd600ad428a... |