Allow deferred intent when the screen is locked

chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java

Index: chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java b/chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
index e0e242be504ec625522842e3e5b488246395c6e3..b3f47b054cf2d065728c874337159ae30a8ee555 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
@@ -10,6 +10,7 @@ import android.app.SearchManager;
 import android.content.ComponentName;
 import android.content.Context;
 import android.content.Intent;
+import android.content.IntentFilter;
 import android.net.Uri;
 import android.os.Bundle;
 import android.os.SystemClock;
@@ -226,6 +227,11 @@ public class IntentHandler {
     private final String mPackageName;
     private KeyguardManager mKeyguardManager;
 
+    /**
+     * Receiver for screen unlock broadcast.
+     */
+    private KeyguardBroadcastReceiver mKeyguardBroadcastReceiver;
+
     public static enum TabOpenType {
         OPEN_NEW_TAB,
         // Tab is reused only if the URLs perfectly match.
@@ -331,13 +337,30 @@ public class IntentHandler {
         }
     }
 
+    @VisibleForTesting
+    void tryUnregisterLockscreenIntentReceiver(Context context, Intent intent) {
+        // Only unregister receiver when the intent was specially marked or the receiver no longer
+        // contain any valid deferred intent.
+        // If the intent does not contain the special mark, it should be treated as normal intent
+        // instead. This is helping the scenario when the screen is unlocked, another intent is
+        // fired before the broadcast.

[Maria, 2016/12/13 21:36:09]

I think that if another event was fired before the broadcast, but after unlock,
we should unregister receiver and drop the deferred intent..

[Sherry, 2016/12/13 22:13:54]

If the deferred intent is still within the lifetime range, I believe it is
better to wait for the deferred intent since it might be triggered very soon.
Also, the lifetime for the deferred intent is not too long, so it won't hold the
receiver forever. The worst case would be after the deferred intent lifetime
expired, the broadcast is not triggered, and it will be recycled by next coming
intent. 

[Sherry, 2016/12/13 22:32:29]

My bad, you are right. If the new irrelevant intent was fired before the
deferred intent. We should drop the deferred intent and unregister the receiver.
The current logic also handle the intents this way. I will update the comment
here.

[Ted C, 2016/12/13 22:47:13]

I agree with Maria.  If another intent was received, we should cancel the
deferred one.  A new intent received implies the user took a different action. 
If we chose to honor the deferred intent after the new user driven intent, it
could potentially clobber their action or at least impede it.

The likelihood of it happening is very low, but I still don't think acting on it
makes sense.

Ha...just saw your update...looks like we are on the same page.

+        if (mKeyguardBroadcastReceiver != null
+                && !mKeyguardBroadcastReceiver.hasValidDeferredIntent(intent)) {
+            context.unregisterReceiver(mKeyguardBroadcastReceiver);
+            mKeyguardBroadcastReceiver = null;
+        }
+    }
+
     /**
      * Handles an Intent after the ChromeTabbedActivity decides that it shouldn't ignore the
      * Intent.
-     *
+     * @param context Android Context.
+     * @param intent Target intent.
      * @return Whether the Intent was successfully handled.
      */
     boolean onNewIntent(Context context, Intent intent) {
+        tryUnregisterLockscreenIntentReceiver(context, intent);
+
         assert intentHasValidUrl(intent);
         String url = getUrlFromIntent(intent);
         boolean hasUserGesture =
@@ -688,13 +711,33 @@ public class IntentHandler {
             // We must check for screen state at this point.
             // These might be slow.
             boolean internalOrVisible = isInternal || isIntentUserVisible(context);
-            return !internalOrVisible;
+            if (!internalOrVisible) {
+                // Register keyguard broadcast receiver to receive ACTION_USER_PRESENT when the
+                // screen is unlocked.
+                // Avoid register KeyguardBroadcastReceiver if there is already one cached to allow
+                // unregister on the same instance.
+                // The ACTION_USER_PRESENT is sent by platform to indicates when user is present.
+                if (mKeyguardBroadcastReceiver == null) {
+                    mKeyguardBroadcastReceiver = new KeyguardBroadcastReceiver(intent);
+                    context.registerReceiver(mKeyguardBroadcastReceiver,
+                            new IntentFilter(Intent.ACTION_USER_PRESENT));
+                } else {
+                    mKeyguardBroadcastReceiver.updateDeferredIntent(intent);
+                }

[Maria, 2016/12/13 21:36:09]

maybe move these lines into a method registerLockscreenIntentReceiver to stay
symmetric with unregistering.

[Sherry, 2016/12/13 22:13:54]

Sounds good to me. Will update.

[Ted C, 2016/12/13 22:47:13]

Also, I would have the receiver unregister itself after
VALID_DEFERRED_PERIOD_MS.

If we get an intent while the screen is locked, but it never becomes unlocked,
then I think we should proactively remove the receiver.

[Sherry, 2016/12/14 00:54:07]

Then we need a background task that continuously check the time and drop the
intent on expiration. This is less efficient than holding the instance until
some intent to trigger the invalidation.

+                return true;
+            }
+            return false;
         } catch (Throwable t) {
             return true;
         }
     }
 
     @VisibleForTesting
+    void setKeyguardBroadcastReceiverForTesting(KeyguardBroadcastReceiver receiver) {
+        mKeyguardBroadcastReceiver = receiver;
+    }
+
+    @VisibleForTesting
     boolean intentHasValidUrl(Intent intent) {
         String url = getUrlFromIntent(intent);
 
@@ -761,7 +804,8 @@ public class IntentHandler {
         return false;
     }
 
-    private boolean isIntentUserVisible(Context context) {
+    @VisibleForTesting
+    boolean isIntentUserVisible(Context context) {
         // Only process Intents if the screen is on and the device is unlocked;
         // i.e. the user will see what is going on.
         if (mKeyguardManager == null) {