Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2568843003:
Allow deferred intent when the screen is locked (Closed)
DescriptionAllow deferred intent when the screen is locked
The intent might be ignored if the screen is locked and user just input
pin to unlock it. Instead of simply discard the intent, add a deferred
intent and hold it for a short period of time. Once the USER_PRESENT has
been broadcast, check back the deferred intent and move forward with it.
The other option to handle this scenario would be set the intent aside,
and start an async task to continuously for the screen status, and react
once the screen unlocked. This would be less optimal option since it
requires extra background task while screen is locked.
BUG=455126
TEST=out/Default/bin/run_chrome_public_test_apk -f \*IntentHandlerTest\*
also verified by file an intent when screen is locked and then
unlcok screen with pin.
Committed: https://crrev.com/8dec237dce0b0430a9f9c5d7f50bce4943be5554
Cr-Commit-Position: refs/heads/master@{#439566}
Patch Set 1 #Patch Set 2 : Allow deferred intent when the screen is locked #
Total comments: 26
Patch Set 3 : Allow deferred when the screen is locked #Patch Set 4 : Allow deferred when the screen is locked #Patch Set 5 : Allow deferred when the screen is locked #
Total comments: 6
Patch Set 6 : Allow deferred when the screen is locked #
Total comments: 14
Patch Set 7 : Allow deferred when the screen is locked #Patch Set 8 : Allow deferred when the screen is locked #
Total comments: 11
Patch Set 9 : Allow deferred when the screen is locked #
Messages
Total messages: 43 (12 generated)
Description was changed from
==========
Allow deferred when the screen is locked
The intent might be ignored if the screen is locked and user just input
pin to unlock it. Instead of simply discard the intent, add a deferred
intent and hold it for a short period of time. Once the USER_PRESENT has
been broadcast, check back the deferred intent and move forward with it.
The other option to handle this scenario would be set the intent aside,
and start an async task to continuously for the screen status, and react
once the screen unlocked. This would be less optimal option since it
requires extra background task while screen is locked.
BUG=455126
TEST=out/Default/bin/run_chrome_public_test_apk -f \*IntentHandlerTest\*
also verified by file an intent when screen is locked and then
unlcok screen with pin.
==========
to
==========
Allow deferred intent when the screen is locked
The intent might be ignored if the screen is locked and user just input
pin to unlock it. Instead of simply discard the intent, add a deferred
intent and hold it for a short period of time. Once the USER_PRESENT has
been broadcast, check back the deferred intent and move forward with it.
The other option to handle this scenario would be set the intent aside,
and start an async task to continuously for the screen status, and react
once the screen unlocked. This would be less optimal option since it
requires extra background task while screen is locked.
BUG=455126
TEST=out/Default/bin/run_chrome_public_test_apk -f \*IntentHandlerTest\*
also verified by file an intent when screen is locked and then
unlcok screen with pin.
==========
wenjinm@amazon.com changed reviewers: + tedchoc@chromium.org
Hi Ted, Here's the proposed fix for Issue 455126, please help take a look. Thanks, Sherry
tedchoc@chromium.org changed reviewers: + mariakhomenko@chromium.org
+mariakhomenko for the intent handling
https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:346: // fired before the broadcast. I think that if another event was fired before the broadcast, but after unlock, we should unregister receiver and drop the deferred intent.. https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:726: } maybe move these lines into a method registerLockscreenIntentReceiver to stay symmetric with unregistering. https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java (right): https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:14: * Broadcast receiver when the screen is unlocked from pin, pattern, or password. I would add a note that this class handles exactly one intent unlock + dispatch and then is expected to be destroyed and cannot be reused right now (mShouldFireIntent is never reset) https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:40: public boolean hasValidDeferredIntent(final Intent intent) { style: add javadoc to public methods https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:42: && System.currentTimeMillis() - mDeferredIntentCreatedTime style: parenthesis around subtraction https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:49: mDeferredIntentCreatedTime = System.currentTimeMillis(); For timing events use SystemClock.elapsedRealtime() instead -- it's guaranteed to be monotonic.
https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:346: // fired before the broadcast. On 2016/12/13 21:36:09, Maria wrote: > I think that if another event was fired before the broadcast, but after unlock, > we should unregister receiver and drop the deferred intent.. 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. https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:726: } On 2016/12/13 21:36:09, Maria wrote: > maybe move these lines into a method registerLockscreenIntentReceiver to stay > symmetric with unregistering. Sounds good to me. Will update. https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java (right): https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:14: * Broadcast receiver when the screen is unlocked from pin, pattern, or password. On 2016/12/13 21:36:09, Maria wrote: > I would add a note that this class handles exactly one intent unlock + dispatch > and then is expected to be destroyed and cannot be reused right now > (mShouldFireIntent is never reset) Will do. https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:40: public boolean hasValidDeferredIntent(final Intent intent) { On 2016/12/13 21:36:09, Maria wrote: > style: add javadoc to public methods Will do. https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:42: && System.currentTimeMillis() - mDeferredIntentCreatedTime On 2016/12/13 21:36:09, Maria wrote: > style: parenthesis around subtraction Will update. https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:49: mDeferredIntentCreatedTime = System.currentTimeMillis(); On 2016/12/13 21:36:09, Maria wrote: > For timing events use SystemClock.elapsedRealtime() instead -- it's guaranteed > to be monotonic. Will update.
https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:346: // fired before the broadcast. On 2016/12/13 22:13:54, Sherry wrote: > On 2016/12/13 21:36:09, Maria wrote: > > I think that if another event was fired before the broadcast, but after > unlock, > > we should unregister receiver and drop the deferred intent.. > > 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. 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.
https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:346: // fired before the broadcast. On 2016/12/13 22:13:54, Sherry wrote: > On 2016/12/13 21:36:09, Maria wrote: > > I think that if another event was fired before the broadcast, but after > unlock, > > we should unregister receiver and drop the deferred intent.. > > 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. 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. https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:726: } On 2016/12/13 22:13:54, Sherry wrote: > On 2016/12/13 21:36:09, Maria wrote: > > maybe move these lines into a method registerLockscreenIntentReceiver to stay > > symmetric with unregistering. > > Sounds good to me. Will update. 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. https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java (right): https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:16: public class KeyguardBroadcastReceiver extends BroadcastReceiver { based on my comment in the other file about unregistering itself based on a timer. I wonder if we could encapsulate all of the logic in this file. Back of napkin thinking is that updateDeferredIntent would be the only public method. Passing null would unregister. A non-null value would register the receiver if needed (and post a unregister task to take place after the valid duration). If we unregister based on the timeout, we no longer need the hasValidDeferredIntent. We just check that it is non-null. https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:25: mShouldFireIntent = true; can use just use the null-ness of mDeferredIntent instead? Seems like we just set the intent to null after we call startActivity and everything should be good. https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:34: context.startActivity(mDeferredIntent); what context is this referring to? In particular, I'm assuming this is not an activity context, so I think these intents would need ensure they have NEW_TASK or NEW_DOCUMENT associated with them. Or maybe we don't save intents that don't include this?
Fixed all comments in previous review.
On 2016/12/14 00:08:14, Sherry wrote: > Fixed all comments in previous review. I don't see the changes addressing Ted's comments in #9?
Reply with Ted's comments https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:726: } On 2016/12/13 22:47:13, Ted C wrote: > On 2016/12/13 22:13:54, Sherry wrote: > > On 2016/12/13 21:36:09, Maria wrote: > > > maybe move these lines into a method registerLockscreenIntentReceiver to > stay > > > symmetric with unregistering. > > > > Sounds good to me. Will update. > > 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. 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. https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java (right): https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:16: public class KeyguardBroadcastReceiver extends BroadcastReceiver { On 2016/12/13 22:47:13, Ted C wrote: > based on my comment in the other file about unregistering itself based on a > timer. > > I wonder if we could encapsulate all of the logic in this file. Back of napkin > thinking is that updateDeferredIntent would be the only public method. Passing > null would unregister. A non-null value would register the receiver if needed > (and post a unregister task to take place after the valid duration). > > If we unregister based on the timeout, we no longer need the > hasValidDeferredIntent. We just check that it is non-null. It sounds ideal, however, that will have this class keep the context as a member so that it could register/unregister the receiver freely. It will cause more problems since ChromeActivity holds the instance of IntentHandler, IntentHandler holds the instance of the receiver, the receiver holds the instance of the Context which could potentially be ChromeActivity. If possible, I would prefer the receiver never holds the Context instance as a member. What's more, another backwards of doing so would be the receiver tries to register/unregister itself to the context. However, it is hard to track the right instance registered/unregistered without keeping a track of the receiver instance. It might be more messy to keep the receiver instance as a static member. https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:25: mShouldFireIntent = true; On 2016/12/13 22:47:13, Ted C wrote: > can use just use the null-ness of mDeferredIntent instead? > > Seems like we just set the intent to null after we call startActivity and > everything should be good. That was my first intuition as well. I used to think it was not working as expected since it would cause troubles in hasValidDeferredIntent where it checks if the deferred intent is null. If mDeferredIntent was simply set to null in this class, then in method IntentHandler.tryUnregisterLockscreenIntentReceiver will not have its logic working correctly since hasValidDeferredIntent will always return false. But think about this solution again, it might lead to the actual behavior we expect. Once the deferred intent was fired, there's no reason to make hasValidDeferredIntent return true. Which lead to the results that tryUnregisterLockscreenIntentReceiver will always unregister the receiver when triggered from onNewIntent. And that would be the expectation where under any situation, when onNewIntent was triggered, as long as mReceiver is not null, it will be unregistered and dropped. The check of hasValidDeferredIntent became unnecessary since it will always return false in this situation. But I may still keep it in case this method was shared in other functions in the future where checking the validity of deferred intent is still correct. https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:34: context.startActivity(mDeferredIntent); On 2016/12/13 22:47:13, Ted C wrote: > what context is this referring to? > > In particular, I'm assuming this is not an activity context, so I think these > intents would need ensure they have NEW_TASK or NEW_DOCUMENT associated with > them. > > Or maybe we don't save intents that don't include this? According to https://developer.android.com/reference/android/content/BroadcastReceiver.htm..., android.content.Intent), the context is where the receiver is running. It is not the same one as the activity. In the shouldIgnoreIntent method, the receiver will only be registered when the intent is neither internal nor visible, which guards the deferred intent was sent from external and not expecting internal context.
On 2016/12/14 00:53:23, Maria wrote: > On 2016/12/14 00:08:14, Sherry wrote: > > Fixed all comments in previous review. > > I don't see the changes addressing Ted's comments in #9? You are right, I missed Ted's comments. Will have another patch to address it with the reply #12.
Update with Ted's comments and my replies in #12.
https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java (right): https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:16: public class KeyguardBroadcastReceiver extends BroadcastReceiver { On 2016/12/14 00:54:07, Sherry wrote: > On 2016/12/13 22:47:13, Ted C wrote: > > based on my comment in the other file about unregistering itself based on a > > timer. > > > > I wonder if we could encapsulate all of the logic in this file. Back of > napkin > > thinking is that updateDeferredIntent would be the only public method. > Passing > > null would unregister. A non-null value would register the receiver if needed > > (and post a unregister task to take place after the valid duration). > > > > If we unregister based on the timeout, we no longer need the > > hasValidDeferredIntent. We just check that it is non-null. > > It sounds ideal, however, that will have this class keep the context as a member > so that it could register/unregister the receiver freely. It will cause more > problems since ChromeActivity holds the instance of IntentHandler, IntentHandler > holds the instance of the receiver, the receiver holds the instance of the > Context which could potentially be ChromeActivity. If possible, I would prefer > the receiver never holds the Context instance as a member. Can you not use the application context (ContextUtils.getApplicationContext())? Then you are isolated from GC issues. > > What's more, another backwards of doing so would be the receiver tries to > register/unregister itself to the context. However, it is hard to track the > right instance registered/unregistered without keeping a track of the receiver > instance. It might be more messy to keep the receiver instance as a static > member. Huh? This class is the instance so there is certainly no tracking that needs to be done. After discussing this with Maria, we both agree that my proposed approach is what we want to see. Some things I think we should do: 1.) Not name this as a <...>Receiver (or even Keyguard). I would name this something like DelayedScreenLockIntentHandler (something that makes it clearer that it is purely here to handle intents while the screen is locked/off). That name could likely be refined. 2.) IntentHandler creates an instance and stores it. It calls updateIntent(null), updateIntent(validIntent) as the only two entry points. 3.) Internally, this class registers the receiver. If the application context does not work, the passing in a context to updateIntent is fine (and preferred). 4.) In the constructor, create a new Handler() object. In update intent w/ a valid intent, post a task to will unregister after the timeout (you need to do Handler#removeCallbacks on subsequent calls to update to prevent early unregistration). In general, IntentHandler is complicated enough and any and all effort should be done to isolate this logic into its own file.
https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java (right): https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:16: public class KeyguardBroadcastReceiver extends BroadcastReceiver { On 2016/12/14 19:55:06, Ted C wrote: > On 2016/12/14 00:54:07, Sherry wrote: > > On 2016/12/13 22:47:13, Ted C wrote: > > > based on my comment in the other file about unregistering itself based on a > > > timer. > > > > > > I wonder if we could encapsulate all of the logic in this file. Back of > > napkin > > > thinking is that updateDeferredIntent would be the only public method. > > Passing > > > null would unregister. A non-null value would register the receiver if > needed > > > (and post a unregister task to take place after the valid duration). > > > > > > If we unregister based on the timeout, we no longer need the > > > hasValidDeferredIntent. We just check that it is non-null. > > > > It sounds ideal, however, that will have this class keep the context as a > member > > so that it could register/unregister the receiver freely. It will cause more > > problems since ChromeActivity holds the instance of IntentHandler, > IntentHandler > > holds the instance of the receiver, the receiver holds the instance of the > > Context which could potentially be ChromeActivity. If possible, I would prefer > > the receiver never holds the Context instance as a member. > > Can you not use the application context (ContextUtils.getApplicationContext())? > > Then you are isolated from GC issues. > > > > > What's more, another backwards of doing so would be the receiver tries to > > register/unregister itself to the context. However, it is hard to track the > > right instance registered/unregistered without keeping a track of the receiver > > instance. It might be more messy to keep the receiver instance as a static > > member. > > Huh? This class is the instance so there is certainly no tracking that needs > to be done. > > After discussing this with Maria, we both agree that my proposed approach is > what we want to see. > > Some things I think we should do: > > 1.) Not name this as a <...>Receiver (or even Keyguard). I would name > this something like DelayedScreenLockIntentHandler (something that > makes it clearer that it is purely here to handle intents while > the screen is locked/off). That name could likely be refined. > > 2.) IntentHandler creates an instance and stores it. It calls > updateIntent(null), updateIntent(validIntent) as the only two > entry points. > > 3.) Internally, this class registers the receiver. If the application > context does not work, the passing in a context to updateIntent > is fine (and preferred). > > 4.) In the constructor, create a new Handler() object. In update > intent w/ a valid intent, post a task to will unregister after > the timeout (you need to do Handler#removeCallbacks on subsequent > calls to update to prevent early unregistration). > > In general, IntentHandler is complicated enough and any and all > effort should be done to isolate this logic into its own file. See my replies to each of your 4 points: 1.) Agree, I will update the name to DelayedScreenLockIntentHandler instead. 2.) I could move forward with that. However, I have some minor questions below: 2.a) on updateIntent(null), should I unregister the receiver? 2.b) if there are two different places unregister the receiver, do you concern about the conflicts of unregister a receiver that is already unregistered? 3.) Agree, it should not be a big deal. 4.) What will happen if the receiver was unregistered due to timeout and a new deferred intent came. e.g. I fire an intent which creates the receiver instance and registered to the context. Keep the screen locked. On timeout, the recevier is unregistered from the context. Then fire another intent. The second intent will never be handled since the receiver has been unregistered. But in my expectation, the second intent should replace the first one and be handled correctly when the screen is unlocked. Besides, with current unregistraion solution, the worst case would be leaving the receiver registered and almost do nothing. The cost of having it around would be low. With the logic to unregister on timeout, it might add more complexity to the logic and potentially introducing more corner cases that cause problems. In general, I will move all logic into the receiver and keep IntentHandler clean. I still have concerns for the proposed way of handling receiver unregistration though.
Came up with some ideas for the delayed unregistration. https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java (right): https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:16: public class KeyguardBroadcastReceiver extends BroadcastReceiver { On 2016/12/14 22:25:46, Sherry wrote: > On 2016/12/14 19:55:06, Ted C wrote: > > On 2016/12/14 00:54:07, Sherry wrote: > > > On 2016/12/13 22:47:13, Ted C wrote: > > > > based on my comment in the other file about unregistering itself based on > a > > > > timer. > > > > > > > > I wonder if we could encapsulate all of the logic in this file. Back of > > > napkin > > > > thinking is that updateDeferredIntent would be the only public method. > > > Passing > > > > null would unregister. A non-null value would register the receiver if > > needed > > > > (and post a unregister task to take place after the valid duration). > > > > > > > > If we unregister based on the timeout, we no longer need the > > > > hasValidDeferredIntent. We just check that it is non-null. > > > > > > It sounds ideal, however, that will have this class keep the context as a > > member > > > so that it could register/unregister the receiver freely. It will cause more > > > problems since ChromeActivity holds the instance of IntentHandler, > > IntentHandler > > > holds the instance of the receiver, the receiver holds the instance of the > > > Context which could potentially be ChromeActivity. If possible, I would > prefer > > > the receiver never holds the Context instance as a member. > > > > Can you not use the application context > (ContextUtils.getApplicationContext())? > > > > Then you are isolated from GC issues. > > > > > > > > What's more, another backwards of doing so would be the receiver tries to > > > register/unregister itself to the context. However, it is hard to track the > > > right instance registered/unregistered without keeping a track of the > receiver > > > instance. It might be more messy to keep the receiver instance as a static > > > member. > > > > Huh? This class is the instance so there is certainly no tracking that needs > > to be done. > > > > After discussing this with Maria, we both agree that my proposed approach is > > what we want to see. > > > > Some things I think we should do: > > > > 1.) Not name this as a <...>Receiver (or even Keyguard). I would name > > this something like DelayedScreenLockIntentHandler (something that > > makes it clearer that it is purely here to handle intents while > > the screen is locked/off). That name could likely be refined. > > > > 2.) IntentHandler creates an instance and stores it. It calls > > updateIntent(null), updateIntent(validIntent) as the only two > > entry points. > > > > 3.) Internally, this class registers the receiver. If the application > > context does not work, the passing in a context to updateIntent > > is fine (and preferred). > > > > 4.) In the constructor, create a new Handler() object. In update > > intent w/ a valid intent, post a task to will unregister after > > the timeout (you need to do Handler#removeCallbacks on subsequent > > calls to update to prevent early unregistration). > > > > In general, IntentHandler is complicated enough and any and all > > effort should be done to isolate this logic into its own file. > > See my replies to each of your 4 points: > 1.) Agree, I will update the name to DelayedScreenLockIntentHandler instead. > 2.) I could move forward with that. However, I have some minor questions below: > 2.a) on updateIntent(null), should I unregister the receiver? > 2.b) if there are two different places unregister the receiver, do you > concern about the conflicts of unregister a receiver that is already > unregistered? > 3.) Agree, it should not be a big deal. > 4.) What will happen if the receiver was unregistered due to timeout and a new > deferred intent came. e.g. I fire an intent which creates the receiver instance > and registered to the context. Keep the screen locked. On timeout, the recevier > is unregistered from the context. Then fire another intent. The second intent > will never be handled since the receiver has been unregistered. But in my > expectation, the second intent should replace the first one and be handled > correctly when the screen is unlocked. > Besides, with current unregistraion solution, the worst case would be leaving > the receiver registered and almost do nothing. The cost of having it around > would be low. With the logic to unregister on timeout, it might add more > complexity to the logic and potentially introducing more corner cases that cause > problems. > > In general, I will move all logic into the receiver and keep IntentHandler > clean. I still have concerns for the proposed way of handling receiver > unregistration though. I have a plan to make the delayed task for unregistration work. It could be something like: class DelayedScreenLockIntentHandler { private boolean isRegistered = false; private synchronized void registerReceiver() { if (!isRegistered) { // register the receiver here isRegistered = true; } private synchronized void unregisterReceiver() { if (registered) { // unregister the receiver here isRegistered = false; } } And invoke register when updateDeferredIntent with a valid intent is triggered. But it will actually register at most once. unregisterReceiver will be triggered in three different ways: - When onNewIntent() from IntentHandler. - When onReceive() handled the deferred intent already. - When timeout for the current deferredIntent. I'm going to use Handler.postDelayed to post a delayed task for unregistration in updateDeferredIntent as you proposed in 4.). Since I introduced synchronized in the register/unregister function, it should not cause any synchronous issue if the unregister is triggered from a different thread.
https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java (right): https://codereview.chromium.org/2568843003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/KeyguardBroadcastReceiver.java:16: public class KeyguardBroadcastReceiver extends BroadcastReceiver { On 2016/12/16 21:10:59, Sherry wrote: > On 2016/12/14 22:25:46, Sherry wrote: > > On 2016/12/14 19:55:06, Ted C wrote: > > > On 2016/12/14 00:54:07, Sherry wrote: > > > > On 2016/12/13 22:47:13, Ted C wrote: > > > > > based on my comment in the other file about unregistering itself based > on > > a > > > > > timer. > > > > > > > > > > I wonder if we could encapsulate all of the logic in this file. Back of > > > > napkin > > > > > thinking is that updateDeferredIntent would be the only public method. > > > > Passing > > > > > null would unregister. A non-null value would register the receiver if > > > needed > > > > > (and post a unregister task to take place after the valid duration). > > > > > > > > > > If we unregister based on the timeout, we no longer need the > > > > > hasValidDeferredIntent. We just check that it is non-null. > > > > > > > > It sounds ideal, however, that will have this class keep the context as a > > > member > > > > so that it could register/unregister the receiver freely. It will cause > more > > > > problems since ChromeActivity holds the instance of IntentHandler, > > > IntentHandler > > > > holds the instance of the receiver, the receiver holds the instance of the > > > > Context which could potentially be ChromeActivity. If possible, I would > > prefer > > > > the receiver never holds the Context instance as a member. > > > > > > Can you not use the application context > > (ContextUtils.getApplicationContext())? > > > > > > Then you are isolated from GC issues. > > > > > > > > > > > What's more, another backwards of doing so would be the receiver tries to > > > > register/unregister itself to the context. However, it is hard to track > the > > > > right instance registered/unregistered without keeping a track of the > > receiver > > > > instance. It might be more messy to keep the receiver instance as a static > > > > member. > > > > > > Huh? This class is the instance so there is certainly no tracking that > needs > > > to be done. > > > > > > After discussing this with Maria, we both agree that my proposed approach is > > > what we want to see. > > > > > > Some things I think we should do: > > > > > > 1.) Not name this as a <...>Receiver (or even Keyguard). I would name > > > this something like DelayedScreenLockIntentHandler (something that > > > makes it clearer that it is purely here to handle intents while > > > the screen is locked/off). That name could likely be refined. > > > > > > 2.) IntentHandler creates an instance and stores it. It calls > > > updateIntent(null), updateIntent(validIntent) as the only two > > > entry points. > > > > > > 3.) Internally, this class registers the receiver. If the application > > > context does not work, the passing in a context to updateIntent > > > is fine (and preferred). > > > > > > 4.) In the constructor, create a new Handler() object. In update > > > intent w/ a valid intent, post a task to will unregister after > > > the timeout (you need to do Handler#removeCallbacks on subsequent > > > calls to update to prevent early unregistration). > > > > > > In general, IntentHandler is complicated enough and any and all > > > effort should be done to isolate this logic into its own file. > > > > See my replies to each of your 4 points: > > 1.) Agree, I will update the name to DelayedScreenLockIntentHandler instead. > > 2.) I could move forward with that. However, I have some minor questions > below: > > 2.a) on updateIntent(null), should I unregister the receiver? > > 2.b) if there are two different places unregister the receiver, do you > > concern about the conflicts of unregister a receiver that is already > > unregistered? > > 3.) Agree, it should not be a big deal. > > 4.) What will happen if the receiver was unregistered due to timeout and a new > > deferred intent came. e.g. I fire an intent which creates the receiver > instance > > and registered to the context. Keep the screen locked. On timeout, the > recevier > > is unregistered from the context. Then fire another intent. The second intent > > will never be handled since the receiver has been unregistered. But in my > > expectation, the second intent should replace the first one and be handled > > correctly when the screen is unlocked. > > Besides, with current unregistraion solution, the worst case would be leaving > > the receiver registered and almost do nothing. The cost of having it around > > would be low. With the logic to unregister on timeout, it might add more > > complexity to the logic and potentially introducing more corner cases that > cause > > problems. > > > > In general, I will move all logic into the receiver and keep IntentHandler > > clean. I still have concerns for the proposed way of handling receiver > > unregistration though. > > I have a plan to make the delayed task for unregistration work. It could be > something like: > class DelayedScreenLockIntentHandler { > private boolean isRegistered = false; > private synchronized void registerReceiver() { > if (!isRegistered) { > // register the receiver here > isRegistered = true; > } > private synchronized void unregisterReceiver() { > if (registered) { > // unregister the receiver here > isRegistered = false; > } > } > > And invoke register when updateDeferredIntent with a valid intent is triggered. > But it will actually register at most once. > unregisterReceiver will be triggered in three different ways: > - When onNewIntent() from IntentHandler. > - When onReceive() handled the deferred intent already. > - When timeout for the current deferredIntent. I'm going to use > Handler.postDelayed to post a delayed task for unregistration in > updateDeferredIntent as you proposed in 4.). > > Since I introduced synchronized in the register/unregister function, it should > not cause any synchronous issue if the unregister is triggered from a different > thread. My bad, just realized Handler is running the task on the same thread where it was created. Hence there should be no synchronize problems. Will remove the synchronous keywords.
Updates: - Renamed KeyguardBroadcastReceiver to DelayedScreenLockIntentHandler - Move receiver register/unregister to DelayedScreenLockIntentHandler - All unregister the receiver when the deferred intent expires
https://codereview.chromium.org/2568843003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:29: private Handler mTaskHandler; final https://codereview.chromium.org/2568843003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:30: private Runnable mUnregisterTask; final https://codereview.chromium.org/2568843003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:90: // If reboot unexpectedly, deltatime could be smaller than 0. :) I don't think we'd have this object anymore if the phone rebooted
https://codereview.chromium.org/2568843003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:29: private Handler mTaskHandler; On 2016/12/17 00:02:59, Maria wrote: > final Done. https://codereview.chromium.org/2568843003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:30: private Runnable mUnregisterTask; On 2016/12/17 00:02:59, Maria wrote: > final Done. https://codereview.chromium.org/2568843003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:90: // If reboot unexpectedly, deltatime could be smaller than 0. On 2016/12/17 00:02:59, Maria wrote: > :) I don't think we'd have this object anymore if the phone rebooted Will remove this comment. :)
Looks much better IMO thank you! Some comments about the test methods needed and some small cleanups, but getting quite close. https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:29: private final Handler mTaskHandler; nit, I put final's at the top https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:38: public DelayedScreenLockIntentHandler(Intent deferredIntent) { I wouldn't pass in an intent here, I would make updateDeferredIntent the only entry point. https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:54: && hasValidDeferredIntent(mDeferredIntent)) { why do you need this check anymore? We unregister on null and the receiver is automatically unregistered on the timeout. I would do a null check purely to cover up any delays between the call to unregister and Android receiving it, but I don't think we need anything more than that. https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:75: mTaskHandler.removeCallbacks(mUnregisterTask); I'd put this line at the top (not strictly needed in the null case, but I think it makes it a bit more explicit. https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:97: void setCreateTimeForTesting(final long time) { Can you mock SystemClock instead? ShadowSystemClock.setCurrentTimeMillis(ShadowSystemClock.elapsedRealtime() + amount); https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:102: void setContextForTesting(final Context context) { Can the test not call initApplicationContextForTests? https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:111: if (!mReceiverRegistered) { nit, I think it is cleaner to do early returns instead: if (mReceiverRegistered) return; Then the rest doesn't need to be indented.
https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:29: private final Handler mTaskHandler; On 2016/12/17 00:44:00, Ted C wrote: > nit, I put final's at the top Done. https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:38: public DelayedScreenLockIntentHandler(Intent deferredIntent) { On 2016/12/17 00:43:59, Ted C wrote: > I wouldn't pass in an intent here, I would make updateDeferredIntent the only > entry point. Done. https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:54: && hasValidDeferredIntent(mDeferredIntent)) { On 2016/12/17 00:43:59, Ted C wrote: > why do you need this check anymore? > > We unregister on null and the receiver is automatically unregistered on the > timeout. I would do a null check purely to cover up any delays between the call > to unregister and Android receiving it, but I don't think we need anything more > than that. ok, will remove this method and only do the null check here. https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:75: mTaskHandler.removeCallbacks(mUnregisterTask); On 2016/12/17 00:43:59, Ted C wrote: > I'd put this line at the top (not strictly needed in the null case, but I think > it makes it a bit more explicit. Done. https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:97: void setCreateTimeForTesting(final long time) { On 2016/12/17 00:44:00, Ted C wrote: > Can you mock SystemClock instead? > > ShadowSystemClock.setCurrentTimeMillis(ShadowSystemClock.elapsedRealtime() + > amount); This might not be necessary since I removed the hasValidDeferredIntent method. https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:102: void setContextForTesting(final Context context) { On 2016/12/17 00:43:59, Ted C wrote: > Can the test not call initApplicationContextForTests? Didn't realize this method. Thanks for showing me! https://codereview.chromium.org/2568843003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:111: if (!mReceiverRegistered) { On 2016/12/17 00:43:59, Ted C wrote: > nit, I think it is cleaner to do early returns instead: > > if (mReceiverRegistered) return; > > Then the rest doesn't need to be indented. Done.
Update comments in patch #6. In patch #7, removed the redundant mContextForTesting.
lgtm w/ a last couple comments https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:37: unregisterReceiver(); I would do updateDeferredIntent(null) here instead. Then we can ensure it is set to null in this case. https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:343: mDelayedScreenIntentHandler.updateDeferredIntent(intent); I think you need to still wrap this in a if (mDelayedScreenIntentHandler != null) { ... } since you lazily create it when the intent is non null the first time.
https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:79: } style nit: return on the same line, e.g. if (mReceiverRegistered) return; https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:94: return; style nit: same as above https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:343: mDelayedScreenIntentHandler.updateDeferredIntent(intent); On 2016/12/19 16:01:17, Ted C (OOO till 12.27) wrote: > I think you need to still wrap this in a > > if (mDelayedScreenIntentHandler != null) { > ... > } > > since you lazily create it when the intent is non null the first time. I think what Ted means to say is that you should keep the null check here from before and then guard the update (otherwise lazy creation isn't buying us anything, this will create a handler on the first intent that comes in because it's called from onNewIntent) e.g. if (mDelayedScreenIntentHandler == null && intent != null) { mDelayedScreenIntentHandler = new ...(); } if (mDelayedScreenIntentHandler != null) { mDelayedScreenIntentHandler.updateDeferredIntent(intent) }
https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:20: * and cannot be reused. Update comment here, this is no longer accurate since we reuse this class for multiple intents now.
For comments from both Ted and Maria. https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:20: * and cannot be reused. On 2016/12/19 18:03:51, Maria wrote: > Update comment here, this is no longer accurate since we reuse this class for > multiple intents now. Done. https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:37: unregisterReceiver(); On 2016/12/19 16:01:17, Ted C (OOO till 12.27) wrote: > I would do updateDeferredIntent(null) here instead. Then we can ensure it is > set to null in this case. Done. https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:79: } On 2016/12/19 17:55:33, Maria wrote: > style nit: return on the same line, e.g. if (mReceiverRegistered) return; Done. https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DelayedScreenLockIntentHandler.java:94: return; On 2016/12/19 17:55:33, Maria wrote: > style nit: same as above Done. https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java (right): https://codereview.chromium.org/2568843003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:343: mDelayedScreenIntentHandler.updateDeferredIntent(intent); On 2016/12/19 17:55:33, Maria wrote: > On 2016/12/19 16:01:17, Ted C (OOO till 12.27) wrote: > > I think you need to still wrap this in a > > > > if (mDelayedScreenIntentHandler != null) { > > ... > > } > > > > since you lazily create it when the intent is non null the first time. > > I think what Ted means to say is that you should keep the null check here from > before and then guard the update (otherwise lazy creation isn't buying us > anything, this will create a handler on the first intent that comes in because > it's called from onNewIntent) > > e.g. > > if (mDelayedScreenIntentHandler == null && intent != null) { > mDelayedScreenIntentHandler = new ...(); > } > > if (mDelayedScreenIntentHandler != null) { > mDelayedScreenIntentHandler.updateDeferredIntent(intent) > } Done.
Fix comments
lgtm
The CQ bit was checked by wenjinm@amazon.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wenjinm@amazon.com
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2568843003/#ps160001 (title: "Allow deferred when the screen is locked")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1482181973365970,
"parent_rev": "39f7889647e653c06d17c0e5dd4d0f1fac59d578", "commit_rev":
"79a46d5348b590f5cf26b685fbc4445977223dcc"}
Message was sent while issue was closed.
Description was changed from
==========
Allow deferred intent when the screen is locked
The intent might be ignored if the screen is locked and user just input
pin to unlock it. Instead of simply discard the intent, add a deferred
intent and hold it for a short period of time. Once the USER_PRESENT has
been broadcast, check back the deferred intent and move forward with it.
The other option to handle this scenario would be set the intent aside,
and start an async task to continuously for the screen status, and react
once the screen unlocked. This would be less optimal option since it
requires extra background task while screen is locked.
BUG=455126
TEST=out/Default/bin/run_chrome_public_test_apk -f \*IntentHandlerTest\*
also verified by file an intent when screen is locked and then
unlcok screen with pin.
==========
to
==========
Allow deferred intent when the screen is locked
The intent might be ignored if the screen is locked and user just input
pin to unlock it. Instead of simply discard the intent, add a deferred
intent and hold it for a short period of time. Once the USER_PRESENT has
been broadcast, check back the deferred intent and move forward with it.
The other option to handle this scenario would be set the intent aside,
and start an async task to continuously for the screen status, and react
once the screen unlocked. This would be less optimal option since it
requires extra background task while screen is locked.
BUG=455126
TEST=out/Default/bin/run_chrome_public_test_apk -f \*IntentHandlerTest\*
also verified by file an intent when screen is locked and then
unlcok screen with pin.
Review-Url: https://codereview.chromium.org/2568843003
==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from
==========
Allow deferred intent when the screen is locked
The intent might be ignored if the screen is locked and user just input
pin to unlock it. Instead of simply discard the intent, add a deferred
intent and hold it for a short period of time. Once the USER_PRESENT has
been broadcast, check back the deferred intent and move forward with it.
The other option to handle this scenario would be set the intent aside,
and start an async task to continuously for the screen status, and react
once the screen unlocked. This would be less optimal option since it
requires extra background task while screen is locked.
BUG=455126
TEST=out/Default/bin/run_chrome_public_test_apk -f \*IntentHandlerTest\*
also verified by file an intent when screen is locked and then
unlcok screen with pin.
Review-Url: https://codereview.chromium.org/2568843003
==========
to
==========
Allow deferred intent when the screen is locked
The intent might be ignored if the screen is locked and user just input
pin to unlock it. Instead of simply discard the intent, add a deferred
intent and hold it for a short period of time. Once the USER_PRESENT has
been broadcast, check back the deferred intent and move forward with it.
The other option to handle this scenario would be set the intent aside,
and start an async task to continuously for the screen status, and react
once the screen unlocked. This would be less optimal option since it
requires extra background task while screen is locked.
BUG=455126
TEST=out/Default/bin/run_chrome_public_test_apk -f \*IntentHandlerTest\*
also verified by file an intent when screen is locked and then
unlcok screen with pin.
Committed: https://crrev.com/8dec237dce0b0430a9f9c5d7f50bce4943be5554
Cr-Commit-Position: refs/heads/master@{#439566}
==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8dec237dce0b0430a9f9c5d7f50bce4943be5554 Cr-Commit-Position: refs/heads/master@{#439566} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
