Refactor: Finalize AppMenuControl Interface Extraction [chromium/src : main]
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 5 comments![]( "Open in Gerrit")
(GetView, ExecuteCommand, GetMenuModel) to the interface and updates allYouseff Bourouphelwait, are these all only needed because of tests? If so, I don't think it's a good idea to expose them to the interface. See the other comments about whether we can use `views::ElementTrackerViews::GetInstance()->GetFirstMatchingView` with `views::AsViewClass<xxx>` in the tests, avoiding exposing these methods in the interface.
Done
Change-Id: I489fb4dc51a193b6eceaf11007fa01445c0f4860Youseff Bourouphelnit: do you want to include `bug: 470045312`?
updated to proper bug
GetAppMenuControl()->ExecuteCommand(command);Youseff Bourouphelsimilarly, would the following code work?
```
auto* button = views::AsViewClass<BrowserAppMenuButton>(
views::ElementTrackerViews::GetInstance()->GetFirstMatchingView(
kToolbarAppMenuButtonElementId,
browser_->window()->GetElementContext()));
button->app_menu()->ExecuteCommand(command, 0);
```
Done
virtual views::View* GetView() = 0;Youseff BourouphelIs this only used in tests? if so, can we get rid of it, like using views::ElementTrackerViews::GetInstance()->GetFirstMatchingView()? See https://chromium-review.git.corp.google.com/c/chromium/src/+/7705932/comment/6ac80970_91019625/.
By using it, the test doesn't need to know where the button lives or how to access its specific C++ getter, it just asks the tracker for "the app menu button" in the current context. When the WebUI Toolbar replaces the native views toolbar, the WebUI app menu button will also need to be tagged with kToolbarAppMenuButtonElementId. Tests using ElementTracker will be much easier to migrate (or simply just work). Remember to include new headers and remove no longer needed headers if changing to it.
If it does not work for your case, we can rename this to GetViewForTest (if it's indeed only used in test)
removed the test only methods
auto* model = toolbar->GetAppMenuControl()->GetMenuModel();Youseff BourouphelWould the following code work instead?
```
auto* button = views::AsViewClass<BrowserAppMenuButton>(
views::ElementTrackerViews::GetInstance()->GetFirstMatchingView(
kToolbarAppMenuButtonElementId,
browser()->window()->GetElementContext()));
auto* model = button->app_menu_model();
```
Done
Related details
- Qingxin Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qingxin Wu (Gerrit)
Qingxin Wu added 7 comments![]( "Open in Gerrit")
{toolbar_button_provider->GetAppMenuControl()->GetAnchor()},Include What You Use.
Remove #include "chrome/browser/ui/views/toolbar/browser_app_menu_button.h", and add #include "chrome/browser/ui/views/toolbar/app_menu_control.h"
(also suggested the same in a few other places. Double chedck if you can remove browser_app_menu_button.h in those places)
#if BUILDFLAG(GOOGLE_CHROME_BRANDING) && \
(BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX))let's just revert to what it was on the left side?
anchor = browser_view->toolbar()->GetAppMenuControl()->GetAnchor();Remove #include "chrome/browser/ui/views/toolbar/browser_app_menu_button.h", and add #include "chrome/browser/ui/views/toolbar/app_menu_control.h"
EXPECT_TRUE(toolbar->GetAppMenuControl()->IsDrawn());Remove #include "chrome/browser/ui/views/toolbar/browser_app_menu_button.h", and add #include "chrome/browser/ui/views/toolbar/app_menu_control.h"
->GetAppMenuControl()Remove #include "chrome/browser/ui/views/toolbar/browser_app_menu_button.h", and add #include "chrome/browser/ui/views/toolbar/app_menu_control.h"
->app_menu_button());see if this is the only place that needs "chrome/browser/ui/views/toolbar/browser_app_menu_button.h". If so, remove the include.
Check other test files as well to see if they can also remove the include.
Related details
- Youseff Bourouphel
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. |
Qingxin Wu (Gerrit)
Qingxin Wu voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Youseff Bourouphel
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. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 7 comments![]( "Open in Gerrit")
{toolbar_button_provider->GetAppMenuControl()->GetAnchor()},Include What You Use.
Remove #include "chrome/browser/ui/views/toolbar/browser_app_menu_button.h", and add #include "chrome/browser/ui/views/toolbar/app_menu_control.h"
(also suggested the same in a few other places. Double chedck if you can remove browser_app_menu_button.h in those places)
Done
#if BUILDFLAG(GOOGLE_CHROME_BRANDING) && \
(BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX))let's just revert to what it was on the left side?
Done
#elseYouseff Bourouphelis this change intentional?
yeah, this else is also in the original. I now updated so they more clearly match.
anchor = browser_view->toolbar()->GetAppMenuControl()->GetAnchor();Remove #include "chrome/browser/ui/views/toolbar/browser_app_menu_button.h", and add #include "chrome/browser/ui/views/toolbar/app_menu_control.h"
Done
Remove #include "chrome/browser/ui/views/toolbar/browser_app_menu_button.h", and add #include "chrome/browser/ui/views/toolbar/app_menu_control.h"
Done
Remove #include "chrome/browser/ui/views/toolbar/browser_app_menu_button.h", and add #include "chrome/browser/ui/views/toolbar/app_menu_control.h"
Done
->app_menu_button());see if this is the only place that needs "chrome/browser/ui/views/toolbar/browser_app_menu_button.h". If so, remove the include.
Check other test files as well to see if they can also remove the include.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qingxin Wu (Gerrit)
Qingxin Wu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Qingxin Wu
| Code-Review | +1 |
1 comment
AppMenuControl* GetAppMenuControl() override;was it intentional to move this up? It's still an override of ToolbarButtonProvider, right?
Related details
- Youseff Bourouphel
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. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 1 comment![]( "Open in Gerrit")
AppMenuControl* GetAppMenuControl() override;was it intentional to move this up? It's still an override of ToolbarButtonProvider, right?
fixed, i had made it public but found a way to not to have to.
Related details
- Qingxin Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qingxin Wu (Gerrit)
Qingxin Wu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Qingxin Wu
| Code-Review | +1 |
- Youseff Bourouphel
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. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 1 comment![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |