Refactor SubAppsInstallDialogController to separate view and logic [chromium/src : main]
Edman Anjos (Gerrit)
Edman Anjos added 9 comments![]( "Open in Gerrit")
controller->callback_ = std::move(callback);Edman AnjosPass to the constructor directly?
Done
views::Widget* SubAppsInstallDialogController::CreateWidget(Edman AnjosCan't we switch to `constrained_window::ShowWebModalDialogViewsOwned()`? It will make the stored `raw_ptr` a bit less ugly and b) automatically destroy the widget once `AddCallInfo` goes out of scope?
Changed to be tab owned
base::Unretained(controller_ptr)),Edman AnjosCan you share why it's safe to use `base::Unretained()` here? Can we use `base::WeakPtr` instead?
Done, changed to weak_ptr.
My rationale in that PS was that the widget owns the controller, but I'm not entirely sure I missed something. The close flow of dialogs looks complicated.
.SetDialogDestroyingCallback(base::BindOnce(Dibyajyoti PalIt might be a bit clearer to inherit `SubAppsInstallDialogController` from `ui::DialogModelDelegate` and pass it to `ui::DialogModel::Builder()` so that all closures use the same unretained ptr.
Or, given that this looks like a tab-scoped dialog, rely on [tab_dialog_manager](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/web_apps/protocol_handler_picker_coordinator.cc;l=225-226;drc=4641a043d7914ce31515751c357158d46f4c937a;bpv=0;bpt=1)
Edman AnjosThis, or it might also be nice to completely move the `views` specific logic to a separate entity (like a function of some sorts), so that the controller and view is separated, as [said here](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/chrome_browser_design_principles.md#:~:text=For%20simple%20features%20that%20do%20not%20require%20data%20persistence%2C%20we%20only%20require%20separation%20of%20controller%20from%20view.).
We do this for all web app related dialogs. We have the [controller](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/web_apps/web_app_modal_dialog_delegate.h), and then the views are separate, like [this dialog](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/web_apps/web_app_simple_install_dialog.cc).
Regarding the use of `tab_dialog_manager`, from the comments above `SubAppsInstallDialogController::Show()`, it seems like it is a browser modal dialog? In that case, `CreateBrowserModalDialogViews()` SGTM.
I split the view/controller, and IMO the result is much better. Thanks a lot for the pointers to `web_app_simple_install_dialog`, very helpful!
I'm not sure about the `CreateBrowserModalDialogViews` vs `ShowWebModalDialogViews` choice. I think it should be a tab modal dialog. IWAs don't open in a tab, but I'm not entirely sure what should happen to the dialog e.g. on navigation. If we surface subapps to PWAs, then probably tab modal makes more sense?
I'm currently using `ShowWebModalDialogViews`. Let me know what you think.
if (callback_) {Edman AnjosI'd `CHECK()` here and elsewhere? There's also a magic thing called `Widget::MakeCloseSynchronous()`, but I don't remember what exactly it does
Done, added CHECK in OnAccept.
There's also a magic thing called Widget::MakeCloseSynchronous(), but I don't remember what exactly it does
It's not clear to me what to do with this. If this is still relevant, can you please phrase the comment in an actionable way?
void SubAppsInstallDialogController::OnClose() {Dibyajyoti PalI believe the dialog destroying callback will also be called when the dialog is being canceled.
Edman AnjosHaving `base::Unretained()` in the callbacks here make me scared that since the controller gets destroyed on cancelling it (as it's lifetime is tied to a widget), the dialog destruction callback runs again, and tries to access `callback_`. Won't that create reentrancy issues?
It's weird that the `DialogCancelled` test does not crash here, I wonder if there are better semantics of widget ownership that can be used here. Maybe `AddCallInfo` can store a unique pointer to the widget so that the order of destruction can be controlled with the more fine-grained `MakeCloseSynchronous()` [1]?
Done, changed to weak ptrs.
My understanding of the call ordering here is the callback passed to `SetDialogDestroyingCallback` is called before the widget is destroyed, not after. So the controller should still be alive at that point.
void SubAppsInstallDialogController::OnCancel() {
if (callback_) {
std::move(callback_).Run(false);
}
}
void SubAppsInstallDialogController::OnClose() {
if (callback_) {
std::move(callback_).Run(false);
}
}Edman AnjosIf both of them are doing the same thing, can we instead make this one function (say `SubAppsIntallDialogController::OnClose()` and use it for both the callback in `AddCancelButton()` and `SetDialogDestroyingCallback()`?
Done
There is the slight difference that `OnCancel` is called at most once and `callback_` must be valid, while `OnClose` is called at least once and `callback_` may be invalid.
I unified this in `OnClose` and tried to explain this in a comment.
install_dialog_widget->CloseWithReason(Dibyajyoti PalHmmm, Gemini pointed out an edge case that might be happening here.
If this is being called when `AddCallInfo` is being destructed, the callback being run as part of destruction is `SubAppsServiceImpl::ProcessDialogResponse()`. Will the `CHECK_DEREF` call fail in that case?
Edman AnjosAsynchronous widget closing can be a massive problem with reentrancy issues, can we instead use `MakeCloseSynchronous()` [1], and store an `unique_ptr<>` to the widget here so that the lifetime is more properly controlled? It seems like `CloseWithReason()` is not suggested anymmore to be used [2].
[1] https://source.chromium.org/chromium/chromium/src/+/main:ui/views/widget/widget.h;l=876-902;bpv=1;bpt=1?q=views::Widget&ss=chromium%2Fchromium%2Fsrc
[2] https://source.chromium.org/chromium/chromium/src/+/main:ui/views/widget/widget.h;l=866-868;bpv=1;bpt=1?q=views::Widget&ss=chromium%2Fchromium%2Fsrc
I've change the ownership of the widget from `AddCallInfo` to being a tab modal. I believe that fixes the reentrancy concerns.
Related details
- Andrew Rayskiy
- Dibyajyoti Pal
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted Code-Review+1![]( "Open in Gerrit")
- Dibyajyoti Pal
- Edman Anjos
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal voted and added 9 comments![]( "Open in Gerrit")
Votes added by Dibyajyoti Pal
| Code-Review | +1 |
9 comments
Not a lot of blocking feedback, so LGTM'ing to unblock.
HandleAutomaticActionForTesting( // IN-TESTIs this being used anywhere? If not, can we remove?
I find this a bit weird, that we completely skip the dialog for testing, [but we have precedence for it in web_applications/ code](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/test/web_app_install_test_utils.cc;l=53-63?q=AcceptDialogCallback) anyway so it's fine.
.AddOkButton(One quick thing to note here, by default the focus will be on the `OK` button. So it could be possible that a "malicious" IWA uses keyloggers to press Enter fast enough to automatically install a bunch of sub apps. Consider making either the Cancel button as the default, or the NONE button [1] using `OverrideDefaultButton()`.
// To be called when the dialog is cancelled, dismissed, or destryoed.Please fix this WARNING reported by Spellchecker: "destryoed" is a possible misspelling of "destroyed".
To bypass Spellchecker, a...
"destryoed" is a possible misspelling of "destroyed".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
SetAutomaticActionForTesting(DialogActionForTesting auto_accept);nit: Add a comment about what this function does ("used to accept/cancel the dialog in testing").
return true;This condition at first seemed unreachable, as `g_dialog_override_for_testing` is guaranteed to either exist with values, or to be `std::nullopt`, but then I realized this is to return true if `g_dialog_override_for_testing` is set after the callback has run. Maybe we can rewrite as the following using early exits to improve readability?
```
bool SubAppsInstallDialogController::
HandleAutomaticActionForTesting( // IN-TEST
base::OnceCallback<void(bool)>& callback) {
if (!g_dialog_override_for_testing) {
return false;
}
switch (g_dialog_override_for_testing.value()) {
case DialogActionForTesting::kAccept:
std::move(callback).Run(true);
break;
case DialogActionForTesting::kCancel:
std::move(callback).Run(false);
break;
return true;
}
}
```
std::move(callback_).Run(false);nitty-nit: Personally I prefer early exits and then doing the task the function is supposed to do for readability, like:
```
if(!callback_) {
return;
}
std::move(callback_).Run(false);
```
But this is fine too.
.SetDialogDestroyingCallback(base::BindOnce(Dibyajyoti PalIt might be a bit clearer to inherit `SubAppsInstallDialogController` from `ui::DialogModelDelegate` and pass it to `ui::DialogModel::Builder()` so that all closures use the same unretained ptr.
Or, given that this looks like a tab-scoped dialog, rely on [tab_dialog_manager](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/web_apps/protocol_handler_picker_coordinator.cc;l=225-226;drc=4641a043d7914ce31515751c357158d46f4c937a;bpv=0;bpt=1)
Edman AnjosThis, or it might also be nice to completely move the `views` specific logic to a separate entity (like a function of some sorts), so that the controller and view is separated, as [said here](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/chrome_browser_design_principles.md#:~:text=For%20simple%20features%20that%20do%20not%20require%20data%20persistence%2C%20we%20only%20require%20separation%20of%20controller%20from%20view.).
We do this for all web app related dialogs. We have the [controller](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/web_apps/web_app_modal_dialog_delegate.h), and then the views are separate, like [this dialog](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/web_apps/web_app_simple_install_dialog.cc).
Regarding the use of `tab_dialog_manager`, from the comments above `SubAppsInstallDialogController::Show()`, it seems like it is a browser modal dialog? In that case, `CreateBrowserModalDialogViews()` SGTM.
I split the view/controller, and IMO the result is much better. Thanks a lot for the pointers to `web_app_simple_install_dialog`, very helpful!
I'm not sure about the `CreateBrowserModalDialogViews` vs `ShowWebModalDialogViews` choice. I think it should be a tab modal dialog. IWAs don't open in a tab, but I'm not entirely sure what should happen to the dialog e.g. on navigation. If we surface subapps to PWAs, then probably tab modal makes more sense?
I'm currently using `ShowWebModalDialogViews`. Let me know what you think.
I was pointing out that your initial implementation of using `CreateBrowserModalDialogViews()` was good enough actually. Can you share why we moved to using a web contents modal dialog? In my head, sub apps are going to be related to IWAs and IWAs can't open in a new browser tab, it makes more sense to be tied to a browser window, right? But do let me know if I'm missing something.
CHECK_DEREF(base::FindOrNull(add_call_info_, add_call_id));This seems redundant if we exit early in the beginning of the function, let's remove that?
Related details
- Edman Anjos
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
base::BindRepeating(OpenAppSettingsForParentApp, parent_app_id,nit: binding a `Profile*` like that doesn't sound right
Edman Anjos (Gerrit)
Edman Anjos added 9 comments![]( "Open in Gerrit")
Is this being used anywhere? If not, can we remove?
I find this a bit weird, that we completely skip the dialog for testing, [but we have precedence for it in web_applications/ code](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/test/web_app_install_test_utils.cc;l=53-63?q=AcceptDialogCallback) anyway so it's fine.
It is used in a few browsertests via `SetAutomaticActionForTesting`. I agree this is not ideal, and filed crbug.com/474650885 to consider removing it.
base::BindRepeating(OpenAppSettingsForParentApp, parent_app_id,nit: binding a `Profile*` like that doesn't sound right
Changed the function to take a weak pointer.
There's a similar function `OpenAppSettingsForInstalledRelatedApp` at https://crsrc.org/c/chrome/browser/ui/web_applications/web_app_ui_utils.h;l=41;drc=3c59c0d203ad0e7e9688ebe9901341604856faac that takes a `Profile*`.
Should we update that as well?
One quick thing to note here, by default the focus will be on the `OK` button. So it could be possible that a "malicious" IWA uses keyloggers to press Enter fast enough to automatically install a bunch of sub apps. Consider making either the Cancel button as the default, or the NONE button [1] using `OverrideDefaultButton()`.
Done, changed to `kNone`
that's a good point, thanks
// To be called when the dialog is cancelled, dismissed, or destryoed.Please fix this WARNING reported by Spellchecker: "destryoed" is a possible misspelling of "destroyed".
To bypass Spellchecker, a...
"destryoed" is a possible misspelling of "destroyed".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Done
SetAutomaticActionForTesting(DialogActionForTesting auto_accept);nit: Add a comment about what this function does ("used to accept/cancel the dialog in testing").
Done
This condition at first seemed unreachable, as `g_dialog_override_for_testing` is guaranteed to either exist with values, or to be `std::nullopt`, but then I realized this is to return true if `g_dialog_override_for_testing` is set after the callback has run. Maybe we can rewrite as the following using early exits to improve readability?
```
bool SubAppsInstallDialogController::
HandleAutomaticActionForTesting( // IN-TEST
base::OnceCallback<void(bool)>& callback) {
if (!g_dialog_override_for_testing) {
return false;
}
switch (g_dialog_override_for_testing.value()) {
case DialogActionForTesting::kAccept:
std::move(callback).Run(true);
break;
case DialogActionForTesting::kCancel:
std::move(callback).Run(false);
break;
return true;
}
}
```
Done
nitty-nit: Personally I prefer early exits and then doing the task the function is supposed to do for readability, like:
```
if(!callback_) {
return;
}
std::move(callback_).Run(false);
```But this is fine too.
Done
.SetDialogDestroyingCallback(base::BindOnce(Dibyajyoti PalIt might be a bit clearer to inherit `SubAppsInstallDialogController` from `ui::DialogModelDelegate` and pass it to `ui::DialogModel::Builder()` so that all closures use the same unretained ptr.
Or, given that this looks like a tab-scoped dialog, rely on [tab_dialog_manager](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/web_apps/protocol_handler_picker_coordinator.cc;l=225-226;drc=4641a043d7914ce31515751c357158d46f4c937a;bpv=0;bpt=1)
Edman AnjosThis, or it might also be nice to completely move the `views` specific logic to a separate entity (like a function of some sorts), so that the controller and view is separated, as [said here](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/chrome_browser_design_principles.md#:~:text=For%20simple%20features%20that%20do%20not%20require%20data%20persistence%2C%20we%20only%20require%20separation%20of%20controller%20from%20view.).
We do this for all web app related dialogs. We have the [controller](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/web_apps/web_app_modal_dialog_delegate.h), and then the views are separate, like [this dialog](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/web_apps/web_app_simple_install_dialog.cc).
Regarding the use of `tab_dialog_manager`, from the comments above `SubAppsInstallDialogController::Show()`, it seems like it is a browser modal dialog? In that case, `CreateBrowserModalDialogViews()` SGTM.
Dibyajyoti PalI split the view/controller, and IMO the result is much better. Thanks a lot for the pointers to `web_app_simple_install_dialog`, very helpful!
I'm not sure about the `CreateBrowserModalDialogViews` vs `ShowWebModalDialogViews` choice. I think it should be a tab modal dialog. IWAs don't open in a tab, but I'm not entirely sure what should happen to the dialog e.g. on navigation. If we surface subapps to PWAs, then probably tab modal makes more sense?
I'm currently using `ShowWebModalDialogViews`. Let me know what you think.
I was pointing out that your initial implementation of using `CreateBrowserModalDialogViews()` was good enough actually. Can you share why we moved to using a web contents modal dialog? In my head, sub apps are going to be related to IWAs and IWAs can't open in a new browser tab, it makes more sense to be tied to a browser window, right? But do let me know if I'm missing something.
Changed to `CreateBrowserModalDialogViews`
My thought process was that if subapps becomes available for PWAs it can be called from a regular tab and IIUC `ShowWebModalDialogViews` would then make more sense.
CHECK_DEREF(base::FindOrNull(add_call_info_, add_call_id));This seems redundant if we exit early in the beginning of the function, let's remove that?
Reverted back to CHECK_DEREF
looking closely I think the CHECK_DEREF is more appropriate. AFAICT the `add_call_id` should be valid when this is called
Related details
- Andrew Rayskiy
- Dibyajyoti Pal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted Code-Review+1![]( "Open in Gerrit")
- Dibyajyoti Pal
- Edman Anjos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dibyajyoti Pal
| Code-Review | +1 |
2 comments
LGTM, thanks for iterating on this, I like where we ended up here!
base::BindRepeating(OpenAppSettingsForParentApp, parent_app_id,Edman Anjosnit: binding a `Profile*` like that doesn't sound right
Changed the function to take a weak pointer.
There's a similar function `OpenAppSettingsForInstalledRelatedApp` at https://crsrc.org/c/chrome/browser/ui/web_applications/web_app_ui_utils.h;l=41;drc=3c59c0d203ad0e7e9688ebe9901341604856faac that takes a `Profile*`.
Should we update that as well?
That example is a good example of a function that takes in a `Profile*`, but it's not a good example of binding that Andrew brought up, since the function is not used as a callback anywhere, and is [instead called directly](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/site_data/page_specific_site_data_dialog.cc;l=320-324;drc=aeb46299fefab41457b0c50e5386616d2e8d64a2;bpv=1;bpt=1). This implementation LGTM!
Related details
- Edman Anjos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Edman Anjos (Gerrit)
Edman Anjos voted and added 1 comment![]( "Open in Gerrit")
Votes added by Edman Anjos
| Commit-Queue | +2 |
1 comment
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
Refactor SubAppsInstallDialogController to separate view and logic
SubAppsInstallDialogController is currently responsible for everything
related to this dialog.
This CL splits out the presentation concerns into
c/b/ui/views/web_apps/sub_apps_install_dialog.cc, so
SubAppsInstallDialogController remains focused on controller concerns.
The dialog is now created as a self-owned browser-modal widget using
CreateBrowserModalDialogViews. The controller is owned by the dialog
model, which is in turn owned by the widget.
- M chrome/browser/ui/BUILD.gn
- M chrome/browser/ui/views/web_apps/BUILD.gn
- R chrome/browser/ui/views/web_apps/sub_apps_install_dialog.cc
- A chrome/browser/ui/views/web_apps/sub_apps_install_dialog_controller.cc
- A chrome/browser/ui/views/web_apps/sub_apps_install_dialog_controller.h
- R chrome/browser/ui/views/web_apps/sub_apps_install_dialog_controller_browsertest.cc
- M chrome/browser/ui/views/web_apps/web_app_integration_test_driver.cc
- M chrome/browser/ui/web_applications/BUILD.gn
- M chrome/browser/ui/web_applications/sub_apps_admin_policy_browsertest.cc
- D chrome/browser/ui/web_applications/sub_apps_install_dialog_controller.h
- M chrome/browser/ui/web_applications/sub_apps_service_impl.cc
- M chrome/browser/ui/web_applications/sub_apps_service_impl.h
- M chrome/browser/ui/web_applications/sub_apps_service_impl_browsertest.cc
- M chrome/browser/ui/web_applications/web_app_dialogs.h
- M chrome/browser/ui/web_applications/web_app_ui_utils.cc
- M chrome/browser/ui/web_applications/web_app_ui_utils.h
Code-Review: +1 by Andrew Rayskiy, +1 by Dibyajyoti Pal
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |