Make Blink support multiline text insertion

Make Blink support multiline text insertion

When inserting multiline text, Blink will insert the text before '\n' and the 
text after the '\n' separately, e.g., for "hello\nworld", Blink will insert 
"hello", then a paragraph separator, then "world".

However, after inserting "hello", the text is updated to "hello" incorrectly,
so the remaining "world" will not be inserted. This CL moves the 
text-updating logic to upper level to make it right. Besides, this CL adjusts
the selection after each insertion for incremental insertion.

BUG=683885
Review-Url: https://codereview.chromium.org/2680733002
Cr-Commit-Position: refs/heads/master@{#448920}
Committed: https://chromium.googlesource.com/chromium/src/+/286c2ce88f848617aff81090e9cc41afdf5de7e5

[yabinh, 2017-02-07 08:40:30]

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

[commit-bot: I haz the power, 2017-02-07 08:40:38]

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

[yabinh, 2017-02-07 09:17:42]

Description was changed from

==========
Fix a bug when inserting text with new line

BUG=683885
==========

to

==========
Fix a bug when inserting text with '\n'

When inserting text with ‘\n’, Blink will insert the text before ‘\n’ and the 
text after the ‘\n’ separately, e.g., for “hello\nworld”, Blink will insert 
“hello”, then a paragraph separator, and “world”.

Unfortunately, after inserting “hello”, the text is updated to be “hello” 
incorrectly, so “world” will not be inserted. This CL moves the text-updating 
logic to upper level to fix the bug. 

When inserting text incrementally, the selection will be adjusted after all 
insertions completed. This is not correct. This CL adjusts the selection 
after each insertion.

BUG=683885
==========

[yabinh, 2017-02-07 09:40:58]

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

[yabinh, 2017-02-07 09:48:23]

Description was changed from

==========
Fix a bug when inserting text with '\n'

When inserting text with ‘\n’, Blink will insert the text before ‘\n’ and the 
text after the ‘\n’ separately, e.g., for “hello\nworld”, Blink will insert 
“hello”, then a paragraph separator, and “world”.

Unfortunately, after inserting “hello”, the text is updated to be “hello” 
incorrectly, so “world” will not be inserted. This CL moves the text-updating 
logic to upper level to fix the bug. 

When inserting text incrementally, the selection will be adjusted after all 
insertions completed. This is not correct. This CL adjusts the selection 
after each insertion.

BUG=683885
==========

to

==========
Fix a bug when inserting text with '\n'

When inserting text with ‘\n’, Blink will insert the text before ‘\n’ and the 
text after the ‘\n’ separately, e.g., for “hello\nworld”, Blink will insert 
“hello”, then a paragraph separator, then “world”.

Unfortunately, after inserting “hello”, the text is updated to “hello” 
incorrectly,
so “world” will not be inserted. This CL moves the text-updating 
logic to upper level to fix the bug. 

When inserting text incrementally, the selection will be adjusted after all 
insertions completed. This is not correct. This CL adjusts the selection 
after each insertion.

BUG=683885
==========

[yabinh, 2017-02-07 09:48:49]

Description was changed from

==========
Fix a bug when inserting text with '\n'

When inserting text with ‘\n’, Blink will insert the text before ‘\n’ and the 
text after the ‘\n’ separately, e.g., for “hello\nworld”, Blink will insert 
“hello”, then a paragraph separator, then “world”.

Unfortunately, after inserting “hello”, the text is updated to “hello” 
incorrectly,
so “world” will not be inserted. This CL moves the text-updating 
logic to upper level to fix the bug. 

When inserting text incrementally, the selection will be adjusted after all 
insertions completed. This is not correct. This CL adjusts the selection 
after each insertion.

BUG=683885
==========

to

==========
Fix a bug when inserting text with '\n'

When inserting text with ‘\n’, Blink will insert the text before ‘\n’ and the 
text after the ‘\n’ separately, e.g., for “hello\nworld”, Blink will insert 
“hello”, then a paragraph separator, then “world”.

Unfortunately, after inserting “hello”, the text is updated to “hello” 
incorrectly,
so “world” will not be inserted. This CL moves the text-updating logic to upper
level to fix the bug. 

When inserting text incrementally, the selection will be adjusted after all 
insertions completed. This is not correct. This CL adjusts the selection 
after each insertion.

BUG=683885
==========

[yabinh, 2017-02-07 09:49:01]

Description was changed from

==========
Fix a bug when inserting text with '\n'

When inserting text with ‘\n’, Blink will insert the text before ‘\n’ and the 
text after the ‘\n’ separately, e.g., for “hello\nworld”, Blink will insert 
“hello”, then a paragraph separator, then “world”.

Unfortunately, after inserting “hello”, the text is updated to “hello” 
incorrectly,
so “world” will not be inserted. This CL moves the text-updating logic to upper
level to fix the bug. 

When inserting text incrementally, the selection will be adjusted after all 
insertions completed. This is not correct. This CL adjusts the selection 
after each insertion.

BUG=683885
==========

to

==========
Fix a bug when inserting text with '\n'

When inserting text with ‘\n’, Blink will insert the text before ‘\n’ and the 
text after the ‘\n’ separately, e.g., for “hello\nworld”, Blink will insert 
“hello”, then a paragraph separator, then “world”.

Unfortunately, after inserting “hello”, the text is updated to “hello”
incorrectly,
so “world” will not be inserted. This CL moves the text-updating logic to upper
level to fix the bug. 

When inserting text incrementally, the selection will be adjusted after all 
insertions completed. This is not correct. This CL adjusts the selection 
after each insertion.

BUG=683885
==========

[yabinh, 2017-02-07 09:50:41]

yabinh@chromium.org changed reviewers:
- yosin@chromium.org

[yabinh, 2017-02-07 10:14:33]

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

[commit-bot: I haz the power, 2017-02-07 10:14:51]

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-02-07 11:42:49]

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

[commit-bot: I haz the power, 2017-02-07 11:42:50]

Dry run: This issue passed the CQ dry run.

[yabinh, 2017-02-07 11:48:22]

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

[yabinh, 2017-02-07 11:49:02]

yosin@, PTAL, thanks!

https://codereview.chromium.org/2680733002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp (left):

https://codereview.chromium.org/2680733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:527: const
bool notSelectInsertedText = false;
In the old implementation, if |text| == "hello\nworld", after
insertTextRunWithoutNewlines(), |text| will be updated to "hello" incorrectly.

[yosin_UTC9, 2017-02-08 01:46:52]

code change seems OK.

Could you update summary and description what changes you did?
"Fix something" isn't good explanation for patch.

[yabinh, 2017-02-08 02:00:31]

Description was changed from

==========
Fix a bug when inserting text with '\n'

When inserting text with ‘\n’, Blink will insert the text before ‘\n’ and the 
text after the ‘\n’ separately, e.g., for “hello\nworld”, Blink will insert 
“hello”, then a paragraph separator, then “world”.

Unfortunately, after inserting “hello”, the text is updated to “hello”
incorrectly,
so “world” will not be inserted. This CL moves the text-updating logic to upper
level to fix the bug. 

When inserting text incrementally, the selection will be adjusted after all 
insertions completed. This is not correct. This CL adjusts the selection 
after each insertion.

BUG=683885
==========

to

==========
Make Blink support multiline text insertion

When inserting multiline text, Blink will insert the text before ‘\n’ and the 
text after the ‘\n’ separately, e.g., for “hello\nworld”, Blink will insert 
“hello”, then a paragraph separator, then “world”.

However, after inserting “hello”, the text is updated to “hello” incorrectly,
so the remanding “world” will not be inserted. This CL moves the 
text-updating logic to upper level to make it right. Besides, this CL adjusts
the selection after each insertion for incremental insertion.
==========

[yabinh, 2017-02-08 02:15:20]

On 2017/02/08 01:46:52, yosin_BlinkOn_slow wrote:
> code change seems OK.
> 
> Could you update summary and description what changes you did?
> "Fix something" isn't good explanation for patch.

Done. Can you take another look at the description?

[yabinh, 2017-02-08 02:19:25]

Description was changed from

==========
Make Blink support multiline text insertion

When inserting multiline text, Blink will insert the text before ‘\n’ and the 
text after the ‘\n’ separately, e.g., for “hello\nworld”, Blink will insert 
“hello”, then a paragraph separator, then “world”.

However, after inserting “hello”, the text is updated to “hello” incorrectly,
so the remanding “world” will not be inserted. This CL moves the 
text-updating logic to upper level to make it right. Besides, this CL adjusts
the selection after each insertion for incremental insertion.
==========

to

==========
Make Blink support multiline text insertion

When inserting multiline text, Blink will insert the text before ‘\n’ and the 
text after the ‘\n’ separately, e.g., for “hello\nworld”, Blink will insert 
“hello”, then a paragraph separator, then “world”.

However, after inserting “hello”, the text is updated to “hello” incorrectly,
so the remainding “world” will not be inserted. This CL moves the 
text-updating logic to upper level to make it right. Besides, this CL adjusts
the selection after each insertion for incremental insertion.
==========

[yabinh, 2017-02-08 02:19:48]

Description was changed from

==========
Make Blink support multiline text insertion

When inserting multiline text, Blink will insert the text before ‘\n’ and the 
text after the ‘\n’ separately, e.g., for “hello\nworld”, Blink will insert 
“hello”, then a paragraph separator, then “world”.

However, after inserting “hello”, the text is updated to “hello” incorrectly,
so the remainding “world” will not be inserted. This CL moves the 
text-updating logic to upper level to make it right. Besides, this CL adjusts
the selection after each insertion for incremental insertion.
==========

to

==========
Make Blink support multiline text insertion

When inserting multiline text, Blink will insert the text before ‘\n’ and the 
text after the ‘\n’ separately, e.g., for “hello\nworld”, Blink will insert 
“hello”, then a paragraph separator, then “world”.

However, after inserting “hello”, the text is updated to “hello” incorrectly,
so the remaining “world” will not be inserted. This CL moves the 
text-updating logic to upper level to make it right. Besides, this CL adjusts
the selection after each insertion for incremental insertion.
==========

[yosin_UTC9, 2017-02-08 03:39:37]

Just in case. Could you replace quotes to ASCII quotes?
Windows Command Prompt doesn't handle UTF-8 characters well.

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

https://codereview.chromium.org/2680733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:272:
(compositionType() == TextCompositionUpdate) ? m_selectionStart : end;
nit: We don't need to have parenthesis for conditional expression.

https://codereview.chromium.org/2680733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:277:
setEndingVisibleSelection(selection);
Could you use |SelectionInDOMTree| rather than |VisibleSelection|?

Since, createSelection() is used only here. We would like to reduce usage of
|VisibleSelection|.
At this time, for comparison against |frame->selection()|, it is OK to use
|createVisibleSelection()|.
So, it could be:

const SelectionInDOMTree& selection = createSelection(...);
if (createVisibleSelection(selection) == frame->selection().selection())
  return;

setEndingSelection(selection)
frame->selection().setSelection(selection);


We'll get rid of |createVisibleSelection()| once we'll make |FrameSelection| to
hold |SelectionInDOMTree|.
This will be happened soon.

https://codereview.chromium.org/2680733002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/TypingCommand.h (right):

https://codereview.chromium.org/2680733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/TypingCommand.h:98: void
adjustSelectionAfterIncrementalInsertion(LocalFrame*,
Good! (^_^)b

[yabinh, 2017-02-08 03:45:58]

Description was changed from

==========
Make Blink support multiline text insertion

When inserting multiline text, Blink will insert the text before ‘\n’ and the 
text after the ‘\n’ separately, e.g., for “hello\nworld”, Blink will insert 
“hello”, then a paragraph separator, then “world”.

However, after inserting “hello”, the text is updated to “hello” incorrectly,
so the remaining “world” will not be inserted. This CL moves the 
text-updating logic to upper level to make it right. Besides, this CL adjusts
the selection after each insertion for incremental insertion.
==========

to

==========
Make Blink support multiline text insertion

When inserting multiline text, Blink will insert the text before '\n' and the 
text after the '\n' separately, e.g., for "hello\nworld", Blink will insert 
"hello", then a paragraph separator, then "world".

However, after inserting "hello", the text is updated to "hello" incorrectly,
so the remaining "world" will not be inserted. This CL moves the 
text-updating logic to upper level to make it right. Besides, this CL adjusts
the selection after each insertion for incremental insertion.
==========

[yosin_UTC9, 2017-02-08 03:55:35]

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

https://codereview.chromium.org/2680733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:277:
setEndingVisibleSelection(selection);
On 2017/02/08 at 03:39:37, yosin_BlinkOn_slow wrote:
> Could you use |SelectionInDOMTree| rather than |VisibleSelection|?
> 
> Since, createSelection() is used only here. We would like to reduce usage of
|VisibleSelection|.
> At this time, for comparison against |frame->selection()|, it is OK to use
|createVisibleSelection()|.
> So, it could be:
> 
> const SelectionInDOMTree& selection = createSelection(...);
> if (createVisibleSelection(selection) == frame->selection().selection())
>   return;
> 
> setEndingSelection(selection)
> frame->selection().setSelection(selection);
> 
> 
> We'll get rid of |createVisibleSelection()| once we'll make |FrameSelection|
to hold |SelectionInDOMTree|.
> This will be happened soon.

Correction, it is better to use |VisibleSelection::asSelection()| instead of
using |createVisibleSelection()|.
So, we could write:

if (selection == frame->selection().selection().asSelection())
  return;

[yabinh, 2017-02-08 04:13:29]

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

[commit-bot: I haz the power, 2017-02-08 04:13:49]

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

[yabinh, 2017-02-08 04:15:52]

Done.PTAL :)

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

https://codereview.chromium.org/2680733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:272:
(compositionType() == TextCompositionUpdate) ? m_selectionStart : end;
On 2017/02/08 03:39:37, yosin_BlinkOn_slow wrote:
> nit: We don't need to have parenthesis for conditional expression.

Done.

https://codereview.chromium.org/2680733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp:277:
setEndingVisibleSelection(selection);
On 2017/02/08 03:55:35, yosin_BlinkOn_slow wrote:
> On 2017/02/08 at 03:39:37, yosin_BlinkOn_slow wrote:
> > Could you use |SelectionInDOMTree| rather than |VisibleSelection|?
> > 
> > Since, createSelection() is used only here. We would like to reduce usage of
> |VisibleSelection|.
> > At this time, for comparison against |frame->selection()|, it is OK to use
> |createVisibleSelection()|.
> > So, it could be:
> > 
> > const SelectionInDOMTree& selection = createSelection(...);
> > if (createVisibleSelection(selection) == frame->selection().selection())
> >   return;
> > 
> > setEndingSelection(selection)
> > frame->selection().setSelection(selection);
> > 
> > 
> > We'll get rid of |createVisibleSelection()| once we'll make |FrameSelection|
> to hold |SelectionInDOMTree|.
> > This will be happened soon.
> 
> Correction, it is better to use |VisibleSelection::asSelection()| instead of
> using |createVisibleSelection()|.
> So, we could write:
> 
> if (selection == frame->selection().selection().asSelection())
>   return;

Done.

[yabinh, 2017-02-08 05:27:31]

Description was changed from

==========
Make Blink support multiline text insertion

When inserting multiline text, Blink will insert the text before '\n' and the 
text after the '\n' separately, e.g., for "hello\nworld", Blink will insert 
"hello", then a paragraph separator, then "world".

However, after inserting "hello", the text is updated to "hello" incorrectly,
so the remaining "world" will not be inserted. This CL moves the 
text-updating logic to upper level to make it right. Besides, this CL adjusts
the selection after each insertion for incremental insertion.
==========

to

==========
Make Blink support multiline text insertion

When inserting multiline text, Blink will insert the text before '\n' and the 
text after the '\n' separately, e.g., for "hello\nworld", Blink will insert 
"hello", then a paragraph separator, then "world".

However, after inserting "hello", the text is updated to "hello" incorrectly,
so the remaining "world" will not be inserted. This CL moves the 
text-updating logic to upper level to make it right. Besides, this CL adjusts
the selection after each insertion for incremental insertion.

BUG=683885
==========

[yosin_UTC9, 2017-02-08 05:34:40]

lgtm

Thanks!

[commit-bot: I haz the power, 2017-02-08 05:35:25]

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

[commit-bot: I haz the power, 2017-02-08 05:35:25]

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

[yabinh, 2017-02-08 05:38:43]

The CQ bit was checked by yabinh@chromium.org

[commit-bot: I haz the power, 2017-02-08 05:38:58]

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-02-08 06:15:03]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486532323442730,
"parent_rev": "97088372718202b776960811b01ddb74eb5b95b1", "commit_rev":
"286c2ce88f848617aff81090e9cc41afdf5de7e5"}

[commit-bot: I haz the power, 2017-02-08 06:15:57]

Description was changed from

==========
Make Blink support multiline text insertion

When inserting multiline text, Blink will insert the text before '\n' and the 
text after the '\n' separately, e.g., for "hello\nworld", Blink will insert 
"hello", then a paragraph separator, then "world".

However, after inserting "hello", the text is updated to "hello" incorrectly,
so the remaining "world" will not be inserted. This CL moves the 
text-updating logic to upper level to make it right. Besides, this CL adjusts
the selection after each insertion for incremental insertion.

BUG=683885
==========

to

==========
Make Blink support multiline text insertion

When inserting multiline text, Blink will insert the text before '\n' and the 
text after the '\n' separately, e.g., for "hello\nworld", Blink will insert 
"hello", then a paragraph separator, then "world".

However, after inserting "hello", the text is updated to "hello" incorrectly,
so the remaining "world" will not be inserted. This CL moves the 
text-updating logic to upper level to make it right. Besides, this CL adjusts
the selection after each insertion for incremental insertion.

BUG=683885

Review-Url: https://codereview.chromium.org/2680733002
Cr-Commit-Position: refs/heads/master@{#448920}
Committed:
https://chromium.googlesource.com/chromium/src/+/286c2ce88f848617aff81090e9cc...
==========

[commit-bot: I haz the power, 2017-02-08 06:15:58]

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