Prompt dialog to confirm protocol handlers registered by extensions [chromium/src : main]
Javier Fernandez (Gerrit)
Javier Fernandez added 1 comment![]( "Open in Gerrit")
Hi @solomonkinard, I'd appreciate some early-feedback about this CL, even though it's still work in progress.
The main functionality, as described in the design document, is complete. There are some missing translation and proper unit and browser tests, but I have doubts about the right approach to implement the custom dialog for this case.
It seems that there are different ways, from subclassing BubbleDialogDelegateView or directly from DialogDelegateView to the new composite approach described in the /class_splitting.md. In this patch I got inspiration from the ExternalHandlerDialog, which uses the static method defined in the cosntrained_window namespace. There are there comments about the preference of using the TabDialogManager.
Related details
- Solomon Kinard
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Javier Fernandez (Gerrit)
Javier Fernandez added 1 comment![]( "Open in Gerrit")
Hi @solomonkinard
Although the patch is still WIP, I marked it ready for review because I'm afraid you wouldn't get the notification.
I'd just need some early review regarding the best approach to implement the new prompt dialog for this use case, as explained in the design doc.
Thanks.
Related details
- Solomon Kinard
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Solomon Kinard (Gerrit)
Solomon Kinard added 5 comments![]( "Open in Gerrit")
Thanks.
class ConfirmProtocolHandlerDialog : public views::DialogDelegateView {I didn't see a mock in the DD (design doc). Can a SS (screenshot) be added e.g. a link to a crbug.com image upload in the commit message?
VLOG(0) << "ConfirmProtocolHandlerDialog::OnDialogCancelled -- ";^
VLOG(0) << "ProtocolHandlerNavigationThrottle::RequestPermissionForHandler "Are these VLOG's intentional, here and elsewhere?
Related details
- Javier Fernandez
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. |
Javier Fernandez (Gerrit)
Javier Fernandez added 4 comments![]( "Open in Gerrit")
Thanks @solomonkinard for the comments.
Bear in mind that the main issue I'd like to clarify is what's the best approach to implement the prompt dialog; apparently there are different ways to do it and it's not clear what fits better in this use case.
It's also worth mentioning that the custom_handlers feature is not available in android, so we are targeting just Desktop platforms.
class ConfirmProtocolHandlerDialog : public views::DialogDelegateView {I didn't see a mock in the DD (design doc). Can a SS (screenshot) be added e.g. a link to a crbug.com image upload in the commit message?
Done
VLOG(0) << "ConfirmProtocolHandlerDialog::OnDialogCancelled -- ";Javier Fernandez^
Ditto
VLOG(0) << "ProtocolHandlerNavigationThrottle::RequestPermissionForHandler "Are these VLOG's intentional, here and elsewhere?
Well, as I said before, this is just a prototype to evaluate different ways to intercept the Navigation and launch a prompt dialog to ask the user for confirmation. These logs are going to be removed probably.
Related details
- Solomon Kinard
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. |
Javier Fernandez (Gerrit)
Javier Fernandez added 1 comment![]( "Open in Gerrit")
base::Time::Now(), true,Javier Fernandezhttps://google.github.io/styleguide/cppguide.html#Function_Argument_Comments ?
Done
Solomon Kinard (Gerrit)
Solomon Kinard added 1 comment![]( "Open in Gerrit")
emiliapaz@ might know the best approach to implement the prompt dialog? If not, feel free to add back as a reviewer.
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. |
Emilia Paz (Gerrit)
Emilia Paz added 5 comments![]( "Open in Gerrit")
Thanks Javier! I only looked at the dialog construction. There is a new preferred way to construct them using `ui::DialogModel`. Linkes to some examples, but please reach out if you have any questions.
I didn't look at the protocol handler parts
class ConfirmProtocolHandlerDialog : public views::DialogDelegateView {It's preferred to use `ui::DialogModel` rather than `views::DialogDelegateView` for constructing dialogs. The model makes it easier to build the dialog, can see a guide here:
https://chromium.googlesource.com/chromium/src/+/main/chrome/browser/ui/views/extensions/dialogs/README.md
Some examples:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/extensions/extension_install_friction_dialog.cc?q=extension_install_friction&ss=chromium%2Fchromium%2Fsrc
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/extensions/dialogs/mv2_deprecation_reenable_dialog.cc
VLOG(0) << "ProtocolHandlerThrottle::RunConfirmProtocolHandlerDialog -- ";nit: We shouldn't add logs into landed code unless absolutely necessary (see [guide](https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#logging))
Confirm protocol handler.We may need to include the extension name here?
Usually extensions dialogs have the extension name, or are anchored to thge extension action in the toolbar. What triggers the dialog?
friend class ::custom_handlers::ConfirmProtocolHandlerDialog;We shouldn't add more classes to this. We shouldn't need this if we use `ui::DialogModel`
Related details
- Javier Fernandez
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. |
Javier Fernandez (Gerrit)
Javier Fernandez added 5 comments![]( "Open in Gerrit")
Many thanks for the insightful feedback @emiliapaz.
I still need to do some additional work in the dialog, like identifying the extension in the prompt message and deal with the translation issues, but I hope the CL is going in the right direction.
@solomonkinard, perhaps you'd have some comments regarding the navigation interception logic.
Thanks.
class ConfirmProtocolHandlerDialog : public views::DialogDelegateView {It's preferred to use `ui::DialogModel` rather than `views::DialogDelegateView` for constructing dialogs. The model makes it easier to build the dialog, can see a guide here:
https://chromium.googlesource.com/chromium/src/+/main/chrome/browser/ui/views/extensions/dialogs/README.mdSome examples:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/extensions/extension_install_friction_dialog.cc?q=extension_install_friction&ss=chromium%2Fchromium%2Fsrc
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/extensions/dialogs/mv2_deprecation_reenable_dialog.cc
Thank you very much for the examples. I submitted a new patch trying to follow the same approach.
VLOG(0) << "ProtocolHandlerThrottle::RunConfirmProtocolHandlerDialog -- ";nit: We shouldn't add logs into landed code unless absolutely necessary (see [guide](https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#logging))
Done
Confirm protocol handler.We may need to include the extension name here?
Usually extensions dialogs have the extension name, or are anchored to thge extension action in the toolbar. What triggers the dialog?
Thanks for the suggestion. I think mentioning the extension that is responsible of this prompt makes a lot of sense. However, this is perhaps a special case, so I'm going to try to describe it briefly here.
An extension may register a few protocol handlers for some specific URL's schemes. Instead of asking for permission when the handler is registered (as it happens with the Web API), these handlers are "unconfirmed" until there is a navigation request to a url with a scheme for which there such "unconfirmed" handler. In that case, the user may allow the protocol handler to take care of the navigation requests or deny it. It optionally may remember the decision for any future navigation request to the **same scheme**.
I think in this contest, it makes sense to inform the user what extension is responsible of the custom handler that is about to be used.
In this CL I didn't implemented that behavior, though. but I definitively will.
friend class ::custom_handlers::ConfirmProtocolHandlerDialog;We shouldn't add more classes to this. We shouldn't need this if we use `ui::DialogModel`
- Emilia Paz
- Solomon Kinard
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. |
Javier Fernandez (Gerrit)
Javier Fernandez added 1 comment![]( "Open in Gerrit")
VLOG(0) << "ProtocolHandlerNavigationThrottle::RequestPermissionForHandler "Javier FernandezAre these VLOG's intentional, here and elsewhere?
Well, as I said before, this is just a prototype to evaluate different ways to intercept the Navigation and launch a prompt dialog to ask the user for confirmation. These logs are going to be removed probably.
These logs has been removed in the last patch.
Related details
- Emilia Paz
- Solomon Kinard
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. |