Skip to content

Commit

Permalink
Records handlers which failed to consume back press.
Browse files Browse the repository at this point in the history
Refactor BackPressHandler so that it can return a value to indicate
whether back press is correctly consumed. If a handler fails,
the back press will pass to remaining handlers.
Introduce BackPressResult to represent the result of handling back
press. The reason why boolean is not used is because I want to
differentiate between handleBackPress and traditional onBackPressed,
and also to emphasize not handling back press is a failure which
may have already made chrome working inappropriately.

For some handlers it might be hard to verify, so we return UNKNOWN.

This CL also adds a new histogram to record them.

Bug: 1414606, 1414832
Change-Id: Ib841f9626fc30b73682f0f8d079a65288a894850
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4251777
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Donn Denman <donnd@chromium.org>
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Reviewed-by: Sinan Sahin <sinansahin@google.com>
Commit-Queue: Lijin Shen <lazzzis@google.com>
Cr-Commit-Position: refs/heads/main@{#1108448}
  • Loading branch information
Lijin Shen authored and Chromium LUCI CQ committed Feb 22, 2023
1 parent 7a43337 commit 3358fe5
Show file tree
Hide file tree
Showing 40 changed files with 289 additions and 80 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ public boolean onBackPressed() {
}

@Override
public void handleBackPress() {
mMediator.handleBackPress();
public @BackPressResult int handleBackPress() {
return mMediator.handleBackPress();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -298,9 +298,8 @@ boolean onBackPressed() {
}

@Override
public void handleBackPress() {
boolean ret = onBackPressed();
assert ret : "This should only be called when mBackPressChangedSupplier yields true";
public @BackPressResult int handleBackPress() {
return onBackPressed() ? BackPressResult.SUCCESS : BackPressResult.FAILURE;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -973,9 +973,10 @@ void onHide() {
}

@Override
public void handleBackPress() {
onBackPressedInternal();
public @BackPressResult int handleBackPress() {
boolean ret = onBackPressedInternal();
notifyBackPressStateChanged();
return ret ? BackPressResult.SUCCESS : BackPressResult.FAILURE;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -252,8 +252,8 @@ public boolean onBackPressed() {
}

@Override
public void handleBackPress() {
selectTheCurrentTab();
public @BackPressResult int handleBackPress() {
return BackPressResult.FAILURE;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,9 +221,10 @@ public boolean handleBackPressed() {
}

@Override
public void handleBackPress() {
public @BackPressResult int handleBackPress() {
mMediator.hideDialog(true);
RecordUserAction.record("TabGridDialog.Exit");
return isVisible() ? BackPressResult.FAILURE : BackPressResult.SUCCESS;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,8 @@ public boolean onBackPressed() {
}

@Override
public void handleBackPress() {
mMediator.handleBackPress();
public @BackPressResult int handleBackPress() {
return mMediator.handleBackPress();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -416,8 +416,9 @@ public boolean onBackPressed() {
}

@Override
public void handleBackPress() {
if (mTabGridDialogController != null) mTabGridDialogController.handleBackPress();
public @BackPressResult int handleBackPress() {
if (mTabGridDialogController != null) return mTabGridDialogController.handleBackPress();
return BackPressResult.FAILURE;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -291,8 +291,10 @@ public boolean handleBackPressed() {
}

@Override
public void handleBackPress() {
public @BackPressResult int handleBackPress() {
int result = isEditorVisible() ? BackPressResult.SUCCESS : BackPressResult.FAILURE;
mNavigationProvider.goBack();
return result;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -882,9 +882,8 @@ private boolean onBackPressedInternal() {
}

@Override
public void handleBackPress() {
boolean ret = onBackPressedInternal();
assert ret;
public @BackPressResult int handleBackPress() {
return onBackPressedInternal() ? BackPressResult.SUCCESS : BackPressResult.FAILURE;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ public boolean onBackPressed() {
}

@Override
public void handleBackPress() {}
public @BackPressResult int handleBackPress() {
return BackPressResult.FAILURE;
}

@Override
public ObservableSupplier<Boolean> getHandleBackPressChangedSupplier() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ public boolean onBackPressed() {
}

@Override
public void handleBackPress() {
onBackPressed();
public @BackPressResult int handleBackPress() {
return onBackPressed() ? BackPressResult.SUCCESS : BackPressResult.FAILURE;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ public abstract class ChromeActivity<C extends ChromeActivityComponent>
// returned by {@link isInPictureInPictureMode}.
private boolean mLastPictureInPictureModeForTesting;

protected BackPressManager mBackPressManager = new BackPressManager();
protected BackPressManager mBackPressManager = new BackPressManager(this::handleOnBackPressed);
private TextBubbleBackPressHandler mTextBubbleBackPressHandler;
private SelectionPopupBackPressHandler mSelectionPopupBackPressHandler;
private Callback<TabModelSelector> mSelectionPopupBackPressInitCallback;
Expand Down Expand Up @@ -2193,8 +2193,6 @@ protected boolean isFirstActivity() {

/** Handles back press events for Chrome in various states. */
protected final boolean handleOnBackPressed() {
assert !BackPressManager.isEnabled()
: "Back press should be handled by implementors of BackPressHandler if enabled";
RecordUserAction.record(
mNativeInitialized ? "SystemBack" : "SystemBackBeforeNativeInitialized");
if (isActivityFinishingOrDestroyed()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -375,9 +375,8 @@ private void onBackPressStateChanged() {

// BackPressHandler Overrides
@Override
public void handleBackPress() {
var ret = onBackPressed();
assert ret;
public @BackPressResult int handleBackPress() {
return onBackPressed() ? BackPressResult.SUCCESS : BackPressResult.FAILURE;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -987,8 +987,7 @@ public ObservableSupplier<Boolean> getHandleBackPressChangedSupplier() {
}

@Override
public void handleBackPress() {
boolean ret = onBackPressed();
assert ret;
public @BackPressResult int handleBackPress() {
return onBackPressed() ? BackPressResult.SUCCESS : BackPressResult.FAILURE;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1155,14 +1155,14 @@ public boolean onBackPressed() {
}

@Override
public void handleBackPress() {
public @BackPressResult int handleBackPress() {
for (SceneOverlay sceneOverlay : mSceneOverlays) {
Boolean enabled = sceneOverlay.getHandleBackPressChangedSupplier().get();
if (enabled != null && enabled) {
sceneOverlay.handleBackPress();
return;
return sceneOverlay.handleBackPress();
}
}
return BackPressResult.FAILURE;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -379,8 +379,11 @@ public void onClick(View v) {
}

@Override
public void handleBackPress() {
public @BackPressResult int handleBackPress() {
int result =
shouldDeactivateByBackPress() ? BackPressResult.SUCCESS : BackPressResult.FAILURE;
deactivate();
return result;
}

@Override
Expand Down Expand Up @@ -682,7 +685,7 @@ protected void handleDeactivation(boolean clearSelection) {

private void setCurrentState(@FindLocationBarState int state) {
mCurrentState = state;
mBackPressStateSupplier.set(mCurrentState == FindLocationBarState.SHOWN);
mBackPressStateSupplier.set(shouldDeactivateByBackPress());

// Notify the observers if we hit the transition states.
if (mObserver != null) {
Expand All @@ -706,6 +709,11 @@ private void setCurrentState(@FindLocationBarState int state) {
}
}

// Whether the find toolbar should be deactivated by back press.
private boolean shouldDeactivateByBackPress() {
return mCurrentState == FindLocationBarState.SHOWN;
}

/**
* Requests focus for the query input field and shows the keyboard.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -418,11 +418,11 @@ public void onStart() {
}

@Override
public void handleBackPress() {
public @BackPressResult int handleBackPress() {
// Terminate if we are still waiting for the native or for Android EDU / GAIA Child checks.
if (!mPostNativeAndPolicyPagesCreated) {
abortFirstRunExperience();
return;
return BackPressResult.SUCCESS;
}

mFirstRunFlowSequencer.updateFirstRunProperties(mFreProperties);
Expand All @@ -437,6 +437,7 @@ public void handleBackPress() {
} else {
setCurrentItemForPager(position);
}
return BackPressResult.SUCCESS;
}

// FirstRunPageDelegate:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ public ObservableSupplier<Boolean> getHandleBackPressChangedSupplier() {
* Called when back press is intercepted.
*/
@Override
public abstract void handleBackPress();
public abstract @BackPressResult int handleBackPress();

protected void flushPersistentData() {
if (mNativeInitialized) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,9 @@ public void onDestroy() {
}

@Override
public void handleBackPress() {
public @BackPressResult int handleBackPress() {
abortFirstRunExperience();
return BackPressResult.SUCCESS;
}

private void abortFirstRunExperience() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -689,9 +689,8 @@ public boolean onBackPressed() {

// BackPressHandler implementation.
@Override
public void handleBackPress() {
var ret = onBackPressed();
assert ret;
public @BackPressResult int handleBackPress() {
return onBackPressed() ? BackPressResult.SUCCESS : BackPressResult.FAILURE;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,10 @@ public boolean onBackPressed() {
}

@Override
public void handleBackPress() {
assert shouldInterceptBackPress();
public @BackPressResult int handleBackPress() {
int result = shouldInterceptBackPress() ? BackPressResult.SUCCESS : BackPressResult.FAILURE;
mPresenter.dismissCurrentDialog(DialogDismissalCause.NAVIGATE_BACK_OR_TOUCH_OUTSIDE);
return result;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,13 @@ protected void onObservingDifferentTab(Tab tab, boolean hint) {
}

@Override
public void handleBackPress() {
public @BackPressResult int handleBackPress() {
Tab tab = mActivityTabProvider.get();
int result = shouldInterceptBackPress() ? BackPressResult.SUCCESS : BackPressResult.FAILURE;
ReadingListUtils.showReadingList(tab.isIncognito());
WebContents webContents = tab.getWebContents();
if (webContents != null) webContents.dispatchBeforeUnload(false);
return result;
}

@Override
Expand All @@ -53,8 +55,11 @@ public void destroy() {
}

private void onBackPressStateChanged() {
mBackPressChangedSupplier.set(shouldInterceptBackPress());
}

private boolean shouldInterceptBackPress() {
Tab tab = mActivityTabProvider.get();
mBackPressChangedSupplier.set(
tab != null && tab.getLaunchType() == TabLaunchType.FROM_READING_LIST);
return tab != null && tab.getLaunchType() == TabLaunchType.FROM_READING_LIST;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -126,12 +126,13 @@ private void onBackPressStateChanged() {
}

@Override
public void handleBackPress() {
public @BackPressResult int handleBackPress() {
Tab tab = mActivityTabProvider.get();
assert tab != null
&& !tab.canGoBack()
boolean res = tab != null && !tab.canGoBack() && isTabFromStartSurface(tab);
assert res
: String.format("tab %s; back press state %s", tab, tab != null && tab.canGoBack());
mOnBackPressedCallback.run();
return res ? BackPressResult.SUCCESS : BackPressResult.FAILURE;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2123,7 +2123,7 @@ private void onBackPressStateChanged() {
}

@Override
public void handleBackPress() {
public @BackPressResult int handleBackPress() {
boolean ret = back();
if (!ret) {
var bc = mBottomControlsCoordinatorSupplier != null
Expand All @@ -2138,6 +2138,7 @@ public void handleBackPress() {
tab != null && tab.isDestroyed());
assert false : msg;
}
return ret ? BackPressResult.SUCCESS : BackPressResult.FAILURE;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ public boolean hasActiveArSession() {
}

@Override
public void handleBackPress() {
onBackPressed();
public @BackPressResult int handleBackPress() {
return onBackPressed() ? BackPressResult.SUCCESS : BackPressResult.FAILURE;
}

@Override
Expand Down

0 comments on commit 3358fe5

Please sign in to comment.