Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2791383002:
Fix Incorrect webapp behavior on Kitkat when reusing old WebappActivity. (Closed)
DescriptionFix Incorrect webapp behavior on Kitkat when reusing old WebappActivity.
If we creates 11 PWA shortcuts and open them all, the last one uses one of
the already existing activities but isn't navigated to the new URL. To fix this,
we finish the existing activity if it is still running, and therefore, a new
WebappActivity will be created and splash screen will show up too.
BUG=702998
Review-Url: https://codereview.chromium.org/2791383002
Cr-Commit-Position: refs/heads/master@{#462115}
Committed: https://chromium.googlesource.com/chromium/src/+/50b74d197d3c435b1b3d1dd590239b7f40ba90ee
Patch Set 1 #
Total comments: 2
Patch Set 2 : pkotwicz@'s comments. #
Total comments: 8
Patch Set 3 : Nits. #
Total comments: 1
Patch Set 4 : Nits. #
Messages
Total messages: 22 (12 generated)
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
Is it also possible to remove the id comparison in WebappActivity#onNewIntent() ? https://codereview.chromium.org/2791383002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2791383002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:140: activityName += String.valueOf(activityIndex); Nit: new line
We only want to finish the old activity if the WebappActivity id is different. (We don't want to restart the WebappActivity when a user taps the homescreen shortcut for the same WebappActivity)
The CQ bit was checked by hanxi@chromium.org 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...
https://codereview.chromium.org/2791383002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2791383002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:142: // Finishes the old activity if it has been used before. See crbug.com/702998. I have not tested this. I wonder whether the FLAG_ACTIVITY_CLEAR_TASK is equivalent to this code?
https://codereview.chromium.org/2791383002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2791383002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:140: activityName += String.valueOf(activityIndex); On 2017/04/04 02:46:07, pkotwicz wrote: > Nit: new line Done. https://codereview.chromium.org/2791383002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2791383002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:142: // Finishes the old activity if it has been used before. See crbug.com/702998. On 2017/04/04 14:11:35, pkotwicz wrote: > I have not tested this. I wonder whether the FLAG_ACTIVITY_CLEAR_TASK is > equivalent to this code? It does the finish part, but it also finishes the current (not old) activity when user clicks the shortcut from the home screen again. We need to keep the current logic.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits https://codereview.chromium.org/2791383002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2791383002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:106: ApiCompatibilityUtils.finishAndRemoveTask(this); We can probably simplify this logic to: if (newWebappInfo != null && newWebappInfo.shouldForceNavigation() && mIsInitialized) { ... } And remove the ApiCompatibilityUtils.finishAndRemoveTask() part https://codereview.chromium.org/2791383002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2791383002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:142: // Finishes the old activity if it has been used before. See crbug.com/702998. How about: "Finish the old activity if it has been assigned to a different WebappActivity." https://codereview.chromium.org/2791383002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:149: WebappActivity webappActivity = (WebappActivity) activity; For the sake of consistency: if (TextUtils.equals(webappActivity.mWebappInfo.id(), info.id()) { continue; }
Description was changed from ========== Fix Incorrect webapp behavior on Kitkat when reusing old WebappActivity. If we creates 11 PWA shortcuts and open them all, the last one uses one of the already existing activity but isn't navigate to the new URL. To fix this, we finish the existing activity if it is still running, and therefore, a new WebappActivity will be created and splash screen will show up too. BUG=702998 ========== to ========== Fix Incorrect webapp behavior on Kitkat when reusing old WebappActivity. If we creates 11 PWA shortcuts and open them all, the last one uses one of the already existing activities but isn't navigated to the new URL. To fix this, we finish the existing activity if it is still running, and therefore, a new WebappActivity will be created and splash screen will show up too. BUG=702998 ==========
hanxi@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi Dan: could you please take a look? Thanks! https://codereview.chromium.org/2791383002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2791383002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:106: ApiCompatibilityUtils.finishAndRemoveTask(this); On 2017/04/04 17:24:12, pkotwicz wrote: > We can probably simplify this logic to: > > if (newWebappInfo != null && newWebappInfo.shouldForceNavigation() && > mIsInitialized) { > ... > } > > And remove the ApiCompatibilityUtils.finishAndRemoveTask() part Hmm, this involves behavior changes when we can't parse the new Intent. Defer it to the OWNER. https://codereview.chromium.org/2791383002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2791383002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:142: // Finishes the old activity if it has been used before. See crbug.com/702998. On 2017/04/04 17:24:13, pkotwicz wrote: > How about: "Finish the old activity if it has been assigned to a different > WebappActivity." Done. https://codereview.chromium.org/2791383002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:149: WebappActivity webappActivity = (WebappActivity) activity; On 2017/04/04 17:24:12, pkotwicz wrote: > For the sake of consistency: > > if (TextUtils.equals(webappActivity.mWebappInfo.id(), info.id()) { > continue; > } The logic isn't the same with this change. As long as we find the webappActivity with the same class name, we will break from the for loop, no matter whether the id equals.
lgtm https://codereview.chromium.org/2791383002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java (right): https://codereview.chromium.org/2791383002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappLauncherActivity.java:146: if (activity == null || !(activity instanceof WebappActivity) activity instanceof WebappActivity is false if activity == null, so you don't need to check that explicitly
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2791383002/#ps60001 (title: "Nits.")
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": 60001, "attempt_start_ts": 1491404460306180,
"parent_rev": "38034c40428f4556b87606fe7069c57cc37a5607", "commit_rev":
"b1905a4a93d426c5b71162112ccbf6ddbaaa8101"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491404460306180,
"parent_rev": "ae47bdf7f95637309d76c3b68fbe49d26aaeb790", "commit_rev":
"50b74d197d3c435b1b3d1dd590239b7f40ba90ee"}
Message was sent while issue was closed.
Description was changed from ========== Fix Incorrect webapp behavior on Kitkat when reusing old WebappActivity. If we creates 11 PWA shortcuts and open them all, the last one uses one of the already existing activities but isn't navigated to the new URL. To fix this, we finish the existing activity if it is still running, and therefore, a new WebappActivity will be created and splash screen will show up too. BUG=702998 ========== to ========== Fix Incorrect webapp behavior on Kitkat when reusing old WebappActivity. If we creates 11 PWA shortcuts and open them all, the last one uses one of the already existing activities but isn't navigated to the new URL. To fix this, we finish the existing activity if it is still running, and therefore, a new WebappActivity will be created and splash screen will show up too. BUG=702998 Review-Url: https://codereview.chromium.org/2791383002 Cr-Commit-Position: refs/heads/master@{#462115} Committed: https://chromium.googlesource.com/chromium/src/+/50b74d197d3c435b1b3d1dd59023... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/50b74d197d3c435b1b3d1dd59023... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
