Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903693005:
Introduce a wrapper class to handle text node in TextIterator (Closed)
|
Created:
3 years, 7 months ago by Xiaocheng Modified:
3 years, 6 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce a wrapper class to handle text node in TextIterator
This patch adds (in-place) a wrapper class for the text node handling code
in TextIterator. With this patch, TextIterator doesn't need to know the
details of the text layout, so it can switch to Layout NG easier.
A follow-up patch will remove the wrapper class to dedicated files.
BUG=721957
TEST=n/a; no behavioral changes
Review-Url: https://codereview.chromium.org/2903693005
Cr-Commit-Position: refs/heads/master@{#475411}
Committed: https://chromium.googlesource.com/chromium/src/+/8d639e9e2ac1811b38e6bacc13e73049b27750f4
Patch Set 1 #
Total comments: 11
Patch Set 2 : Thu May 25 16:38:58 PDT 2017 #Patch Set 3 : Thu May 25 17:26:18 PDT 2017 #Patch Set 4 : Thu May 25 17:35:21 PDT 2017 #Patch Set 5 : Thu May 25 17:36:28 PDT 2017 #
Total comments: 6
Patch Set 6 : Fri May 26 14:55:33 PDT 2017 #
Total comments: 4
Patch Set 7 : Sun May 28 22:55:59 PDT 2017 #
Total comments: 4
Patch Set 8 : Mon May 29 01:57:14 PDT 2017 #
Total comments: 2
Messages
Total messages: 43 (27 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 ========== Introduce a wrapper class to handle text node in TextIterator BUG= ========== to ========== Introduce a wrapper class to handle text node in TextIterator This patch adds (in-place) a wrapper class for the text node handling code in TextIterator. With this patch, TextIterator doesn't need to know the details of the text layout, so it can switch to Layout NG easier. A follow-up patch will remove the wrapper class to dedicated files. BUG=721957 TEST=n/a; no behavioral changes ==========
xiaochengh@chromium.org changed reviewers: + yosin@chromium.org
PTAL.
https://codereview.chromium.org/2903693005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2903693005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:158: : start_offset_(0), Initialization of member variables should be done in class declaration for ease of adding/removing member variables. https://codereview.chromium.org/2903693005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:741: text_state_.EmitText(text_node_, layout_object, space_run_start, Let's introduce TextNodeContentExtract::EmitText() to make patch smaller and ease of use. I think we will have base class to share this function between Legacy and NG. It seems we can move TI::EmitText() to TextNodeContentExtarct. https://codereview.chromium.org/2903693005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:744: text_state_.SpliceBuffer(kSpaceCharacter, text_node_, 0, run_start, Let's introduce TextNodeContentExtract::SpliceBuffer() to make patch smaller and ease of use. I think we will have base class to share this function between Legacy and NG. https://codereview.chromium.org/2903693005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/iterators/TextIterator.h (right): https://codereview.chromium.org/2903693005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:53: class CORE_EXPORT TextNodeContentExtractor { - Please add class comment. - s/CORE_EXPORT// since we use TextNodeContentExtract only in TI, at this time. https://codereview.chromium.org/2903693005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:57: TextNodeContentExtractor(const TextIteratorBehavior&, TextIteratorTextState&); nit: s/TextIteratorState&/TextIterator*/ https://codereview.chromium.org/2903693005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:70: bool HandleTextNode(Text*); Please add a comment for |bool| return value.
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_...)
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...
Thanks for reviewing. Do you have an idea of any better class name? I'm not quite a fan of the current name but haven't found a better name... https://codereview.chromium.org/2903693005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2903693005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:741: text_state_.EmitText(text_node_, layout_object, space_run_start, On 2017/05/25 at 06:44:09, yosin_UTC9 wrote: > Let's introduce TextNodeContentExtract::EmitText() to make patch smaller and ease of use. > I think we will have base class to share this function between Legacy and NG. > > It seems we can move TI::EmitText() to TextNodeContentExtarct. Done. https://codereview.chromium.org/2903693005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:744: text_state_.SpliceBuffer(kSpaceCharacter, text_node_, 0, run_start, On 2017/05/25 at 06:44:09, yosin_UTC9 wrote: > Let's introduce TextNodeContentExtract::SpliceBuffer() to make patch smaller and ease of use. > I think we will have base class to share this function between Legacy and NG. Done. https://codereview.chromium.org/2903693005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/iterators/TextIterator.h (right): https://codereview.chromium.org/2903693005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:53: class CORE_EXPORT TextNodeContentExtractor { On 2017/05/25 at 06:44:09, yosin_UTC9 wrote: > - Please add class comment. > - s/CORE_EXPORT// since we use TextNodeContentExtract only in TI, at this time. Done. https://codereview.chromium.org/2903693005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:57: TextNodeContentExtractor(const TextIteratorBehavior&, TextIteratorTextState&); On 2017/05/25 at 06:44:09, yosin_UTC9 wrote: > nit: s/TextIteratorState&/TextIterator*/ Is there a style guide? I've seen a lot of classes passing non-const references to their constructors: https://cs.chromium.org/search/?q=LocalFrame%26&type=cs https://codereview.chromium.org/2903693005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:70: bool HandleTextNode(Text*); On 2017/05/25 at 06:44:09, yosin_UTC9 wrote: > Please add a comment for |bool| return value. Done.
I've renamed it into TextIteratorTextNodeHandler, which aligns with TextIteratorTextState. Other than its verbosity, the name looks good.
https://codereview.chromium.org/2903693005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIterator.h (right): https://codereview.chromium.org/2903693005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:60: TextIteratorTextState&); nit: s/TextIteratorState&/TextIterator*/ https://codereview.chromium.org/2903693005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:140: }; nit: We should have |DISALLOW_COPY_AND_ASSIGN(...)|. https://codereview.chromium.org/2903693005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:328: TextIteratorTextNodeHandler text_node_handler_; It is better to use |TextIteratorTextNodeHandler*| to move class declaration into ".cpp" file to hide implementation details. In addition of this, we'll use pointer for switching legacy and ng.
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
https://codereview.chromium.org/2903693005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIterator.h (right): https://codereview.chromium.org/2903693005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:60: TextIteratorTextState&); On 2017/05/26 at 01:47:25, yosin_UTC9 wrote: > nit: s/TextIteratorState&/TextIterator*/ Done. https://codereview.chromium.org/2903693005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:140: }; On 2017/05/26 at 01:47:25, yosin_UTC9 wrote: > nit: We should have |DISALLOW_COPY_AND_ASSIGN(...)|. Done. https://codereview.chromium.org/2903693005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:328: TextIteratorTextNodeHandler text_node_handler_; On 2017/05/26 at 01:47:25, yosin_UTC9 wrote: > It is better to use |TextIteratorTextNodeHandler*| to move class declaration into ".cpp" file > to hide implementation details. > > In addition of this, we'll use pointer for switching legacy and ng. Added a TODO for it. I don't want to handle memory issues now.
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.
https://codereview.chromium.org/2903693005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/TextIterator.h (right): https://codereview.chromium.org/2903693005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:55: class TextIteratorTextNodeHandler { nit: s/{/final {/ https://codereview.chromium.org/2903693005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:139: TextIteratorTextState* text_state_; nit: s/TextIteratorTextState*/TextIteratorTextState&/ This makes this patch smaller.
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...
https://codereview.chromium.org/2903693005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/TextIterator.h (right): https://codereview.chromium.org/2903693005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:55: class TextIteratorTextNodeHandler { On 2017/05/29 at 04:46:05, yosin_UTC9 wrote: > nit: s/{/final {/ Done. https://codereview.chromium.org/2903693005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/TextIterator.h:139: TextIteratorTextState* text_state_; On 2017/05/29 at 04:46:05, yosin_UTC9 wrote: > nit: s/TextIteratorTextState*/TextIteratorTextState&/ > > This makes this patch smaller. Done.
https://codereview.chromium.org/2903693005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2903693005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:196: void TextIteratorTextNodeHandler::SetBoundaries(Node* start_container, This function name can be misleading, e.g. we can call this function multiple times. It is better to call |Initialize()| with DCHECK(!start_container), DCHECK(!end_container), DDCHKE_EQ(start_offset_, 0), DCHECK_EQ(end_offset_, 0) It seems we'll call this function multiple times, it it better to call |Start()|?
https://codereview.chromium.org/2903693005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2903693005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:196: void TextIteratorTextNodeHandler::SetBoundaries(Node* start_container, On 2017/05/29 at 06:42:31, yosin_UTC9 wrote: > This function name can be misleading, e.g. we can call this function multiple times. > It is better to call |Initialize()| with DCHECK(!start_container), DCHECK(!end_container), DDCHKE_EQ(start_offset_, 0), DCHECK_EQ(end_offset_, 0) > > It seems we'll call this function multiple times, it it better to call |Start()|? This function is only called once. It copies the iteration range from TextIterator to TextNodeHandler, so that the code moved to TextNodeHandler can still use them, so that the code change is small. In fact, as stated by the TODO (in TI.h), this function will be completely removed by a subsequent patch. So there's no need to rename it to unnecessarily lengthen the iteration.
https://codereview.chromium.org/2903693005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2903693005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:196: void TextIteratorTextNodeHandler::SetBoundaries(Node* start_container, On 2017/05/29 at 07:01:37, Xiaocheng wrote: > On 2017/05/29 at 06:42:31, yosin_UTC9 wrote: > > This function name can be misleading, e.g. we can call this function multiple times. > > It is better to call |Initialize()| with DCHECK(!start_container), DCHECK(!end_container), DDCHKE_EQ(start_offset_, 0), DCHECK_EQ(end_offset_, 0) > > > > It seems we'll call this function multiple times, it it better to call |Start()|? > > This function is only called once. It copies the iteration range from TextIterator to TextNodeHandler, so that the code moved to TextNodeHandler can still use them, so that the code change is small. > > In fact, as stated by the TODO (in TI.h), this function will be completely removed by a subsequent patch. So there's no need to rename it to unnecessarily lengthen the iteration. Could you add TODO about we'll remove this function? I think it is better to have DCHECK() to verify this function called once.
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...
Updated. PTAL. https://codereview.chromium.org/2903693005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2903693005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:196: void TextIteratorTextNodeHandler::SetBoundaries(Node* start_container, On 2017/05/29 at 07:43:56, yosin_UTC9 wrote: > On 2017/05/29 at 07:01:37, Xiaocheng wrote: > > On 2017/05/29 at 06:42:31, yosin_UTC9 wrote: > > > This function name can be misleading, e.g. we can call this function multiple times. > > > It is better to call |Initialize()| with DCHECK(!start_container), DCHECK(!end_container), DDCHKE_EQ(start_offset_, 0), DCHECK_EQ(end_offset_, 0) > > > > > > It seems we'll call this function multiple times, it it better to call |Start()|? > > > > This function is only called once. It copies the iteration range from TextIterator to TextNodeHandler, so that the code moved to TextNodeHandler can still use them, so that the code change is small. > > > > In fact, as stated by the TODO (in TI.h), this function will be completely removed by a subsequent patch. So there's no need to rename it to unnecessarily lengthen the iteration. > > Could you add TODO about we'll remove this function? > I think it is better to have DCHECK() to verify this function called once. Renamed and added DCHECK.
lgtm w/ nit https://codereview.chromium.org/2903693005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2903693005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:201: DCHECK(!start_container_); nit: Please add DCHECK(start_container) and DCHECK(end_contianer) When we call Initialize() with |startoffset == nullptr|, we can call this function twice. Another option is DCHECK_LT(Position(start_container_, start_offset), Position(end_container, end_offset)) since Position::operator<=() caused DCHECK for null-Position.
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
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2903693005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2903693005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:201: DCHECK(!start_container_); On 2017/05/29 at 09:12:59, yosin_UTC9 wrote: > nit: Please add DCHECK(start_container) and DCHECK(end_contianer) > When we call Initialize() with |startoffset == nullptr|, we can call this function twice. This has been DCHECKed in TI::Initialize. > > Another option is DCHECK_LT(Position(start_container_, start_offset), Position(end_container, end_offset)) > since Position::operator<=() caused DCHECK for null-Position.
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496113150530560,
"parent_rev": "872e79082cecd9b706c932978d4160d03f4d1fa2", "commit_rev":
"8d639e9e2ac1811b38e6bacc13e73049b27750f4"}
Message was sent while issue was closed.
Description was changed from ========== Introduce a wrapper class to handle text node in TextIterator This patch adds (in-place) a wrapper class for the text node handling code in TextIterator. With this patch, TextIterator doesn't need to know the details of the text layout, so it can switch to Layout NG easier. A follow-up patch will remove the wrapper class to dedicated files. BUG=721957 TEST=n/a; no behavioral changes ========== to ========== Introduce a wrapper class to handle text node in TextIterator This patch adds (in-place) a wrapper class for the text node handling code in TextIterator. With this patch, TextIterator doesn't need to know the details of the text layout, so it can switch to Layout NG easier. A follow-up patch will remove the wrapper class to dedicated files. BUG=721957 TEST=n/a; no behavioral changes Review-Url: https://codereview.chromium.org/2903693005 Cr-Commit-Position: refs/heads/master@{#475411} Committed: https://chromium.googlesource.com/chromium/src/+/8d639e9e2ac1811b38e6bacc13e7... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/8d639e9e2ac1811b38e6bacc13e7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
