extensions: Move ToolbarActionViewDelegateViews methods [chromium/src : main]
Shuhei Takahashi (Gerrit)
Shuhei Takahashi added 2 comments![]( "Open in Gerrit")
return BrowserView::GetBrowserViewForBrowser(browser_)
->toolbar()
->GetExtensionsButton();I assume this code was never executed because `GetReferenceButtonForPopup()` was called only for the toolbar action's delegate.
return GetFocusManager();I assume that `GetFocusManager()` returns the same instance within the same window.
Related details
- Darryl James
- Emilia Paz
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. |
Darryl James (Gerrit)
Darryl James voted and added 2 comments![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
2 comments
return GetFocusManager();I assume that `GetFocusManager()` returns the same instance within the same window.
Yes! As long as the views are all parented to the same widget, `GetFocusManager()` will be the same 👍
Related details
- Emilia Paz
- 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. |
Emilia Paz (Gerrit)
Emilia Paz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Emilia Paz
| Code-Review | +1 |
2 comments
Thanks Shuhei!
return BrowserView::GetBrowserViewForBrowser(browser_)
->toolbar()
->GetExtensionsButton();I assume this code was never executed because `GetReferenceButtonForPopup()` was called only for the toolbar action's delegate.
I would expect this to be triggered when extensions menu button is pressed and popup is triggered. Extensions menu button should be the delegate of the action view controller that is called when the action is executed: `ExtensionsMenuButton::ButtonPressed()` -> ` ExtensionActionViewController::ExecuteUserAction` -> ... -> `ExtensionActionPlatformDelegateViews::ShowPopup` -> `GetDelegateViews()->GetReferenceButtonForPopup()` where the delegate is the extensions menu button.
That being said, if this is called then the popup reference button would be the extensions button (puzzle piece) and not the action in the toolbar.. which is not what happens.
Code is so tangled that maybe somewhere else we are using the other controller. Thus, changing this to be in the container and always returning the toolbar action view as the popup reference button seems like a great improvement!
Tested the CL locally to make sure the popup is correctly anchored and button focused. Looks good!
Related details
- 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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Shuhei Takahashi
| Commit-Queue | +1 |
2 comments
Thanks for reviews! I need your approval again as I lost CR+2 on rebasing.
return BrowserView::GetBrowserViewForBrowser(browser_)
->toolbar()
->GetExtensionsButton();Emilia PazI assume this code was never executed because `GetReferenceButtonForPopup()` was called only for the toolbar action's delegate.
I would expect this to be triggered when extensions menu button is pressed and popup is triggered. Extensions menu button should be the delegate of the action view controller that is called when the action is executed: `ExtensionsMenuButton::ButtonPressed()` -> ` ExtensionActionViewController::ExecuteUserAction` -> ... -> `ExtensionActionPlatformDelegateViews::ShowPopup` -> `GetDelegateViews()->GetReferenceButtonForPopup()` where the delegate is the extensions menu button.
That being said, if this is called then the popup reference button would be the extensions button (puzzle piece) and not the action in the toolbar.. which is not what happens.
Code is so tangled that maybe somewhere else we are using the other controller. Thus, changing this to be in the container and always returning the toolbar action view as the popup reference button seems like a great improvement!Tested the CL locally to make sure the popup is correctly anchored and button focused. Looks good!
- Darryl James
- Emilia Paz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
- Emilia Paz
- Shuhei Takahashi
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 voted Code-Review+1![]( "Open in Gerrit")
- Shuhei Takahashi
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 1 comment![]( "Open in Gerrit")
// Returns the FocusManager to use when registering accelerators.
virtual views::FocusManager* GetFocusManagerForAccelerator() = 0;
// Returns the reference button for the extension action's popup. Rather than
// relying on the button being a MenuButton, the button returned should have a
// MenuButtonController. This is part of the ongoing work from
// http://crbug.com/901183 to simplify the button hierarchy by migrating
// controller logic into a separate class leaving MenuButton as an empty class
// to be deprecated.
virtual views::BubbleAnchor GetReferenceButtonForPopup(
const extensions::ExtensionId& action_id) = 0;I should have caught the use of `views::View` here. This CL will depend on comment thread [here](https://chromium-review.googlesource.com/c/chromium/src/+/7064871/comments/35002115_8ecbf796)
Related details
- 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")
// Returns the FocusManager to use when registering accelerators.
virtual views::FocusManager* GetFocusManagerForAccelerator() = 0;
// Returns the reference button for the extension action's popup. Rather than
// relying on the button being a MenuButton, the button returned should have a
// MenuButtonController. This is part of the ongoing work from
// http://crbug.com/901183 to simplify the button hierarchy by migrating
// controller logic into a separate class leaving MenuButton as an empty class
// to be deprecated.
virtual views::BubbleAnchor GetReferenceButtonForPopup(
const extensions::ExtensionId& action_id) = 0;I should have caught the use of `views::View` here. This CL will depend on comment thread [here](https://chromium-review.googlesource.com/c/chromium/src/+/7064871/comments/35002115_8ecbf796)
Ack. I will put this on hold until we resolve the thread.
Shuhei Takahashi (Gerrit)
Shuhei Takahashi added 1 comment![]( "Open in Gerrit")
// Returns the FocusManager to use when registering accelerators.
virtual views::FocusManager* GetFocusManagerForAccelerator() = 0;
// Returns the reference button for the extension action's popup. Rather than
// relying on the button being a MenuButton, the button returned should have a
// MenuButtonController. This is part of the ongoing work from
// http://crbug.com/901183 to simplify the button hierarchy by migrating
// controller logic into a separate class leaving MenuButton as an empty class
// to be deprecated.
virtual views::BubbleAnchor GetReferenceButtonForPopup(
const extensions::ExtensionId& action_id) = 0;Shuhei TakahashiI should have caught the use of `views::View` here. This CL will depend on comment thread [here](https://chromium-review.googlesource.com/c/chromium/src/+/7064871/comments/35002115_8ecbf796)
Ack. I will put this on hold until we resolve the thread.
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")
// Returns the FocusManager to use when registering accelerators.
virtual views::FocusManager* GetFocusManagerForAccelerator() = 0;
// Returns the reference button for the extension action's popup. Rather than
// relying on the button being a MenuButton, the button returned should have a
// MenuButtonController. This is part of the ongoing work from
// http://crbug.com/901183 to simplify the button hierarchy by migrating
// controller logic into a separate class leaving MenuButton as an empty class
// to be deprecated.
virtual views::BubbleAnchor GetReferenceButtonForPopup(
const extensions::ExtensionId& action_id) = 0;Shuhei TakahashiI should have caught the use of `views::View` here. This CL will depend on comment thread [here](https://chromium-review.googlesource.com/c/chromium/src/+/7064871/comments/35002115_8ecbf796)
Shuhei TakahashiAck. I will put this on hold until we resolve the thread.
Resolved.
Will rebase on crrev.com/c/7064871 once I submit it. Marking this thread unresolved until then.
Related details
- Emilia Paz
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 and added 2 comments![]( "Open in Gerrit")
Votes added by Shuhei Takahashi
| Commit-Queue | +1 |
2 comments
Rebased on top of crrev.com/c/7064871 and I need your reviews again. Thanks!
// Returns the FocusManager to use when registering accelerators.
virtual views::FocusManager* GetFocusManagerForAccelerator() = 0;
// Returns the reference button for the extension action's popup. Rather than
// relying on the button being a MenuButton, the button returned should have a
// MenuButtonController. This is part of the ongoing work from
// http://crbug.com/901183 to simplify the button hierarchy by migrating
// controller logic into a separate class leaving MenuButton as an empty class
// to be deprecated.
virtual views::BubbleAnchor GetReferenceButtonForPopup(
const extensions::ExtensionId& action_id) = 0;Shuhei TakahashiI should have caught the use of `views::View` here. This CL will depend on comment thread [here](https://chromium-review.googlesource.com/c/chromium/src/+/7064871/comments/35002115_8ecbf796)
Shuhei TakahashiAck. I will put this on hold until we resolve the thread.
Shuhei TakahashiResolved.
Will rebase on crrev.com/c/7064871 once I submit it. Marking this thread unresolved until then.
- Darryl James
- Emilia Paz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
1 comment
- Emilia Paz
- Shuhei Takahashi
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 voted Code-Review+1![]( "Open in Gerrit")
- 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: Move ToolbarActionViewDelegateViews methods
This is a part of the patch series to move stateful UI logic from
ExtensionActionViewController to ExtensionActionPlatformDelegate.
This patch moves all methods in ToolbarActionViewDelegateViews to
ExtensionsContainer, namely:
- GetFocusManagerForAccelerator()
- GetReferenceButtonForPopup()
These methods were used only by ExtensionActionPlatformDelegateViews.
Now they call into corresponding methods in ExtensionsContainer
directly.
This ends up leaving no methods in ToolbarActionViewDelegateViews.
This interface will removed in the next patch.
This is a pure refactor. No functional changes are expected.
- M chrome/browser/ui/views/extensions/extension_action_platform_delegate_views.cc
- M chrome/browser/ui/views/extensions/extensions_container_views.h
- M chrome/browser/ui/views/extensions/extensions_menu_button.cc
- M chrome/browser/ui/views/extensions/extensions_menu_button.h
- M chrome/browser/ui/views/extensions/extensions_menu_test_util.cc
- 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_delegate_views.h
- 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, +1 by Darryl James
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |