Bug 805162 - l. Avoid unnecessarily setting selection, which can lead to side-effects in automated tests; r=cpeterson
authorJim Chen <nchen@mozilla.com>
Thu, 01 Nov 2012 16:11:03 -0400
changeset 112079 320a9b77053b9f91a95688c8606bf603ac1dc911
parent 112078 abda35a08bd9b2cca100c08f034d81e9e16a9ed4
child 112080 3ff61da528fa92d9652f7d69b826233918994a1c
push id23790
push userryanvm@gmail.com
push dateFri, 02 Nov 2012 01:26:40 +0000
treeherdermozilla-central@556b9cfb269f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscpeterson
bugs805162
milestone19.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 805162 - l. Avoid unnecessarily setting selection, which can lead to side-effects in automated tests; r=cpeterson
mobile/android/base/GeckoEditable.java
widget/android/nsWindow.cpp
--- a/mobile/android/base/GeckoEditable.java
+++ b/mobile/android/base/GeckoEditable.java
@@ -58,19 +58,19 @@ final class GeckoEditable
     private InputFilter[] mFilters;
 
     private final SpannableStringBuilder mText;
     private final Editable mProxy;
     private GeckoEditableListener mListener;
     private final ActionQueue mActionQueue;
 
     private int mSavedSelectionStart;
-    private int mSelectionSeqno;
-    private int mCompositionSeqno;
-    private int mLastUpdatedCompositionSeqno;
+    private volatile int mGeckoUpdateSeqno;
+    private int mUIUpdateSeqno;
+    private int mLastUIUpdateSeqno;
     private boolean mUpdateGecko;
 
     /* An action that alters the Editable
 
        Each action corresponds to a Gecko event. While the Gecko event is being sent to the Gecko
        thread, the action stays on top of mActions queue. After the Gecko event is processed and
        replied, the action is removed from the queue
     */
@@ -90,16 +90,17 @@ final class GeckoEditable
         static final int TYPE_REMOVE_SPAN = 4;
 
         final int mType;
         int mStart;
         int mEnd;
         CharSequence mSequence;
         Object mSpanObject;
         int mSpanFlags;
+        boolean mShouldUpdate;
 
         Action(int type) {
             mType = type;
         }
 
         static Action newReplaceText(CharSequence text, int start, int end) {
             if (start < 0 || start > end) {
                 throw new IllegalArgumentException("invalid replace text offsets");
@@ -144,16 +145,21 @@ final class GeckoEditable
             mActions = new ConcurrentLinkedQueue<Action>();
             mActionsActive = new Semaphore(1);
         }
 
         void offer(Action action) {
             if (DEBUG) {
                 GeckoApp.assertOnUiThread();
             }
+            /* Events don't need update because they generate text/selection
+               notifications which will do the updating for us */
+            if (action.mType != Action.TYPE_EVENT) {
+                action.mShouldUpdate = mUpdateGecko;
+            }
             if (mActions.isEmpty()) {
                 mActionsActive.acquireUninterruptibly();
                 mActions.offer(action);
             } else synchronized(this) {
                 // tryAcquire here in case Gecko thread has just released it
                 mActionsActive.tryAcquire();
                 mActions.offer(action);
             }
@@ -165,17 +171,17 @@ final class GeckoEditable
                 GeckoAppShell.sendEventToGecko(
                         GeckoEvent.createIMEEvent(GeckoEvent.IME_SYNCHRONIZE));
                 break;
             case Action.TYPE_REPLACE_TEXT:
                 GeckoAppShell.sendEventToGecko(GeckoEvent.createIMEReplaceEvent(
                         action.mStart, action.mEnd, action.mSequence.toString()));
                 break;
             }
-            ++mCompositionSeqno;
+            ++mUIUpdateSeqno;
         }
 
         void poll() {
             if (DEBUG) {
                 GeckoApp.assertOnGeckoThread();
             }
             if (mActions.isEmpty()) {
                 throw new IllegalStateException("empty actions queue");
@@ -229,34 +235,40 @@ final class GeckoEditable
                 PROXY_INTERFACES, this);
     }
 
     private static void geckoPostToUI(Runnable runnable) {
         GeckoApp.mAppContext.mMainHandler.post(runnable);
     }
 
     private void geckoUpdateGecko(final boolean force) {
-        if (mUpdateGecko) {
-            geckoPostToUI(new Runnable() {
-                public void run() {
+        /* We do not increment the seqno here, but only check it, because geckoUpdateGecko is a
+           request for update. If we incremented the seqno here, geckoUpdateGecko would have
+           prevented other updates from occurring */
+        final int seqnoWhenPosted = mGeckoUpdateSeqno;
+
+        geckoPostToUI(new Runnable() {
+            public void run() {
+                mActionQueue.syncWithGecko();
+                if (seqnoWhenPosted == mGeckoUpdateSeqno) {
                     uiUpdateGecko(force);
                 }
-            });
-        }
+            }
+        });
     }
 
     private void uiUpdateGecko(boolean force) {
 
-        if (!force && mCompositionSeqno == mLastUpdatedCompositionSeqno) {
+        if (!force && mUIUpdateSeqno == mLastUIUpdateSeqno) {
             if (DEBUG) {
                 Log.d(LOGTAG, "uiUpdateGecko() skipped");
             }
             return;
         }
-        mLastUpdatedCompositionSeqno = mCompositionSeqno;
+        mLastUIUpdateSeqno = mUIUpdateSeqno;
         mActionQueue.syncWithGecko();
 
         if (DEBUG) {
             Log.d(LOGTAG, "uiUpdateGecko()");
         }
 
         final int selStart = mText.getSpanStart(Selection.SELECTION_START);
         final int selEnd = mText.getSpanEnd(Selection.SELECTION_END);
@@ -427,17 +439,19 @@ final class GeckoEditable
                     }
                 }
             });
             break;
         case Action.TYPE_SET_SPAN:
             mText.setSpan(action.mSpanObject, action.mStart, action.mEnd, action.mSpanFlags);
             break;
         }
-        geckoUpdateGecko(false);
+        if (action.mShouldUpdate) {
+            geckoUpdateGecko(false);
+        }
         mActionQueue.poll();
     }
 
     @Override
     public void notifyIME(final int type, final int state) {
         if (DEBUG) {
             // GeckoEditableListener methods should all be called from the Gecko thread
             GeckoApp.assertOnGeckoThread();
@@ -480,30 +494,33 @@ final class GeckoEditable
     public void onSelectionChange(final int start, final int end) {
         if (DEBUG) {
             // GeckoEditableListener methods should all be called from the Gecko thread
             GeckoApp.assertOnGeckoThread();
         }
         if (start < 0 || start > end || end > mText.length()) {
             throw new IllegalArgumentException("invalid selection notification range");
         }
-        if (!mUpdateGecko) {
-            // Selection will be updated at the end of the batch operation
-            return;
-        }
-        final int seqnoWhenPosted = ++mSelectionSeqno;
+        final int seqnoWhenPosted = ++mGeckoUpdateSeqno;
 
         geckoPostToUI(new Runnable() {
             public void run() {
                 mActionQueue.syncWithGecko();
                 /* check to see there has not been another action that potentially changed the
                    selection. If so, we can skip this update because we know there is another
                    update right after this one that will replace the effect of this update */
-                if (mSelectionSeqno == seqnoWhenPosted) {
+                if (mGeckoUpdateSeqno == seqnoWhenPosted) {
+                    /* In this case, Gecko's selection has changed and it's notifying us to change
+                       Java's selection. In the normal case, whenever Java's selection changes,
+                       we go back and set Gecko's selection as well. However, in this case,
+                       since Gecko's selection is already up-to-date, we skip this step. */
+                    boolean oldUpdateGecko = mUpdateGecko;
+                    mUpdateGecko = false;
                     Selection.setSelection(mProxy, start, end);
+                    mUpdateGecko = oldUpdateGecko;
                 }
             }
         });
     }
 
     @Override
     public void onTextChange(final String text, final int start,
                       final int unboundedOldEnd, final int unboundedNewEnd) {
@@ -531,17 +548,16 @@ final class GeckoEditable
                 // Replace using saved text to preserve spans
                 mText.replace(start, oldEnd, action.mSequence,
                               0, action.mSequence.length());
             } else {
                 mText.replace(start, oldEnd, text, 0, text.length());
             }
         } else {
             mText.replace(start, oldEnd, text, 0, text.length());
-            geckoUpdateGecko(true);
             geckoPostToUI(new Runnable() {
                 public void run() {
                     if (mListener != null) {
                         mListener.onTextChange(text, start, oldEnd, newEnd);
                     }
                 }
             });
         }
--- a/widget/android/nsWindow.cpp
+++ b/widget/android/nsWindow.cpp
@@ -2189,16 +2189,18 @@ nsWindow::OnIMETextChange(uint32_t aStar
     DispatchEvent(&event);
     if (!event.mSucceeded)
         return NS_OK;
 
     AndroidBridge::NotifyIMEChange(event.mReply.mString.get(),
                                    event.mReply.mString.Length(),
                                    aStart, aOldEnd, aNewEnd);
 
+    /* Make sure Java's selection is up-to-date */
+    OnIMESelectionChange();
     return NS_OK;
 }
 
 NS_IMETHODIMP
 nsWindow::OnIMESelectionChange(void)
 {
     if (mIMEMaskSelectionUpdate)
         return NS_OK;