Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2901263002:
Introduce SelectionPaintRange in LayoutSelection (Closed)
|
Created:
3 years, 7 months ago by yoichio Modified:
3 years, 7 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce SelectionPaintRange in LayoutSelection
SelectionPaintRange has members of |selection_start_|,|selection_end_,
|selection_start_pos_| and |selection_end_pos_| which LayoutSelection
had directly.
BUG=708453
TEST=No change in behavior
Review-Url: https://codereview.chromium.org/2901263002
Cr-Commit-Position: refs/heads/master@{#474909}
Committed: https://chromium.googlesource.com/chromium/src/+/e7eadd47df235c4a2b583e12e56279022c9bc944
Patch Set 1 #
Total comments: 15
Patch Set 2 : Make SelectionPaintRange immutable #
Total comments: 4
Patch Set 3 : Introduce IsNull() #
Total comments: 12
Patch Set 4 : Add null check #
Messages
Total messages: 40 (26 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 SelectionPaintRange in LayoutSelection BUG=708453 ========== to ========== Introduce SelectionPaintRange in LayoutSelection SelectionPaintRange has members of |selection_start_|,|selection_end_, |selection_start_pos_| and |selection_end_pos_| which LayoutSelection had directly. BUG=708453 TEST=No change in behavior ==========
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/2901263002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:181: DCHECK(new_range.start_layout_object_); Once ctor has DCHECK's for members, we don't need to check here. https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:214: (new_range.start_layout_object_ == obj && Can we use SelectionPaintRange::operator!=()? e.g. new_range != paint_range_ https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/LayoutSelection.h (right): https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.h:33: struct SelectionPaintRange { I think it is better to use |class| and make this object as *virtual* immutable. Once we constructor this object, we don't need to change members. https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.h:48: LayoutObject* start_layout_object_; nit: Member variable of |struct| should not end with "_". https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.h:54: : start_layout_object_(nullptr), nit: Let's use inline initialization, e.g. LayoutObject* start_layout_object_ = nullptr; ... Then we can use |= default;| https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.h:65: end_offset_(end_offset){}; nit: s/{}/ {}/ We should have DCHECK for parameters. DCHECK(start_layout_object); DCHECK(end_layout_object); DCHECK_GE(start_offset, 0); DCHECK_GE(end_offset, 0); if (start_layout_object == end_layout_object) DCHECK_LT(start_offset, end_offset); This ctor should be ".cpp" file. https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.h:67: bool operator==(const SelectionPaintRange& other) { nit: s/{/ const {/ This function should be ".cpp" file.
PTAL https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:181: DCHECK(new_range.start_layout_object_); On 2017/05/24 08:38:46, yosin_UTC9 wrote: > Once ctor has DCHECK's for members, we don't need to check here. How about null SelectionPaintRange? https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:214: (new_range.start_layout_object_ == obj && On 2017/05/24 08:38:46, yosin_UTC9 wrote: > Can we use SelectionPaintRange::operator!=()? > e.g. new_range != paint_range_ This is not SelectionPaintRange compare. https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/LayoutSelection.h (right): https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.h:33: struct SelectionPaintRange { On 2017/05/24 08:38:46, yosin_UTC9 wrote: > I think it is better to use |class| and make this object as *virtual* immutable. > Once we constructor this object, we don't need to change members. Done. https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.h:48: LayoutObject* start_layout_object_; On 2017/05/24 08:38:46, yosin_UTC9 wrote: > nit: Member variable of |struct| should not end with "_". Acknowledged. https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.h:54: : start_layout_object_(nullptr), On 2017/05/24 08:38:46, yosin_UTC9 wrote: > nit: Let's use inline initialization, e.g. > > LayoutObject* start_layout_object_ = nullptr; > ... > > Then we can use |= default;| Done. https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.h:65: end_offset_(end_offset){}; On 2017/05/24 08:38:46, yosin_UTC9 wrote: > nit: s/{}/ {}/ > > We should have DCHECK for parameters. > DCHECK(start_layout_object); > DCHECK(end_layout_object); > DCHECK_GE(start_offset, 0); > DCHECK_GE(end_offset, 0); > if (start_layout_object == end_layout_object) > DCHECK_LT(start_offset, end_offset); > > This ctor should be ".cpp" file. Done. https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.h:67: bool operator==(const SelectionPaintRange& other) { On 2017/05/24 08:38:46, yosin_UTC9 wrote: > nit: s/{/ const {/ > > This function should be ".cpp" file. Done.
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/2901263002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2901263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:181: DCHECK(new_range.start_layout_object_); On 2017/05/24 at 09:09:29, yoichio wrote: > On 2017/05/24 08:38:46, yosin_UTC9 wrote: > > Once ctor has DCHECK's for members, we don't need to check here. > > How about null SelectionPaintRange? We need to check null SelectionPaintRange. Is it better to have |SelectionPaintRange::IsNull()| as |EphemeralRange|. https://codereview.chromium.org/2901263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2901263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:204: DCHECK(new_range != SelectionPaintRange()); I prefer to use |IsNull()| as |EpehemralRange| https://codereview.chromium.org/2901263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.h (right): https://codereview.chromium.org/2901263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.h:46: LayoutObject* StartLayoutObject() const { return start_layout_object_; } Let's have |IsNull()| and we should not make getters to called for non-null range. e.g. LayoutObject* SelectionPaintRange::StartLayoutObject() const { DCHECK(!IsNull()); reutrn start_layout_object_; }
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 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/2901263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2901263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:204: DCHECK(new_range != SelectionPaintRange()); On 2017/05/24 09:31:18, yosin_UTC9 wrote: > I prefer to use |IsNull()| as |EpehemralRange| Done. https://codereview.chromium.org/2901263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.h (right): https://codereview.chromium.org/2901263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.h:46: LayoutObject* StartLayoutObject() const { return start_layout_object_; } On 2017/05/24 09:31:18, yosin_UTC9 wrote: > Let's have |IsNull()| and we should not make getters to called for > non-null range. > > e.g. > > LayoutObject* SelectionPaintRange::StartLayoutObject() const { > DCHECK(!IsNull()); > reutrn start_layout_object_; > } Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:45: if (start_layout_object_ == end_layout_object_) nit: early-return style is better. https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:213: if (runner != range.StartLayoutObject() && We have a pattern her: L213 runner != range.start() && runner != range.end() && runner->CanBeSelectionLeaf() L397 runner == range.start() || runner == range.end() || runner->CanBeSelectionLeaf() L425 runner != range.start() && runner != range.end() && !runner->CanBeSelectionLeaf() We can have a function // Better name please. PaintSelectionRange::IsInside(LayoutObject* o) { return start_ == o && end_ == o; } https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:396: while (os && os != stop) { Note: We can rewrite this while-statement to for-statement as LayoutSelection::InvalidatePaintForSelection() for (LayoutObjet* os = paint_range_.StartLayoutObject(); os && os != stop; os = os->NextInPreOrder()) { ... } Both functions use same stop condition, range.EndLayoutObject() and step, NextInPreOrder(). So, we can use range pattern in SelectionPaintRange. e.g. class SelectionPaintRange { class Iterator : public std::iterator<std::input_iterator_tag, LayoutObject*> { ... Iterator& operator++() { current_ = current_->NextInPreOrder(); return *this; } bool operator==(const Iterator& other) const { return current_ == other.current_; } ... private: LayoutObject* crrent_; }; Iterator begin() const { return Iterator(StartLayoutObject()); } Iterator end() const { return Iterator(EndLayoutObject()); } }; https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.h (right): https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.h:33: class SelectionPaintRange { nit: Could you add class comment for this? e.g. This class represents a selection range in layout tree for painting. blah blah... https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.h:64: // |end_offset_|. editing/ passes them as offsets in the DOM tree but layout nit: s/DOM tree buit/DOM tree but/
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...
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_...)
Patchset #4 (id:60001) has been deleted
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/2901263002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:45: if (start_layout_object_ == end_layout_object_) On 2017/05/25 04:29:15, yosin_UTC9 wrote: > nit: early-return style is better. Done. https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:213: if (runner != range.StartLayoutObject() && On 2017/05/25 04:29:16, yosin_UTC9 wrote: > We have a pattern her: > > L213 > runner != range.start() && runner != range.end() && runner->CanBeSelectionLeaf() > > L397 > runner == range.start() || runner == range.end() || runner->CanBeSelectionLeaf() > > L425 > runner != range.start() && runner != range.end() && > !runner->CanBeSelectionLeaf() > > We can have a function > // Better name please. > PaintSelectionRange::IsInside(LayoutObject* o) { return start_ == o && end_ == > o; } > > Acknowledged. https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:396: while (os && os != stop) { On 2017/05/25 04:29:16, yosin_UTC9 wrote: > Note: We can rewrite this while-statement to for-statement as > LayoutSelection::InvalidatePaintForSelection() > > for (LayoutObjet* os = paint_range_.StartLayoutObject(); os && os != stop; os = > os->NextInPreOrder()) { > ... > } > > Both functions use same stop condition, range.EndLayoutObject() and step, > NextInPreOrder(). > So, we can use range pattern in SelectionPaintRange. > e.g. > > class SelectionPaintRange { > class Iterator > : public std::iterator<std::input_iterator_tag, LayoutObject*> { > ... > Iterator& operator++() { current_ = current_->NextInPreOrder(); return > *this; } > bool operator==(const Iterator& other) const { return current_ == > other.current_; } > ... > private: > LayoutObject* crrent_; > }; > Iterator begin() const { return Iterator(StartLayoutObject()); } > Iterator end() const { return Iterator(EndLayoutObject()); } > }; Acknowledged. https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.h (right): https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.h:33: class SelectionPaintRange { On 2017/05/25 04:29:16, yosin_UTC9 wrote: > nit: Could you add class comment for this? > > e.g. > This class represents a selection range in layout tree for painting. > blah blah... Done. https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.h:64: // |end_offset_|. editing/ passes them as offsets in the DOM tree but layout On 2017/05/25 04:29:16, yosin_UTC9 wrote: > nit: s/DOM tree buit/DOM tree but/ Done.
https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/LayoutSelection.cpp (right): https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:213: if (runner != range.StartLayoutObject() && On 2017/05/25 at 08:29:03, yoichio wrote: > On 2017/05/25 04:29:16, yosin_UTC9 wrote: > > We have a pattern her: > > > > L213 > > runner != range.start() && runner != range.end() && runner->CanBeSelectionLeaf() > > > > L397 > > runner == range.start() || runner == range.end() || runner->CanBeSelectionLeaf() > > > > L425 > > runner != range.start() && runner != range.end() && > > !runner->CanBeSelectionLeaf() > > > > We can have a function > > // Better name please. > > PaintSelectionRange::IsInside(LayoutObject* o) { return start_ == o && end_ == > > o; } > > > > > > Acknowledged. What do you mean "Acknowledged"? I would like to introduce IsInside() to replace them to simplify code. https://codereview.chromium.org/2901263002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/LayoutSelection.cpp:396: while (os && os != stop) { On 2017/05/25 at 08:29:03, yoichio wrote: > On 2017/05/25 04:29:16, yosin_UTC9 wrote: > > Note: We can rewrite this while-statement to for-statement as > > LayoutSelection::InvalidatePaintForSelection() > > > > for (LayoutObjet* os = paint_range_.StartLayoutObject(); os && os != stop; os = > > os->NextInPreOrder()) { > > ... > > } > > > > Both functions use same stop condition, range.EndLayoutObject() and step, > > NextInPreOrder(). > > So, we can use range pattern in SelectionPaintRange. > > e.g. > > > > class SelectionPaintRange { > > class Iterator > > : public std::iterator<std::input_iterator_tag, LayoutObject*> { > > ... > > Iterator& operator++() { current_ = current_->NextInPreOrder(); return > > *this; } > > bool operator==(const Iterator& other) const { return current_ == > > other.current_; } > > ... > > private: > > LayoutObject* crrent_; > > }; > > Iterator begin() const { return Iterator(StartLayoutObject()); } > > Iterator end() const { return Iterator(EndLayoutObject()); } > > }; > > Acknowledged. Since introducing iterator is big. We should do in another patch.
> > Acknowledged. > > What do you mean "Acknowledged"? I would like to introduce IsInside() to replace > them to simplify code. > > > > > Acknowledged. > > Since introducing iterator is big. We should do in another patch. I will refactor this code for other few steps. Anyway those changes are not related to this CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL?
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": 80001, "attempt_start_ts": 1495773758649900,
"parent_rev": "f3d24ad5c4879b43f34cce80a66aa1317c233bc3", "commit_rev":
"e7eadd47df235c4a2b583e12e56279022c9bc944"}
Message was sent while issue was closed.
Description was changed from ========== Introduce SelectionPaintRange in LayoutSelection SelectionPaintRange has members of |selection_start_|,|selection_end_, |selection_start_pos_| and |selection_end_pos_| which LayoutSelection had directly. BUG=708453 TEST=No change in behavior ========== to ========== Introduce SelectionPaintRange in LayoutSelection SelectionPaintRange has members of |selection_start_|,|selection_end_, |selection_start_pos_| and |selection_end_pos_| which LayoutSelection had directly. BUG=708453 TEST=No change in behavior Review-Url: https://codereview.chromium.org/2901263002 Cr-Commit-Position: refs/heads/master@{#474909} Committed: https://chromium.googlesource.com/chromium/src/+/e7eadd47df235c4a2b583e12e562... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/e7eadd47df235c4a2b583e12e562... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
