Removing invalidateActionMode

content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java

Index: content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java b/content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
index 8d9c66e4913e7dbcb70f356c1bf969a0af14d8e1..c410296aec18b34d8d5862a768232070642119db 100644
--- a/content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
+++ b/content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java
@@ -196,12 +196,13 @@ public class SelectionPopupController extends ActionModeCallbackHelper {
 
         // Just refreshes the view if it is already showing.
         if (isActionModeValid()) {
-            invalidateActionMode();
-            return true;
-        }
-
-        if (mView.getParent() != null) {
-             // On ICS, startActionMode throws an NPE when getParent() is null.
+            // Try/catch necessary for framework bug, crbug.com/446717.
+            try {
+                mActionMode.invalidate();
+            } catch (NullPointerException e) {
+                Log.w(TAG, "Ignoring NPE from ActionMode.invalidate() as workaround for L", e);
+            }
+        } else {
             assert mWebContents != null;
             ActionMode actionMode = supportsFloatingActionMode()
                     ? startFloatingActionMode()
@@ -212,6 +213,7 @@ public class SelectionPopupController extends ActionModeCallbackHelper {
             }
             mActionMode = actionMode;
         }
+        hideActionMode(false);

[Tima Vaisburd, 2017/03/28 02:54:42]

Can the action mode be hidden (i.e. mHidden == true) when isActionModeValid()
returns false?
I've assumed not, but I could be mistaken.

If the action mode, indeed, cannot be hidden when mActionMode == null and the
call to hideActionMode(false) will always be a no-op in this case, maybe move it
to |if (isActionModeValid() {...}| block?

[boliu, 2017/03/28 17:09:19]

So only SELECTION_HANDLES_SHOWN can fall into invalidate. So the question is can
SELECTION_HANDLES_SHOWN happen when action mode is hidden?

I can't prove it can't be hidden, so better be safe?

[Tima Vaisburd, 2017/03/28 17:39:52]

Action mode can be hidden, but it should exist.

I meant, I think that the hidden property of the ActionMode should reflect the
state of the _existing_ ActionMode object. If that object does not exist (not
yet created, or already finished) then mHidden must be false, or there is an
inconsistency. I do not think we want to set it hidden in advance?

With that hideActionMode(false) should always be a no-op for the else case, but
this is not a big issue. The code as-is is fine with me too.

[boliu, 2017/03/28 17:44:12]

Oh, ok. Yeah I got confused, old code in invalidate used to unhide, so yeah,
this is fine.

[amaralp, 2017/03/28 18:15:08]

The only times mHidden is set to true is when a scroll starts or when a
selection handle
drag begins. The only times it is set back to false is when the scroll ends or
the
drag ends. Right now I can't think of a scenario where a scroll/drag would start
but not end.
I've gone ahead and moved the unhiding back to being only done for the
invalidation case. I
think the bigger problem is that SelectionPopupController is holding state of
the action mode
and it is difficult to reason when this state is being reset. I'm thinking of
making a wrapper
class for the action mode that could keep track of mHidden,
mUnselectAllOnDismiss, etc., but
I'll wait until timav@'s patch lands before trying that. 

         mUnselectAllOnDismiss = true;

[Jinsuk Kim, 2017/03/28 02:19:59]

|mUnselectAllOnDismiss| gets turned on but didn't before. Is this okay?

[boliu, 2017/03/28 17:09:19]

Looks ok, it would have already been false in the invalidate case afaict. Every
place where it's set to false also finishes the action mode.

[amaralp, 2017/03/28 18:15:08]

It would have been already false. Also I've changed the code so that it doesn't
get set anymore.

         return isActionModeValid();
     }
@@ -297,28 +299,6 @@ public class SelectionPopupController extends ActionModeCallbackHelper {
     }
 
     /**
-     * @see ActionMode#invalidate()
-     * Note that invalidation will also reset visibility state. The caller
-     * should account for this when making subsequent visibility updates.
-     */
-    private void invalidateActionMode() {
-        if (!isActionModeValid()) return;
-        if (mHidden) {
-            assert canHideActionMode();
-            mHidden = false;
-            mView.removeCallbacks(mRepeatingHideRunnable);
-            mPendingInvalidateContentRect = false;
-        }
-
-        // Try/catch necessary for framework bug, crbug.com/446717.
-        try {
-            mActionMode.invalidate();
-        } catch (NullPointerException e) {
-            Log.w(TAG, "Ignoring NPE from ActionMode.invalidate() as workaround for L", e);
-        }
-    }
-
-    /**
      * @see ActionMode#invalidateContentRect()
      */
     public void invalidateContentRect() {