[Android] Remove zero length composing spans.

Remove zero length composing spans.

This fixes the following logcat error:
E/SpannableStringBuilder(14995): SPAN_EXCLUSIVE_EXCLUSIVE spans cannot have a zero length

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

[aurimas (slooooooooow), 2013-04-09 17:09:25]

You should open a bug first and then set BUG=BUG_NUMBER_IN_CRBUG

https://codereview.chromium.org/13820016/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
(right):

https://codereview.chromium.org/13820016/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:575:
mPendingUpdate = false;
Can you explain this PendingUpdate flag and why we need it?

[Fredrik Öhrn, 2013-04-10 08:58:11]

Update suppression moved to https://codereview.chromium.org/13909003

[aurimas (slooooooooow), 2013-04-10 14:24:42]

LGTM

+nileshagrawal for OWNERs approval.

[nilesh, 2013-04-10 16:19:04]

LGTM with nit.

https://codereview.chromium.org/13820016/diff/2003/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
(right):

https://codereview.chromium.org/13820016/diff/2003/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:554:
removeComposingSpans(getEditable());
Is there a reason to not use the local var 'editable' instead of calling
getEditable() ?

[Fredrik Öhrn, 2013-04-10 16:23:08]

https://codereview.chromium.org/13820016/diff/2003/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
(right):

https://codereview.chromium.org/13820016/diff/2003/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:554:
removeComposingSpans(getEditable());
On 2013/04/10 16:19:05, nilesh wrote:
> Is there a reason to not use the local var 'editable' instead of calling
> getEditable() ?

Copy and paste laziness on my part, will fix.

[Fredrik Öhrn, 2013-04-10 17:29:08]

getEditable() replaced.

[nilesh, 2013-04-10 17:30:00]

LGTM

[Fredrik Öhrn, 2013-04-12 08:40:47]

On 2013/04/10 17:30:00, nilesh wrote:
> LGTM

Now that it's approved what's the next step? (I'm a noob.)

[aurimas (slooooooooow), 2013-04-12 13:55:12]

On 2013/04/12 08:40:47, Fredrik Öhrn wrote:
> On 2013/04/10 17:30:00, nilesh wrote:
> > LGTM
> 
> Now that it's approved what's the next step? (I'm a noob.)

You check the "Commit:" checkbox in this page underneath the list of the files
in this patch. This will put your patch to a commit queue. Commit queue will run
all the try bots (tests on all platforms). Once all of them pass, it will get
committed to the repository. 

Ideally it would be smart enough to only run Android tests since you are only
changing Android code, but its not that smart and it will run everything.

[commit-bot: I haz the power, 2013-04-12 13:56:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ohrn@opera.com/13820016/13001

[commit-bot: I haz the power, 2013-04-12 13:56:43]

Failed to apply patch for
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
  Hunk #1 FAILED at 549.
  Hunk #2 FAILED at 742.
  2 out of 2 hunks FAILED -- saving rejects to file
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java.rej

Patch:      
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
Index:
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
diff --git
a/content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
b/content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
index
1f73502d70a17ff4705137121f602660c23553eb..8bd96368e208ba0b5fd7473e416a60c4c3a2c79a
100644
---
a/content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
+++
b/content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
@@ -549,7 +549,12 @@ class ImeAdapter {
             }
 
             Selection.setSelection(editable, selectionStart, selectionEnd);
-            super.setComposingRegion(compositionStart, compositionEnd);
+
+            if (compositionStart == compositionEnd) {
+                removeComposingSpans(editable);
+            } else {
+                super.setComposingRegion(compositionStart, compositionEnd);
+            }
 
             if (mIgnoreTextInputStateUpdates) return;
             updateSelection(selectionStart, selectionEnd, compositionStart,
compositionEnd);
@@ -742,7 +747,11 @@ class ImeAdapter {
             if (DEBUG) Log.w(TAG, "setComposingRegion [" + start + " " + end +
"]");
             int a = Math.min(start, end);
             int b = Math.max(start, end);
-            super.setComposingRegion(a, b);
+            if (a == b) {
+                removeComposingSpans(getEditable());
+            } else {
+                super.setComposingRegion(a, b);
+            }
             return mImeAdapter.setComposingRegion(a, b);
         }

[aurimas (slooooooooow), 2013-04-12 14:00:12]

On 2013/04/12 13:56:43, I haz the power (commit-bot) wrote:
> Failed to apply patch for
> content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   patching file
> content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
>   Hunk #1 FAILED at 549.
>   Hunk #2 FAILED at 742.
>   2 out of 2 hunks FAILED -- saving rejects to file
>
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java.rej
> 
> Patch:      
> content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
> Index:
> content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
> diff --git
> a/content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
> b/content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
> index
>
1f73502d70a17ff4705137121f602660c23553eb..8bd96368e208ba0b5fd7473e416a60c4c3a2c79a
> 100644
> ---
> a/content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
> +++
> b/content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
> @@ -549,7 +549,12 @@ class ImeAdapter {
>              }
>  
>              Selection.setSelection(editable, selectionStart, selectionEnd);
> -            super.setComposingRegion(compositionStart, compositionEnd);
> +
> +            if (compositionStart == compositionEnd) {
> +                removeComposingSpans(editable);
> +            } else {
> +                super.setComposingRegion(compositionStart, compositionEnd);
> +            }
>  
>              if (mIgnoreTextInputStateUpdates) return;
>              updateSelection(selectionStart, selectionEnd, compositionStart,
> compositionEnd);
> @@ -742,7 +747,11 @@ class ImeAdapter {
>              if (DEBUG) Log.w(TAG, "setComposingRegion [" + start + " " + end
+
> "]");
>              int a = Math.min(start, end);
>              int b = Math.max(start, end);
> -            super.setComposingRegion(a, b);
> +            if (a == b) {
> +                removeComposingSpans(getEditable());
> +            } else {
> +                super.setComposingRegion(a, b);
> +            }
>              return mImeAdapter.setComposingRegion(a, b);
>          }

Bah! This is my fault. You need to rebase your patch because I moved
ImeAdapter.java.

[Fredrik Öhrn, 2013-04-12 14:15:09]

On 2013/04/12 14:00:12, aurimas wrote:
> 
> Bah! This is my fault. You need to rebase your patch because I moved
> ImeAdapter.java.

Np, rebased.

[aurimas (slooooooooow), 2013-04-12 14:21:05]

On 2013/04/12 14:15:09, Fredrik Öhrn wrote:
> On 2013/04/12 14:00:12, aurimas wrote:
> > 
> > Bah! This is my fault. You need to rebase your patch because I moved
> > ImeAdapter.java.
> 
> Np, rebased.

You have to check the Commit button again.

[commit-bot: I haz the power, 2013-04-12 14:22:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ohrn@opera.com/13820016/20001

[Fredrik Öhrn, 2013-04-12 14:23:36]

On 2013/04/12 14:21:05, aurimas wrote:
>
> You have to check the Commit button again.

Ok, I assumed a new review approval was needed.

[aurimas (slooooooooow), 2013-04-12 14:29:13]

On 2013/04/12 14:23:36, Fredrik Öhrn wrote:
> On 2013/04/12 14:21:05, aurimas wrote:
> >
> > You have to check the Commit button again.
> 
> Ok, I assumed a new review approval was needed.

The review system does require to get another LGTM once you get one (a little
scary, right?). The general rule is if you patch changed a lot, you ping the
reviewer to look at it again. However, if it is a rebase or a small nit fix, you
can land it without asking them again.

[commit-bot: I haz the power, 2013-04-12 16:17:15]

Change committed as 193948