Adding missing UpdateTextInputState calls after each ime event.

Adding missing UpdateTextInputState calls after each ime event.

Adding the missing UpdateTextInputState calls after each ime event and
preventing updates midway through ime event.

Added missing calls will start sending UpdateSelection updates to the
IMEs.

i.e. confirmComposition can cause multiple didChangeSelection calls but
we only care about the last one.

BUG=172845
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182534

[aurimas (slooooooooow), 2013-01-30 19:15:36]

Hello Seigo,

Could you take a look at this CL?

Thanks,
Aurimas

[aurimas (slooooooooow), 2013-01-30 22:31:58]

Hey Yusuf,

PTAL.

Aurimas

[Seigo Nonaka, 2013-01-31 06:19:14]

lgtm

lgtm

I think this patch is safe for desktop text input.
Just in case, I checked on my lumpy device and works fine including Google
drive.

https://codereview.chromium.org/12093068/diff/3001/content/renderer/render_wi...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/12093068/diff/3001/content/renderer/render_wi...
content/renderer/render_widget.h:541: // Are we currently handling an ime event?
We must call UpdateTextInputState every time after |handling_ime_event_| flip
from true to false, right?

If so, please add comment.

[aurimas (slooooooooow), 2013-01-31 06:31:13]

https://codereview.chromium.org/12093068/diff/3001/content/renderer/render_wi...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/12093068/diff/3001/content/renderer/render_wi...
content/renderer/render_widget.h:541: // Are we currently handling an ime event?
On 2013/01/31 06:19:14, Seigo Nonaka wrote:
> We must call UpdateTextInputState every time after |handling_ime_event_| flip
> from true to false, right?
> 
> If so, please add comment.

This bool is just used to make sure that we don't send UpdateInputState during
the handing_ime_event. For example confirmComposition has an in-between stage
where the word in composition is deleted and then a final state when it gets
added fully (all this happens in Editor.cpp).

[Yusuf, 2013-01-31 06:53:14]

lgtm

[aurimas (slooooooooow), 2013-01-31 17:30:31]

Hello James,

Could you take a look at this change please?

I understand that you would like a test of it, but this bug only occurs when
using mozc (japanese IME) and our tests bots use latin ime. We currently do not
have an infrastructure set up to test different IMEs.

Aurimas

[jamesr, 2013-02-01 00:22:10]

https://codereview.chromium.org/12093068/diff/3001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12093068/diff/3001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:1424: handling_ime_event_ = true;
What does this do? I can't find any code that consults the value of this bool,
so why are we setting/resetting it?  It looks like a reentrancy guard, what's
reentrant?

[jamesr, 2013-02-01 00:26:43]

I don't feel at all confident in this patch without tests.  If you can find
another owner who is familiar enough with this code to approve it, feel free,
but this is complicated enough, unfamiliar enough to me and broken often enough
that I'm not going to approve any further untested changes.

https://codereview.chromium.org/12093068/diff/3001/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/12093068/diff/3001/content/renderer/render_wi...
content/renderer/render_widget.cc:1853: if (handling_ime_event_)
ah sorry, missed this file.

This is a bit fragile sounding to me.  What are the possible sources of
reentrancy here?  How do we know that it's safe to ignore all intermediate
states?

[aurimas (slooooooooow), 2013-02-01 00:30:03]

https://chromiumcodereview.appspot.com/12093068/diff/3001/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/12093068/diff/3001/content/renderer/re...
content/renderer/render_view_impl.cc:1424: handling_ime_event_ = true;
On 2013/02/01 00:22:10, jamesr wrote:
> What does this do? I can't find any code that consults the value of this bool,
> so why are we setting/resetting it?  It looks like a reentrancy guard, what's
> reentrant?

This is used in RenderWidget::UpdateTextInputState(), because
UpdateTextInputState can get called during an IME event (some of them are
multi-step internally) through didChangeSelection.

[aurimas (slooooooooow), 2013-02-01 00:44:59]

Hey James,

Could you take a look at this patch please?

Thanks,
Aurimas

[James Su, 2013-02-01 04:18:15]

LGTM.
This patch looks safe to me, though I didn't test it.

On 2013/02/01 00:44:59, aurimas wrote:
> Hey James,
> 
> Could you take a look at this patch please?
> 
> Thanks,
> Aurimas

[aurimas (slooooooooow), 2013-02-06 00:45:47]

Hey Ted,

Could you take a look at my Android changes in this CL?

Thanks,
Aurimas

[Ted C, 2013-02-06 00:59:00]

mainly nits...question about the clear focus business though

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
(left):

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:534:
// When WebKit changes the editable field, both the start and the end positions
for
Is this comment applicable for the moved block below still?

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:464:
static public class AdapterInputConnectionFactory {
public should come before static...sadly this whole file is messed up.

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
File
content/public/android/javatests/src/org/chromium/content/browser/ImeTest.java
(right):

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
content/public/android/javatests/src/org/chromium/content/browser/ImeTest.java:137:
static public class TestAdapterInputConnectionFactory extends
public before static....and these should probably be private classes

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
content/public/android/javatests/src/org/chromium/content/browser/ImeTest.java:138:
ImeAdapter.AdapterInputConnectionFactory {
indent +4

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
content/public/android/javatests/src/org/chromium/content/browser/ImeTest.java:146:
static public class TestAdapterInputConnection extends
ImeAdapter.AdapterInputConnection {
same comment as above

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
content/public/android/javatests/src/org/chromium/content/browser/ImeTest.java:147:
public int mSetEditableTextCallCounter;
I would make all these protected.

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/shell/andr...
File content/shell/android/java/src/org/chromium/content_shell/Shell.java
(right):

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/shell/andr...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:149:
mContentView.clearFocus();
why is clearFocus/requestFocus required?  Add comment

[aurimas (slooooooooow), 2013-02-06 02:25:44]

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
(left):

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:534:
// When WebKit changes the editable field, both the start and the end positions
for
On 2013/02/06 00:59:00, Ted C wrote:
> Is this comment applicable for the moved block below still?

This is no longer relevant after
https://chromiumcodereview.appspot.com/12088095/ Each time when webkit changes
the text without IME knowing, we will get cancelComposition()

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:464:
static public class AdapterInputConnectionFactory {
On 2013/02/06 00:59:00, Ted C wrote:
> public should come before static...sadly this whole file is messed up.

Done.

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
File
content/public/android/javatests/src/org/chromium/content/browser/ImeTest.java
(right):

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
content/public/android/javatests/src/org/chromium/content/browser/ImeTest.java:137:
static public class TestAdapterInputConnectionFactory extends
On 2013/02/06 00:59:00, Ted C wrote:
> public before static....and these should probably be private classes

Done.

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
content/public/android/javatests/src/org/chromium/content/browser/ImeTest.java:138:
ImeAdapter.AdapterInputConnectionFactory {
On 2013/02/06 00:59:00, Ted C wrote:
> indent +4

Done.

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
content/public/android/javatests/src/org/chromium/content/browser/ImeTest.java:146:
static public class TestAdapterInputConnection extends
ImeAdapter.AdapterInputConnection {
On 2013/02/06 00:59:00, Ted C wrote:
> same comment as above

Done.

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/public/and...
content/public/android/javatests/src/org/chromium/content/browser/ImeTest.java:147:
public int mSetEditableTextCallCounter;
On 2013/02/06 00:59:00, Ted C wrote:
> I would make all these protected.

Done.

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/shell/andr...
File content/shell/android/java/src/org/chromium/content_shell/Shell.java
(right):

https://chromiumcodereview.appspot.com/12093068/diff/16001/content/shell/andr...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:149:
mContentView.clearFocus();
On 2013/02/06 00:59:00, Ted C wrote:
> why is clearFocus/requestFocus required?  Add comment

Done.

[Ted C, 2013-02-06 02:29:21]

lgtm

https://chromiumcodereview.appspot.com/12093068/diff/16002/content/shell/andr...
File content/shell/android/java/src/org/chromium/content_shell/Shell.java
(right):

https://chromiumcodereview.appspot.com/12093068/diff/16002/content/shell/andr...
content/shell/android/java/src/org/chromium/content_shell/Shell.java:149: //
Remove this when crbug.com/174541 is fixed.
Make this

// TODO(aurimas): Remove ...

[aurimas (slooooooooow), 2013-02-06 02:35:08]

Hello James (jamesr),

Could you take a look at this CL again? I have added a test that will check for
the conditions that caused the bug in the description.

Thanks,
Aurimas

[aurimas (slooooooooow), 2013-02-12 22:22:30]

On 2013/02/06 02:35:08, aurimas wrote:
> Hello James (jamesr),
> 
> Could you take a look at this CL again? I have added a test that will check
for
> the conditions that caused the bug in the description.
> 
> Thanks,
> Aurimas

jamesr: ping

Did you get a chance to look at this CL?

[jamesr, 2013-02-12 22:54:34]

sorry, I missed it. LGTM with one comment

https://codereview.chromium.org/12093068/diff/23005/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/12093068/diff/23005/content/renderer/render_w...
content/renderer/render_widget.cc:1563: handling_ime_event_ = true;
can you put an RAII object on the stack to be responsible for resetting this?
this function is fairly long and it'd be somewhat easy for someone to add an
early return and accidentally break it.  perhaps DCHECK(!handling_ime_event_)
and then use a base::AutoReset<bool> would do the trick?

[aurimas (slooooooooow), 2013-02-13 00:35:50]

jamesr: should I add AutoReset<bool> in other methods too?

https://codereview.chromium.org/12093068/diff/23005/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/12093068/diff/23005/content/renderer/render_w...
content/renderer/render_widget.cc:1563: handling_ime_event_ = true;
On 2013/02/12 22:54:34, jamesr wrote:
> can you put an RAII object on the stack to be responsible for resetting this?
> this function is fairly long and it'd be somewhat easy for someone to add an
> early return and accidentally break it.  perhaps DCHECK(!handling_ime_event_)
> and then use a base::AutoReset<bool> would do the trick?

Done.

[jamesr, 2013-02-13 00:40:10]

On 2013/02/13 00:35:50, aurimas wrote:
> jamesr: should I add AutoReset<bool> in other methods too?

Yes, it'll be shorter (just one line instead of two) and a bit more obvious IMO.

[aurimas (slooooooooow), 2013-02-13 01:08:06]

Hey David,

Could you look over my Java changes to make sure that it looks ok before I land?

Aurimas

[David Trainor- moved to gerrit, 2013-02-13 01:27:19]

lgtm I couldn't see anything obviously wrong with it.

[commit-bot: I haz the power, 2013-02-13 01:28:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/12093068/30006

[commit-bot: I haz the power, 2013-02-13 11:55:49]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-02-13 15:36:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/12093068/30006

[commit-bot: I haz the power, 2013-02-14 20:58:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/12093068/38002

[commit-bot: I haz the power, 2013-02-14 21:00:54]

Change committed as 182534