Change how iteration range is passed to TextIteratorTextNodeHandler

Change how iteration range is passed to TextIteratorTextNodeHandler

TextIteratorTextNodeHandler used to store the full iteration range, and
have comparisons between |text_node_| and |start/end_container_| scattered
in the class to decide the offset range on |text_node_| from which text
should be emitted.

This patch changes it that:
- TextIterator, when calling HandleTextNode(), compares the current text
  node with the start and end containers to decide the offset range on it,
  and pass the offset range to TextNodeHandler
- TextNodeHandler no longer stores the full iteration, but only the offset
  range passed from TextIterator.
- TextNodeHandler no longer decides the offset range by itself, but uses
  the passed-in offset range directly.

BUG=721957
TEST=n/a; no behavioral change

Review-Url: https://codereview.chromium.org/2913673004
Cr-Commit-Position: refs/heads/master@{#476336}
Committed: https://chromium.googlesource.com/chromium/src/+/3c26208e98b83e8c698a213e63a0ac3e5f1e3b42

[Xiaocheng, 2017-05-30 23:57:46]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 23:58:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-05-31 00:11:02]

Description was changed from

==========
Change how iteration range is passed to TextIteratorTextNodeHandler

BUG=
==========

to

==========
Change how iteration range is passed to TextIteratorTextNodeHandler

TextIteratorTextNodeHandler used to store the full iteration range, and
have comparisons between |text_node_| and |start/end_container_| scattered
in the class to decide the offset range on |text_node_| from which text
should be emitted.

This patch changes it that:
- TextIterator, when calling HandleTextNode(), compares the current text
  node with the start and end containers to decide the offset range on it,
  and pass the offset range to TextNodeHandler
- TextNodeHandler no longer stores the full iteration, but only the offset
  range passed from TextIterator.
- TextNodeHandler no longer attempts to decides the offset range, but uses
  the passed-in offset range directly.

BUG=721957
TEST=n/a; no behavioral change
==========

[Xiaocheng, 2017-05-31 01:27:34]

xiaochengh@chromium.org changed reviewers:
+ yosin@chromium.org

[Xiaocheng, 2017-05-31 01:27:35]

PTAL.

[Xiaocheng, 2017-05-31 01:27:49]

Description was changed from

==========
Change how iteration range is passed to TextIteratorTextNodeHandler

TextIteratorTextNodeHandler used to store the full iteration range, and
have comparisons between |text_node_| and |start/end_container_| scattered
in the class to decide the offset range on |text_node_| from which text
should be emitted.

This patch changes it that:
- TextIterator, when calling HandleTextNode(), compares the current text
  node with the start and end containers to decide the offset range on it,
  and pass the offset range to TextNodeHandler
- TextNodeHandler no longer stores the full iteration, but only the offset
  range passed from TextIterator.
- TextNodeHandler no longer attempts to decides the offset range, but uses
  the passed-in offset range directly.

BUG=721957
TEST=n/a; no behavioral change
==========

to

==========
Change how iteration range is passed to TextIteratorTextNodeHandler

TextIteratorTextNodeHandler used to store the full iteration range, and
have comparisons between |text_node_| and |start/end_container_| scattered
in the class to decide the offset range on |text_node_| from which text
should be emitted.

This patch changes it that:
- TextIterator, when calling HandleTextNode(), compares the current text
  node with the start and end containers to decide the offset range on it,
  and pass the offset range to TextNodeHandler
- TextNodeHandler no longer stores the full iteration, but only the offset
  range passed from TextIterator.
- TextNodeHandler no longer decides the offset range by itself, but uses
  the passed-in offset range directly.

BUG=721957
TEST=n/a; no behavioral change
==========

[commit-bot: I haz the power, 2017-05-31 01:58:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 01:58:23]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-05-31 03:47:51]

yosin@chromium.org changed reviewers:
+ yoichio@chromium.org

[yosin_UTC9, 2017-05-31 03:47:52]

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.h
(right):

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.h:44:
bool HandleTextNode(Text*,
It is nice that s/Text*/const Text&/ in the future.

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.h:45:
const int* opt_start_offset,
I'm not a fan of using |nullptr| as missing value.

How about using |std::optional<int>|?
Or, |const int kInvalid = -1| and checking |kInvalid|.

yoiciho@, WDYT?

[yosin_UTC9, 2017-05-31 03:49:21]

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.h
(right):

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.h:45:
const int* opt_start_offset,
On 2017/05/31 at 03:47:52, yosin_UTC9 wrote:
> I'm not a fan of using |nullptr| as missing value.
> 
> How about using |std::optional<int>|?
> Or, |const int kInvalid = -1| and checking |kInvalid|.
> 
> yoiciho@, WDYT?

Actually, |Optional<int>| in Blink.

[Xiaocheng, 2017-05-31 04:03:45]

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.h
(right):

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.h:44:
bool HandleTextNode(Text*,
On 2017/05/31 at 03:47:52, yosin_UTC9 wrote:
> It is nice that s/Text*/const Text&/ in the future.

There are too many classes/functions that are supposed to store a const
reference but are actually using non-const references, and passing non-const
references to other clients. One example is Position.

Maybe we need a meta bug for that.

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.h:45:
const int* opt_start_offset,
On 2017/05/31 at 03:49:21, yosin_UTC9 wrote:
> On 2017/05/31 at 03:47:52, yosin_UTC9 wrote:
> > I'm not a fan of using |nullptr| as missing value.
> > 
> > How about using |std::optional<int>|?
> > Or, |const int kInvalid = -1| and checking |kInvalid|.
> > 
> > yoiciho@, WDYT?
> 
> Actually, |Optional<int>| in Blink.

The documentation says:

"It is recommended to not use base::Optional<T> as a function parameter as it
will force the callers to use base::Optional<T>. Instead, it is recommended to
keep using T* for arguments that can be omitted, with nullptr representing no
value."

https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#How-...

[yosin_UTC9, 2017-05-31 05:29:53]

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.h
(right):

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.h:45:
const int* opt_start_offset,
On 2017/05/31 at 04:03:44, Xiaocheng wrote:
> On 2017/05/31 at 03:49:21, yosin_UTC9 wrote:
> > On 2017/05/31 at 03:47:52, yosin_UTC9 wrote:
> > > I'm not a fan of using |nullptr| as missing value.
> > > 
> > > How about using |std::optional<int>|?
> > > Or, |const int kInvalid = -1| and checking |kInvalid|.
> > > 
> > > yoiciho@, WDYT?
> > 
> > Actually, |Optional<int>| in Blink.
> 
> The documentation says:
> 
> "It is recommended to not use base::Optional<T> as a function parameter as it
will force the callers to use base::Optional<T>. Instead, it is recommended to
keep using T* for arguments that can be omitted, with nullptr representing no
value."
> 
>
https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#How-...

How about using a magic number? pointer dref is slow.

[Xiaocheng, 2017-05-31 05:35:33]

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.h
(right):

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.h:45:
const int* opt_start_offset,
On 2017/05/31 at 05:29:52, yosin_UTC9 wrote:
> On 2017/05/31 at 04:03:44, Xiaocheng wrote:
> > On 2017/05/31 at 03:49:21, yosin_UTC9 wrote:
> > > On 2017/05/31 at 03:47:52, yosin_UTC9 wrote:
> > > > I'm not a fan of using |nullptr| as missing value.
> > > > 
> > > > How about using |std::optional<int>|?
> > > > Or, |const int kInvalid = -1| and checking |kInvalid|.
> > > > 
> > > > yoiciho@, WDYT?
> > > 
> > > Actually, |Optional<int>| in Blink.
> > 
> > The documentation says:
> > 
> > "It is recommended to not use base::Optional<T> as a function parameter as
it will force the callers to use base::Optional<T>. Instead, it is recommended
to keep using T* for arguments that can be omitted, with nullptr representing no
value."
> > 
> >
https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#How-...
> 
> How about using a magic number? pointer dref is slow.

I think the extra cost of ptr deref is negligible for this heavy function, and
it doesn't worth the mess due to defining and spreading an extra magic number
while nullptr is already applicable.

yoichio@, what's your preference?

[yoichio, 2017-05-31 05:36:42]

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.cpp
(right):

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.cpp:129:
: static_cast<int>(layout_object->TextStartOffset() + str.length());
Why don't you calculate them in caller?

[yosin_UTC9, 2017-05-31 05:41:31]

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.h
(right):

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.h:45:
const int* opt_start_offset,
On 2017/05/31 at 05:29:52, yosin_UTC9 wrote:
> On 2017/05/31 at 04:03:44, Xiaocheng wrote:
> > On 2017/05/31 at 03:49:21, yosin_UTC9 wrote:
> > > On 2017/05/31 at 03:47:52, yosin_UTC9 wrote:
> > > > I'm not a fan of using |nullptr| as missing value.
> > > > 
> > > > How about using |std::optional<int>|?
> > > > Or, |const int kInvalid = -1| and checking |kInvalid|.
> > > > 
> > > > yoiciho@, WDYT?
> > > 
> > > Actually, |Optional<int>| in Blink.
> > 
> > The documentation says:
> > 
> > "It is recommended to not use base::Optional<T> as a function parameter as
it will force the callers to use base::Optional<T>. Instead, it is recommended
to keep using T* for arguments that can be omitted, with nullptr representing no
value."
> > 
> >
https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#How-...
> 
> How about using a magic number? pointer dref is slow.

Usage:
  offset_ = opt_start_offset ? *opt_start_offset : 0;
  end_offset_ =
      opt_end_offset
          ? *opt_end_offset
          : static_cast<int>(layout_object->TextStartOffset() + str.length());

Call site:
  return text_node_handler_.HandleTextNode(
      ToText(node_), node_ == start_container_ ? &start_offset_ : nullptr,
      node_ == end_container_ ? &end_offset_ : nullptr);

Proposal:
  return text_node_handler_.HandleTextNode(
    ToText(node_),
    node_ == start_container_ ? start_offset_ : 0,
    node_ == end_container ? end_offser_ :
node_->GetLayoutObject()->TextStartOffset() +
node->GetLayoutObject()->GetText().length());

start_offset computation is simple, we don't need to treat it as missing value.
end_offset computation seems long, but we can introduce a function to compute
it.

[Xiaocheng, 2017-05-31 05:44:27]

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.cpp
(right):

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.cpp:129:
: static_cast<int>(layout_object->TextStartOffset() + str.length());
On 2017/05/31 at 05:36:42, yoichio wrote:
> Why don't you calculate them in caller?

Two reasons:

1. I don't want TextIterator touch details of the text node, like
|node_->GetLayoutObject()->GetText()| and
|node_->GetLayoutObject()->TextStartOffset()|. TextNodeHandler is supposed to
handle all details of the text node.

2. The |end_offset_| stored in TextIterator is DOM offset, which is not
necessarily an offset in |node_->GetLayoutObject()->GetText()|. The current
TextNodeHandler assumes that they are the same, which is wrong in some cases
(e.g., text-transform). By encapsulating everything into TextNodeHandler, we may
fix this issue easier in the future.

[yoichio, 2017-05-31 05:59:15]

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right):

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:479:
ToText(node_), node_ == start_container_ ? &start_offset_ : nullptr,
I like 
|start_container_ ? &start_offset_ : 0| and
|end_container_ ? &end_offset_ : INT_MAX|.
It means "extract text in this range" straightforward.

[yoichio, 2017-05-31 06:03:24]

On 2017/05/31 05:59:15, yoichio wrote:
>
https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp
(right):
> 
>
https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:479:
> ToText(node_), node_ == start_container_ ? &start_offset_ : nullptr,
> I like 
> |start_container_ ? &start_offset_ : 0| and
> |end_container_ ? &end_offset_ : INT_MAX|.
> It means "extract text in this range" straightforward.

Then
TextIteratorTextNodeHandler::HandleTextNode(int start, int end) {
 start_ = std::max(start, 0);
 end_ = std::min(text->Length(), end);
}

[yosin_UTC9, 2017-05-31 06:21:18]

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right):

https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:479:
ToText(node_), node_ == start_container_ ? &start_offset_ : nullptr,
On 2017/05/31 at 05:59:15, yoichio wrote:
> I like 
> |start_container_ ? &start_offset_ : 0| and
> |end_container_ ? &end_offset_ : INT_MAX|.
> It means "extract text in this range" straightforward.

I'm not fan of using magic number. 
Every call should pass valid value.
|nullptr| or |kMissing| is worse, reader can't understand meaning of it until
reading callee.

if (node == start_container_ && node == end_container_)
  return text_node_handler_.HandleTextNodeRange(ToText(node_), start_offset_,
end_offset_);
if (node == start_container_)
  return text_node_handler_.HandleTextNodeFrom(ToText(node_), start_offset_);
if (node == end_container_)
  return text_node_handler_.HandleTextNodeTo(ToText(node_), end_offset_);
return text_node_handler_.HandleTextNodeTo(ToText(node_));

We need to have three additional entries, but they tell us what we want to do
explicitly.

[Xiaocheng, 2017-05-31 06:40:55]

On 2017/05/31 at 06:21:18, yosin wrote:
>
https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp
(right):
> 
>
https://codereview.chromium.org/2913673004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:479:
ToText(node_), node_ == start_container_ ? &start_offset_ : nullptr,
> On 2017/05/31 at 05:59:15, yoichio wrote:
> > I like 
> > |start_container_ ? &start_offset_ : 0| and
> > |end_container_ ? &end_offset_ : INT_MAX|.
> > It means "extract text in this range" straightforward.
> 
> I'm not fan of using magic number. 
> Every call should pass valid value.
> |nullptr| or |kMissing| is worse, reader can't understand meaning of it until
> reading callee.
> 
> if (node == start_container_ && node == end_container_)
>   return text_node_handler_.HandleTextNodeRange(ToText(node_), start_offset_,
end_offset_);
> if (node == start_container_)
>   return text_node_handler_.HandleTextNodeFrom(ToText(node_), start_offset_);
> if (node == end_container_)
>   return text_node_handler_.HandleTextNodeTo(ToText(node_), end_offset_);
> return text_node_handler_.HandleTextNodeTo(ToText(node_));
> 
> We need to have three additional entries, but they tell us what we want to do
explicitly.

Will do. It looks good other than the verbosity, but at least the semantics
becomes clear.

Now it makes a lot of sense to have an OffsetRange class, so that we can specify
a range with unbounded endpoint(s) easier.

[Xiaocheng, 2017-05-31 19:05:12]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 19:06:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 19:15:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 19:15:06]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Xiaocheng, 2017-05-31 19:21:21]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 19:22:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 19:55:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 19:55:28]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Xiaocheng, 2017-05-31 19:59:02]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 20:00:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-05-31 21:55:11]

Updated. PTAL.

[commit-bot: I haz the power, 2017-05-31 22:33:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 22:33:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Xiaocheng, 2017-05-31 22:47:50]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 22:48:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 01:10:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 01:10:46]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-06-01 01:24:22]

lgtm w/ nits

https://codereview.chromium.org/2913673004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right):

https://codereview.chromium.org/2913673004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:488: if (node_
!= start_container_ && node_ != end_container_) {
nit: Could you use early-return style to avoid condensed switch like
if-elseif-else block?

https://codereview.chromium.org/2913673004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:489:
text_node_handler_.HandleTextNode(ToText(node_));
How about HandleTextNodeAll() or HandleTextNodeWhole() to explicitly state we
process entier Text node instead of partial.
Since this change is already verbose, this renaming doesn't increase
verbose-ness but it tells reader more meaning.

https://codereview.chromium.org/2913673004/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.cpp
(right):

https://codereview.chromium.org/2913673004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.cpp:121:
text_node_ = node;
nit: Could you add following DCHECK to catch unbound character access in string
earlier?

DCHECK_GE(start_offset, node->GetLayoutObject()->TextStartOffset());
DCHECK_LE(end_offset, node->GetLayoutObject()->GetText().length());
DCHECK_LE(start_offset, end_offset);

[Xiaocheng, 2017-06-01 04:01:38]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-06-01 04:01:39]

Thanks for reviewing! Committing with updated patch.

https://codereview.chromium.org/2913673004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right):

https://codereview.chromium.org/2913673004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:488: if (node_
!= start_container_ && node_ != end_container_) {
On 2017/06/01 at 01:24:22, yosin_UTC9 wrote:
> nit: Could you use early-return style to avoid condensed switch like
if-elseif-else block?

Done.

https://codereview.chromium.org/2913673004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:489:
text_node_handler_.HandleTextNode(ToText(node_));
On 2017/06/01 at 01:24:22, yosin_UTC9 wrote:
> How about HandleTextNodeAll() or HandleTextNodeWhole() to explicitly state we
process entier Text node instead of partial.
> Since this change is already verbose, this renaming doesn't increase
verbose-ness but it tells reader more meaning.

Done.

https://codereview.chromium.org/2913673004/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.cpp
(right):

https://codereview.chromium.org/2913673004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.cpp:121:
text_node_ = node;
On 2017/06/01 at 01:24:22, yosin_UTC9 wrote:
> nit: Could you add following DCHECK to catch unbound character access in
string earlier?
> 
> DCHECK_GE(start_offset, node->GetLayoutObject()->TextStartOffset());
> DCHECK_LE(end_offset, node->GetLayoutObject()->GetText().length());

These two are actually legit, because start_offset and end_offset are DOM
offsets. TI assume that DOM offsets and offsets in LayoutText::GetText() are
identical, which is wrong.

LayoutText::TextStartOffset() is the starting offset of remaining text when
there is first-letter, or 0 otherwise.
LayoutText::GetText().length() can be smaller than the length of the DOM string.

I'll check the offsets with the DOM string instead.

> DCHECK_LE(start_offset, end_offset);

Done.

[Xiaocheng, 2017-06-01 04:01:39]

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/2913673004/#ps80001
(title: "Wed May 31 20:34:43 PDT 2017")

[commit-bot: I haz the power, 2017-06-01 04:01:53]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 04:36:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 04:36:38]

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_...)

[Xiaocheng, 2017-06-01 05:00:30]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-06-01 05:00:31]

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/2913673004/#ps100001
(title: "Wed May 31 21:59:32 PDT 2017")

[commit-bot: I haz the power, 2017-06-01 05:00:48]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 06:54:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 06:54:48]

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_...)

[yosin_UTC9, 2017-06-01 07:24:56]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-06-01 07:25:06]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 09:14:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 09:14:21]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xiaocheng, 2017-06-01 15:24:47]

https://codereview.chromium.org/2913673004/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.cpp
(right):

https://codereview.chromium.org/2913673004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/iterators/TextIteratorTextNodeHandler.cpp:123:
DCHECK_LE(end_offset, static_cast<int>(node->data().length()));
Unfortunately we can't even have this check...

It is violated when with text-transform, where the layout string contains more
characters than the DOM string.

[Xiaocheng, 2017-06-01 15:34:51]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-06-01 15:34:52]

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/2913673004/#ps120001
(title: "Thu Jun  1 08:33:52 PDT 2017")

[commit-bot: I haz the power, 2017-06-01 15:35:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 17:20:04]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496331291662300,
"parent_rev": "3c5fceeca3187c6781b24b52fabb88353075e28a", "commit_rev":
"3c26208e98b83e8c698a213e63a0ac3e5f1e3b42"}

[commit-bot: I haz the power, 2017-06-01 17:20:14]

Description was changed from

==========
Change how iteration range is passed to TextIteratorTextNodeHandler

TextIteratorTextNodeHandler used to store the full iteration range, and
have comparisons between |text_node_| and |start/end_container_| scattered
in the class to decide the offset range on |text_node_| from which text
should be emitted.

This patch changes it that:
- TextIterator, when calling HandleTextNode(), compares the current text
  node with the start and end containers to decide the offset range on it,
  and pass the offset range to TextNodeHandler
- TextNodeHandler no longer stores the full iteration, but only the offset
  range passed from TextIterator.
- TextNodeHandler no longer decides the offset range by itself, but uses
  the passed-in offset range directly.

BUG=721957
TEST=n/a; no behavioral change
==========

to

==========
Change how iteration range is passed to TextIteratorTextNodeHandler

TextIteratorTextNodeHandler used to store the full iteration range, and
have comparisons between |text_node_| and |start/end_container_| scattered
in the class to decide the offset range on |text_node_| from which text
should be emitted.

This patch changes it that:
- TextIterator, when calling HandleTextNode(), compares the current text
  node with the start and end containers to decide the offset range on it,
  and pass the offset range to TextNodeHandler
- TextNodeHandler no longer stores the full iteration, but only the offset
  range passed from TextIterator.
- TextNodeHandler no longer decides the offset range by itself, but uses
  the passed-in offset range directly.

BUG=721957
TEST=n/a; no behavioral change

Review-Url: https://codereview.chromium.org/2913673004
Cr-Commit-Position: refs/heads/master@{#476336}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c26208e98b83e8c698a213e63a0...
==========

[commit-bot: I haz the power, 2017-06-01 17:20:16]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/3c26208e98b83e8c698a213e63a0...