Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2448433003:
Finish ChromeTabbedActivity if its tabs have been merged into another instance (Closed)
DescriptionFinish ChromeTabbedActivity if its tabs have been merged into another instance
If Chrome's process is restarted while two ChromeTabbedActivity instances are running
it is possible to end up in an odd state where tabs are merged into one instance
and the task for the second instance is still running. This second task doesn't
have an activity associated with it yet, so we cannot kill the task when we
merge into the first instance. To work around this, when the second instance is
resumed it is finished immediately if its tabs have been merged into another instance.
BUG=657418
Committed: https://crrev.com/73988c5b3c8f504ad9372b07c9a493660ca286cf
Cr-Commit-Position: refs/heads/master@{#427441}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Finish ChromeTabbedActivity if its tabs have been merged into another instance #Patch Set 4 : Use #isStartedUpCorrectly #Patch Set 5 : Remove extra blank lines #
Total comments: 4
Patch Set 6 : Changes from dfalcantara@ review #Patch Set 7 : Fix typo #Messages
Total messages: 29 (22 generated)
The CQ bit was checked by twellington@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by twellington@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...
The CQ bit was checked by twellington@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...
twellington@chromium.org changed reviewers: + dfalcantara@chromium.org
ptal
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 twellington@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...
The CQ bit was checked by twellington@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/2448433003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2448433003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1100: // If tabs were merged into a different ChromeTabbedActivity instance, then this If tabs from this instance? https://codereview.chromium.org/2448433003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1103: return sMergedInstanceTaskId == 0 || sMergedInstanceTaskId == getTaskId(); I'm worried about cases where the user can't get back into Chrome because this ID incorrectly stays set. Should the merged instance task ID be reset in more cases than the one below? Does it make sense to reset it to 0 if sMergedInstanceTaskId != getTaskId()?
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 twellington@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/2448433003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2448433003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1100: // If tabs were merged into a different ChromeTabbedActivity instance, then this On 2016/10/25 18:33:10, dfalcantara (slow 10.24) wrote: > If tabs from this instance? Done. https://codereview.chromium.org/2448433003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1103: return sMergedInstanceTaskId == 0 || sMergedInstanceTaskId == getTaskId(); On 2016/10/25 18:33:10, dfalcantara (slow 10.24) wrote: > I'm worried about cases where the user can't get back into Chrome because this > ID incorrectly stays set. Should the merged instance task ID be reset in more > cases than the one below? Does it make sense to reset it to 0 if > sMergedInstanceTaskId != getTaskId()? Done.
lgtm
The CQ bit was checked by twellington@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2448433003/#ps120001 (title: "Fix typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Finish ChromeTabbedActivity if its tabs have been merged into another instance If Chrome's process is restarted while two ChromeTabbedActivity instances are running it is possible to end up in an odd state where tabs are merged into one instance and the task for the second instance is still running. This second task doesn't have an activity associated with it yet, so we cannot kill the task when we merge into the first instance. To work around this, when the second instance is resumed it is finished immediately if its tabs have been merged into another instance. BUG=657418 ========== to ========== Finish ChromeTabbedActivity if its tabs have been merged into another instance If Chrome's process is restarted while two ChromeTabbedActivity instances are running it is possible to end up in an odd state where tabs are merged into one instance and the task for the second instance is still running. This second task doesn't have an activity associated with it yet, so we cannot kill the task when we merge into the first instance. To work around this, when the second instance is resumed it is finished immediately if its tabs have been merged into another instance. BUG=657418 Committed: https://crrev.com/73988c5b3c8f504ad9372b07c9a493660ca286cf Cr-Commit-Position: refs/heads/master@{#427441} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/73988c5b3c8f504ad9372b07c9a493660ca286cf Cr-Commit-Position: refs/heads/master@{#427441} |
