actions revamp: Open popup via ETB and ExtensionActionDelegate [chromium/src : main]
0 views
Skip to first unread message
Masa Fujita (Gerrit)
Jan 6, 2026, 8:47:10 AM (3 days ago) Jan 6
to chromium...@chromium.org, peilinwa...@google.com, chromium-a...@chromium.org, hanxi...@chromium.org, extension...@chromium.org
Masa Fujita has uploaded the change for review![]( "Open in Gerrit")
Commit message
actions revamp: Open popup via ETB and ExtensionActionDelegate
Change-Id: Ic817bdf8d614a54bbf5e0be4beb4edbaf61150bc
Change diff
Change information
Files:
- M chrome/browser/ui/android/extensions/extension_action_delegate_android.cc
- M chrome/browser/ui/android/extensions/extension_action_delegate_android.h
- M chrome/browser/ui/android/extensions/extension_action_popup_contents.cc
- M chrome/browser/ui/android/extensions/extensions_toolbar_bridge.cc
- M chrome/browser/ui/android/extensions/extensions_toolbar_bridge.h
- M chrome/browser/ui/android/extensions/java/src/org/chromium/chrome/browser/ui/extensions/ExtensionActionPopupContents.java
- M chrome/browser/ui/android/extensions/java/src/org/chromium/chrome/browser/ui/extensions/ExtensionsToolbarBridge.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/extensions/ExtensionActionListMediator.java
Change size: M
Delta: 8 files changed, 72 insertions(+), 15 deletions(-)
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Jan 7, 2026, 5:18:53 AM (2 days ago) Jan 7
to chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Jan 7, 2026, 5:33:47 AM (2 days ago) Jan 7
to Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Jan 7, 2026, 6:14:07 AM (2 days ago) Jan 7
to Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Jan 7, 2026, 7:06:39 AM (2 days ago) Jan 7
to Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Jan 7, 2026, 7:28:16 AM (2 days ago) Jan 7
to Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Jan 7, 2026, 7:50:09 AM (2 days ago) Jan 7
to Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Jan 7, 2026, 9:40:41 AM (2 days ago) Jan 7
to Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Jan 7, 2026, 10:45:39 AM (2 days ago) Jan 7
to Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Jan 7, 2026, 12:29:58 PM (2 days ago) Jan 7
to Emilia Paz, Shuhei Takahashi, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Attention needed from Emilia Paz and Shuhei Takahashi
Masa Fujita added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 15 (Latest):
Masa Fujita . resolved
This is the integration point of ExtensionsToolbar and ETVM for Android :D
Related details
Attention is currently required from:
- Emilia Paz
- Shuhei Takahashi
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Jan 7, 2026, 12:55:37 PM (2 days ago) Jan 7
to Emilia Paz, Shuhei Takahashi, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Attention needed from Emilia Paz and Shuhei Takahashi
Masa Fujita added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Masa Fujita . resolved
This is the integration point of ExtensionsToolbar and ETVM for Android :D
Masa Fujita
Failing tests seem unrelated. Will take a look at the merge conflict tomorrow...
Masa Fujita (Gerrit)
Jan 7, 2026, 10:44:06 PM (2 days ago) Jan 7
to Shuhei Takahashi, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Jan 8, 2026, 5:18:33 AM (yesterday) Jan 8
to Shuhei Takahashi, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Jan 8, 2026, 7:09:14 AM (yesterday) Jan 8
to Shuhei Takahashi, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Jan 8, 2026, 7:34:27 AM (yesterday) Jan 8
to Shuhei Takahashi, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Jan 8, 2026, 12:07:38 PM (23 hours ago) Jan 8
to Shuhei Takahashi, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
Jan 8, 2026, 1:19:41 PM (21 hours ago) Jan 8
to Emilia Paz, Shuhei Takahashi, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Attention needed from Emilia Paz and Shuhei Takahashi
Masa Fujita added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Masa Fujita . resolved
Masa FujitaThis is the integration point of ExtensionsToolbar and ETVM for Android :D
Failing tests seem unrelated. Will take a look at the merge conflict tomorrow...
Masa Fujita
Everything should be fixed now, PTAL! (this CL is not directly related to the `ExtensionContainer` refactor)
Related details
Attention is currently required from:
- Emilia Paz
- Shuhei Takahashi
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emilia Paz (Gerrit)
1:05 AM (10 hours ago) 1:05 AM
to Masa Fujita, Shuhei Takahashi, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Attention needed from Masa Fujita and Shuhei Takahashi
Emilia Paz added 7 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 21 (Latest):
Emilia Paz . resolved
Thanks Masa! I mainly took a look at the cpp code. I'll look at the java side tomorrow.. but wanted to provide some feedback today (meant to send this earlier but forgot to hit send!)
File chrome/browser/ui/android/extensions/extension_action_delegate_android.h
Line 55, Patchset 21 (Latest):
const raw_ptr<extensions::ExtensionsToolbarBridge> bridge_;Emilia Paz . unresolved
nit: rename to `toolbar_bridge` and update comment to reflect its for the extensions toolbar
Line 45, Patchset 21 (Latest):
void ShowContextMenuAsFallback() override /* */;Emilia Paz . unresolved
nit: typo?
File chrome/browser/ui/android/extensions/extension_action_delegate_android.cc
Line 68, Patchset 21 (Latest):
bridge_->TriggerPopup(action_id_, std::move(host));Emilia Paz . unresolved
optional - separate CL: I know we have already named it extensions_toolbar_bridge, but what about re-naming it to extensions_toolbar_delegate_android? It would fit the action and menu structure of having a 'DelegateAndroid' that is used as a bridge. Then, we can have ExtensionsToolbarBridge.java
The benefit of the rename is having the same structure as the other UIs. Also, here `toolbar_delegate_android_->TriggerPopup(...)` it's more clear imo
File chrome/browser/ui/android/extensions/extension_action_popup_contents.cc
Line 148, Patchset 21 (Latest):
extension_view_host_ptr));Emilia Paz . unresolved
Investigate: Is ExtensionViewHost platform-agnostic? If so, would it make more sense to have this at the 'model' level?
File chrome/browser/ui/android/extensions/extensions_menu_delegate_android.cc
Line 33, Patchset 21 (Latest):
// TODO(crbug.com/461981075): Pass a `bridge` instance instead of a nullptr.Emilia Paz . unresolved
investigate: what could be the best way to pass information from 'toolbar bridge' to 'menu delegate android'?
File chrome/browser/ui/android/extensions/extensions_toolbar_bridge.cc
Line 112, Patchset 21 (Latest):
ToolbarActionViewModel* model =
toolbar_view_model_->GetActionModelForId(action_id);
model->ExecuteUserAction(source);Emilia Paz . unresolved
This is platform-agnostic, we could handle it on `ExtensionsToolbarViewModel` which owns the `ToolbarActionViewModel`:
```
toolbar_view_model_->ExecuteUserAction(action_id, source)`
```
Related details
Attention is currently required from:
- Masa Fujita
- Shuhei Takahashi
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
2:30 AM (8 hours ago) 2:30 AM
to Emilia Paz, Shuhei Takahashi, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Attention needed from Emilia Paz and Shuhei Takahashi
Masa Fujita voted and added 4 comments![]( "Open in Gerrit")
Votes added by Masa Fujita
| Commit-Queue | +1 |
4 comments
File chrome/browser/ui/android/extensions/extension_action_delegate_android.h
Line 55, Patchset 21:
const raw_ptr<extensions::ExtensionsToolbarBridge> bridge_;Emilia Paz . resolved
nit: rename to `toolbar_bridge` and update comment to reflect its for the extensions toolbar
Masa Fujita
Done
Line 45, Patchset 21:
void ShowContextMenuAsFallback() override /* */;Emilia Paz . resolved
Masa Fujitanit: typo?
Sorry!
File chrome/browser/ui/android/extensions/extension_action_delegate_android.cc
Line 68, Patchset 21:
bridge_->TriggerPopup(action_id_, std::move(host));Emilia Paz . unresolved
optional - separate CL: I know we have already named it extensions_toolbar_bridge, but what about re-naming it to extensions_toolbar_delegate_android? It would fit the action and menu structure of having a 'DelegateAndroid' that is used as a bridge. Then, we can have ExtensionsToolbarBridge.java
The benefit of the rename is having the same structure as the other UIs. Also, here `toolbar_delegate_android_->TriggerPopup(...)` it's more clear imo
Masa Fujita
`ExtensionsToolbarBridge` will also contain methods that go the opposite way (Java methods trying to get `PinnedActionIds()` etc.) so I thought it might not be ideal to name it delegate, which might imply one direction - WDYT?
File chrome/browser/ui/android/extensions/extensions_toolbar_bridge.cc
Line 112, Patchset 21:
ToolbarActionViewModel* model =
toolbar_view_model_->GetActionModelForId(action_id);
model->ExecuteUserAction(source);Emilia Paz . resolved
This is platform-agnostic, we could handle it on `ExtensionsToolbarViewModel` which owns the `ToolbarActionViewModel`:
```
toolbar_view_model_->ExecuteUserAction(action_id, source)`
```
Attention is currently required from:
- Emilia Paz
- Shuhei Takahashi
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shuhei Takahashi (Gerrit)
2:42 AM (8 hours ago) 2:42 AM
to Masa Fujita, Emilia Paz, Shuhei Takahashi, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Attention needed from Emilia Paz and Masa Fujita
Shuhei Takahashi added 5 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 25:
Shuhei Takahashi . resolved
Sorry for delay, some Java-side comments.
File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
Line 2353, Patchset 25:
// The C++ TabList dies when the selector is destroyed.Shuhei Takahashi . unresolved
We often suffer from this kind of destruction order issue... :(
That said, rather than calling destroy() here, I think we should ensure the correct destruction order in existing destroy() callsites.
File chrome/browser/ui/android/extensions/java/src/org/chromium/chrome/browser/ui/extensions/ExtensionsToolbarBridge.java
Line 29, Patchset 25:
public ExtensionsToolbarBridge(ChromeAndroidTask task) {Shuhei Takahashi . unresolved
Can we take Delegate in the constructor and remove @Nullable from mDelegate? Then we don't need to worry about null in triggerPopup.
File chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/extensions/ExtensionActionListMediator.java
Line 43, Patchset 25:
ExtensionActionListContainer mContainer;Shuhei Takahashi . unresolved
Can it be `private final`?
Line 65, Patchset 25:
mExtensionsToolbarBridge = extensionsToolbarBridge;Shuhei Takahashi . unresolved
Can we construct ExtensionsToolbarBridge here since this class owns it?
Related details
Attention is currently required from:
- Emilia Paz
- Masa Fujita
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Masa Fujita (Gerrit)
2:42 AM (8 hours ago) 2:42 AM
to Emilia Paz, Shuhei Takahashi, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, hanxi...@chromium.org, peilinwa...@google.com
Attention needed from Emilia Paz and Shuhei Takahashi
Masa Fujita voted Commit-Queue+1![]( "Open in Gerrit")
Attention is currently required from:
- Emilia Paz
- Shuhei Takahashi
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages