Skip to content

Commit

Permalink
[Undo Refocus] Add didCloseAlone boolean to #willCloseTab to differen…
Browse files Browse the repository at this point in the history
…tiate single vs multiple tab closure. Used to determine refocus during undo

Functionality changes are in TabModelImpl (added a new param to willCloseTab) and UndoRefocusHelper (using the new param). Rest of the classes were updated due to the new param and should have no impact.

Use case this handles:
GTS contains: tab1 |tab2 tab3| tab4 with (tab2, tab3) in a group and tab3 marked as selected
Close group -> Close tab1 -> Undo -> Undo

Before fix:  group is selected after both undo
After fix: tab1 is selected after both undo

Bug: 1351406
Change-Id: I8990ef6b8bcb12679f5d33a0ca64ae7e33c50943
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3828046
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Commit-Queue: Sirisha Kavuluru <skavuluru@google.com>
Reviewed-by: Neil Coronado <nemco@google.com>
Cr-Commit-Position: refs/heads/main@{#1035081}
  • Loading branch information
Sirisha Kavuluru authored and Chromium LUCI CQ committed Aug 15, 2022
1 parent 6b567cb commit 9ee1473
Show file tree
Hide file tree
Showing 27 changed files with 93 additions and 97 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ public void testPasswordTabRestoredWhenClosingTabIsUndone() {
mLastMockWebContents, AccessoryTabType.PASSWORDS, new PropertyProvider<>());

// Simulate closing the tab (uncommitted):
mMediator.getTabModelObserverForTesting().willCloseTab(tab, false);
mMediator.getTabModelObserverForTesting().willCloseTab(tab, false, true);
mMediator.getTabObserverForTesting().onHidden(tab, TabHidingType.CHANGED_TABS);
getStateForBrowserTab().getWebContentsObserverForTesting().wasHidden();
// The state should be kept if the closure wasn't committed.
Expand Down Expand Up @@ -1185,7 +1185,7 @@ private void switchBrowserTab(ManualFillingMediator mediator, @Nullable Tab from
* @param tabToBeClosed The mocked {@link Tab} to be closed. Needs |getId()|.
*/
private void closeBrowserTab(ManualFillingMediator mediator, Tab tabToBeClosed) {
mediator.getTabModelObserverForTesting().willCloseTab(tabToBeClosed, false);
mediator.getTabModelObserverForTesting().willCloseTab(tabToBeClosed, false, true);
mediator.getTabObserverForTesting().onHidden(tabToBeClosed, TabHidingType.CHANGED_TABS);
mCache.getStateFor(mLastMockWebContents).getWebContentsObserverForTesting().wasHidden();
mLastMockWebContents = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ public void onTabModelSelected(TabModel newModel, TabModel oldModel) {
// normal tabs.
mNormalTabModelObserver = new TabModelObserver() {
@Override
public void willCloseTab(Tab tab, boolean animate) {
public void willCloseTab(Tab tab, boolean animate, boolean didCloseAlone) {
if (mStartSurfaceState == StartSurfaceState.SHOWN_HOMEPAGE
&& mTabModelSelector.getModel(false).getCount() <= 1) {
setTabCarouselVisibility(false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -396,13 +396,13 @@ public void hideTabCarouselWhenClosingLastTab() {
assertThat(mPropertyModel.get(IS_TAB_CAROUSEL_VISIBLE), equalTo(true));
assertThat(mPropertyModel.get(IS_TAB_CAROUSEL_TITLE_VISIBLE), equalTo(true));

mTabModelObserverCaptor.getValue().willCloseTab(mock(Tab.class), false);
mTabModelObserverCaptor.getValue().willCloseTab(mock(Tab.class), false, true);
assertThat(mPropertyModel.get(IS_SHOWING_OVERVIEW), equalTo(true));
assertThat(mPropertyModel.get(IS_TAB_CAROUSEL_VISIBLE), equalTo(true));
assertThat(mPropertyModel.get(IS_TAB_CAROUSEL_TITLE_VISIBLE), equalTo(true));

doReturn(1).when(mNormalTabModel).getCount();
mTabModelObserverCaptor.getValue().willCloseTab(mock(Tab.class), false);
mTabModelObserverCaptor.getValue().willCloseTab(mock(Tab.class), false, true);
assertThat(mPropertyModel.get(IS_SHOWING_OVERVIEW), equalTo(true));
assertThat(mPropertyModel.get(IS_TAB_CAROUSEL_VISIBLE), equalTo(false));
assertThat(mPropertyModel.get(IS_TAB_CAROUSEL_TITLE_VISIBLE), equalTo(false));
Expand All @@ -426,7 +426,7 @@ public void reshowTabCarouselWhenTabClosureUndone() {
verify(mNormalTabModel).addObserver(mTabModelObserverCaptor.capture());

mediator.setStartSurfaceState(StartSurfaceState.SHOWN_HOMEPAGE);
mTabModelObserverCaptor.getValue().willCloseTab(mock(Tab.class), false);
mTabModelObserverCaptor.getValue().willCloseTab(mock(Tab.class), false, true);
assertThat(mPropertyModel.get(IS_SHOWING_OVERVIEW), equalTo(true));
assertThat(mPropertyModel.get(IS_TAB_CAROUSEL_VISIBLE), equalTo(false));
assertThat(mPropertyModel.get(IS_TAB_CAROUSEL_TITLE_VISIBLE), equalTo(false));
Expand All @@ -438,10 +438,10 @@ public void reshowTabCarouselWhenTabClosureUndone() {

doReturn(2).when(mNormalTabModel).getCount();
mediator.setStartSurfaceState(StartSurfaceState.SHOWN_TABSWITCHER);
mTabModelObserverCaptor.getValue().willCloseTab(mock(Tab.class), false);
mTabModelObserverCaptor.getValue().willCloseTab(mock(Tab.class), false, true);
mTabModelObserverCaptor.getValue().tabClosureUndone(mock(Tab.class));
doReturn(0).when(mNormalTabModel).getCount();
mTabModelObserverCaptor.getValue().willCloseTab(mock(Tab.class), false);
mTabModelObserverCaptor.getValue().willCloseTab(mock(Tab.class), false, true);
assertThat(mPropertyModel.get(IS_TAB_CAROUSEL_VISIBLE), equalTo(false));
assertThat(mPropertyModel.get(IS_TAB_CAROUSEL_TITLE_VISIBLE), equalTo(false));
assertThat(mPropertyModel.get(IS_SECONDARY_SURFACE_VISIBLE), equalTo(true));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ public void didSelectTab(Tab tab, int type, int lastId) {
}

@Override
public void willCloseTab(Tab tab, boolean animate) {
public void willCloseTab(Tab tab, boolean animate, boolean didCloseAlone) {
List<Tab> relatedTabs = getRelatedTabs(tab.getId());
// If the group is empty, update the animation and hide the dialog.
if (relatedTabs.size() == 0) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ && getTabsToShowForId(lastId).contains(tab)) {
}

@Override
public void willCloseTab(Tab tab, boolean animate) {
public void willCloseTab(Tab tab, boolean animate, boolean didCloseAlone) {
if (!mIsTabGroupUiVisible) return;
// The strip should hide when users close the second-to-last tab in strip. The
// tabCountToHide for group is 1 because tab group status is updated with this
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -715,7 +715,7 @@ public void didAddTab(
}

@Override
public void willCloseTab(Tab tab, boolean animate) {
public void willCloseTab(Tab tab, boolean animate, boolean didCloseAlone) {
if (mModel.indexFromId(tab.getId()) == TabModel.INVALID_TAB_INDEX) return;
tab.removeObserver(mTabObserver);
mModel.removeAt(mModel.indexFromId(tab.getId()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public void didAddTab(Tab tab, int type, @TabCreationState int creationState) {
}

@Override
public void willCloseTab(Tab tab, boolean animate) {
public void willCloseTab(Tab tab, boolean animate, boolean didCloseAlone) {
if (isEditorVisible()) hide();
}
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ public void restoreCompleted() {
}

@Override
public void willCloseTab(Tab tab, boolean animate) {
public void willCloseTab(Tab tab, boolean animate, boolean didCloseAlone) {
if (mTabModelSelector.getCurrentModel().getCount() == 1) {
messageItemsController.removeAllAppendedMessage();
} else if (mPriceMessageService != null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public void didAddTab(
}

@Override
public void willCloseTab(Tab tab, boolean animate) {
public void willCloseTab(Tab tab, boolean animate, boolean didCloseAlone) {
mSnackbarManager.dismissSnackbars(UndoGroupSnackbarController.this);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public void tabClosureUndone(Tab tab) {
}

@Override
public void willCloseTab(Tab tab, boolean animate) {
public void willCloseTab(Tab tab, boolean animate, boolean didCloseAlone) {
onTabContextChanged(TabContextChangeReason.TAB_CLOSED);
}
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ public void tabClosure_NotLast_NotCurrent() {
mModel.set(TabGridPanelProperties.HEADER_TITLE, null);
mModel.set(TabGridPanelProperties.IS_DIALOG_VISIBLE, true);

mTabModelObserverCaptor.getValue().willCloseTab(mTab2, false);
mTabModelObserverCaptor.getValue().willCloseTab(mTab2, false, true);

// Current tab ID should not update.
assertThat(mMediator.getCurrentTabIdForTesting(), equalTo(TAB1_ID));
Expand All @@ -465,7 +465,7 @@ public void tabClosure_NotLast_Current() {
mModel.set(TabGridPanelProperties.HEADER_TITLE, null);
mModel.set(TabGridPanelProperties.IS_DIALOG_VISIBLE, true);

mTabModelObserverCaptor.getValue().willCloseTab(mTab2, false);
mTabModelObserverCaptor.getValue().willCloseTab(mTab2, false, true);

// Current tab ID should be updated to TAB1_ID now.
assertThat(mMediator.getCurrentTabIdForTesting(), equalTo(TAB1_ID));
Expand All @@ -483,7 +483,7 @@ public void tabClosure_Last_Current() {
mModel.set(TabGridPanelProperties.ANIMATION_SOURCE_VIEW, mView);
mModel.set(TabGridPanelProperties.IS_DIALOG_VISIBLE, true);

mTabModelObserverCaptor.getValue().willCloseTab(mTab1, false);
mTabModelObserverCaptor.getValue().willCloseTab(mTab1, false, true);

assertThat(mModel.get(TabGridPanelProperties.ANIMATION_SOURCE_VIEW), equalTo(null));
verify(mDialogController).resetWithListOfTabs(null);
Expand All @@ -506,7 +506,7 @@ public void tabClosure_NotLast_Current_WithDialogHidden() {
mModel.set(TabGridPanelProperties.HEADER_TITLE, null);
mModel.set(TabGridPanelProperties.IS_DIALOG_VISIBLE, false);

mTabModelObserverCaptor.getValue().willCloseTab(mTab2, false);
mTabModelObserverCaptor.getValue().willCloseTab(mTab2, false, true);

// Current tab ID should be updated to TAB1_ID now.
assertThat(mMediator.getCurrentTabIdForTesting(), equalTo(TAB1_ID));
Expand Down Expand Up @@ -538,7 +538,7 @@ public void tabClosure_NonRootTab_StillGroupAfterClosure_WithStoredTitle() {
assertThat(mTabGroupTitleEditor.getTabGroupTitle(
CriticalPersistedTabData.from(mTab1).getRootId()),
equalTo(CUSTOMIZED_DIALOG_TITLE));
mTabModelObserverCaptor.getValue().willCloseTab(newTab, false);
mTabModelObserverCaptor.getValue().willCloseTab(newTab, false, true);

// Dialog title should still be the stored title.
assertThat(
Expand All @@ -563,7 +563,7 @@ public void tabClosure_RootTab_StillGroupAfterClosure_WithStoredTitle() {
// Mock that we have a stored title stored with reference to root ID of newTab.
doReturn(CUSTOMIZED_DIALOG_TITLE).when(mTabGroupTitleEditor).getTabGroupTitle(TAB3_ID);

mTabModelObserverCaptor.getValue().willCloseTab(newTab, false);
mTabModelObserverCaptor.getValue().willCloseTab(newTab, false, true);

// Dialog title should still be the stored title even if the root tab is closed.
assertThat(
Expand All @@ -586,7 +586,7 @@ public void tabClosure_SingleTabAfterClosure_WithStoredTitle() {
// Mock that we have a stored title stored with reference to root ID of tab1.
doReturn(CUSTOMIZED_DIALOG_TITLE).when(mTabGroupTitleEditor).getTabGroupTitle(TAB1_ID);

mTabModelObserverCaptor.getValue().willCloseTab(mTab2, false);
mTabModelObserverCaptor.getValue().willCloseTab(mTab2, false, true);

// Even if there is a stored title for tab1, it is now a single tab, so we won't show the
// stored title.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@ public void tabClosure_NotLastTabInGroup() {
initAndAssertProperties(mTab2);

// Mock closing tab 2, and tab 3 then gets selected. They are in the same group.
mTabModelObserverArgumentCaptor.getValue().willCloseTab(mTab2, true);
mTabModelObserverArgumentCaptor.getValue().willCloseTab(mTab2, true, true);
mTabModelObserverArgumentCaptor.getValue().didSelectTab(
mTab3, TabSelectionType.FROM_CLOSE, TAB2_ID);

Expand All @@ -541,7 +541,7 @@ public void tabClosure_LastTabInGroup_GroupUiNotVisible() {
initAndAssertProperties(mTab1);

// Mock closing tab 1, and tab 2 then gets selected. They are in different group.
mTabModelObserverArgumentCaptor.getValue().willCloseTab(mTab1, true);
mTabModelObserverArgumentCaptor.getValue().willCloseTab(mTab1, true, true);
mTabModelObserverArgumentCaptor.getValue().didSelectTab(
mTab2, TabSelectionType.FROM_CLOSE, TAB1_ID);

Expand All @@ -557,11 +557,11 @@ public void tabClosure_LastTabInGroup_GroupUiVisible() {

// Mock closing tab 2 and tab, then tab 1 gets selected. They are in different group. Right
// now tab group UI is visible.
mTabModelObserverArgumentCaptor.getValue().willCloseTab(mTab2, true);
mTabModelObserverArgumentCaptor.getValue().willCloseTab(mTab2, true, true);
mTabModelObserverArgumentCaptor.getValue().didSelectTab(
mTab3, TabSelectionType.FROM_CLOSE, TAB2_ID);
doReturn(new ArrayList<>()).when(mTabGroupModelFilter).getRelatedTabList(TAB3_ID);
mTabModelObserverArgumentCaptor.getValue().willCloseTab(mTab3, true);
mTabModelObserverArgumentCaptor.getValue().willCloseTab(mTab3, true, true);
mTabModelObserverArgumentCaptor.getValue().didSelectTab(
mTab1, TabSelectionType.FROM_CLOSE, TAB3_ID);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,7 @@ public void tabClosure() {

assertThat(mModel.size(), equalTo(2));

mTabModelObserverCaptor.getValue().willCloseTab(mTab2, false);
mTabModelObserverCaptor.getValue().willCloseTab(mTab2, false, true);

assertThat(mModel.size(), equalTo(1));
assertThat(mModel.get(0).model.get(TabProperties.TAB_ID), equalTo(TAB1_ID));
Expand All @@ -696,7 +696,7 @@ public void tabClosure_IgnoresUpdatesForTabsOutsideOfModel() {
initAndAssertAllProperties();

mTabModelObserverCaptor.getValue().willCloseTab(
prepareTab(TAB3_ID, TAB3_TITLE, TAB3_URL), false);
prepareTab(TAB3_ID, TAB3_TITLE, TAB3_URL), false, true);

assertThat(mModel.size(), equalTo(2));
}
Expand Down Expand Up @@ -2315,7 +2315,7 @@ public void testTabObserverRemovedFromClosedTab() {
mMediator.setActionOnAllRelatedTabsForTesting(true);

assertThat(mModel.size(), equalTo(2));
mTabModelObserverCaptor.getValue().willCloseTab(mTab2, false);
mTabModelObserverCaptor.getValue().willCloseTab(mTab2, false, true);
verify(mTab2).removeObserver(mTabObserverCaptor.getValue());
assertThat(mModel.size(), equalTo(1));
assertThat(mModel.get(0).model.get(TabProperties.TAB_ID), equalTo(TAB1_ID));
Expand All @@ -2327,7 +2327,7 @@ public void testTabObserverReattachToUndoClosedTab() {
mMediator.setActionOnAllRelatedTabsForTesting(true);

assertThat(mModel.size(), equalTo(2));
mTabModelObserverCaptor.getValue().willCloseTab(mTab2, false);
mTabModelObserverCaptor.getValue().willCloseTab(mTab2, false, true);
assertThat(mModel.size(), equalTo(1));

// Assume that TabModelFilter is already updated to reflect closed tab is undone.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -604,12 +604,12 @@ public void removeMessageItemsWhenCloseLastTab() {
initAndAssertAllProperties();
// Mock that mTab1 is not the only tab in the current tab model and it will be closed.
doReturn(2).when(mTabModel).getCount();
mTabModelObserverCaptor.getValue().willCloseTab(mTab1, false);
mTabModelObserverCaptor.getValue().willCloseTab(mTab1, false, true);
verify(mMessageItemsController, never()).removeAllAppendedMessage();

// Mock that mTab1 is the only tab in the current tab model and it will be closed.
doReturn(1).when(mTabModel).getCount();
mTabModelObserverCaptor.getValue().willCloseTab(mTab1, false);
mTabModelObserverCaptor.getValue().willCloseTab(mTab1, false, true);
verify(mMessageItemsController).removeAllAppendedMessage();
}

Expand All @@ -634,17 +634,17 @@ public void removePriceWelcomeMessageWhenCloseBindingTab() {

doReturn(1).when(mTabModel).getCount();
doReturn(TAB1_ID).when(mPriceMessageService).getBindingTabId();
mTabModelObserverCaptor.getValue().willCloseTab(mTab1, false);
mTabModelObserverCaptor.getValue().willCloseTab(mTab1, false, true);
verify(mPriceWelcomeMessageController, times(0)).removePriceWelcomeMessage();

doReturn(2).when(mTabModel).getCount();
doReturn(TAB2_ID).when(mPriceMessageService).getBindingTabId();
mTabModelObserverCaptor.getValue().willCloseTab(mTab1, false);
mTabModelObserverCaptor.getValue().willCloseTab(mTab1, false, true);
verify(mPriceWelcomeMessageController, times(0)).removePriceWelcomeMessage();

doReturn(2).when(mTabModel).getCount();
doReturn(TAB1_ID).when(mPriceMessageService).getBindingTabId();
mTabModelObserverCaptor.getValue().willCloseTab(mTab1, false);
mTabModelObserverCaptor.getValue().willCloseTab(mTab1, false, true);
verify(mPriceWelcomeMessageController, times(1)).removePriceWelcomeMessage();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public void testMoveTab() {
public void testCloseTab() {
TabContextObserverTestHelper tabContextObserverTestHelper =
new TabContextObserverTestHelper(mTabModelSelector);
tabContextObserverTestHelper.mTabModelObserver.willCloseTab(null, false);
tabContextObserverTestHelper.mTabModelObserver.willCloseTab(null, false, true);
Assert.assertEquals(TabContextObserver.TabContextChangeReason.TAB_CLOSED,
tabContextObserverTestHelper.getChangeReason());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ public void didSelectTab(Tab tab, @TabSelectionType int type, int lastId) {
}

@Override
public void willCloseTab(Tab tab, boolean animate) {
public void willCloseTab(Tab tab, boolean animate, boolean didCloseAlone) {
// If this is the last tab to close, make sure a signal is sent to the observers.
if (mTabModelSelector.getCurrentModel().getCount() <= 1) {
triggerActivityTabChangeEvent(null);
Expand Down

0 comments on commit 9ee1473

Please sign in to comment.