Reland: Fix setComposingText with empty text when newCursorPosition != 1

Reland: Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) should cancel the
composition (if any), and move the cursor according to
|newCursorPosition|.

Note that we shouldn't close typing when moving the cursor in this
case. In other cases, like in
InputMethodController::confirmCompositionOrInsertText, we should
close typing when moving the cursor. Thus, we need an extra parameter.

This is a reland CL. The previous CL caused a bug
crbug.com/633840.

BUG=570920, 633840
Committed: https://crrev.com/dd6d1b62de7b449d997d159d63207abe87d3cb46
Cr-Commit-Position: refs/heads/master@{#413683}

[yabinh, 2016-05-30 08:03:19]

Description was changed from

==========
Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) doesn't work fine when newCursorPosition
!= 1. It should behave the same as commitText("", newCursorPosition), because
when the text is empty, commitText will call
InputMethodController::setComposition, which is exactly the same as
setComposingText().

BUG=1995333002
==========

to

==========
Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) doesn't work fine when newCursorPosition
!= 1. It should behave the same as commitText("", newCursorPosition), because
when the text is empty, commitText will call
InputMethodController::setComposition, which is exactly the same as
setComposingText().

BUG=570920
==========

[yabinh, 2016-05-30 08:04:44]

yabinh@chromium.org changed reviewers:
+ aelias@chromium.org, changwan@chromium.org

[yabinh, 2016-05-30 08:04:45]

changwan@, can you take a look at this patch?

[Changwan Ryu, 2016-05-30 08:18:23]

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

https://codereview.chromium.org/2020973002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:500: bool
InputMethodController::setEditableSelectionOffsetsWithBoundaryCheck(int start,
int end)
could you check if you can replace setEditableSelectionOffsets() by this
function?

[yabinh, 2016-05-31 01:53:59]

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

https://codereview.chromium.org/2020973002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:500: bool
InputMethodController::setEditableSelectionOffsetsWithBoundaryCheck(int start,
int end)
On 2016/05/30 08:18:22, Changwan Ryu wrote:
> could you check if you can replace setEditableSelectionOffsets() by this
> function?

Please see patch set 2.

[yabinh, 2016-06-02 01:52:56]

Changwan@, can you take another look at this patch?

[Changwan Ryu, 2016-06-02 02:19:39]

On 2016/06/02 01:52:56, yabinh wrote:
> Changwan@, can you take another look at this patch?

talked to yabinh@ offline, but for the record,
1) don't need to change the method name since boundary check is not an
additional feature.
2) don't need to convolute all the possible combinations of inputs for tests,
please try to add the smallest number of the tests that you really need.

[yabinh, 2016-06-02 04:54:02]

Changwan@, can you take a look at patch set 4?

[yabinh, 2016-06-02 05:14:14]

Description was changed from

==========
Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) doesn't work fine when newCursorPosition
!= 1. It should behave the same as commitText("", newCursorPosition), because
when the text is empty, commitText will call
InputMethodController::setComposition, which is exactly the same as
setComposingText().

BUG=570920
==========

to

==========
Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) doesn't work fine when    
newCursorPosition != 1. It should behave the same as    
commitText("", newCursorPosition), because when the text is empty,    
commitText will call InputMethodController::setComposition, which is    
exactly the same as setComposingText().

BUG=570920
==========

[Changwan Ryu, 2016-06-02 09:57:35]

lgtm

[aelias_OOO_until_Jul13, 2016-06-02 22:25:19]

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

[aelias_OOO_until_Jul13, 2016-06-02 22:25:20]

Source/web lgtm, adding yosin@ for Source/core/editing OWNERS.

[yosin_UTC9, 2016-06-03 07:16:11]

https://codereview.chromium.org/2020973002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.h (right):

https://codereview.chromium.org/2020973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.h:79: bool
setEditableSelectionOffsets(int start, int end);
Could you explain benefit of you replace |PlaintTextRange| by |start| and |end|
pair?
We use |PlaintTextRange| in |InputMethodController| in many places, I don't want
to have two representations of range.

[yabinh, 2016-06-03 07:58:22]

On 2016/06/03 07:16:11, Yosi_UTC9 wrote:
>
https://codereview.chromium.org/2020973002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/InputMethodController.h (right):
> 
>
https://codereview.chromium.org/2020973002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/InputMethodController.h:79: bool
> setEditableSelectionOffsets(int start, int end);
> Could you explain benefit of you replace |PlaintTextRange| by |start| and
|end|
> pair?
> We use |PlaintTextRange| in |InputMethodController| in many places, I don't
want
> to have two representations of range.

If we use |PlaintTextRange|, we should handle the case of exceeding left
boundary(start<0) before we create PlainTextRange, i.e., outside
InputMethodController::setEditableSelectionOffsets(PlainTextRange).



In this cl, in order to handle the case of exceeding right boundary, we create 2
Position objects. Then we create a EphemeralRange object directly from these 2
objects. After that, we use this EphemeralRange object to change the selection.

If we want to handle the case of exceeding right boundary BEFORE creating
PlaintTextRange, we need to convert the above EphemeralRange object to
PlaintTextRange object(because we can't create PlaintTextRange object directly
from Position object.), then we need to convert the PlaintTextRange object to
EphemeralRange object to change the selection. This is redundant. We don't need
a PlaintTextRange object as medium.

If we want to handle the above case AFTER creating PlaintTextRange, we'll have
to handle the boundary problem in 2 different places( left boundary is handled
outside InputMethodController::setEditableSelectionOffsets(PlainTextRange),
right boundary is handled in this function ). I don't think it's a good idea to
do that.

[yosin_UTC9, 2016-06-03 09:41:05]

On 2016/06/03 at 07:58:22, yabinh wrote:
> On 2016/06/03 07:16:11, Yosi_UTC9 wrote:
> >
https://codereview.chromium.org/2020973002/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/editing/InputMethodController.h (right):
> > 
> >
https://codereview.chromium.org/2020973002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/editing/InputMethodController.h:79: bool
> > setEditableSelectionOffsets(int start, int end);
> > Could you explain benefit of you replace |PlaintTextRange| by |start| and
|end|
> > pair?
> > We use |PlaintTextRange| in |InputMethodController| in many places, I don't
want
> > to have two representations of range.
> 
> If we use |PlaintTextRange|, we should handle the case of exceeding left
boundary(start<0) before we create PlainTextRange, i.e., outside
InputMethodController::setEditableSelectionOffsets(PlainTextRange).
> 
> 
> 
> In this cl, in order to handle the case of exceeding right boundary, we create
2 Position objects. Then we create a EphemeralRange object directly from these 2
objects. After that, we use this EphemeralRange object to change the selection.
> 
> If we want to handle the case of exceeding right boundary BEFORE creating
PlaintTextRange, we need to convert the above EphemeralRange object to
PlaintTextRange object(because we can't create PlaintTextRange object directly
from Position object.), then we need to convert the PlaintTextRange object to
EphemeralRange object to change the selection. This is redundant. We don't need
a PlaintTextRange object as medium.
> 
> If we want to handle the above case AFTER creating PlaintTextRange, we'll have
to handle the boundary problem in 2 different places( left boundary is handled
outside InputMethodController::setEditableSelectionOffsets(PlainTextRange),
right boundary is handled in this function ). I don't think it's a good idea to
do that.

So, do you want to pass start < 0 < end and end < start?
If so, it should be done in caller side.

    start = std::max(start, 0);
    end = std::max(end, start);
in setEditableSelectionOffsets()
seems to be bad practice. Caller should care about parameter validness.
This is reason why we use PlainTextRange() to keep start <= end invariant.

[yabinh, 2016-06-07 07:32:46]

yosin@, can you take another look of this patch?

[yosin_UTC9, 2016-06-07 08:11:20]

Could you explain when caller pass start < 0, start > end?
I'm still not clear why setComposition() takes it.

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

https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:506: start =
std::max(start, 0);
Adjusting |start| and |end| should be done in call sites rather than here.
Passing wrong range indicates bad thing happened in call sites.
This kind of treatment is error prone and makes hard to find bug in callers.

https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:515:
TextIteratorBehaviorFlags behaviorFlags =
TextIteratorEmitsObjectReplacementCharacter;
nit: Should have |const|. We can express this in one statement.

https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:520: for (;
!it.atEnd(); it.advance()) {
nit: No need to have {}

[yabinh, 2016-06-07 08:47:50]

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

https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:506: start =
std::max(start, 0);
On 2016/06/07 08:11:20, Yosi_UTC9 wrote:
> Adjusting |start| and |end| should be done in call sites rather than here.
> Passing wrong range indicates bad thing happened in call sites.
> This kind of treatment is error prone and makes hard to find bug in caller

It's easy to handle the left boundary. But for the right boundary, we need do
more things.

Do we need to apply boundary-checking logic in InputMethodController, or
RenderWidget, or other place?

FYI, there are other issues also need this logic, like:
https://codereview.chromium.org/1995333002/
https://codereview.chromium.org/1889053003/

Another question. PlainTextRange::create(start, end) can return a range staying
at right boudary when |start| or |end| exceeds right boundary. But when both of
them exceed the right boundary, it will return a default range [0,0], instead of
staying at the right boundary. If we change it to let it stay at the right
boundary, most of code in this function will be unnecessary. The problem is, if
we do that, many Layout Tests will fail.
Do you think we should keep the logic in this function, or we should change
PlainTextRange::create()?

[yosin_UTC9, 2016-06-07 10:07:13]

On 2016/06/07 at 08:47:50, yabinh wrote:
>
https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):
> 
>
https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:506: start =
std::max(start, 0);
> On 2016/06/07 08:11:20, Yosi_UTC9 wrote:
> > Adjusting |start| and |end| should be done in call sites rather than here.
> > Passing wrong range indicates bad thing happened in call sites.
> > This kind of treatment is error prone and makes hard to find bug in caller
> 
> It's easy to handle the left boundary. But for the right boundary, we need do
more things.
> 
> Do we need to apply boundary-checking logic in InputMethodController, or
RenderWidget, or other place?
very upsteram or originator. In this case, Java side.


> FYI, there are other issues also need this logic, like:
> https://codereview.chromium.org/1995333002/
> https://codereview.chromium.org/1889053003/
> 
> Another question. PlainTextRange::create(start, end) can return a range
staying at right boudary when |start| or |end| exceeds right boundary. But when
both of them exceed the right boundary, it will return a default range [0,0],
instead of staying at the right boundary. If we change it to let it stay at the
right boundary, most of code in this function will be unnecessary. The problem
is, if we do that, many Layout Tests will fail.
> Do you think we should keep the logic in this function, or we should change
PlainTextRange::create()?

PlainTextRange::create()'s signature are:
   static PlainTextRange create(const ContainerNode& scope, const
EphemeralRange&);
   static PlainTextRange create(const ContainerNode& scope, const Range&);
Are you taliking about PlainTextRange(int start, int end) constructor?
Could you make clear?

[yabinh, 2016-06-07 10:14:28]

On 2016/06/07 10:07:13, Yosi_UTC9 wrote:
> On 2016/06/07 at 08:47:50, yabinh wrote:
> >
>
https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/editing/InputMethodController.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/editing/InputMethodController.cpp:506: start
=
> std::max(start, 0);
> > On 2016/06/07 08:11:20, Yosi_UTC9 wrote:
> > > Adjusting |start| and |end| should be done in call sites rather than here.
> > > Passing wrong range indicates bad thing happened in call sites.
> > > This kind of treatment is error prone and makes hard to find bug in caller
> > 
> > It's easy to handle the left boundary. But for the right boundary, we need
do
> more things.
> > 
> > Do we need to apply boundary-checking logic in InputMethodController, or
> RenderWidget, or other place?
> very upsteram or originator. In this case, Java side.
> 
> 
> > FYI, there are other issues also need this logic, like:
> > https://codereview.chromium.org/1995333002/
> > https://codereview.chromium.org/1889053003/
> > 
> > Another question. PlainTextRange::create(start, end) can return a range
> staying at right boudary when |start| or |end| exceeds right boundary. But
when
> both of them exceed the right boundary, it will return a default range [0,0],
> instead of staying at the right boundary. If we change it to let it stay at
the
> right boundary, most of code in this function will be unnecessary. The problem
> is, if we do that, many Layout Tests will fail.
> > Do you think we should keep the logic in this function, or we should change
> PlainTextRange::create()?
> 
> PlainTextRange::create()'s signature are:
>    static PlainTextRange create(const ContainerNode& scope, const
> EphemeralRange&);
>    static PlainTextRange create(const ContainerNode& scope, const Range&);
> Are you taliking about PlainTextRange(int start, int end) constructor?
> Could you make clear?

Sorry. I was talking about:
PlainTextRange::createRange(const ContainerNode& scope) const;

It can be called like this:
const EphemeralRange& startRange = PlainTextRange(0,
start).createRange(*rootEditableElement);

[Changwan Ryu, 2016-06-08 02:25:19]

On 2016/06/07 10:07:13, Yosi_UTC9 wrote:
> On 2016/06/07 at 08:47:50, yabinh wrote:
> >
>
https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/editing/InputMethodController.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/editing/InputMethodController.cpp:506: start
=
> std::max(start, 0);
> > On 2016/06/07 08:11:20, Yosi_UTC9 wrote:
> > > Adjusting |start| and |end| should be done in call sites rather than here.
> > > Passing wrong range indicates bad thing happened in call sites.
> > > This kind of treatment is error prone and makes hard to find bug in caller
> > 
> > It's easy to handle the left boundary. But for the right boundary, we need
do
> more things.
> > 
> > Do we need to apply boundary-checking logic in InputMethodController, or
> RenderWidget, or other place?
> very upsteram or originator. In this case, Java side.
Browser side does not know the current composition / selection ranges. The
latest values may not have been
propagated yet when there is a delay in the renderer thread. Also, JavaScript
can also change the text in the middle, so
subsequent calls cannot assume the current values without actually getting the
updates, by which time the value might already have been changed again.

> 
> 
> > FYI, there are other issues also need this logic, like:
> > https://codereview.chromium.org/1995333002/
> > https://codereview.chromium.org/1889053003/
> > 
> > Another question. PlainTextRange::create(start, end) can return a range
> staying at right boudary when |start| or |end| exceeds right boundary. But
when
> both of them exceed the right boundary, it will return a default range [0,0],
> instead of staying at the right boundary. If we change it to let it stay at
the
> right boundary, most of code in this function will be unnecessary. The problem
> is, if we do that, many Layout Tests will fail.
> > Do you think we should keep the logic in this function, or we should change
> PlainTextRange::create()?
> 
> PlainTextRange::create()'s signature are:
>    static PlainTextRange create(const ContainerNode& scope, const
> EphemeralRange&);
>    static PlainTextRange create(const ContainerNode& scope, const Range&);
> Are you taliking about PlainTextRange(int start, int end) constructor?
> Could you make clear?

[yosin_UTC9, 2016-06-08 04:23:59]

On 2016/06/07 at 10:14:28, yabinh wrote:
> On 2016/06/07 10:07:13, Yosi_UTC9 wrote:
> > On 2016/06/07 at 08:47:50, yabinh wrote:
> > >
> >
https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/editing/InputMethodController.cpp
(right):
> > > 
> > >
> >
https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/editing/InputMethodController.cpp:506:
start =
> > std::max(start, 0);
> > > On 2016/06/07 08:11:20, Yosi_UTC9 wrote:
> > > > Adjusting |start| and |end| should be done in call sites rather than
here.
> > > > Passing wrong range indicates bad thing happened in call sites.
> > > > This kind of treatment is error prone and makes hard to find bug in
caller
> > > 
> > > It's easy to handle the left boundary. But for the right boundary, we need
do
> > more things.
> > > 
> > > Do we need to apply boundary-checking logic in InputMethodController, or
> > RenderWidget, or other place?
> > very upsteram or originator. In this case, Java side.
> > 
> > 
> > > FYI, there are other issues also need this logic, like:
> > > https://codereview.chromium.org/1995333002/
> > > https://codereview.chromium.org/1889053003/
> > > 
> > > Another question. PlainTextRange::create(start, end) can return a range
> > staying at right boudary when |start| or |end| exceeds right boundary. But
when
> > both of them exceed the right boundary, it will return a default range
[0,0],
> > instead of staying at the right boundary. If we change it to let it stay at
the
> > right boundary, most of code in this function will be unnecessary. The
problem
> > is, if we do that, many Layout Tests will fail.
> > > Do you think we should keep the logic in this function, or we should
change
> > PlainTextRange::create()?
> > 
> > PlainTextRange::create()'s signature are:
> >    static PlainTextRange create(const ContainerNode& scope, const
> > EphemeralRange&);
> >    static PlainTextRange create(const ContainerNode& scope, const Range&);
> > Are you taliking about PlainTextRange(int start, int end) constructor?
> > Could you make clear?
> 
> Sorry. I was talking about:
> PlainTextRange::createRange(const ContainerNode& scope) const;
> 
> It can be called like this:
> const EphemeralRange& startRange = PlainTextRange(0,
start).createRange(*rootEditableElement);

Since, |start| and |end| defines a range of text, so they aren't exceeds
boundary.
So, I guess you're saying |start|, |end| in Java side and text composition in
Blink. In this case
|start|, |end| can be out of range of text composition.

Do you attend BlinkOn6? If so, I would like to discuss this issue on BlinkOn.

[yosin_UTC9, 2016-06-08 05:25:02]

On 2016/06/08 at 02:25:19, changwan wrote:
> On 2016/06/07 10:07:13, Yosi_UTC9 wrote:
> > On 2016/06/07 at 08:47:50, yabinh wrote:
> > >
> >
https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/editing/InputMethodController.cpp
(right):
> > > 
> > >
> >
https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/editing/InputMethodController.cpp:506:
start =
> > std::max(start, 0);
> > > On 2016/06/07 08:11:20, Yosi_UTC9 wrote:
> > > > Adjusting |start| and |end| should be done in call sites rather than
here.
> > > > Passing wrong range indicates bad thing happened in call sites.
> > > > This kind of treatment is error prone and makes hard to find bug in
caller
> > > 
> > > It's easy to handle the left boundary. But for the right boundary, we need
do
> > more things.
> > > 
> > > Do we need to apply boundary-checking logic in InputMethodController, or
> > RenderWidget, or other place?
> > very upsteram or originator. In this case, Java side.
> Browser side does not know the current composition / selection ranges. The
latest values may not have been
> propagated yet when there is a delay in the renderer thread. Also, JavaScript
can also change the text in the middle, so
> subsequent calls cannot assume the current values without actually getting the
updates, by which time the value might already have been changed again.
Yes, this issue exists since beginning, four years ago and reported by aurimas@.
I would like to fix this situation somehow.

It seems we are in same time zone. Can we have VC to discuss about Clank and
Blink IME API?
It is nice to meet at BlinkOn6 too.

[Changwan Ryu, 2016-06-08 05:44:25]

On 2016/06/08 05:25:02, Yosi_UTC9 wrote:
> On 2016/06/08 at 02:25:19, changwan wrote:
> > On 2016/06/07 10:07:13, Yosi_UTC9 wrote:
> > > On 2016/06/07 at 08:47:50, yabinh wrote:
> > > >
> > >
>
https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
> > > > File third_party/WebKit/Source/core/editing/InputMethodController.cpp
> (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2020973002/diff/100001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/core/editing/InputMethodController.cpp:506:
> start =
> > > std::max(start, 0);
> > > > On 2016/06/07 08:11:20, Yosi_UTC9 wrote:
> > > > > Adjusting |start| and |end| should be done in call sites rather than
> here.
> > > > > Passing wrong range indicates bad thing happened in call sites.
> > > > > This kind of treatment is error prone and makes hard to find bug in
> caller
> > > > 
> > > > It's easy to handle the left boundary. But for the right boundary, we
need
> do
> > > more things.
> > > > 
> > > > Do we need to apply boundary-checking logic in InputMethodController, or
> > > RenderWidget, or other place?
> > > very upsteram or originator. In this case, Java side.
> > Browser side does not know the current composition / selection ranges. The
> latest values may not have been
> > propagated yet when there is a delay in the renderer thread. Also,
JavaScript
> can also change the text in the middle, so
> > subsequent calls cannot assume the current values without actually getting
the
> updates, by which time the value might already have been changed again.
> Yes, this issue exists since beginning, four years ago and reported by
aurimas@.
> I would like to fix this situation somehow.
> 
> It seems we are in same time zone. Can we have VC to discuss about Clank and
> Blink IME API?
> It is nice to meet at BlinkOn6 too.

I've set up a VC to discuss Clank IME today, but basically
we're handling this issue with the design proposed in
https://docs.google.com/document/d/1MxICLxUwVwt1ofzEvJFkTnjt8t1rwHWmc-SthS9Br...
. Progress can be tracked here: http://crbug.com/551193
(BTW, I'll probably not attend BlinkOn6.)

[yabinh, 2016-06-13 06:07:07]

Hi, yosin@, just want to make sure, is that OK to do the boundary-checking in
InputMethodController.cpp?
If it is, is patch set 6 OK to handle the right boundary(except for some nits)?

[yosin_UTC9, 2016-06-13 07:26:50]

On 2016/06/13 at 06:07:07, yabinh wrote:
> Hi, yosin@, just want to make sure, is that OK to do the boundary-checking in
InputMethodController.cpp?
> If it is, is patch set 6 OK to handle the right boundary(except for some
nits)?

No. When we handle out-of-bound values in API, it is hard to detect callers bug.
Caller should find way
to pass valid values to API.

[yabinh, 2016-06-14 05:43:32]

On 2016/06/13 07:26:50, Yosi_UTC9 wrote:
> On 2016/06/13 at 06:07:07, yabinh wrote:
> > Hi, yosin@, just want to make sure, is that OK to do the boundary-checking
in
> InputMethodController.cpp?
> > If it is, is patch set 6 OK to handle the right boundary(except for some
> nits)?
> 
> No. When we handle out-of-bound values in API, it is hard to detect callers
bug.
> Caller should find way
> to pass valid values to API.

In patch set 7, the out-of bound case is handled in render_widget. yosin@, can
you take another look of this patch?

[yosin_UTC9, 2016-06-15 05:34:41]

https://codereview.chromium.org/2020973002/diff/140001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2020973002/diff/140001/content/renderer/rende...
content/renderer/render_widget.cc:1511: void
RenderWidget::adjustSelectionForBoundary(int& selectionStart,
Please follow Chromimum coding style:
http://dev.chromium.org/developers/coding-style
Roughly similar to Google C++ coding style.

Major thing is you should use pointer for output parameter rather than
reference.
How about using |std::pair<int, int>>|?

https://codereview.chromium.org/2020973002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2020973002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:327: 
Could you add DCHECK_XX() to validate |start| and |end| have valid values?

https://codereview.chromium.org/2020973002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:403: const
EphemeralRange selectedRange = PlainTextRange(start,
end).createRange(*rootEditableElement);
s/const EphemeralRange/const EphmeralRange&/
to avoid copy.

[yabinh, 2016-06-15 06:27:50]

On 2016/06/15 05:34:41, Yosi_BlinkOn_UTC2 wrote:
>
https://codereview.chromium.org/2020973002/diff/140001/content/renderer/rende...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/2020973002/diff/140001/content/renderer/rende...
> content/renderer/render_widget.cc:1511: void
> RenderWidget::adjustSelectionForBoundary(int& selectionStart,
> Please follow Chromimum coding style:
> http://dev.chromium.org/developers/coding-style
> Roughly similar to Google C++ coding style.
> 
> Major thing is you should use pointer for output parameter rather than
> reference.
> How about using |std::pair<int, int>>|?
> 
>
https://codereview.chromium.org/2020973002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):
> 
>
https://codereview.chromium.org/2020973002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:327: 
> Could you add DCHECK_XX() to validate |start| and |end| have valid values?
> 
>
https://codereview.chromium.org/2020973002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:403: const
> EphemeralRange selectedRange = PlainTextRange(start,
> end).createRange(*rootEditableElement);
> s/const EphemeralRange/const EphmeralRange&/
> to avoid copy.

yosin@, In order to add DCHECK, we need to get the right boundary in
InputMethodController. We can do it by creating 2 Position objects(like in patch
set 5), or by using TextIterator(like in patch set 6). Which one is better? By
the way, I think both of them are time-consuming.

[yosin_UTC9, 2016-06-15 19:00:21]

https://codereview.chromium.org/2020973002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2020973002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:327: 
On 2016/06/15 at 05:34:41, Yosi_BlinkOn_UTC2 wrote:
> Could you add DCHECK_XX() to validate |start| and |end| have valid values?

I mean

DCHECK_GE(selctionStart, 0);
DCHECK_LE(selectionStart, selectionEnd);

[yabinh, 2016-06-16 06:42:16]

yosin@, can you take another look of this patch? Thanks.

[yabinh, 2016-06-17 06:15:10]

https://codereview.chromium.org/2020973002/diff/180001/components/test_runner...
File components/test_runner/text_input_controller.cc (right):

https://codereview.chromium.org/2020973002/diff/180001/components/test_runner...
components/test_runner/text_input_controller.cc:262: // variable-length
characters.
Some test will fail because of this bug, like:

https://cs.corp.google.com/clankium/src/third_party/WebKit/LayoutTests/editin...

It didn't fail before because we used to do boundary control in
inputMethodController. So even though it returned an larger length, and exceeded
the right boundary, it could still work. But now we do boundary control in
render_widget, so the bug shows up.

By the way, there is no such bug in ImeTest, because we use std::string16.

[yabinh, 2016-06-17 06:22:24]

On 2016/06/17 06:15:10, yabinh wrote:
>
https://codereview.chromium.org/2020973002/diff/180001/components/test_runner...
> File components/test_runner/text_input_controller.cc (right):
> 
>
https://codereview.chromium.org/2020973002/diff/180001/components/test_runner...
> components/test_runner/text_input_controller.cc:262: // variable-length
> characters.
> Some test will fail because of this bug, like:
> 
>
https://cs.corp.google.com/clankium/src/third_party/WebKit/LayoutTests/editin...
> 
> It didn't fail before because we used to do boundary control in
> inputMethodController. So even though it returned an larger length, and
exceeded
> the right boundary, it could still work. But now we do boundary control in
> render_widget, so the bug shows up.
> 
> By the way, there is no such bug in ImeTest, because we use std::string16.

A typo. Should be "because we use base::string16"

[yabinh, 2016-06-17 06:26:34]

On 2016/06/17 06:15:10, yabinh wrote:
>
https://codereview.chromium.org/2020973002/diff/180001/components/test_runner...
> File components/test_runner/text_input_controller.cc (right):
> 
>
https://codereview.chromium.org/2020973002/diff/180001/components/test_runner...
> components/test_runner/text_input_controller.cc:262: // variable-length
> characters.
> Some test will fail because of this bug, like:
> 
>
https://cs.corp.google.com/clankium/src/third_party/WebKit/LayoutTests/editin...
> 
> It didn't fail before because we used to do boundary control in
> inputMethodController. So even though it returned an larger length, and
exceeded
> the right boundary, it could still work. But now we do boundary control in
> render_widget, so the bug shows up.
> 
> By the way, there is no such bug in ImeTest, because we use std::string16.

A typo. Should be "because we use base::string16"

[yabinh, 2016-06-30 11:03:54]

Patch set 13 is just rebasing.

[yosin_UTC9, 2016-07-01 09:23:38]

lgtm

Sorry for slow response. m(_ _)m

[yabinh, 2016-07-01 09:26:23]

On 2016/07/01 09:23:38, Yosi_UTC9 wrote:
> lgtm
> 
> Sorry for slow response. m(_ _)m

Thanks. :)

[yabinh, 2016-07-01 09:27:18]

The CQ bit was checked by yabinh@chromium.org

[yabinh, 2016-07-01 09:27:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
changwan@chromium.org, aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2020973002/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-01 09:27:34]

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, 2016-07-01 09:34:10]

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

[commit-bot: I haz the power, 2016-07-01 09:34:11]

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

[yabinh, 2016-07-01 09:53:03]

yabinh@chromium.org changed reviewers:
+ esprehn@chromium.org, tkent@chromium.org

[yabinh, 2016-07-01 09:53:04]

Add tkent@ for the owner of text_input_controller.cc, esprehn@ for the owner of
render_widget.

Can you take a look at this cl? Thanks.

[tkent, 2016-07-04 00:51:49]

https://codereview.chromium.org/2020973002/diff/200001/components/test_runner...
File components/test_runner/text_input_controller.cc (right):

https://codereview.chromium.org/2020973002/diff/200001/components/test_runner...
components/test_runner/text_input_controller.cc:264: size_t textLength =
base::string16(newText).length();
Doesn't newText.length() work?

[yabinh, 2016-07-04 01:03:22]

https://codereview.chromium.org/2020973002/diff/200001/components/test_runner...
File components/test_runner/text_input_controller.cc (right):

https://codereview.chromium.org/2020973002/diff/200001/components/test_runner...
components/test_runner/text_input_controller.cc:264: size_t textLength =
base::string16(newText).length();
On 2016/07/04 00:51:49, tkent wrote:
> Doesn't newText.length() work?

No, it doesn't.
For "عالم", its length is 4. But newText.length()==8.
Please see the comment in the previous patch.

[tkent, 2016-07-04 01:08:16]

https://codereview.chromium.org/2020973002/diff/200001/components/test_runner...
File components/test_runner/text_input_controller.cc (right):

https://codereview.chromium.org/2020973002/diff/200001/components/test_runner...
components/test_runner/text_input_controller.cc:264: size_t textLength =
base::string16(newText).length();
On 2016/07/04 at 01:03:22, yabinh wrote:
> On 2016/07/04 00:51:49, tkent wrote:
> > Doesn't newText.length() work?
> 
> No, it doesn't.
> For "عالم", its length is 4. But newText.length()==8.
> Please see the comment in the previous patch.

It shouldn't happen.  It implies WebString has a serious bug, and we need to
investigate it.

[yabinh, 2016-07-04 04:33:42]

On 2016/07/04 01:08:16, tkent wrote:
>
https://codereview.chromium.org/2020973002/diff/200001/components/test_runner...
> File components/test_runner/text_input_controller.cc (right):
> 
>
https://codereview.chromium.org/2020973002/diff/200001/components/test_runner...
> components/test_runner/text_input_controller.cc:264: size_t textLength =
> base::string16(newText).length();
> On 2016/07/04 at 01:03:22, yabinh wrote:
> > On 2016/07/04 00:51:49, tkent wrote:
> > > Doesn't newText.length() work?
> > 
> > No, it doesn't.
> > For "عالم", its length is 4. But newText.length()==8.
> > Please see the comment in the previous patch.
> 
> It shouldn't happen.  It implies WebString has a serious bug, and we need to
> investigate it.

You are right. I forgot it. text.length()==8, but newText.lenght()==4. Please
take a look at the latest patch.

[tkent, 2016-07-04 04:36:23]

test_runner lgtm

[yabinh, 2016-07-06 11:53:55]

https://codereview.chromium.org/2020973002/diff/240001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(left):

https://codereview.chromium.org/2020973002/diff/240001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:142:

I checked, and found it works for ReplicaInputConnection now.

https://codereview.chromium.org/2020973002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2020973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:2318: {
textInputInfo() is quite an expensive function, but it seems that we have no
choice.
See the discussion here:
https://codereview.chromium.org/1995333002/#msg9

https://codereview.chromium.org/2020973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:2380: 
adjustSelectionForBoundary is moved here, thus it can be reused by
WebViewImpl::confirmComposition. It will save a lot of trouble.
See the cl I am working on:
https://codereview.chromium.org/1995333002/

[Changwan Ryu, 2016-07-07 09:09:33]

https://codereview.chromium.org/2020973002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2020973002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:2318: {
On 2016/07/06 11:53:55, yabinh wrote:
> textInputInfo() is quite an expensive function, but it seems that we have no
> choice.
> See the discussion here:
> https://codereview.chromium.org/1995333002/#msg9

You should avoid calling textInputInfo(). textInputInfo() can take a up to 100ms
or more. See crbug.com/532519 for more details.

At the minimum, I think you can call an excerpt from textInputInfo():

EphemeralRange range =
focusedFrame->inputMethodController().compositionEphemeralRange();
if (range.isNotNull()) {
...
}

However, I don't think that you should be doing this here.

The current structure is as follows:

1. Do a predictive boundary check in WebViewImpl::adjustSelectionForBoundary().

But internally it's calling InputMethodController::compositionEphemeralRange()
because
selection boundary check has a dependency on the previous composition and the
new composition. The reason is that the new cursor position is relative to the
new composition region, not the old cursor position.

(See 
https://developer.android.com/reference/android/view/inputmethod/InputConnect...,
int) for more details.)

2. Then call InputMethodController with a valid parameter.


We assumed that parameter check doesn't depend on the internal state of IMC, but
it does. Also, it could have been much simpler if we do the boundary check after
the composition has already been made.

I'd say that selectionStart / selectionEnd are not absolute values you can
simply check, but they are complex relative values that can only be checked in
the middle of the process.

In this sense, I prefer the previous approach where you did the check inside
setComposingText / commitText().

yosin@, what do you think?

[yabinh, 2016-07-22 01:44:01]

Based on the previous discussion, it seems that we'd better do the check in
InputMethodController.
yosin@, what do you think?

[yabinh, 2016-07-29 01:50:11]

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

[commit-bot: I haz the power, 2016-07-29 01:50:25]

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

[yabinh, 2016-07-29 01:58:03]

Due to changwan@'s comment, we check the boundary in InputMethodController in
the latest patch(set 14). It's the same method we used in patch set 6.
yosin@, can you take another look?

[commit-bot: I haz the power, 2016-07-29 03:22:58]

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

[commit-bot: I haz the power, 2016-07-29 03:22:59]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2016-07-29 05:38:32]

https://codereview.chromium.org/2020973002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2020973002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:533:
rightBoundary -= (compositionRange()->endOffset() -
compositionRange()->startOffset());
nit: We don't need to have parenthesis in lhs.

Since |compositionRange()| can hold multiple nodes, |range->endOffset() -
range->startOffset()| can have meaning-less value.
Do you mean |range->text().length()|?

[yabinh, 2016-07-29 05:55:47]

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

[commit-bot: I haz the power, 2016-07-29 05:56:00]

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

[yabinh, 2016-07-29 05:57:40]

On 2016/07/29 05:38:32, Yosi_UTC9 wrote:
>
https://codereview.chromium.org/2020973002/diff/260001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):
> 
>
https://codereview.chromium.org/2020973002/diff/260001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:533:
> rightBoundary -= (compositionRange()->endOffset() -
> compositionRange()->startOffset());
> nit: We don't need to have parenthesis in lhs.
> 
> Since |compositionRange()| can hold multiple nodes, |range->endOffset() -
> range->startOffset()| can have meaning-less value.
> Do you mean |range->text().length()|?

Yes. It's better.
Please check the latest patch.

[commit-bot: I haz the power, 2016-07-29 07:30:51]

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

[commit-bot: I haz the power, 2016-07-29 07:30:52]

Dry run: This issue passed the CQ dry run.

[yabinh, 2016-08-02 09:45:45]

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

[commit-bot: I haz the power, 2016-08-02 09:46:00]

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

[yabinh, 2016-08-02 09:57:53]

The CQ bit was unchecked by yabinh@chromium.org

[yabinh, 2016-08-02 09:57:53]

The CQ bit was checked by yabinh@chromium.org

[yabinh, 2016-08-02 09:57:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
changwan@chromium.org, aelias@chromium.org, yosin@chromium.org,
tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/2020973002/#ps300001
(title: "add null check, and sync.")

[commit-bot: I haz the power, 2016-08-02 09:58: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, 2016-08-02 10:55:00]

Description was changed from

==========
Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) doesn't work fine when    
newCursorPosition != 1. It should behave the same as    
commitText("", newCursorPosition), because when the text is empty,    
commitText will call InputMethodController::setComposition, which is    
exactly the same as setComposingText().

BUG=570920
==========

to

==========
Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) doesn't work fine when    
newCursorPosition != 1. It should behave the same as    
commitText("", newCursorPosition), because when the text is empty,    
commitText will call InputMethodController::setComposition, which is    
exactly the same as setComposingText().

BUG=570920
==========

[commit-bot: I haz the power, 2016-08-02 10:55:02]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-08-02 10:58:05]

Description was changed from

==========
Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) doesn't work fine when    
newCursorPosition != 1. It should behave the same as    
commitText("", newCursorPosition), because when the text is empty,    
commitText will call InputMethodController::setComposition, which is    
exactly the same as setComposingText().

BUG=570920
==========

to

==========
Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) doesn't work fine when
newCursorPosition != 1. It should behave the same as
commitText("", newCursorPosition), because when the text is empty,
commitText will call InputMethodController::setComposition, which is
exactly the same as setComposingText().

BUG=570920

Committed: https://crrev.com/f22aa57c3e53c94264b0a0ac6e556ac03b89f372
Cr-Commit-Position: refs/heads/master@{#409169}
==========

[commit-bot: I haz the power, 2016-08-02 10:58:06]

Patchset 16 (id:??) landed as
https://crrev.com/f22aa57c3e53c94264b0a0ac6e556ac03b89f372
Cr-Commit-Position: refs/heads/master@{#409169}

[Changwan Ryu, 2016-08-10 00:54:58]

A revert of this CL (patchset #16 id:300001) has been created in
https://codereview.chromium.org/2233573002/ by changwan@chromium.org.

The reason for reverting is: This caused a regression
(https://crbug.com/633840), and there doesn't seem to be an easy fix..

[yabinh, 2016-08-23 00:58:07]

Description was changed from

==========
Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) doesn't work fine when
newCursorPosition != 1. It should behave the same as
commitText("", newCursorPosition), because when the text is empty,
commitText will call InputMethodController::setComposition, which is
exactly the same as setComposingText().

BUG=570920

Committed: https://crrev.com/f22aa57c3e53c94264b0a0ac6e556ac03b89f372
Cr-Commit-Position: refs/heads/master@{#409169}
==========

to

==========
Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) doesn't work fine when
newCursorPosition != 1. It should behave the same as
commitText("", newCursorPosition), because when the text is empty,
commitText will call InputMethodController::setComposition, which is
exactly the same as setComposingText().

BUG=570920

Committed: https://crrev.com/f22aa57c3e53c94264b0a0ac6e556ac03b89f372
Cr-Commit-Position: refs/heads/master@{#409169}
==========

[yabinh, 2016-08-23 01:00:21]

Description was changed from

==========
Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) doesn't work fine when
newCursorPosition != 1. It should behave the same as
commitText("", newCursorPosition), because when the text is empty,
commitText will call InputMethodController::setComposition, which is
exactly the same as setComposingText().

BUG=570920

Committed: https://crrev.com/f22aa57c3e53c94264b0a0ac6e556ac03b89f372
Cr-Commit-Position: refs/heads/master@{#409169}
==========

to

==========
Reland: Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) should cancel the 
composition (if any), and move the cursor according to
|newCursorPosition|.

Note that we shouldn't close typing when moving the cursor in this
case. In other cases, like in 
InputMethodController::confirmCompositionOrInsertText, we should
close typing when moving the cursor. Thus, we need an extra parameter.

This is a reland of https://codereview.chromium.org/2020973002/. The
previous CL caused a bug crbug.com/633840.

BUG=570920, 633840
==========

[yabinh, 2016-08-23 01:02:38]

Description was changed from

==========
Reland: Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) should cancel the 
composition (if any), and move the cursor according to
|newCursorPosition|.

Note that we shouldn't close typing when moving the cursor in this
case. In other cases, like in 
InputMethodController::confirmCompositionOrInsertText, we should
close typing when moving the cursor. Thus, we need an extra parameter.

This is a reland of https://codereview.chromium.org/2020973002/. The
previous CL caused a bug crbug.com/633840.

BUG=570920, 633840
==========

to

==========
Reland: Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) should cancel the 
composition (if any), and move the cursor according to
|newCursorPosition|.

Note that we shouldn't close typing when moving the cursor in this
case. In other cases, like in 
InputMethodController::confirmCompositionOrInsertText, we should
close typing when moving the cursor. Thus, we need an extra parameter.

This is a reland CL. The previous CL caused a bug
crbug.com/633840.

BUG=570920, 633840
==========

[yabinh, 2016-08-23 01:11:04]

The CQ bit was checked by yabinh@chromium.org

[yabinh, 2016-08-23 01:11:05]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
yosin@chromium.org, aelias@chromium.org, changwan@chromium.org
Link to the patchset: https://codereview.chromium.org/2020973002/#ps320001
(title: "Reland. Add a parameter.")

[commit-bot: I haz the power, 2016-08-23 01:12:01]

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, 2016-08-23 05:43:55]

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

[commit-bot: I haz the power, 2016-08-23 05:43:57]

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

[yabinh, 2016-08-23 05:51:53]

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

[commit-bot: I haz the power, 2016-08-23 05:52:12]

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, 2016-08-23 07:05:02]

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

[commit-bot: I haz the power, 2016-08-23 07:05:03]

Dry run: This issue passed the CQ dry run.

[yabinh, 2016-08-23 07:05:14]

The CQ bit was checked by yabinh@chromium.org

[commit-bot: I haz the power, 2016-08-23 07:05:32]

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, 2016-08-23 07:09:18]

Description was changed from

==========
Reland: Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) should cancel the 
composition (if any), and move the cursor according to
|newCursorPosition|.

Note that we shouldn't close typing when moving the cursor in this
case. In other cases, like in 
InputMethodController::confirmCompositionOrInsertText, we should
close typing when moving the cursor. Thus, we need an extra parameter.

This is a reland CL. The previous CL caused a bug
crbug.com/633840.

BUG=570920, 633840
==========

to

==========
Reland: Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) should cancel the 
composition (if any), and move the cursor according to
|newCursorPosition|.

Note that we shouldn't close typing when moving the cursor in this
case. In other cases, like in 
InputMethodController::confirmCompositionOrInsertText, we should
close typing when moving the cursor. Thus, we need an extra parameter.

This is a reland CL. The previous CL caused a bug
crbug.com/633840.

BUG=570920, 633840
==========

[commit-bot: I haz the power, 2016-08-23 07:09:20]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-08-23 07:10:46]

Description was changed from

==========
Reland: Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) should cancel the 
composition (if any), and move the cursor according to
|newCursorPosition|.

Note that we shouldn't close typing when moving the cursor in this
case. In other cases, like in 
InputMethodController::confirmCompositionOrInsertText, we should
close typing when moving the cursor. Thus, we need an extra parameter.

This is a reland CL. The previous CL caused a bug
crbug.com/633840.

BUG=570920, 633840
==========

to

==========
Reland: Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) should cancel the
composition (if any), and move the cursor according to
|newCursorPosition|.

Note that we shouldn't close typing when moving the cursor in this
case. In other cases, like in
InputMethodController::confirmCompositionOrInsertText, we should
close typing when moving the cursor. Thus, we need an extra parameter.

This is a reland CL. The previous CL caused a bug
crbug.com/633840.

BUG=570920, 633840

Committed: https://crrev.com/dd6d1b62de7b449d997d159d63207abe87d3cb46
Cr-Commit-Position: refs/heads/master@{#413683}
==========

[commit-bot: I haz the power, 2016-08-23 07:10:47]

Patchset 17 (id:??) landed as
https://crrev.com/dd6d1b62de7b449d997d159d63207abe87d3cb46
Cr-Commit-Position: refs/heads/master@{#413683}