Make TextIterator not stop at IMG element for finding text separating inline image

In text iterator, the SearchBuffer is reset when the iterator passes through
a 0-length boundary, such as IMG element. This causes failure to find texts
containing parts from both before and after the image.

BUG=419524
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183345

[c.shu, 2014-10-01 21:07:26]

c.shu@samsung.com changed reviewers:
+ eseidel@chromium.org, haraken@chromium.org

[haraken, 2014-10-01 23:28:54]

haraken@chromium.org changed reviewers:
+ yosin@chromium.org, yutak@chromium.org

[haraken, 2014-10-01 23:28:55]

ccing editing experts.

[yosin_UTC9, 2014-10-02 01:43:05]

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

[yosin_UTC9, 2014-10-02 01:43:06]

Please change "Subject" to something like "Make TextIterator not stop at IMG
element for finding text separating inline image", to explain what this CL does
rather than what happened.

Could you introduce new flag in |TextIteratorBehavior| for this behavior change?
Some clients want to keep current behavior, stopping at image.

+mlamouri@, he is one of big client of TextIterator.

[mlamouri (slow - plz ping), 2014-10-02 10:44:26]

Some clients might even be interested to get the |alt| text instead of the
image.

https://codereview.chromium.org/624483002/diff/1/Source/core/editing/TextIter...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/624483002/diff/1/Source/core/editing/TextIter...
Source/core/editing/TextIterator.cpp:1748: if (runLength) {
if (!m_textIterator.length())
  continue;
(or use |runLength|, whatever)

[c.shu, 2014-10-02 15:08:51]

On 2014/10/02 01:43:06, Yosi_UTC9 wrote:
> Please change "Subject" to something like "Make TextIterator not stop at IMG
> element for finding text separating inline image", to explain what this CL
does
> rather than what happened.
> 
> Could you introduce new flag in |TextIteratorBehavior| for this behavior
change?
> Some clients want to keep current behavior, stopping at image.
> 
> +mlamouri@, he is one of big client of TextIterator.

I was thinking about this but didn't do it since layout tests are all passed.
But you are right, layout tests may not cover all use cases.

[c.shu, 2014-10-02 17:50:58]

addressed review comments.

[yosin_UTC9, 2014-10-03 07:53:56]

Please limit this change to affect Find-In-Page feature only.

https://codereview.chromium.org/624483002/diff/20001/Source/core/editing/Text...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/624483002/diff/20001/Source/core/editing/Text...
Source/core/editing/TextIterator.cpp:1751: m_atBreak =
m_textIterator.breakAtZeroLengthBoundary();
Please make default value of |m_breakAtZeroLengthBoundary| to true to keep
current behavior. We need to change call sites in Find-In-Page to pass
NotBreakAtZeroLengthBoundary flag to TextIterator.

[Yuta Kitamura, 2014-10-07 09:51:30]

LGTM after revision; naming should be fixed before landing.

https://codereview.chromium.org/624483002/diff/40001/Source/core/editing/Text...
File Source/core/editing/TextIterator.h (right):

https://codereview.chromium.org/624483002/diff/40001/Source/core/editing/Text...
Source/core/editing/TextIterator.h:50: TextIteratorNotBreakAtZeroLengthBoundary
= 1 << 8
The name "NotBreakAtZeroLengthBoundary" does not make sense at all
if one hasn't read the implementation.

The flag should be named after something based on the behavior,
not the implementation. Should be something like
"DoesNotBreakAtReplacedElement".

As a more minor nitpick, you should use the third-person verb form
("breaks" instead of "break", "does not break" instead of "not break"
or "do not break") to align with other flags.

[Yuta Kitamura, 2014-10-07 09:54:39]

Also, please wrap the change description text in every
80 characters or so. Unwrapped text is rendered ugly in
several places like "git show".

[c.shu, 2014-10-07 14:30:26]

Thanks for the review!

https://codereview.chromium.org/624483002/diff/40001/Source/core/editing/Text...
File Source/core/editing/TextIterator.h (right):

https://codereview.chromium.org/624483002/diff/40001/Source/core/editing/Text...
Source/core/editing/TextIterator.h:50: TextIteratorNotBreakAtZeroLengthBoundary
= 1 << 8
On 2014/10/07 09:51:30, Yuta Kitamura wrote:
> The name "NotBreakAtZeroLengthBoundary" does not make sense at all
> if one hasn't read the implementation.
> 
> The flag should be named after something based on the behavior,
> not the implementation. Should be something like
> "DoesNotBreakAtReplacedElement".
> 
> As a more minor nitpick, you should use the third-person verb form
> ("breaks" instead of "break", "does not break" instead of "not break"
> or "do not break") to align with other flags.

Done.

[c.shu, 2014-10-07 14:38:58]

The CQ bit was checked by c.shu@samsung.com

[commit-bot: I haz the power, 2014-10-07 14:39:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/624483002/60001

[commit-bot: I haz the power, 2014-10-07 15:47:18]

Committed patchset #4 (id:60001) as 183345