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. |
Emilia Paz (Gerrit)
Emilia Paz added 11 comments![]( "Open in Gerrit")
Thanks Javier! Extension dialog looks good, just some small comments. I would suggest dividing this change in
(1) protocol handler work
(2) add extension dialog
(3) enable feature by default
void RunConfirmProtocolHandlerDialog(
content::WebContents* web_contents,
const ProtocolHandler& handler,
const std::optional<url::Origin>& initiating_origin,
ProtocolHandlerConfirmCallback callback) const override;
};nit: add comment
static void MaybeCreateAndAdd(content::NavigationThrottleRegistry&,
ProtocolHandlerRegistry*);
nit: add comment
}nit: C++ namespaces should be ended with a comment, using the format // namespace <namespace_name> per https://google.github.io/styleguide/cppguide.html#Namespaces
base::UTF8ToUTF16(extension->name()),Lets use `extensions::util::GetFixupExtensionNameForUIDisplay`, which handles extension long names in the UI
void ShowConfirmProtocolHandlerDialog(
// const ExtensionId& extension_id,
// const std::string& extension_name,nit: remove commented out code and add comment to the function
Confirm protocol handler.Javier FernandezWe 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.
Thanks for the explanation. If I am reading this correctly, the dialog may appear not after a direct interaction with the extension. Thus, I think it's good to include the extension name in the dialog
BASE_FEATURE(kExtensionProtocolHandlers, base::FEATURE_ENABLED_BY_DEFAULT);This change is getting large. I would separate this to a separate change
friend class ::custom_handlers::ConfirmProtocolHandlerDialog;do we need changes on this file? I think it's lefotver from previous patchset
Related details
- Javier Fernandez
- 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 11 comments![]( "Open in Gerrit")
Thanks for the review Emilia !
Regarding splitting up the patch, I could perhaps implement in a separated CL the logic to handle the new ProtocolHandler attribute "is_confirmed", but the rest (eg the NavigaitonThrottle) would only make sense together with the new Dialog. I'll consider it, though.
void RunConfirmProtocolHandlerDialog(
content::WebContents* web_contents,
const ProtocolHandler& handler,
const std::optional<url::Origin>& initiating_origin,
ProtocolHandlerConfirmCallback callback) const override;
};Javier Fernandeznit: add comment
Done
static void MaybeCreateAndAdd(content::NavigationThrottleRegistry&,
ProtocolHandlerRegistry*);
Javier Fernandeznit: add comment
Done
nit: C++ namespaces should be ended with a comment, using the format // namespace <namespace_name> per https://google.github.io/styleguide/cppguide.html#Namespaces
Done
Lets use `extensions::util::GetFixupExtensionNameForUIDisplay`, which handles extension long names in the UI
Done
void ShowConfirmProtocolHandlerDialog(
// const ExtensionId& extension_id,
// const std::string& extension_name,nit: remove commented out code and add comment to the function
Done
same namespace comment on this file
Done
class ConfirmProtocolHandlerDialog : public views::DialogDelegateView {Javier FernandezIt'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.
Done
BASE_FEATURE(kExtensionProtocolHandlers, base::FEATURE_ENABLED_BY_DEFAULT);This change is getting large. I would separate this to a separate change
Yeah, this change was unintentional The feature still needs more work before we can enable it.
friend class ::custom_handlers::ConfirmProtocolHandlerDialog;do we need changes on this file? I think it's lefotver from previous patchset
- Emilia Paz
- Solomon Kinard
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 3 comments![]( "Open in Gerrit")
Thanks Javier! Dialog construction looks good to me. There are some build errors though:
(a) `//chrome/browser/custom_handlers/chrome_protocol_handler_navigation_throttle.cc` needs access to `chrome/browser/ui/extensions/extensions_dialogs.h`
(b) Using views::View on Android included on components/custom_handlers/BUILD.gn
"//ui/views",is this needed? We shouldn't need to use `views::View` on this CL since we are using `ui::DialaogModel`
Related details
- Javier Fernandez
- 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 4 comments![]( "Open in Gerrit")
Thanks Javier! Dialog construction looks good to me. There are some build errors though:
(a) `//chrome/browser/custom_handlers/chrome_protocol_handler_navigation_throttle.cc` needs access to `chrome/browser/ui/extensions/extensions_dialogs.h`
(b) Using views::View on Android included on components/custom_handlers/BUILD.gn
Adding the "chrome/browser/ui/extensions:extensions" dep creates a cycle that deserves its own CL to solve it.
Thanks again for the review, Emilia.
I'm going to fix the dependency cycle issues in a different CL; I may also split up the protocol handlers part as well, as you have suggested.
Meanwhile, I wonder what to do with the translation of the new dialog. I tried to define the text I considered more appropriated, but perhaps someone from the UI/UX team should review it before asking for translations.
is this needed? We shouldn't need to use `views::View` on this CL since we are using `ui::DialaogModel`
No, it's not needed; neither the "//components/constrained_window" dependency.
Thanks !
Related details
- Emilia Paz
- 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 voted and added 3 comments![]( "Open in Gerrit")
Votes added by Javier Fernandez
| Commit-Queue | +1 |
3 comments
Thanks Javier! Extension dialog looks good, just some small comments. I would suggest dividing this change in
(1) protocol handler work
(2) add extension dialog
(3) enable feature by default
Javier Fernandez
The protocol handler work is not implemented in https://chromium-review.googlesource.com/c/chromium/src/+/7156864
In this CL only the extension dialog related changes remain.
The change that enabled the feature was a mistake and has been removed now.
Javier FernandezThanks Javier! Dialog construction looks good to me. There are some build errors though:
(a) `//chrome/browser/custom_handlers/chrome_protocol_handler_navigation_throttle.cc` needs access to `chrome/browser/ui/extensions/extensions_dialogs.h`
(b) Using views::View on Android included on components/custom_handlers/BUILD.gn
Adding the "chrome/browser/ui/extensions:extensions" dep creates a cycle that deserves its own CL to solve it.
This has been addressed in https://chromium-review.googlesource.com/c/chromium/src/+/7130938
Please @emiliapaz could you please take another look at the Extension Dialog related changes ? The translation are still pending, but I'd like to get an agreement regarding the dialog and the text messages before sending the screenshots.
Related details
- Emilia Paz
- Javier Fernandez
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 and added 4 comments![]( "Open in Gerrit")
Votes added by Emilia Paz
| Code-Review | +1 |
4 comments
Thanks Javier! c/b/ui/extensions lgtm. You are going to need [translation screenshots](https://chromium.googlesource.com/chromium/src/+/lkgr/docs/translation_screenshots.md) for the new strings. Unfortunately, this can only be done by a Googler. If you provide them on the description, I can help (although I'll be OOO until Nov 30 -- so may need to ask another reviewer)
recommendation: for UI changes, it's nice to add a screenshot to make sure it looks as intended and for future reference
"confirm_protocol_handler_dialog.cc",lets move this to line 168. `enable_extensions` is used for non desktop android
<action name="ConfirmProtocolHandler.Infobar_Accept">
<owner>Please list the metric's owners. Add more owner tags as needed.</owner>
<description>Please enter the description of the metric.</description>
</action>
<action name="ConfirmProtocolHandler.InfoBar_Deny">
<owner>Please list the metric's owners. Add more owner tags as needed.</owner>
<description>Please enter the description of the metric.</description>
</action>
This is not used on this CL. Remove?
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 2 comments![]( "Open in Gerrit")
lets move this to line 168. `enable_extensions` is used for non desktop android
Done
<action name="ConfirmProtocolHandler.Infobar_Accept">
<owner>Please list the metric's owners. Add more owner tags as needed.</owner>
<description>Please enter the description of the metric.</description>
</action>
<action name="ConfirmProtocolHandler.InfoBar_Deny">
<owner>Please list the metric's owners. Add more owner tags as needed.</owner>
<description>Please enter the description of the metric.</description>
</action>
This is not used on this CL. Remove?
No, not any more. They were used in a previous version of this CL.
Thanls.
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. |
Javier Fernandez (Gerrit)
Javier Fernandez added 1 comment![]( "Open in Gerrit")
Hi @rdevlin.cronin
Emilia already reviewed the UI logic of the the new prompt dialog.
Could you please do a final review ?
Thanks.
Related details
- Devlin Cronin
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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 8 comments![]( "Open in Gerrit")
Thanks, Javier! This generally looks pretty good. A few notes:
- In general, it's best to not remove reviewers after they've reviewed (Gerrit removed Emilia's +1 since she's no longer a reviewer).
- You'll still need a custom_handlers owner for these
- Can we add tests? : )
content::NavigationThrottleRegistry&,nit: please name these (and below)
explicit ChromeProtocolHandlerNavigationThrottle(nit: no need for `explicit` with multi-param ctors
class ChromeProtocolHandlerNavigationThrottlenit: add a top-level comment describing what the class does
ProtocolHandlerRegistry* protocol_handler_registry,nit: #include protocol handler registry
*handler.extension_id(), ExtensionRegistry::EVERYTHING);should we also know at this point that the extension is enabled? (Would we ever show this for a disabled extension?)
base::OnceCallback<void(bool, bool)> callback)nit: let's make this type an alias (we use it twice) and also document what the arguments are
Confirm protocol handler.Have we consulted with the UX team at all, or do these mirror existing strings? I don't think we normally expose "protocol handler" as a concept to users, since it's pretty niche
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 9 comments![]( "Open in Gerrit")
Thanks, Javier! This generally looks pretty good. A few notes:
- In general, it's best to not remove reviewers after they've reviewed (Gerrit removed Emilia's +1 since she's no longer a reviewer).
- You'll still need a custom_handlers owner for these
- Can we add tests? : )
Regarding the custom_handlers owner, I asked Solomon to review the CL with the NavigationThrottle logic, mainly because he has the context about the new extension feature being implemented here; I use to ask Colin Blundell for rubber-stamping these changes latter. I may do the same in this case.
Regarding the tests, could you point me to an example of the kind of interactive tests these prompt dialog would need ? Perhaps it's a good opportunity to add Emilia again into the review loop.
Thanks @rdevlin.cronin for the reviews.
@emiliapaz, could you please point me an example of the kind of interactive tests these dialogs need ? Thanks.
nit: please name these (and below)
Done
nit: no need for `explicit` with multi-param ctors
Done
nit: add a top-level comment describing what the class does
Done
nit: #include protocol handler registry
Done
*handler.extension_id(), ExtensionRegistry::EVERYTHING);should we also know at this point that the extension is enabled? (Would we ever show this for a disabled extension?)
Yes, the protocol handler that is only in the registry if the extension is enabled. Hence, while running the prompt dialog we can assume that the extension id corresponds to an enabled extension, indeed.
nit: let's make this type an alias (we use it twice) and also document what the arguments are
Done
Confirm protocol handler.Have we consulted with the UX team at all, or do these mirror existing strings? I don't think we normally expose "protocol handler" as a concept to users, since it's pretty niche
I haven't consulted the UX team, and perhaps we should, indeed, even though these strings are used in the Web API (via the registerProtocolHandler JS method) when the user is prompt to grant the registration of a protocol handler.
I agree that the protocol handlers concept is kind of niches, but it's already used as I said before. My main doubt is whether the sentence is clear enough for the user and whether it should be rephrased in a different way. I really wanted to check with someone before sending the PNGs for the translation.
Related details
- Devlin Cronin
- 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 1 comment![]( "Open in Gerrit")
Javier FernandezThanks, Javier! This generally looks pretty good. A few notes:
- In general, it's best to not remove reviewers after they've reviewed (Gerrit removed Emilia's +1 since she's no longer a reviewer).
- You'll still need a custom_handlers owner for these
- Can we add tests? : )
Regarding the custom_handlers owner, I asked Solomon to review the CL with the NavigationThrottle logic, mainly because he has the context about the new extension feature being implemented here; I use to ask Colin Blundell for rubber-stamping these changes latter. I may do the same in this case.
Regarding the tests, could you point me to an example of the kind of interactive tests these prompt dialog would need ? Perhaps it's a good opportunity to add Emilia again into the review loop.
`InteractiveBrowserTest` is the best way to test dialogs interactively. For example:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/extensions/controlled_home_dialog_interactive_uitest.cc
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/extensions/dialogs/extension_install_friction_dialog_interactive_uitest.cc?q=InteractiveBrowserTest&ss=chromium%2Fchromium%2Fsrc:chrome%2Fbrowser%2Fui%2Fviews%2Fextensions%2F
Related details
- Devlin Cronin
- Javier Fernandez
Devlin Cronin (Gerrit)
Devlin Cronin added 2 comments![]( "Open in Gerrit")
Confirm protocol handler.Javier FernandezHave we consulted with the UX team at all, or do these mirror existing strings? I don't think we normally expose "protocol handler" as a concept to users, since it's pretty niche
I haven't consulted the UX team, and perhaps we should, indeed, even though these strings are used in the Web API (via the registerProtocolHandler JS method) when the user is prompt to grant the registration of a protocol handler.
I agree that the protocol handlers concept is kind of niches, but it's already used as I said before. My main doubt is whether the sentence is clear enough for the user and whether it should be rephrased in a different way. I really wanted to check with someone before sending the PNGs for the translation.
I'm not sure I'm looking at the right strings. The one I see at the link you pasted is IDS_REGISTER_PROTOCOL_HANDLER_CONFIRM_FRAGMENT, which corresponds to [this string](https://source.chromium.org/chromium/chromium/src/+/main:components/protocol_handler_strings.grdp;l=20-22;drc=b1060a61978fdff6cc40a11e387ded44e8e108b4):
Open <ph name="PROTOCOL">$1<ex>search</ex></ph> links
Which would be, e.g. "Open search links" (or "open web+burger links", since apparently that's an example [we like](https://developer.mozilla.org/en-US/docs/Web/API/Navigator/registerProtocolHandler#registering_a_handler_for_a_custom_protocol). That phrasing is much more inline with what I'd expect -- "open these links" rather than "registered a custom handler for this scheme".
Maybe "The extension <name> would like to open <protocol> links", to match that? Or something similar? (Relatedly, mind including a screenshot in the CL description? I know there's one in the doc, but it's a bit buried)
Related details
- Javier Fernandez
Javier Fernandez (Gerrit)
Javier Fernandez voted and added 4 comments![]( "Open in Gerrit")
Votes added by Javier Fernandez
| Commit-Queue | +1 |
Javier FernandezThanks, Javier! This generally looks pretty good. A few notes:
- In general, it's best to not remove reviewers after they've reviewed (Gerrit removed Emilia's +1 since she's no longer a reviewer).
- You'll still need a custom_handlers owner for these
- Can we add tests? : )
Emilia PazRegarding the custom_handlers owner, I asked Solomon to review the CL with the NavigationThrottle logic, mainly because he has the context about the new extension feature being implemented here; I use to ask Colin Blundell for rubber-stamping these changes latter. I may do the same in this case.
Regarding the tests, could you point me to an example of the kind of interactive tests these prompt dialog would need ? Perhaps it's a good opportunity to add Emilia again into the review loop.
`InteractiveBrowserTest` is the best way to test dialogs interactively. For example:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/extensions/controlled_home_dialog_interactive_uitest.cc
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/extensions/dialogs/extension_install_friction_dialog_interactive_uitest.cc?q=InteractiveBrowserTest&ss=chromium%2Fchromium%2Fsrc:chrome%2Fbrowser%2Fui%2Fviews%2Fextensions%2F
Done
Thanks for the reviews, Emilia and Devlin.
I've updated the strings and added a few UI interaction tests.
Could you please both take another look ?
Thanks.
recommendation: for UI changes, it's nice to add a screenshot to make sure it looks as intended and for future reference
Done
Confirm protocol handler.Javier FernandezHave we consulted with the UX team at all, or do these mirror existing strings? I don't think we normally expose "protocol handler" as a concept to users, since it's pretty niche
Devlin CroninI haven't consulted the UX team, and perhaps we should, indeed, even though these strings are used in the Web API (via the registerProtocolHandler JS method) when the user is prompt to grant the registration of a protocol handler.
I agree that the protocol handlers concept is kind of niches, but it's already used as I said before. My main doubt is whether the sentence is clear enough for the user and whether it should be rephrased in a different way. I really wanted to check with someone before sending the PNGs for the translation.
I'm not sure I'm looking at the right strings. The one I see at the link you pasted is IDS_REGISTER_PROTOCOL_HANDLER_CONFIRM_FRAGMENT, which corresponds to [this string](https://source.chromium.org/chromium/chromium/src/+/main:components/protocol_handler_strings.grdp;l=20-22;drc=b1060a61978fdff6cc40a11e387ded44e8e108b4):
Open <ph name="PROTOCOL">$1<ex>search</ex></ph> linksWhich would be, e.g. "Open search links" (or "open web+burger links", since apparently that's an example [we like](https://developer.mozilla.org/en-US/docs/Web/API/Navigator/registerProtocolHandler#registering_a_handler_for_a_custom_protocol). That phrasing is much more inline with what I'd expect -- "open these links" rather than "registered a custom handler for this scheme".
Maybe "The extension <name> would like to open <protocol> links", to match that? Or something similar? (Relatedly, mind including a screenshot in the CL description? I know there's one in the doc, but it's a bit buried)
Ah, yeah, I know what you mean now; the protocol handler is only mentioned in the description of those IDs, but the UI message just mention the intention to open links with "special" schemes, indeed.
I've reworded the IDs to match those examples, but mentioning the extension that owns the protocol handler.
Also, as requested, I've added a link in the CL description to the screenshot of the new dialog.
Related details
- Devlin Cronin
- 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. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 2 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
2 comments
Thanks, Javier! High-level LGTM. Please wait for Emilia to take another pass at the UI code and the tests before landing.
Confirm protocol handler.Javier FernandezHave we consulted with the UX team at all, or do these mirror existing strings? I don't think we normally expose "protocol handler" as a concept to users, since it's pretty niche
Devlin CroninI haven't consulted the UX team, and perhaps we should, indeed, even though these strings are used in the Web API (via the registerProtocolHandler JS method) when the user is prompt to grant the registration of a protocol handler.
I agree that the protocol handlers concept is kind of niches, but it's already used as I said before. My main doubt is whether the sentence is clear enough for the user and whether it should be rephrased in a different way. I really wanted to check with someone before sending the PNGs for the translation.
Javier FernandezI'm not sure I'm looking at the right strings. The one I see at the link you pasted is IDS_REGISTER_PROTOCOL_HANDLER_CONFIRM_FRAGMENT, which corresponds to [this string](https://source.chromium.org/chromium/chromium/src/+/main:components/protocol_handler_strings.grdp;l=20-22;drc=b1060a61978fdff6cc40a11e387ded44e8e108b4):
Open <ph name="PROTOCOL">$1<ex>search</ex></ph> linksWhich would be, e.g. "Open search links" (or "open web+burger links", since apparently that's an example [we like](https://developer.mozilla.org/en-US/docs/Web/API/Navigator/registerProtocolHandler#registering_a_handler_for_a_custom_protocol). That phrasing is much more inline with what I'd expect -- "open these links" rather than "registered a custom handler for this scheme".
Maybe "The extension <name> would like to open <protocol> links", to match that? Or something similar? (Relatedly, mind including a screenshot in the CL description? I know there's one in the doc, but it's a bit buried)
Ah, yeah, I know what you mean now; the protocol handler is only mentioned in the description of those IDs, but the UI message just mention the intention to open links with "special" schemes, indeed.
I've reworded the IDs to match those examples, but mentioning the extension that owns the protocol handler.
Also, as requested, I've added a link in the CL description to the screenshot of the new dialog.
- Emilia Paz
- Javier Fernandez
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 10 comments![]( "Open in Gerrit")
Thanks Javier! Overall LGTM, just a couple suggestions. If you update the screenshot in the description, I can generate the screenshot files for the CL
bool remember = dialog_model()
->GetCheckboxByUniqueId(kRememberMeCheckbox)
->is_checked();
std::move(callback_).Run(false, remember);As a user, I would expect clicking on 'Cancel' to not have any effect.
However, here we are passing back `remember`. Does that mean it will be allowed?
Edit: after looking at components/custom_handlers/protocol_handler_navigation_throttle.cc, `remember` is not used when cancel is selected. However, this is not clear from here. What about:
(a) Have different callbacks for accept and cancel. That way we don't need to pass back a boolean for that, and `remember` is only passed to the accept callback
or
(b) `OnDialogCancelled` returns `false` for `remember`, with a comment that explains why.
Leaning towards (a) because it makes the calls easier to understand.
DECLARE_ELEMENT_IDENTIFIER_VALUE(kRememberMeCheckbox);Let's add which dialog this belongs to, so it's not used by mistake on another dialog
constexpr const char extension_manifest[] =
R"({
"name": "An Extension",
"version": "1",
"manifest_version": 3
})";
extensions::TestExtensionDir extension_dir;
extension_dir.WriteManifest(extension_manifest);
extensions::ChromeTestExtensionLoader extension_loader(profile);
extension_loader.set_ignore_manifest_warnings(true);
last_loaded_extension_id_ =
extension_loader.LoadExtension(extension_dir.Pack())->id();nit: an easier way to install an extension is with the `ExtensionBuilder`
```
scoped_refptr<const extensions::Extension> extension =
extensions::ExtensionBuilder(extension_name)
.Build();
extension_registrar()->AddExtension(extension);
```
unless load triggers something differently?
// invokescalls the callback with (granted=true, remember=true) and then closesTypo here, "invokescalls" should probably be "invokes" or "calls".
// Pressing the 'Deby' button invokes the callback with (granted=false,Typo: 'Deby' should be 'Deny'. Although, maybe change it to 'cancel' since that's the buton text and name?
this, web_contents, handler, url,I think this could cause a crash: because PostTask is asynchronous, the NavigationThrottle might be destroyed (e.g., the user closes the tab) before the task runs. When the task finally runs, it could try to access this, causing a crash. We can use a weak ptr and check whether the throttle exists.
FROM_HERE, base::BindOnce(std::move(launch_callback)));hmm we have a double bind here. We may be able to avoid this by restructuring this method:
```
ProtocolHandlerConfirmCallback permission_decided_callback =
base::BindOnce(
&ProtocolHandlerNavigationThrottle::OnProtocolHandlerPermissionDecided,
weak_factory_.GetWeakPtr(), url);
base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
FROM_HERE,
base::BindOnce(
[](...) {
// Ensure the throttle still exists.
if (!throttle)
return;
...
},
weak_factory_.GetWeakPtr(), web_contents, handler, url,
std::move(permission_decided_callback)));
```
The extension "<ph name="EXTENSION_NAME">$1<ex>Google Translate</ex></ph>" wants toI would change this to "<extension name>" wants to open <protocol> links through <host name>" to match other dialog strings like:
- IDS_EXTENSIONS_PRINTING_API_PRINT_REQUEST_BUBBLE_HEADING
- IDS_EXTENSIONS_DOCUMENT_SCAN_API_DISCOVERY_REQUEST_BUBBLE_HEADING
It can be either the title, or have it be the description and a simpler title like "Open <protocol> links"
And for the checkbox, checking it means "always allow" versus just "allow"? If so, I would change it to "Always allow <hot name> to open <protocol> links"
In summary, options:
(a)
Title: "<extension name>" wants to open <protocol> links through <host name>
Checkbox: Always allow <hot name> to open <protocol> links
(b)
Title: Open <protocol> links?
Description: "<extension name>" wants to open <protocol> links through <host name>
Checkbox: Always allow <hot name> to open <protocol> links
<message name="IDS_CONFIRM_PROTOCOL_HANDLER_MESSAGE_WITH_INITIATING_ORIGIN" desc="Warn the user about the navigation request rediction for a known initiator origin.">Typo: 'rediction' should be 'redirection'.
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 9 comments![]( "Open in Gerrit")
Thank you Emilia for the review.
I have applied most of the changes, but left some questions inline.
bool remember = dialog_model()
->GetCheckboxByUniqueId(kRememberMeCheckbox)
->is_checked();
std::move(callback_).Run(false, remember);As a user, I would expect clicking on 'Cancel' to not have any effect.
However, here we are passing back `remember`. Does that mean it will be allowed?Edit: after looking at components/custom_handlers/protocol_handler_navigation_throttle.cc, `remember` is not used when cancel is selected. However, this is not clear from here. What about:
(a) Have different callbacks for accept and cancel. That way we don't need to pass back a boolean for that, and `remember` is only passed to the accept callback
or
(b) `OnDialogCancelled` returns `false` for `remember`, with a comment that explains why.
Leaning towards (a) because it makes the calls easier to understand.
I was tempted to implement b) because it was really simple, but I think you are right that the use of a single callback to handle the Permission's decision treats both boolean values in a very obscure way.
I have implemented a), although I had to update more files; I think it's much clearer now. Thanks for the suggestion,
Let's add which dialog this belongs to, so it's not used by mistake on another dialog
Done
constexpr const char extension_manifest[] =
R"({
"name": "An Extension",
"version": "1",
"manifest_version": 3
})";
extensions::TestExtensionDir extension_dir;
extension_dir.WriteManifest(extension_manifest);
extensions::ChromeTestExtensionLoader extension_loader(profile);
extension_loader.set_ignore_manifest_warnings(true);
last_loaded_extension_id_ =
extension_loader.LoadExtension(extension_dir.Pack())->id();nit: an easier way to install an extension is with the `ExtensionBuilder`
```
scoped_refptr<const extensions::Extension> extension =
extensions::ExtensionBuilder(extension_name)
.Build();extension_registrar()->AddExtension(extension);
```
unless load triggers something differently?
Nice suggestion !
Thanks,
// invokescalls the callback with (granted=true, remember=true) and then closesTypo here, "invokescalls" should probably be "invokes" or "calls".
Done
// Pressing the 'Deby' button invokes the callback with (granted=false,Typo: 'Deby' should be 'Deny'. Although, maybe change it to 'cancel' since that's the buton text and name?
Done
hmm we have a double bind here. We may be able to avoid this by restructuring this method:
```
ProtocolHandlerConfirmCallback permission_decided_callback =
base::BindOnce(
&ProtocolHandlerNavigationThrottle::OnProtocolHandlerPermissionDecided,
weak_factory_.GetWeakPtr(), url);base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
FROM_HERE,
base::BindOnce(
[](...) {
// Ensure the throttle still exists.
if (!throttle)
return;
...
},
weak_factory_.GetWeakPtr(), web_contents, handler, url,
std::move(permission_decided_callback)));
```
Done
The extension "<ph name="EXTENSION_NAME">$1<ex>Google Translate</ex></ph>" wants toI would change this to "<extension name>" wants to open <protocol> links through <host name>" to match other dialog strings like:
- IDS_EXTENSIONS_PRINTING_API_PRINT_REQUEST_BUBBLE_HEADING
- IDS_EXTENSIONS_DOCUMENT_SCAN_API_DISCOVERY_REQUEST_BUBBLE_HEADING
It can be either the title, or have it be the description and a simpler title like "Open <protocol> links"
And for the checkbox, checking it means "always allow" versus just "allow"? If so, I would change it to "Always allow <hot name> to open <protocol> links"
In summary, options:
(a)
Title: "<extension name>" wants to open <protocol> links through <host name>
Checkbox: Always allow <hot name> to open <protocol> links(b)
Title: Open <protocol> links?
Description: "<extension name>" wants to open <protocol> links through <host name>
Checkbox: Always allow <hot name> to open <protocol> links
I think the option b) is clearer, and more similar to the PermissionRequest prompt used when registering protocol handlers using the Web API.
I have some doubts regarding the change in the Checkbox string. One one hand, that string is also used by the PermissionRequest prompt dialog launched when using the Web API, so this change would affect that use case as well. On the other side, the original message used the term "all" as a way to imply "always" or "from now on".
I don't have a strong opinion regarding the use of the "always" term, even in addition to the "all" term being used already. Is the change worth, considering it affects the Web API prompt dialog ?
Alternatively, I could define a new <message> item for this new dialog, although it'd be pretty similar to the one used by the Web API.
<message name="IDS_CONFIRM_PROTOCOL_HANDLER_MESSAGE_WITH_INITIATING_ORIGIN" desc="Warn the user about the navigation request rediction for a known initiator origin.">Typo: 'rediction' should be 'redirection'.
- Devlin Cronin
- 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. |
Javier Fernandez (Gerrit)
Javier Fernandez added 1 comment![]( "Open in Gerrit")
I think this could cause a crash: because PostTask is asynchronous, the NavigationThrottle might be destroyed (e.g., the user closes the tab) before the task runs. When the task finally runs, it could try to access this, causing a crash. We can use a weak ptr and check whether the throttle exists.
Done