Fix bug causing TextIterator to duplicate leading spaces

Fix bug causing TextIterator to duplicate leading spaces

I ran into a TextIterator bug causing spaces at the beginning of a line occuring
immediately after another element to be duplicated. If you're iterating over
ranges, you get a range containing just the space, and then a range containing
the space together with the rest of the line, and if you call plainText() on a
range to get the text, the space occurs twice in the output.

The bug occurs because the line break gets output as one space, and then the
actual space at the beginning of the line is supposed to be skipped over. The
method restoreCollapsedLeadingSpace() is called, which incorrectly causes
TextIterator to output the real space in addition to the space from the line
break.

It appears this method was originally added to fix a text copying bug. Text
copying does use the plainText() function, but does not exhibit the bug because
it sets the emitsImageAltText behavior, and this method immediately returns if
that flag is set.

It appears that the correct fix is to remove this method. It may have been
necessary at some point but now it just appears to be introducing this bug. I
took out this method and re-ran all the webkit_unit_tests and blink layout
tests, and the only test that failed was one with a TODO saying to fix the
behavior. So my change actually fixes another bug there.

Additionally, I think the change in https://codereview.chromium.org/2723913002
is just a workaround for this bug and is no longer necessary, so I'm reverting
it. I don't understand it super well though so I could be wrong.

BUG=697654
Review-Url: https://codereview.chromium.org/2746153008
Cr-Commit-Position: refs/heads/master@{#458365}
Committed: https://chromium.googlesource.com/chromium/src/+/477e319330d3ad4eccfb32fbd5c7d28a75c6d7e3

[rlanday, 2017-03-16 01:26:39]

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

[commit-bot: I haz the power, 2017-03-16 01:27:01]

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-03-16 01:44:54]

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

[commit-bot: I haz the power, 2017-03-16 01:44:55]

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

[rlanday, 2017-03-16 02:39:19]

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

[commit-bot: I haz the power, 2017-03-16 02:39:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-03-16 04:06:26]

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

[rlanday, 2017-03-16 04:06:27]

rlanday@chromium.org changed required reviewers:
+ yosin@chromium.org

[commit-bot: I haz the power, 2017-03-16 04:13:21]

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

[commit-bot: I haz the power, 2017-03-16 04:13:23]

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

[yoichio, 2017-03-16 05:41:13]

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

[yoichio, 2017-03-16 05:41:14]

lgtm

Cool!

https://codereview.chromium.org/2746153008/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2746153008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:1042: // Then we don't
need |hasNonSeparatorCharacter|.
Please remove this line.

[rlanday, 2017-03-16 16:36:39]

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

[commit-bot: I haz the power, 2017-03-16 16:37:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-03-16 16:40:17]

https://codereview.chromium.org/2746153008/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2746153008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:1042: // Then we don't
need |hasNonSeparatorCharacter|.
On 2017/03/16 at 05:41:14, yoichio wrote:
> Please remove this line.

Done

[commit-bot: I haz the power, 2017-03-16 18:09:07]

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

[commit-bot: I haz the power, 2017-03-16 18:09:08]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-03-21 09:08:06]

lgtm

Thanks for better solution!

[yosin_UTC9, 2017-03-21 09:08:08]

The CQ bit was checked by yosin@chromium.org

[yosin_UTC9, 2017-03-21 09:08:08]

The patchset sent to the CQ was uploaded after l-g-t-m from yoichio@chromium.org
Link to the patchset: https://codereview.chromium.org/2746153008/#ps40001
(title: "Remove obsolete comment")

[commit-bot: I haz the power, 2017-03-21 09:08:27]

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-03-21 10:30:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490087288265400,
"parent_rev": "297eef179f7362b9c100009f3196bf379c73b353", "commit_rev":
"477e319330d3ad4eccfb32fbd5c7d28a75c6d7e3"}

[commit-bot: I haz the power, 2017-03-21 10:31:21]

Description was changed from

==========
Fix bug causing TextIterator to duplicate leading spaces

I ran into a TextIterator bug causing spaces at the beginning of a line occuring
immediately after another element to be duplicated. If you're iterating over
ranges, you get a range containing just the space, and then a range containing
the space together with the rest of the line, and if you call plainText() on a
range to get the text, the space occurs twice in the output.

The bug occurs because the line break gets output as one space, and then the
actual space at the beginning of the line is supposed to be skipped over. The
method restoreCollapsedLeadingSpace() is called, which incorrectly causes
TextIterator to output the real space in addition to the space from the line
break.

It appears this method was originally added to fix a text copying bug. Text
copying does use the plainText() function, but does not exhibit the bug because
it sets the emitsImageAltText behavior, and this method immediately returns if
that flag is set.

It appears that the correct fix is to remove this method. It may have been
necessary at some point but now it just appears to be introducing this bug. I
took out this method and re-ran all the webkit_unit_tests and blink layout
tests, and the only test that failed was one with a TODO saying to fix the
behavior. So my change actually fixes another bug there.

Additionally, I think the change in https://codereview.chromium.org/2723913002
is just a workaround for this bug and is no longer necessary, so I'm reverting
it. I don't understand it super well though so I could be wrong.

BUG=697654
==========

to

==========
Fix bug causing TextIterator to duplicate leading spaces

I ran into a TextIterator bug causing spaces at the beginning of a line occuring
immediately after another element to be duplicated. If you're iterating over
ranges, you get a range containing just the space, and then a range containing
the space together with the rest of the line, and if you call plainText() on a
range to get the text, the space occurs twice in the output.

The bug occurs because the line break gets output as one space, and then the
actual space at the beginning of the line is supposed to be skipped over. The
method restoreCollapsedLeadingSpace() is called, which incorrectly causes
TextIterator to output the real space in addition to the space from the line
break.

It appears this method was originally added to fix a text copying bug. Text
copying does use the plainText() function, but does not exhibit the bug because
it sets the emitsImageAltText behavior, and this method immediately returns if
that flag is set.

It appears that the correct fix is to remove this method. It may have been
necessary at some point but now it just appears to be introducing this bug. I
took out this method and re-ran all the webkit_unit_tests and blink layout
tests, and the only test that failed was one with a TODO saying to fix the
behavior. So my change actually fixes another bug there.

Additionally, I think the change in https://codereview.chromium.org/2723913002
is just a workaround for this bug and is no longer necessary, so I'm reverting
it. I don't understand it super well though so I could be wrong.

BUG=697654

Review-Url: https://codereview.chromium.org/2746153008
Cr-Commit-Position: refs/heads/master@{#458365}
Committed:
https://chromium.googlesource.com/chromium/src/+/477e319330d3ad4eccfb32fbd5c7...
==========

[commit-bot: I haz the power, 2017-03-21 10:31:22]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/477e319330d3ad4eccfb32fbd5c7...