Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1311913007:
Do not select a Tab while closing all tabs or undoing (Closed)
|
Created:
5 years, 3 months ago by Jaekyun Seok (inactive) Modified:
5 years, 3 months ago CC:
chromium-reviews, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not select a Tab while closing all tabs or undoing
BUG=517060
Committed: https://crrev.com/ff2e6be166f90dd79b48e2e6279dc191c8349050
Cr-Commit-Position: refs/heads/master@{#347332}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Apply Ted's comments #
Total comments: 6
Patch Set 3 : Update getNextTabIfClosed #
Total comments: 2
Patch Set 4 : Apply Ted's comments #Patch Set 5 : Fix UndoTabModelTest #
Messages
Total messages: 18 (6 generated)
jaekyun@chromium.org changed reviewers: + dtrainor@chromium.org, tedchoc@chromium.org
dtrainor@ and tedchoc@, please review this CL.
I'm wondering if we can avoid changing the APIs at all and keep the logic isolated into the tab model base logic. https://codereview.chromium.org/1311913007/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/undo/UndoBarPopupController.java (right): https://codereview.chromium.org/1311913007/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/snackbar/undo/UndoBarPopupController.java:164: cancelTabClosure((Integer) actionData, false); somewhat like my other comment, what if we change cancelTabClosure to only select a tab if none are currently selected. This would change the behavior slightly from this patch, but limiting the api complexity (and the ability for calling sites to get the model into an unexpected state -- i.e. where no tabs are selected) seems preferred to me than giving this amount of control. https://codereview.chromium.org/1311913007/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java (right): https://codereview.chromium.org/1311913007/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:355: while (getCount() > 0) { instead of requiring a bunch of calling sites to change, could we change the logic here to loop over all tabs in the model and set them all as closing before we start calling closeTab? Then we could change the tab selection logic to look for any non-closing tab when determining the next for selection, which would find none in this case thus avoiding the the api change.
PTAL. I modified codes to keep the logic isolated into the tab model base logic. https://codereview.chromium.org/1311913007/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/undo/UndoBarPopupController.java (right): https://codereview.chromium.org/1311913007/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/snackbar/undo/UndoBarPopupController.java:164: cancelTabClosure((Integer) actionData, false); On 2015/09/02 20:30:56, Ted C wrote: > somewhat like my other comment, what if we change cancelTabClosure to only > select a tab if none are currently selected. This would change the behavior > slightly from this patch, but limiting the api complexity (and the ability for > calling sites to get the model into an unexpected state -- i.e. where no tabs > are selected) seems preferred to me than giving this amount of control. Done. https://codereview.chromium.org/1311913007/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java (right): https://codereview.chromium.org/1311913007/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:355: while (getCount() > 0) { On 2015/09/02 20:30:56, Ted C wrote: > instead of requiring a bunch of calling sites to change, could we change the > logic here to loop over all tabs in the model and set them all as closing before > we start calling closeTab? > > Then we could change the tab selection logic to look for any non-closing tab > when determining the next for selection, which would find none in this case thus > avoiding the the api change. Done.
https://codereview.chromium.org/1311913007/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java (right): https://codereview.chromium.org/1311913007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:255: if (wasNothingSelected) { why not check mIndex == INVALID_TAB_INDEX here? mIndex only seems to get incremented if it is greater than the insertIndex which "can't" happen if there is nothing selected. https://codereview.chromium.org/1311913007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:448: tab.hide(); any reason we need to do this now? What was missing about it before? Looking at TabModelSelectorImpl#requestToShowTab, it does more things when actually hiding the tab. https://codereview.chromium.org/1311913007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:454: Tab nextTab = getNextTabIfClosed(closingTabId); Should we change the logic in getNextTabIfClosed to actually check for isClosing internally to try and return something usable here?
PTAL. https://codereview.chromium.org/1311913007/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java (right): https://codereview.chromium.org/1311913007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:255: if (wasNothingSelected) { On 2015/09/03 18:26:21, Ted C wrote: > why not check mIndex == INVALID_TAB_INDEX here? > > mIndex only seems to get incremented if it is greater than the insertIndex which > "can't" happen if there is nothing selected. Done. https://codereview.chromium.org/1311913007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:448: tab.hide(); On 2015/09/03 18:26:21, Ted C wrote: > any reason we need to do this now? What was missing about it before? > > Looking at TabModelSelectorImpl#requestToShowTab, it does more things when > actually hiding the tab. Removed. https://codereview.chromium.org/1311913007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:454: Tab nextTab = getNextTabIfClosed(closingTabId); On 2015/09/03 18:26:21, Ted C wrote: > Should we change the logic in getNextTabIfClosed to actually check for isClosing > internally to try and return something usable here? Done.
lgtm https://codereview.chromium.org/1311913007/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java (right): https://codereview.chromium.org/1311913007/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:403: for (Tab tab : mTabs) { I would use the for (int i = 0; i < ...) version since it avoids the allocation of an iterator.
https://codereview.chromium.org/1311913007/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java (right): https://codereview.chromium.org/1311913007/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java:403: for (Tab tab : mTabs) { On 2015/09/04 00:06:19, Ted C wrote: > I would use the for (int i = 0; i < ...) version since it avoids the allocation > of an iterator. Done.
The CQ bit was checked by jaekyun@chromium.org
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/1311913007/#ps60001 (title: "Apply Ted's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1311913007/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1311913007/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by jaekyun@chromium.org
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/1311913007/#ps80001 (title: "Fix UndoTabModelTest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1311913007/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1311913007/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ff2e6be166f90dd79b48e2e6279dc191c8349050 Cr-Commit-Position: refs/heads/master@{#347332} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
