Allow some InputConnection methods to be called on UI thread

Allow some InputConnection methods to be called on UI thread

Assertions are removed from some of the InputConnection methods that
merely post on UI thread. In doing so, mPendingAccent was changed to be
accessed only on UI thread.

However, beginBatchEdit() / endBatchEdit() implementations are quite
complicated and no evidence calling those on UI thread was found so far.

Also, for get* methods we return cached results.

BUG=643477
Committed: https://crrev.com/c8a5f86ac17908905756c36a9d4ff87439401994
Cr-Commit-Position: refs/heads/master@{#417499}

[Changwan Ryu, 2016-09-02 07:27:51]

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

[commit-bot: I haz the power, 2016-09-02 07:28:06]

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

[Changwan Ryu, 2016-09-02 07:28:12]

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

[commit-bot: I haz the power, 2016-09-02 08:04:48]

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

[commit-bot: I haz the power, 2016-09-02 08:04:49]

Dry run: This issue passed the CQ dry run.

[aelias_OOO_until_Jul13, 2016-09-03 08:22:31]

https://codereview.chromium.org/2299913003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
(right):

https://codereview.chromium.org/2299913003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:94:
private final AtomicInteger mNumNestedBatchEdits = new AtomicInteger();
Instead of making this atomic, I'd prefer to slap "synchronized" on the relevant
methods in this class.  I don't want to have to reason about interleaved
execution.

https://codereview.chromium.org/2299913003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:214:
return mCachedTextInputState;
Can we try returning an empty default one instead?  I don't want this to even
seem to work.  I'd like to try breaking whatever minor feature relies on this,
without crashing, so that users aren't overly disturbed, and LG is forced to
stop doing this to fix their minor feature.  And nobody ever tries to do this
again.

[aelias_OOO_until_Jul13, 2016-09-03 08:29:49]

https://codereview.chromium.org/2299913003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
(right):

https://codereview.chromium.org/2299913003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:94:
private final AtomicInteger mNumNestedBatchEdits = new AtomicInteger();
On 2016/09/03 at 08:22:31, aelias wrote:
> Instead of making this atomic, I'd prefer to slap "synchronized" on the
relevant methods in this class.  I don't want to have to reason about
interleaved execution.

On second thought, I'd prefer that you simply keep the thread assertions on the
batch edit methods.  There is not evidence that there is existing use of them on
the UI thread, so why loosen it?

I'm cool with removing the assertion for all the methods that simply post an
async task on the UI thread.  But for those that do something more complicated,
let's try and maintain the guarantees as tight as we can.

[Changwan Ryu, 2016-09-05 07:10:19]

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

[commit-bot: I haz the power, 2016-09-05 07:10:31]

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

[Changwan Ryu, 2016-09-05 07:12:47]

Description was changed from

==========
Allow InputConnection methods to be called on UI thread

Assertions are removed from all the InputConnection methods. This does
not affect methods such as setSelection() which merely post on the
UI thread. But I had to fix some implementations to make this work
correctly as follows:

- change mPendingAccent to be accessed only on UI thread
- return cached results for get* methods
- change mNumNestedBatchEdits to AtomicInteger and allow it to be accessed
  on multiple threads

BUG=643477
==========

to

==========
Allow some InputConnection methods to be called on UI thread

Assertions are removed from some of the InputConnection methods. This does
not affect methods such as setSelection() which merely post on the
UI thread. To make this happen, we change mPendingAccent to be accessed
only on UI thread.

However, beginBatchEdit() / endBatchEdit() implementations are quite
complicated and no evidence calling those on UI thread found so far.

Also, get* methods cannot be accessed on UI thread. We simply return null.

BUG=643477
==========

[Changwan Ryu, 2016-09-05 07:13:15]

On 2016/09/03 08:29:49, aelias wrote:
>
https://codereview.chromium.org/2299913003/diff/1/content/public/android/java...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
> (right):
> 
>
https://codereview.chromium.org/2299913003/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:94:
> private final AtomicInteger mNumNestedBatchEdits = new AtomicInteger();
> On 2016/09/03 at 08:22:31, aelias wrote:
> > Instead of making this atomic, I'd prefer to slap "synchronized" on the
> relevant methods in this class.  I don't want to have to reason about
> interleaved execution.
> 
> On second thought, I'd prefer that you simply keep the thread assertions on
the
> batch edit methods.  There is not evidence that there is existing use of them
on
> the UI thread, so why loosen it?
> 
> I'm cool with removing the assertion for all the methods that simply post an
> async task on the UI thread.  But for those that do something more
complicated,
> let's try and maintain the guarantees as tight as we can.

Sounds good to me. Done as suggested.

[Changwan Ryu, 2016-09-05 07:40:16]

Description was changed from

==========
Allow some InputConnection methods to be called on UI thread

Assertions are removed from some of the InputConnection methods. This does
not affect methods such as setSelection() which merely post on the
UI thread. To make this happen, we change mPendingAccent to be accessed
only on UI thread.

However, beginBatchEdit() / endBatchEdit() implementations are quite
complicated and no evidence calling those on UI thread found so far.

Also, get* methods cannot be accessed on UI thread. We simply return null.

BUG=643477
==========

to

==========
Allow some InputConnection methods to be called on UI thread

Assertions are removed from some of the InputConnection methods that
merely post on UI thread. In doing so, mPendingAccent was changed to be
accessed only on UI thread.

However, beginBatchEdit() / endBatchEdit() implementations are quite
complicated and no evidence calling those on UI thread was found so far.

Also, get* methods cannot be accessed on UI thread. We simply return null
in this case.

BUG=643477
==========

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

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

[commit-bot: I haz the power, 2016-09-05 08:23:24]

Dry run: This issue passed the CQ dry run.

[aelias_OOO_until_Jul13, 2016-09-07 03:12:32]

https://codereview.chromium.org/2299913003/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
(right):

https://codereview.chromium.org/2299913003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:211:
return null;
OK, I think we can't get away with this after reading the LG crash stack and the
description of what they're doing in http://crbug.com/637242#c14.  The stack
indicates their actionMode implementation is checking whether selection is
nonempty in order to start cut/copy operation.  The likely result of returning
null here is that copy/cut stops doing anything at all, which is too serious a
bug to ship.

Please bring back the cached text for now.  For the longer run, I've filed
http://crbug.com/644590 to eliminate the need for LG to be doing this at all
(and in medium run, maybe we can also discuss with them a better way to detect
if selection exists).

[Changwan Ryu, 2016-09-07 06:19:42]

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

[commit-bot: I haz the power, 2016-09-07 06:19:52]

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

[Changwan Ryu, 2016-09-07 06:20:21]

https://codereview.chromium.org/2299913003/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java
(right):

https://codereview.chromium.org/2299913003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:211:
return null;
On 2016/09/07 03:12:32, aelias wrote:
> OK, I think we can't get away with this after reading the LG crash stack and
the
> description of what they're doing in http://crbug.com/637242#c14.  The stack
> indicates their actionMode implementation is checking whether selection is
> nonempty in order to start cut/copy operation.  The likely result of returning
> null here is that copy/cut stops doing anything at all, which is too serious a
> bug to ship.
> 
> Please bring back the cached text for now.  For the longer run, I've filed
> http://crbug.com/644590 to eliminate the need for LG to be doing this at all
> (and in medium run, maybe we can also discuss with them a better way to detect
> if selection exists).

Done.

[Changwan Ryu, 2016-09-07 06:23:17]

Description was changed from

==========
Allow some InputConnection methods to be called on UI thread

Assertions are removed from some of the InputConnection methods that
merely post on UI thread. In doing so, mPendingAccent was changed to be
accessed only on UI thread.

However, beginBatchEdit() / endBatchEdit() implementations are quite
complicated and no evidence calling those on UI thread was found so far.

Also, get* methods cannot be accessed on UI thread. We simply return null
in this case.

BUG=643477
==========

to

==========
Allow some InputConnection methods to be called on UI thread

Assertions are removed from some of the InputConnection methods that
merely post on UI thread. In doing so, mPendingAccent was changed to be
accessed only on UI thread.

However, beginBatchEdit() / endBatchEdit() implementations are quite
complicated and no evidence calling those on UI thread was found so far.

Also, for get* methods we return cached results.

BUG=643477
==========

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

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

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

Dry run: This issue passed the CQ dry run.

[Changwan Ryu, 2016-09-09 02:29:03]

ping. Since we need a signed build to test against OEM apps,
we may need to try different versions and identify problems asap.

PTAL.

[aelias_OOO_until_Jul13, 2016-09-09 03:09:38]

lgtm

[Changwan Ryu, 2016-09-09 03:11:39]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2016-09-09 03:12:12]

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-09-09 04:05:06]

Description was changed from

==========
Allow some InputConnection methods to be called on UI thread

Assertions are removed from some of the InputConnection methods that
merely post on UI thread. In doing so, mPendingAccent was changed to be
accessed only on UI thread.

However, beginBatchEdit() / endBatchEdit() implementations are quite
complicated and no evidence calling those on UI thread was found so far.

Also, for get* methods we return cached results.

BUG=643477
==========

to

==========
Allow some InputConnection methods to be called on UI thread

Assertions are removed from some of the InputConnection methods that
merely post on UI thread. In doing so, mPendingAccent was changed to be
accessed only on UI thread.

However, beginBatchEdit() / endBatchEdit() implementations are quite
complicated and no evidence calling those on UI thread was found so far.

Also, for get* methods we return cached results.

BUG=643477
==========

[commit-bot: I haz the power, 2016-09-09 04:05:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-09 04:08:13]

Description was changed from

==========
Allow some InputConnection methods to be called on UI thread

Assertions are removed from some of the InputConnection methods that
merely post on UI thread. In doing so, mPendingAccent was changed to be
accessed only on UI thread.

However, beginBatchEdit() / endBatchEdit() implementations are quite
complicated and no evidence calling those on UI thread was found so far.

Also, for get* methods we return cached results.

BUG=643477
==========

to

==========
Allow some InputConnection methods to be called on UI thread

Assertions are removed from some of the InputConnection methods that
merely post on UI thread. In doing so, mPendingAccent was changed to be
accessed only on UI thread.

However, beginBatchEdit() / endBatchEdit() implementations are quite
complicated and no evidence calling those on UI thread was found so far.

Also, for get* methods we return cached results.

BUG=643477

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

[commit-bot: I haz the power, 2016-09-09 04:08:15]

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