[Esc Secondary Activities] Backpress/Escape handling for History Page [chromium/src : main]

[Moe Adel (Gerrit)]

Attention needed from Lijin Shen and Theresa Sullivan

Moe Adel added 1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Moe Adel . resolved

Hi, would you please review this change?
@twell...@chromium.org, this includes the out of the box centralization discussed [here](https://chromium-review.googlesource.com/c/chromium/src/+/6931457/comment/ca224976_9517131f/).

Open in Gerrit

Related details

Attention is currently required from:

• Lijin Shen
• Theresa Sullivan

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I7f6927a5a7e69a72f025dd24d613ce3be651ef0b

Gerrit-Change-Number: 6978162

Gerrit-PatchSet: 4

Gerrit-Owner: Moe Adel <ad...@google.com>

Gerrit-Reviewer: Lijin Shen <laz...@google.com>

Gerrit-Reviewer: Moe Adel <ad...@google.com>

Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Mark Schillaci <mschi...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Theresa Sullivan <twell...@chromium.org>

Gerrit-Attention: Lijin Shen <laz...@google.com>

Gerrit-Comment-Date: Thu, 25 Sep 2025 16:00:58 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Theresa Sullivan (Gerrit)]

Attention needed from Lijin Shen

Theresa Sullivan added 1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Theresa Sullivan . unresolved

Did a (very quick pass) and looks good overall!

@laz...@google.com -- will you please do a detailed pass? Then I'll re-review and add owners approval

Open in Gerrit

Related details

Attention is currently required from:

• Lijin Shen

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I7f6927a5a7e69a72f025dd24d613ce3be651ef0b

Gerrit-Change-Number: 6978162

Gerrit-PatchSet: 4

Gerrit-Owner: Moe Adel <ad...@google.com>

Gerrit-Reviewer: Lijin Shen <laz...@google.com>

Gerrit-Reviewer: Moe Adel <ad...@google.com>

Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Mark Schillaci <mschi...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Lijin Shen <laz...@google.com>

Gerrit-Comment-Date: Thu, 25 Sep 2025 16:18:22 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Lijin Shen (Gerrit)]

Attention needed from Moe Adel

Lijin Shen added 4 comments

Commit Message

Line 7, Patchset 4 (Latest):[Esc Secondary Activities] Backpress/Escape handling for History Page

Lijin Shen . unresolved

Could you please update the commit description to also mention the changes of the bookmark functionality and back press manager refactoring?

Optional nit: for easier review, would you consider splitting it into a few smaller, chained CLs?

File chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java

Line 102, Patchset 4 (Parent):

Lijin Shen . unresolved

unintended change here?

File chrome/android/java/src/org/chromium/chrome/browser/history/HistoryPage.java

Line 78, Patchset 4 (Latest): if (ChromeFeatureList.isEnabled(
ChromeFeatureList.ENABLE_ESCAPE_HANDLING_FOR_SECONDARY_ACTIVITIES)) {

Lijin Shen . unresolved

Since it looks like this logic will be used across multiple activities, what do you think about moving it into a static helper method in BackPressHelper or BackPressManager?

Centralizing it there would also make it easier to gate with a Finch parameter in the future if needed.

File components/browser_ui/widget/android/java/src/org/chromium/components/browser_ui/widget/selectable_list/SelectableListLayout.java

Line 574, Patchset 4 (Latest): if (mToolbar.isLargeScreenWithKeyboard()) {
if (mToolbar.hasSearchText()) {
mToolbar.clearSearch();
return true;

Lijin Shen . unresolved

is this a new behavior change?

Open in Gerrit

Related details

Attention is currently required from:

• Moe Adel

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review
• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I7f6927a5a7e69a72f025dd24d613ce3be651ef0b

Gerrit-Change-Number: 6978162

Gerrit-PatchSet: 4

Gerrit-Owner: Moe Adel <ad...@google.com>

Gerrit-Reviewer: Lijin Shen <laz...@google.com>

Gerrit-Reviewer: Moe Adel <ad...@google.com>

Gerrit-Reviewer: Theresa Sullivan <twell...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Mark Schillaci <mschi...@google.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Moe Adel <ad...@google.com>

Gerrit-Comment-Date: Thu, 25 Sep 2025 18:25:08 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No