ui: Refactor file dialog handling into BrowserSelectFileDialogController [chromium/src : main]
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz added 5 comments![]( "Open in Gerrit")
Missed a few things in the initial request - left a comment for some nice changes we can bundle into this CL.
Bug: nonenit: let's tag the bug `496674143`
browser()->tab_strip_model()->GetActiveWebContents(),
browser()->window()->GetNativeWindow(),It looks like all invocations of `BrowserSelectFileDialogController::OpenFile` pass the currently active web contents and the browser's native window.
I think this is a good opportunity to update the `BrowserSelectFileDialogController` constructor to also take a `TabStripModel*`, `ui::BaseWindow*` and `content::PageNavigator*` param, store them as a `raw_ref` and use them when `OpenFile` is called instead of having to pass them to the method each time.
E.g. the constructor would end up looking like
```
BrowserSelectFileDialogController::BrowserSelectFileDialogController(
Profile* profile,
TabStripModel* tab_strip_model,
ui::BaseWindow* base_window,
content::PageNavigator* page_navigator)
: profile_(CHECK_DEREF(profile)),
tab_strip_model_(CHECK_DEREF(tab_strip_model)),
base_window_(CHECK_DEREF(base_window)),
page_navigator_(CHECK_DEREF(page_navigator)) {}
```
Similar to how its done for [`BrowserLiveTabContext`](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/browser_live_tab_context.cc;l=93;drc=c165bab1ca702bf8e7f9539808505a2269deea4b).
Note: We might have to move where the `BrowserSelectFileDialog` is currently constructed in `BrowserWindowFeatures::Init()` to `BrowserWindowFeatures::InitPostWindowConstruction()` (so that the `BrowserWindow` is constructed for the `ui::BaseWindow`).
base::BindOnce(&Browser::OnFileSelectedFromDialog,wrt above - for context on why we pass by pointer instead of by reference, see the second last paragraph in the style guide section [here](https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs).
It's a little controversial but the reason is effectively there's a risk the reference param can unintentionally bind a temporary, which would result in logic and lifetime issues if retained by the callee.
void OnFileSelectedFromDialog(const GURL& url);I think we can actually remove this, and using the `content::PageNavigator` param in `BrowserSelectFileDialogController` we can directly call `PageNavigator::OpenURL` using the same params as what `Browser` uses (browser call to `OpenURL` below.
```
OpenURL(OpenURLParams(url, Referrer(), WindowOpenDisposition::CURRENT_TAB,
ui::PAGE_TRANSITION_TYPED, false),
/*navigation_handle_callback=*/{});
```
Related details
- Nathan
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. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz added 4 comments![]( "Open in Gerrit")
Change looks awesome! Just a few minor nits
void OpenFile();nit: Lets move this declaration below the destructor below (guidance for ordering methods is [here](https://google.github.io/styleguide/cppguide.html#Declaration_Order)).
// to notify anymore. The controller just waits for the next invocation.nit: We don't need to reset the callback anymore (doesn't exist) but we should keep the `select_file_dialog_.reset();` just to keep behavior as similar as possible to the original implementation.
BrowserSelectFileDialogController::OpenFile()nit: This probably needs to be
```
browser()
->GetFeatures()
.browser_select_file_dialog_controller()
->OpenFile();
```
Also in addition to building chrome it may help to build the `chrome browser_tests unit_tests` targets since there are often test files that are missed in just a regular chrome compile.
Related details
- Nathan
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. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz added 1 comment![]( "Open in Gerrit")
if (browser_select_file_dialog_controller_) {
browser_select_file_dialog_controller_->reset();
}This needs to be `.reset()` so it applies to the `std::unique_ptr`. When you use the `->` operator, the definition of `std::unique_ptr` dereferences the templated type as a pointer and call a method on it.
You can also avoid the if-check since even if the `std::unique_ptr` is empty `.reset()` simply no-ops.
Related details
- Nathan
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. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz added 2 comments![]( "Open in Gerrit")
->->GetFeatures()nit: only one 1 `->` dereference necessary
#include "chrome/browser/ui/browser_window/public/browser_window_features.h"nit: It looks like we also need to add the `chrome/browser/ui/browser_select_file_dialog_controller.h` include here (transitive include issue again only on linux 😞).
fyi chrome is trying to do more include-what-you-use to avoid these sorts of issues going forward.
Related details
- Nathan
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. |
Nathan (Gerrit)
Nathan added 9 comments![]( "Open in Gerrit")
nit: let's tag the bug `496674143`
Acknowledged
nit: only one 1 `->` dereference necessary
Marked as resolved.
browser()->tab_strip_model()->GetActiveWebContents(),
browser()->window()->GetNativeWindow(),It looks like all invocations of `BrowserSelectFileDialogController::OpenFile` pass the currently active web contents and the browser's native window.
I think this is a good opportunity to update the `BrowserSelectFileDialogController` constructor to also take a `TabStripModel*`, `ui::BaseWindow*` and `content::PageNavigator*` param, store them as a `raw_ref` and use them when `OpenFile` is called instead of having to pass them to the method each time.
E.g. the constructor would end up looking like
```
BrowserSelectFileDialogController::BrowserSelectFileDialogController(
Profile* profile,
TabStripModel* tab_strip_model,
ui::BaseWindow* base_window,
content::PageNavigator* page_navigator)
: profile_(CHECK_DEREF(profile)),
tab_strip_model_(CHECK_DEREF(tab_strip_model)),
base_window_(CHECK_DEREF(base_window)),
page_navigator_(CHECK_DEREF(page_navigator)) {}
```Similar to how its done for [`BrowserLiveTabContext`](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/browser_live_tab_context.cc;l=93;drc=c165bab1ca702bf8e7f9539808505a2269deea4b).
Note: We might have to move where the `BrowserSelectFileDialog` is currently constructed in `BrowserWindowFeatures::Init()` to `BrowserWindowFeatures::InitPostWindowConstruction()` (so that the `BrowserWindow` is constructed for the `ui::BaseWindow`).
Acknowledged
I think we can actually remove this, and using the `content::PageNavigator` param in `BrowserSelectFileDialogController` we can directly call `PageNavigator::OpenURL` using the same params as what `Browser` uses (browser call to `OpenURL` below.
```
OpenURL(OpenURLParams(url, Referrer(), WindowOpenDisposition::CURRENT_TAB,
ui::PAGE_TRANSITION_TYPED, false),
/*navigation_handle_callback=*/{});
```
Acknowledged
nit: Lets move this declaration below the destructor below (guidance for ordering methods is [here](https://google.github.io/styleguide/cppguide.html#Declaration_Order)).
Acknowledged
// to notify anymore. The controller just waits for the next invocation.nit: We don't need to reset the callback anymore (doesn't exist) but we should keep the `select_file_dialog_.reset();` just to keep behavior as similar as possible to the original implementation.
Acknowledged
if (browser_select_file_dialog_controller_) {
browser_select_file_dialog_controller_->reset();
}This needs to be `.reset()` so it applies to the `std::unique_ptr`. When you use the `->` operator, the definition of `std::unique_ptr` dereferences the templated type as a pointer and call a method on it.
You can also avoid the if-check since even if the `std::unique_ptr` is empty `.reset()` simply no-ops.
Done
#include "chrome/browser/ui/browser_window/public/browser_window_features.h"nit: It looks like we also need to add the `chrome/browser/ui/browser_select_file_dialog_controller.h` include here (transitive include issue again only on linux 😞).
fyi chrome is trying to do more include-what-you-use to avoid these sorts of issues going forward.
Done
nit: This probably needs to be
```
browser()
->GetFeatures()
.browser_select_file_dialog_controller()
->OpenFile();
```
Also in addition to building chrome it may help to build the `chrome browser_tests unit_tests` targets since there are often test files that are missed in just a regular chrome compile.
- Thomas Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz added 5 comments![]( "Open in Gerrit")
Looks great! Seems like the bots will pass, at this point just a few remaining nits
// Whether the specified WebContents can be saved.
// Saving can be disabled e.g. for the DevTools window.
bool CanSaveContents(content::WebContents* web_contents) const;nit: It looks like we should be able to remove this (I don't see it used anywhere, perhaps an artifact from a previous patchset).
#include "chrome/browser/ui/browser_finder.h"nit: We can drop this include (doesn't look necessary - and hopefully completely deprecated now!)
#include "chrome/browser/ui/browser_window.h"nit: We should be able to replace this with the `ui::BaseWindow` include `#include ui/base/base_window.h`
#include "content/public/browser/web_contents.h"nit: We should add `content/public/browser/page_navigator.h` for iwyu also
Related details
- Nathan
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. |
Nathan (Gerrit)
Nathan added 6 comments![]( "Open in Gerrit")
// Whether the specified WebContents can be saved.
// Saving can be disabled e.g. for the DevTools window.
bool CanSaveContents(content::WebContents* web_contents) const;nit: It looks like we should be able to remove this (I don't see it used anywhere, perhaps an artifact from a previous patchset).
Done
nit: We can drop this include (doesn't look necessary - and hopefully completely deprecated now!)
Done
nit: We should be able to replace this with the `ui::BaseWindow` include `#include ui/base/base_window.h`
Done
nit: We should be able to replace this with the `ui::BaseWindow` include `#include ui/base/base_window.h`
Done
nit: We should add `content/public/browser/page_navigator.h` for iwyu also
Done
nit: We should add `content/public/browser/page_navigator.h` for iwyu also
- Thomas Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz added 3 comments![]( "Open in Gerrit")
bool Browser::CanSaveContents(content::WebContents* web_contents) const {
return chrome::CanSavePage(this);
}nit: We can remove this impl also
void OpenFile();nit: Let's add a comment
```
// Opens a file selection dialog for the browser's currently active tab.
```
void ShowUi(const std::string& /* name */) override {You can just leave this as `const std::string& name` (we typically just keep the param name even if unused by convention).
Related details
- Nathan
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. |
Nathan (Gerrit)
Nathan added 3 comments![]( "Open in Gerrit")
bool Browser::CanSaveContents(content::WebContents* web_contents) const {
return chrome::CanSavePage(this);
}nit: We can remove this impl also
Done
void OpenFile();nit: Let's add a comment
```
// Opens a file selection dialog for the browser's currently active tab.
```
Done
void ShowUi(const std::string& /* name */) override {You can just leave this as `const std::string& name` (we typically just keep the param name even if unused by convention).
- Thomas Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Thomas Lukaszewicz
| Code-Review | +1 |
2 comments
#include "chrome/browser/ui/browser_finder.h"Probably due to another ToT change breaking a transitive include -_-.
Tested locally and adding
```
#include "chrome/browser/ui/browser_select_file_dialog_controller.h"
```
fixes the build.
Related details
- Nathan
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. |
Nathan (Gerrit)
Nathan added 1 comment![]( "Open in Gerrit")
#include "chrome/browser/ui/browser_finder.h"Probably due to another ToT change breaking a transitive include -_-.
Tested locally and adding
```
#include "chrome/browser/ui/browser_select_file_dialog_controller.h"
```fixes the build.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Lukaszewicz
| Code-Review | +1 |
1 comment
Related details
- Hidehiko Abe
- Nathan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe voted and added 1 comment![]( "Open in Gerrit")
Votes added by Hidehiko Abe
| Code-Review | +1 |
1 comment
- Nathan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Nathan
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
ui: Refactor file dialog handling into BrowserSelectFileDialogController
Decouple the file dialog initialization and event handling logic out of
the monolithic Browser class and into a dedicated UI feature controller.
This simplifies Browser and prepares it for cleaner dependency injection
and localized test management.
- M AUTHORS
- M chrome/browser/ash/file_manager/DEPS
- M chrome/browser/ash/file_manager/file_manager_browsertest_base.cc
- M chrome/browser/ui/browser.cc
- M chrome/browser/ui/browser.h
- M chrome/browser/ui/browser_command_controller.cc
- M chrome/browser/ui/browser_select_file_dialog_controller.cc
- M chrome/browser/ui/browser_select_file_dialog_controller.h
- M chrome/browser/ui/browser_window/internal/browser_window_features.cc
- M chrome/browser/ui/views/extensions/extension_dialog_bounds_browsertest.cc
- M chrome/browser/ui/views/select_file_dialog_extension/select_file_dialog_extension_browsertest.cc
Code-Review: +1 by Thomas Lukaszewicz, +1 by Hidehiko Abe
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |