extensions: Remove context menu callbacks from EAVC [chromium/src : main]
Shuhei Takahashi (Gerrit)
Shuhei Takahashi voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Emilia Paz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shuhei Takahashi (Gerrit)
Shuhei Takahashi added 1 comment![]( "Open in Gerrit")
kerenzhu@: PTAL for chrome/browser/ui/webui_browser. Thanks!
Related details
- Emilia Paz
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu voted and added 1 comment![]( "Open in Gerrit")
Related details
- Emilia Paz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shuhei Takahashi (Gerrit)
Shuhei Takahashi added 1 comment![]( "Open in Gerrit")
Thanks Keren! But I need your approval again after rebasing... thanks!
Related details
- Emilia Paz
- Keren Zhu
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)
Emilia Paz added 3 comments![]( "Open in Gerrit")
Idea: what about having the observer on ExtensionToolbarContainer? That way we avoid going: extension context menu controller -> toolbar action view | extension menu item view -> extensions toolbar container. Then, the container can handle it if the source is toolbar action view.
We could also have the observer in `ExtensionsToolbarContainerViewController`, which my idea was to make it platform-agnostic and move the platform dependent to `ExtensionToolbarContainer` or introduce a `ExtensionsToolbarContainerPlatformDelegateViews`
explicit ExtensionContextMenuController(nit: can drop `explicit`
void ExtensionMenuItemView::OnContextMenuShown() {}nit: Lets add a comment that there are no side effects to the menu item when the context menu is shown (that way empty methods looks intentional vs incomplete)
-- although also see proposal in other comment
Related details
- Keren Zhu
- Shuhei Takahashi
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)
Shuhei Takahashi added 1 comment![]( "Open in Gerrit")
Idea: what about having the observer on ExtensionToolbarContainer? That way we avoid going: extension context menu controller -> toolbar action view | extension menu item view -> extensions toolbar container. Then, the container can handle it if the source is toolbar action view.
We could also have the observer in `ExtensionsToolbarContainerViewController`, which my idea was to make it platform-agnostic and move the platform dependent to `ExtensionToolbarContainer` or introduce a `ExtensionsToolbarContainerPlatformDelegateViews`
Thanks for your suggestion! Let me check if I understand it correctly. The change is going to be like:
- Add the `ContextMenuSource` argument to `ExtensionContextMenuController::Observer::OnContextMenu{Shown,Closed}()`
- Make `ExtensionToolbarContainer` directly implement `ExtensionContextMenuController::Observer`. It will handle the event only when the source is the toolbar.
- Drop the `ExtensionContextMenuController::Observer` implementation from `ToolbarActionView` and `ExtensionMenuItemView`.
- Add the `ExtensionContextMenuController::Observer*` argument to the constructor of `ToolbarActionView` and `ExtensionMenuItemView`.
- Let `ToolbarActionView` and `ExtensionMenuItemView` register the given observer as the observer of `ExtensionContextMenuController`.
Am I right?
Related details
- Emilia Paz
- Keren Zhu
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. |
Emilia Paz (Gerrit)
Emilia Paz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Emilia Paz
| Code-Review | +1 |
1 comment
Shuhei TakahashiIdea: what about having the observer on ExtensionToolbarContainer? That way we avoid going: extension context menu controller -> toolbar action view | extension menu item view -> extensions toolbar container. Then, the container can handle it if the source is toolbar action view.
We could also have the observer in `ExtensionsToolbarContainerViewController`, which my idea was to make it platform-agnostic and move the platform dependent to `ExtensionToolbarContainer` or introduce a `ExtensionsToolbarContainerPlatformDelegateViews`
Thanks for your suggestion! Let me check if I understand it correctly. The change is going to be like:
- Add the `ContextMenuSource` argument to `ExtensionContextMenuController::Observer::OnContextMenu{Shown,Closed}()`
- Make `ExtensionToolbarContainer` directly implement `ExtensionContextMenuController::Observer`. It will handle the event only when the source is the toolbar.
- Drop the `ExtensionContextMenuController::Observer` implementation from `ToolbarActionView` and `ExtensionMenuItemView`.
- Add the `ExtensionContextMenuController::Observer*` argument to the constructor of `ToolbarActionView` and `ExtensionMenuItemView`.
- Let `ToolbarActionView` and `ExtensionMenuItemView` register the given observer as the observer of `ExtensionContextMenuController`.
Am I right?
(1), (2) and (3) correct
For registering the observer, I was thinking of using `ScopedObservation` in `ExtensionsToolbarContainer` and register it directly... but that means the extensions toolbar container needs a reference to extensions context menu controller and that wouldn't work because we have two controllers (toolbar action view and menu item view). Just realized that.
So for my original proposal, you are right. We would need to pass `ExtensionContextMenuController::Observer*` to the constructor of `ToolbarActionView and ExtensionMenuItemView`.. and that would be tricky because we would need to pass `extensions_toolbar_container` to the views.
I was trying to avoid adding more stuff to `ToolbarActionView::Delegate`.. but seems to be the best path. Sorry for the back and forwards, it's a bit hard to see all the connections with all the tangled code :')
Related details
- Keren Zhu
- Shuhei Takahashi
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. |
Keren Zhu (Gerrit)
Keren Zhu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Keren Zhu
| Code-Review | +1 |
1 comment
- Shuhei Takahashi
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)
Shuhei Takahashi added 2 comments![]( "Open in Gerrit")
Shuhei TakahashiIdea: what about having the observer on ExtensionToolbarContainer? That way we avoid going: extension context menu controller -> toolbar action view | extension menu item view -> extensions toolbar container. Then, the container can handle it if the source is toolbar action view.
We could also have the observer in `ExtensionsToolbarContainerViewController`, which my idea was to make it platform-agnostic and move the platform dependent to `ExtensionToolbarContainer` or introduce a `ExtensionsToolbarContainerPlatformDelegateViews`
Emilia PazThanks for your suggestion! Let me check if I understand it correctly. The change is going to be like:
- Add the `ContextMenuSource` argument to `ExtensionContextMenuController::Observer::OnContextMenu{Shown,Closed}()`
- Make `ExtensionToolbarContainer` directly implement `ExtensionContextMenuController::Observer`. It will handle the event only when the source is the toolbar.
- Drop the `ExtensionContextMenuController::Observer` implementation from `ToolbarActionView` and `ExtensionMenuItemView`.
- Add the `ExtensionContextMenuController::Observer*` argument to the constructor of `ToolbarActionView` and `ExtensionMenuItemView`.
- Let `ToolbarActionView` and `ExtensionMenuItemView` register the given observer as the observer of `ExtensionContextMenuController`.
Am I right?
(1), (2) and (3) correct
For registering the observer, I was thinking of using `ScopedObservation` in `ExtensionsToolbarContainer` and register it directly... but that means the extensions toolbar container needs a reference to extensions context menu controller and that wouldn't work because we have two controllers (toolbar action view and menu item view). Just realized that.So for my original proposal, you are right. We would need to pass `ExtensionContextMenuController::Observer*` to the constructor of `ToolbarActionView and ExtensionMenuItemView`.. and that would be tricky because we would need to pass `extensions_toolbar_container` to the views.
I was trying to avoid adding more stuff to `ToolbarActionView::Delegate`.. but seems to be the best path. Sorry for the back and forwards, it's a bit hard to see all the connections with all the tangled code :')
No worries, thanks for putting a thought on this!
Thanks for reviews!
Shuhei Takahashi (Gerrit)
Shuhei Takahashi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Shuhei Takahashi
| Commit-Queue | +1 |
1 comment
Removed on top of crrev.com/c/7064871 and I need your approval again. Thanks!
Related details
- Emilia Paz
- Keren Zhu
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)
Shuhei Takahashi voted Commit-Queue+0![]( "Open in Gerrit")
| Commit-Queue | +0 |
Shuhei Takahashi (Gerrit)
Shuhei Takahashi voted and added 2 comments![]( "Open in Gerrit")
Votes added by Shuhei Takahashi
| Commit-Queue | +1 |
2 comments
explicit ExtensionContextMenuController(Shuhei Takahashinit: can drop `explicit`
Done
nit: Lets add a comment that there are no side effects to the menu item when the context menu is shown (that way empty methods looks intentional vs incomplete)
-- although also see proposal in other comment
Oops, overlooked this comment earlier. Added comments.
Related details
- Emilia Paz
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shuhei Takahashi (Gerrit)
Shuhei Takahashi added 1 comment![]( "Open in Gerrit")
Removed on top of crrev.com/c/7064871 and I need your approval again. Thanks!
I meant, rebased.
Emilia Paz (Gerrit)
Emilia Paz voted Code-Review+1![]( "Open in Gerrit")
- Keren Zhu
- Shuhei Takahashi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
extensions: Remove context menu callbacks from EAVC
This is a part of the patch series to move stateful UI logic from
ExtensionActionViewController to ExtensionActionPlatformDelegate.
ExtensionsToolbarContainer needs to observe shown/closed events for
context menus attached to toolbar action buttons to update some internal
states. And we used to notify these events in the following path:
ExtensionContextMenuController::ShowContextMenuForViewImpl()
-> ToolbarActionViewController::OnContextMenuShown()
= ExtensionActionViewController::OnContextMenuShown()
-> ExtensionsContainer::OnContextMenuShownFromToolbar()
= ExtensionsToolbarContainer::OnContextMenuShownFromToolbar()
To reduce UI logic from ExtensionActionViewController, we don't want
ExtensionActionViewController to appear in the middle.
To achieve this, this patch reworks how these events are notified. We
introduce the observer interface for ExtensionContextMenuController that
allows its owner to subscribe to context menu events. The observer
interface is implemented by two classes with following behavior:
1. ToolbarActionView: notifies its owner via a delegate.
2. ExtensionMenuItemView: does nothing.
ToolbarActionView::Delegate is already implemented by
ExtensionsToolbarContainer, so it can detect context menu events.
The path for the example above will become:
ExtensionContextMenuController::ShowContextMenuForViewImpl()
-> ExtensionContextMenuController::Observer::OnContextMenuShown()
= ToolbarActionView::OnContextMenuShown()
-> ToolbarActionView::Delegate::OnContextMenuShown()
= ExtensionsToolbarContainer::OnContextMenuShown()
This is a pure refactor. No functional changes are expected.
- M chrome/browser/ui/extensions/extension_action_view_controller.cc
- M chrome/browser/ui/extensions/extension_action_view_controller.h
- M chrome/browser/ui/toolbar/toolbar_action_view_controller.h
- M chrome/browser/ui/views/extensions/extension_context_menu_controller.cc
- M chrome/browser/ui/views/extensions/extension_context_menu_controller.h
- M chrome/browser/ui/views/extensions/extensions_container_views.h
- M chrome/browser/ui/views/extensions/extensions_menu_item_view.cc
- M chrome/browser/ui/views/extensions/extensions_menu_item_view.h
- M chrome/browser/ui/views/extensions/extensions_toolbar_container.cc
- M chrome/browser/ui/views/extensions/extensions_toolbar_container.h
- M chrome/browser/ui/views/toolbar/toolbar_action_view.cc
- M chrome/browser/ui/views/toolbar/toolbar_action_view.h
- M chrome/browser/ui/views/toolbar/toolbar_action_view_unittest.cc
- M chrome/browser/ui/webui_browser/webui_browser_extensions_container.cc
- M chrome/browser/ui/webui_browser/webui_browser_extensions_container.h
Code-Review: +1 by Emilia Paz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |