Removing invalidateActionMode

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.content.browser;
 
 import android.annotation.TargetApi;
 import android.app.Activity;
 import android.app.SearchManager;
 import android.content.ClipboardManager;
@@ skipping 178 matching lines @@
      *
      * <p>Action mode in floating mode is tried first, and then falls back to
      * a normal one.
      * @return {@code true} if the action mode started successfully or is already on.
      */
     public boolean showActionMode() {
         if (isEmpty()) return false;
 
         // 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()
                     : mView.startActionMode(mCallback);
             if (actionMode != null) {
                 // This is to work around an LGE email issue. See crbug.com/651706 for more details.
                 LGEmailActionModeWorkaround.runIfNecessary(mContext, actionMode);
             }
             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();
     }
 
     @TargetApi(Build.VERSION_CODES.M)
     private ActionMode startFloatingActionMode() {
         ActionMode actionMode = mView.startActionMode(
                 new FloatingActionModeCallback(this, mCallback), ActionMode.TYPE_FLOATING);
         return actionMode;
     }
 
@@ skipping 64 matching lines @@
     public void finishActionMode() {
         if (isActionModeValid()) {
             mActionMode.finish();
 
             // Should be nulled out in case #onDestroyActionMode() is not invoked in response.
             mActionMode = null;
         }
     }
 
     /**
-     * @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() {
         if (supportsFloatingActionMode()) {
             if (mHidden) {
                 mPendingInvalidateContentRect = true;
             } else {
                 mPendingInvalidateContentRect = false;
                 if (isActionModeValid()) mActionMode.invalidateContentRect();
             }
@@ skipping 571 matching lines @@
         mIsInsertion = insertion;
     }
 
     private boolean isShareAvailable() {
         Intent intent = new Intent(Intent.ACTION_SEND);
         intent.setType("text/plain");
         return mContext.getPackageManager().queryIntentActivities(intent,
                 PackageManager.MATCH_DEFAULT_ONLY).size() > 0;
     }
 }