Intercept navigation when trying to use an unconfirmed protocol handler [chromium/src : main]
Javier Fernandez (Gerrit)
Message from Javier Fernandez![]( "Open in Gerrit")
Set Ready For Review
Related details
- Javier Fernandez
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")
This CL is the result of splitting up the CL 7031939, which has been mostly reviewed for the //extension part. This patch covers the //custom_handlers related logic.
Hi @blundell, could you please review the //custom_handlers related changes ?
Hi @emiliapaz, could you please review the //extension related changes ? It's just a change in the browser tests to add a dummy NavigationThrottle callaback so that the previously defined tests pass.
Related details
- Colin Blundell
- Emilia Paz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
Thanks, Javier! This is a quite substantial CL and I don't have the context to do a proper technical review. Is there someone who does?
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. |
Javier Fernandez (Gerrit)
Javier Fernandez added 1 comment![]( "Open in Gerrit")
Thanks, Javier! This is a quite substantial CL and I don't have the context to do a proper technical review. Is there someone who does?
Yeah, this change is mostly related to a new Extension API that I'm implementing, so probably an extension owner would have the proper context.
The navigation throttle could be useful for other use cases in the future, but for now it's restricted to intercept only handlers registered via this new Extension API.
Thanks anyway, I'll ask to an extensions owner, but probably you'd have to rubber-stamp the CL eventually.
Related details
- Colin Blundell
- Emilia Paz
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 could you please review this CL, in the context of the previous series of patches to implement the "protocol_handlers" Manifest Key in Extensions ?
Related details
- Colin Blundell
- 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 3 comments![]( "Open in Gerrit")
Thanks. Are you requesting a review from me for protocol_handlers_manager_browsertest.cc, or other files too (can you specify)?
std::move(callback).Run(true /*permission_granted*/,
true /*remember*/);?
```suggestion
std::move(callback).Run(/*permission_granted=*/true,
/*remember=*/true);
```
Related details
- Colin Blundell
- 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 1 comment![]( "Open in Gerrit")
Thanks. Are you requesting a review from me for protocol_handlers_manager_browsertest.cc, or other files too (can you specify)?
I think that since you were reviewing the patches of this new Extension API since the beginning you'd have enough context to review the protocol handlers related changes as well.
Devlin could also do it, but it seem is OOO until December. Do you know if someone else would be more appropriated to review the protocol handler files ?
Related details
- Colin Blundell
- 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. |
Solomon Kinard (Gerrit)
Solomon Kinard added 1 comment![]( "Open in Gerrit")
Javier FernandezThanks. Are you requesting a review from me for protocol_handlers_manager_browsertest.cc, or other files too (can you specify)?
I think that since you were reviewing the patches of this new Extension API since the beginning you'd have enough context to review the protocol handlers related changes as well.
Devlin could also do it, but it seem is OOO until December. Do you know if someone else would be more appropriated to review the protocol handler files ?
Would you like to let me know which files exactly? You are welcome to request any reviewer that you'd like. I only called out the one file shown in Gerrit with my name associated after clicking "Add owners". I have been asked before, when creating CLs, to specify which reviewer should look at which files, to speed up review and reduce questions. Unfortunately, Gerrit doesn't allow for only specific files, so it's all or nothing. Hence the clarification request. Can you clarify exactly which files you'd like for me to review? It looks like you're the owner of the related files in components/* and I'm only an owner of one file, in this CL.
Related details
- Colin Blundell
- 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 1 comment![]( "Open in Gerrit")
Javier FernandezThanks. Are you requesting a review from me for protocol_handlers_manager_browsertest.cc, or other files too (can you specify)?
Solomon KinardI think that since you were reviewing the patches of this new Extension API since the beginning you'd have enough context to review the protocol handlers related changes as well.
Devlin could also do it, but it seem is OOO until December. Do you know if someone else would be more appropriated to review the protocol handler files ?
Would you like to let me know which files exactly? You are welcome to request any reviewer that you'd like. I only called out the one file shown in Gerrit with my name associated after clicking "Add owners". I have been asked before, when creating CLs, to specify which reviewer should look at which files, to speed up review and reduce questions. Unfortunately, Gerrit doesn't allow for only specific files, so it's all or nothing. Hence the clarification request. Can you clarify exactly which files you'd like for me to review? It looks like you're the owner of the related files in components/* and I'm only an owner of one file, in this CL.
Hi, sorry If I wasn't clear before. I know you are not owner of the //custom_handlers component, but there aren't many. I'd you you to review the whole CL, if possible. The idea is that Colin would rubber-stamp the review later.
The change affects to the Chrome navigation when there is a protocol handler registered for a specific URL scheme. It shouldn't affect in any other case.
Related details
- Colin Blundell
- 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 2 comments![]( "Open in Gerrit")
#include "base/test/test_future.h"Javier FernandezNeeded?
Not anymore; probably a leftover of a previous version of the patch.
std::move(callback).Run(true /*permission_granted*/,
true /*remember*/);?
```suggestion
std::move(callback).Run(/*permission_granted=*/true,
/*remember=*/true);
```
Done
Related details
- Colin Blundell
- 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")
hi @solomonkinard
Could you please take a look at the //custom_handlers logic, in the context of the new Extension API described in the design document.
Thanks.
Related details
- Colin Blundell
- 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. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
- 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. |
Solomon Kinard (Gerrit)
Solomon Kinard added 9 comments![]( "Open in Gerrit")
#if !BUILDFLAG(IS_ANDROID)Rationale behind !android?
SetConfirmTestingCallback(/*permission_granted=*/true, /*remember=*/false);Please fix this WARNING reported by ClangTidy: check: bugprone-argument-comment
argument name 'permission_granted' in comment ...
check: bugprone-argument-comment
argument name 'permission_granted' in comment does not match parameter name 'grant' (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-argument-comment` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
// TODO(crbug.com/40482153: Implement the prompt dialog.?
```suggestion
// TODO(crbug.com/40482153): Implement the prompt dialog.
```
std::move(callback).Run(true /*permission_granted*/,
true /*remember*/);?
```suggestion
std::move(callback).Run(/*permission_granted=*/true,
/*remember=*/true);
```
SetConfirmTestingCallback(/*permission_granted=*/true, /*remember=*/false);Please fix this WARNING reported by ClangTidy: check: bugprone-argument-comment
argument name 'permission_granted' in comment ...
check: bugprone-argument-comment
argument name 'permission_granted' in comment does not match parameter name 'grant' (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-argument-comment` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
SetConfirmTestingCallback(/*permission_granted=*/false, /*remember=*/false);Please fix this WARNING reported by ClangTidy: check: bugprone-argument-comment
argument name 'permission_granted' in comment ...
check: bugprone-argument-comment
argument name 'permission_granted' in comment does not match parameter name 'grant' (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-argument-comment` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
SetConfirmTestingCallback(/*permission_granted=*/true, /*remember=*/false);Please fix this WARNING reported by ClangTidy: check: bugprone-argument-comment
argument name 'permission_granted' in comment ...
check: bugprone-argument-comment
argument name 'permission_granted' in comment does not match parameter name 'grant' (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-argument-comment` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
SetConfirmTestingCallback(/*permission_granted=*/false, /*remember=*/false);Please fix this WARNING reported by ClangTidy: check: bugprone-argument-comment
argument name 'permission_granted' in comment ...
check: bugprone-argument-comment
argument name 'permission_granted' in comment does not match parameter name 'grant' (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-argument-comment` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
SetConfirmTestingCallback(/*permission_granted=*/false, /*remember=*/false);Please fix this WARNING reported by ClangTidy: check: bugprone-argument-comment
argument name 'permission_granted' in comment ...
check: bugprone-argument-comment
argument name 'permission_granted' in comment does not match parameter name 'grant' (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-argument-comment` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
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 10 comments![]( "Open in Gerrit")
Thanks for the review @solomonkinard,
I've submitted a new patch addressing the comments. Please, take a look.
#if !BUILDFLAG(IS_ANDROID)Rationale behind !android?
The support of the Protocol Handlers features in Android is very limited [1], that's why the RegisterProtocolHandlerPermissionRequest class is not available [2] in android either.
I think that one of the challenges is to create an android version of the ChromeProtocolHandlerRegistryDelegate, which is the responsible of implementing the OS interaction logic associated to the protocol handler.
Hence, given that the purpose of the new NavigationThrottle is to confirm the protocol handler registration, I decided to exclude it from the android build target.
[1] https://issues.chromium.org/issues/40964464
[2] https://source.chromium.org/chromium/chromium/src/+/main:components/custom_handlers/BUILD.gn;l=33;drc=ffa56f9ae683a0d03075f976685e28f0cb53bff1
SetConfirmTestingCallback(/*permission_granted=*/true, /*remember=*/false);Please fix this WARNING reported by ClangTidy: check: bugprone-argument-comment
argument name 'permission_granted' in comment ...
check: bugprone-argument-comment
argument name 'permission_granted' in comment does not match parameter name 'grant' (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-argument-comment` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Done
// TODO(crbug.com/40482153: Implement the prompt dialog.?
```suggestion
// TODO(crbug.com/40482153): Implement the prompt dialog.
```
Done
std::move(callback).Run(true /*permission_granted*/,
true /*remember*/);?
```suggestion
std::move(callback).Run(/*permission_granted=*/true,
/*remember=*/true);
```
Done
SetConfirmTestingCallback(/*permission_granted=*/true, /*remember=*/false);Please fix this WARNING reported by ClangTidy: check: bugprone-argument-comment
argument name 'permission_granted' in comment ...
check: bugprone-argument-comment
argument name 'permission_granted' in comment does not match parameter name 'grant' (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-argument-comment` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Done
SetConfirmTestingCallback(/*permission_granted=*/false, /*remember=*/false);Please fix this WARNING reported by ClangTidy: check: bugprone-argument-comment
argument name 'permission_granted' in comment ...
check: bugprone-argument-comment
argument name 'permission_granted' in comment does not match parameter name 'grant' (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-argument-comment` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Done
SetConfirmTestingCallback(/*permission_granted=*/true, /*remember=*/false);Please fix this WARNING reported by ClangTidy: check: bugprone-argument-comment
argument name 'permission_granted' in comment ...
check: bugprone-argument-comment
argument name 'permission_granted' in comment does not match parameter name 'grant' (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-argument-comment` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Done
SetConfirmTestingCallback(/*permission_granted=*/false, /*remember=*/false);Please fix this WARNING reported by ClangTidy: check: bugprone-argument-comment
argument name 'permission_granted' in comment ...
check: bugprone-argument-comment
argument name 'permission_granted' in comment does not match parameter name 'grant' (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-argument-comment` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Done
SetConfirmTestingCallback(/*permission_granted=*/false, /*remember=*/false);Please fix this WARNING reported by ClangTidy: check: bugprone-argument-comment
argument name 'permission_granted' in comment ...
check: bugprone-argument-comment
argument name 'permission_granted' in comment does not match parameter name 'grant' (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-argument-comment` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
- 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. |
Solomon Kinard (Gerrit)
Solomon Kinard added 9 comments![]( "Open in Gerrit")
Thanks. A few quick questions.
#if !BUILDFLAG(IS_ANDROID)Javier FernandezRationale behind !android?
The support of the Protocol Handlers features in Android is very limited [1], that's why the RegisterProtocolHandlerPermissionRequest class is not available [2] in android either.
I think that one of the challenges is to create an android version of the ChromeProtocolHandlerRegistryDelegate, which is the responsible of implementing the OS interaction logic associated to the protocol handler.
Hence, given that the purpose of the new NavigationThrottle is to confirm the protocol handler registration, I decided to exclude it from the android build target.
[1] https://issues.chromium.org/issues/40964464
[2] https://source.chromium.org/chromium/chromium/src/+/main:components/custom_handlers/BUILD.gn;l=33;drc=ffa56f9ae683a0d03075f976685e28f0cb53bff1
Acknowledged
// could be null in some unit tests.Complete sentence with leading capital?
#if !BUILDFLAG(IS_ANDROID)
std::unique_ptr<apps::LinkCapturingNavigationThrottle::Delegate>
link_capturing_delegate;
#if BUILDFLAG(IS_CHROMEOS)
link_capturing_delegate =
std::make_unique<apps::ChromeOsLinkCapturingDelegate>();
bool url_to_apps_throttle_created =
#else // BUILDFLAG(IS_CHROMEOS)
link_capturing_delegate =
std::make_unique<web_app::WebAppLinkCapturingDelegate>();
#endif // BUILDFLAG(IS_CHROMEOS)
apps::LinkCapturingNavigationThrottle::MaybeCreateAndAdd(
registry, std::move(link_capturing_delegate));
#if BUILDFLAG(IS_CHROMEOS)
// TODO(crbug.com/366547977): This currently does nothing and allows all
// navigations to proceed if v2 is enabled on ChromeOS. Implement.
bool chromeos_reimpl_navigation_throttle_created =
apps::ChromeOsReimplNavigationCapturingThrottle::MaybeCreateAndAdd(
registry);
// Verify the v1 and reimpl throttles have not been created at the same time.
CHECK(!chromeos_reimpl_navigation_throttle_created ||
!url_to_apps_throttle_created);
#endif // BUILDFLAG(IS_CHROMEOS)
web_app::NavigationCapturingRedirectionThrottle::MaybeCreateAndAdd(registry);
#endif // !BUILDFLAG(IS_ANDROID)
Profile* profile =
Profile::FromBrowserContext(handle.GetWebContents()->GetBrowserContext());
#if !BUILDFLAG(IS_ANDROID)
if (base::FeatureList::IsEnabled(
extensions_features::kExtensionProtocolHandlers)) {
// could be null in some unit tests.
if (auto* protocol_handler_registry =
ProtocolHandlerRegistryFactory::GetForBrowserContext(profile)) {
custom_handlers::ProtocolHandlerNavigationThrottle::MaybeCreateAndAdd(
protocol_handler_registry, registry);
}
}
#endif // !BUILDFLAG(IS_ANDROID)Is it better or worse to reuse the existing `!ANDROID` block vs making a new one?
```suggestion
Profile* profile =
Profile::FromBrowserContext(handle.GetWebContents()->GetBrowserContext());
#if !BUILDFLAG(IS_ANDROID)
std::unique_ptr<apps::LinkCapturingNavigationThrottle::Delegate>
link_capturing_delegate;
#if BUILDFLAG(IS_CHROMEOS)
link_capturing_delegate =
std::make_unique<apps::ChromeOsLinkCapturingDelegate>();
bool url_to_apps_throttle_created =
#else // BUILDFLAG(IS_CHROMEOS)
link_capturing_delegate =
std::make_unique<web_app::WebAppLinkCapturingDelegate>();
#endif // BUILDFLAG(IS_CHROMEOS)
apps::LinkCapturingNavigationThrottle::MaybeCreateAndAdd(
registry, std::move(link_capturing_delegate));
#if BUILDFLAG(IS_CHROMEOS)
// TODO(crbug.com/366547977): This currently does nothing and allows all
// navigations to proceed if v2 is enabled on ChromeOS. Implement.
bool chromeos_reimpl_navigation_throttle_created =
apps::ChromeOsReimplNavigationCapturingThrottle::MaybeCreateAndAdd(
registry);
// Verify the v1 and reimpl throttles have not been created at the same time.
CHECK(!chromeos_reimpl_navigation_throttle_created ||
!url_to_apps_throttle_created);
#endif // BUILDFLAG(IS_CHROMEOS)
web_app::NavigationCapturingRedirectionThrottle::MaybeCreateAndAdd(registry);
if (base::FeatureList::IsEnabled(
extensions_features::kExtensionProtocolHandlers)) {
// could be null in some unit tests.
if (auto* protocol_handler_registry =
ProtocolHandlerRegistryFactory::GetForBrowserContext(profile)) {
custom_handlers::ProtocolHandlerNavigationThrottle::MaybeCreateAndAdd(
protocol_handler_registry, registry);
}
}
#endif // !BUILDFLAG(IS_ANDROID)
```
// navitation process to prompt the user about the registered custom handler.```suggestion
// navigation process to prompt the user about the registered custom handler.
```
// If there is an "unconfirmed" handler for the Navigation Requests's url, a?
```suggestion
// If there is an "unconfirmed" handler for the Navigation Request's url, a
```
// registered for the navigation request's url scheme. The onging navigation?
class ProtocolHandlerNavigationThrottleBrowserTestAre any more tests planned? Asking because several of these tests are identical except for maybe a boolean value, variable name, and comment.
void ConfirmProtocolHandler(std::string_view scheme, bool save);OOC (out of curiosity), is it possible that this method can exit without confirmation, e.g. in the `!RegisterProtocolHandler(handler, USER)` case? If so maybe `MaybeConfirmProtocolHandler`? Maybe it's a moot point because it can be confirmed afterward with `IsProtocolHandlerConfirmed`?
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 voted and added 8 comments![]( "Open in Gerrit")
Votes added by Javier Fernandez
| Commit-Queue | +1 |
8 comments
Thanks for the review.
I commented inline.
Complete sentence with leading capital?
Done
#if !BUILDFLAG(IS_ANDROID)Moving the Profile declaration above would allow to define the new code inside the already existent !IS_ANDROID buildflag block. Thanks for the suggestion, I think it's better, indeed.
// navitation process to prompt the user about the registered custom handler.```suggestion
// navigation process to prompt the user about the registered custom handler.
```
Done
// If there is an "unconfirmed" handler for the Navigation Requests's url, a?
```suggestion
// If there is an "unconfirmed" handler for the Navigation Request's url, a
```
Done
// registered for the navigation request's url scheme. The onging navigationJavier Fernandez?
s/onging/ongoing
class ProtocolHandlerNavigationThrottleBrowserTestAre any more tests planned? Asking because several of these tests are identical except for maybe a boolean value, variable name, and comment.
I didn´t have more tests in mind. I think they cover the 4 basic cases this new logic implements:
1- Confirm (without remember decision)
2- Deny (without remember decision)
3- Deny after having confirmed first
4- Confirm (remember the decision)
It's true that some of this tests could be parameterized, giving us the opportunity to do a more exhaustive testing, but I'm not sure it's worth.
void ConfirmProtocolHandler(std::string_view scheme, bool save);OOC (out of curiosity), is it possible that this method can exit without confirmation, e.g. in the `!RegisterProtocolHandler(handler, USER)` case? If so maybe `MaybeConfirmProtocolHandler`? Maybe it's a moot point because it can be confirmed afterward with `IsProtocolHandlerConfirmed`?
The code you mention is defined to ignore invalid handlers, which is not possible to happen at this point, because the extension's Manifest parsing shouldn't permit to register invalid handlers. Maybe we could change the implementation of this method to avoid the confusion.
The implementation also ignores handlers already confirmed; do you think this alone justifies adding the Maybe prefix ?
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. |
Solomon Kinard (Gerrit)
Solomon Kinard added 6 comments![]( "Open in Gerrit")
A few questions.
// only for testingComplete sentence: capital latter at the start and a period at the end?
auto& test_callback = GetDialogLaunchCallbackForTesting(); // IN-TEST1. Could this be called something like `callback_for_test` instead.
1. [`// IN-TEST`](crsrc.org/s/?q=%22%2F%2F%20IN-TEST%22%20f:cc$%20f:extension) is optional?
1. Could CHECK_IS_TEST() work within this `{}`?
class ProtocolHandlerNavigationThrottleBrowserTestJavier FernandezAre any more tests planned? Asking because several of these tests are identical except for maybe a boolean value, variable name, and comment.
I didn´t have more tests in mind. I think they cover the 4 basic cases this new logic implements:
1- Confirm (without remember decision)
2- Deny (without remember decision)
3- Deny after having confirmed first
4- Confirm (remember the decision)It's true that some of this tests could be parameterized, giving us the opportunity to do a more exhaustive testing, but I'm not sure it's worth.
With respect to `tests are identical`, what do you think about getting started with something like?
```
RegisterProtocolHandler() {
GURL handler_url =
embedded_test_server()->GetURL("/custom_handlers/custom_handler.html");
ProtocolHandler ph1 = CreateUnconfirmedProtocolHandler("news", handler_url);
registry->OnAcceptRegisterProtocolHandler(ph1);
ASSERT_TRUE(registry->IsHandledProtocol("news"));
ASSERT_FALSE(registry->IsProtocolHandlerConfirmed("news"));
}
```
// Only handlers with extension id can be unconfirmed for now.For my own understanding, what do you mean by "handlers with extension id"? Is it expected in general that handlers would be useful without an extension id?
const std::string extension_id("fooabbbbccccddddeeeeffffgggghhhh");1. Is there any significance to this [extension id](crsrc.org/s/?q=fooabbbbccccddddeeeeffffgggghhhh)? Asking only because it's not the usual ascending or full repeated e.g. `a*` or `abcdefghijklmnopabcdefghijklmnop`.
1. Same with `barabbbbccccddddeeeeffffgggghhhh` below.
1. Would it be helpful to put any of these in the class or the file as a const?
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 voted and added 6 comments![]( "Open in Gerrit")
Votes added by Javier Fernandez
| Commit-Queue | +1 |
6 comments
Thanks for the reviews @solomonkinard
I have applied the refactoring you suggested in the //component browser tests. As a matter of fact, I've had to move 2 tests to the //chrome browser tests instead, because the depended on the profile settings and they weren't working as expected before.
I also added a few more tests in the //component browser tests.
Complete sentence: capital latter at the start and a period at the end?
Done
auto& test_callback = GetDialogLaunchCallbackForTesting(); // IN-TEST1. Could this be called something like `callback_for_test` instead.
1. [`// IN-TEST`](crsrc.org/s/?q=%22%2F%2F%20IN-TEST%22%20f:cc$%20f:extension) is optional?
1. Could CHECK_IS_TEST() work within this `{}`?
The presubmit checks showed warnings suggesting to add shot comments in codepaths inside a ForTesting method.
class ProtocolHandlerNavigationThrottleBrowserTestJavier FernandezAre any more tests planned? Asking because several of these tests are identical except for maybe a boolean value, variable name, and comment.
Solomon KinardI didn´t have more tests in mind. I think they cover the 4 basic cases this new logic implements:
1- Confirm (without remember decision)
2- Deny (without remember decision)
3- Deny after having confirmed first
4- Confirm (remember the decision)It's true that some of this tests could be parameterized, giving us the opportunity to do a more exhaustive testing, but I'm not sure it's worth.
With respect to `tests are identical`, what do you think about getting started with something like?
```
RegisterProtocolHandler() {
GURL handler_url =
embedded_test_server()->GetURL("/custom_handlers/custom_handler.html");
ProtocolHandler ph1 = CreateUnconfirmedProtocolHandler("news", handler_url);
registry->OnAcceptRegisterProtocolHandler(ph1);
ASSERT_TRUE(registry->IsHandledProtocol("news"));
ASSERT_FALSE(registry->IsProtocolHandlerConfirmed("news"));
}
```
We can apply this refactoring, indeed.
Thanks !
// Only handlers with extension id can be unconfirmed for now.For my own understanding, what do you mean by "handlers with extension id"? Is it expected in general that handlers would be useful without an extension id?
Yes, absolutely. The ProtocolHandler class is used by the Web API (registerProtocolHandler JS method), or PWAs which would assign an appId instead, or like in this case, an extension via the protocol_handlers Manifest Key.
const std::string extension_id("fooabbbbccccddddeeeeffffgggghhhh");1. Is there any significance to this [extension id](crsrc.org/s/?q=fooabbbbccccddddeeeeffffgggghhhh)? Asking only because it's not the usual ascending or full repeated e.g. `a*` or `abcdefghijklmnopabcdefghijklmnop`.
1. Same with `barabbbbccccddddeeeeffffgggghhhh` below.
1. Would it be helpful to put any of these in the class or the file as a const?
There is no special significance. I've seen that pattern in other extension related tests. Any of the ones you suggested is fine as well; I'm going to change them and define them as const exp as well.
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. |
Solomon Kinard (Gerrit)
Solomon Kinard added 4 comments![]( "Open in Gerrit")
A quick question.
GURL url("news:test");1. For my own understanding, should this be "news://test" for clarity, or is "news:test" intentional here and elsewhere?
1. Are these schemes already [safelisted](https://html.spec.whatwg.org/multipage/system-state.html#normalize-protocol-handler-parameters)?
// Only handlers with extension id can be unconfirmed for now.Javier FernandezFor my own understanding, what do you mean by "handlers with extension id"? Is it expected in general that handlers would be useful without an extension id?
Yes, absolutely. The ProtocolHandler class is used by the Web API (registerProtocolHandler JS method), or PWAs which would assign an appId instead, or like in this case, an extension via the protocol_handlers Manifest Key.
Acknowledged
void ConfirmProtocolHandler(std::string_view scheme, bool save);Javier FernandezOOC (out of curiosity), is it possible that this method can exit without confirmation, e.g. in the `!RegisterProtocolHandler(handler, USER)` case? If so maybe `MaybeConfirmProtocolHandler`? Maybe it's a moot point because it can be confirmed afterward with `IsProtocolHandlerConfirmed`?
The code you mention is defined to ignore invalid handlers, which is not possible to happen at this point, because the extension's Manifest parsing shouldn't permit to register invalid handlers. Maybe we could change the implementation of this method to avoid the confusion.
The implementation also ignores handlers already confirmed; do you think this alone justifies adding the Maybe prefix ?
- 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 1 comment![]( "Open in Gerrit")
GURL url("news:test");1. For my own understanding, should this be "news://test" for clarity, or is "news:test" intentional here and elsewhere?
1. Are these schemes already [safelisted](https://html.spec.whatwg.org/multipage/system-state.html#normalize-protocol-handler-parameters)?
The hier-part ("//") is only mandatory for for hierarchical URLs (eg HTTP), but it's valid for the "news" scheme. Other than that, there is no special reason to avoid it in these tests; it's a pattern used in other ProtocolHandler tests. Also, it may helps to ensure the GURL parsing is still accepting such kind of URLs.
Regarding 2) Yes, the "news" scheme is inclided in the safe-list defined in the HTML spec [1] and implemented accordingly in the protocol_handler_utils.cc [2]
[1] https://html.spec.whatwg.org/#safelisted-scheme
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/common/custom_handlers/protocol_handler_utils.cc;l=108;drc=086db2cecef8efb5794e7cb958d474cdf73dd6a8
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. |
Solomon Kinard (Gerrit)
Solomon Kinard voted and added 2 comments![]( "Open in Gerrit")
Votes added by Solomon Kinard
| Code-Review | +1 |
2 comments
GURL url("news:test");Javier Fernandez1. For my own understanding, should this be "news://test" for clarity, or is "news:test" intentional here and elsewhere?
1. Are these schemes already [safelisted](https://html.spec.whatwg.org/multipage/system-state.html#normalize-protocol-handler-parameters)?
The hier-part ("//") is only mandatory for for hierarchical URLs (eg HTTP), but it's valid for the "news" scheme. Other than that, there is no special reason to avoid it in these tests; it's a pattern used in other ProtocolHandler tests. Also, it may helps to ensure the GURL parsing is still accepting such kind of URLs.
Regarding 2) Yes, the "news" scheme is inclided in the safe-list defined in the HTML spec [1] and implemented accordingly in the protocol_handler_utils.cc [2]
[1] https://html.spec.whatwg.org/#safelisted-scheme
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/common/custom_handlers/protocol_handler_utils.cc;l=108;drc=086db2cecef8efb5794e7cb958d474cdf73dd6a8
Acknowledged
// translate the URL so that the navigation request could be completed```suggestion
// translate the URL so that the navigation request could be completed.
```
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 3 comments![]( "Open in Gerrit")
Thanks. Are you requesting a review from me for protocol_handlers_manager_browsertest.cc, or other files too (can you specify)?
Solomon KinardI think that since you were reviewing the patches of this new Extension API since the beginning you'd have enough context to review the protocol handlers related changes as well.
Devlin could also do it, but it seem is OOO until December. Do you know if someone else would be more appropriated to review the protocol handler files ?
Javier FernandezWould you like to let me know which files exactly? You are welcome to request any reviewer that you'd like. I only called out the one file shown in Gerrit with my name associated after clicking "Add owners". I have been asked before, when creating CLs, to specify which reviewer should look at which files, to speed up review and reduce questions. Unfortunately, Gerrit doesn't allow for only specific files, so it's all or nothing. Hence the clarification request. Can you clarify exactly which files you'd like for me to review? It looks like you're the owner of the related files in components/* and I'm only an owner of one file, in this CL.
Hi, sorry If I wasn't clear before. I know you are not owner of the //custom_handlers component, but there aren't many. I'd you you to review the whole CL, if possible. The idea is that Colin would rubber-stamp the review later.
The change affects to the Chrome navigation when there is a protocol handler registered for a specific URL scheme. It shouldn't affect in any other case.
Done
// translate the URL so that the navigation request could be completed```suggestion
// translate the URL so that the navigation request could be completed.
```
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 @rdevlin.cronin
Colin was the owner I asked initially, but said he lacked enough context, so that's why I have added Solomon to the loop. Solomon gave the lgtm, but it seems I need someone to rubber-stamp the review.
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 1 comment![]( "Open in Gerrit")
Hi @rdevlin.cronin
Colin was the owner I asked initially, but said he lacked enough context, so that's why I have added Solomon to the loop. Solomon gave the lgtm, but it seems I need someone to rubber-stamp the review.
I'm also not an owner of //chrome/browser/custom_handlers/chrome_protocol_handler_navigation_throttle_browsertests.cc, so I won't be able to stamp. Maybe engedy@? (Added)
Related details
- Balazs Engedy
- Javier Fernandez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Balazs Engedy (Gerrit)
Balazs Engedy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Balazs Engedy
| Code-Review | +1 |
1 comment
Apologies for the delay, I was already out of office for the holidays. LGTM, thank you for adding an end-to-end test.
Related details
- Javier Fernandez
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 Commit-Queue+2![]( "Open in Gerrit")
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 Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
20 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: components/custom_handlers/protocol_handler_navigation_throttle.cc
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/custom_handlers/chrome_protocol_handler_navigation_throttle_browsertests.cc
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: components/custom_handlers/protocol_handler_navigation_throttle.h
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: components/custom_handlers/protocol_handler_navigation_throttle_browsertest.cc
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Change information
Intercept navigation when trying to use an unconfirmed protocol handler
The new 'protocol_handlers' Manifest Key allows an extension to register
a few protocol handlers for some specific URL's schemes, similarly to
what the registerProtocolHandler Web API provides. However, unlike the
handlers registered via the Web API, these ones are registered under an
'unconfirmed" state, as explained in the corresponding design doc [1].
The design document specifies that the user must grant permission to use
any protocol handlers registered by an extension, at runtime, just
before it's used. This CL implements the change in the navigation logic
to add a NavigationThrottle if there is a custom handler for the URL's
scheme of the ongoing navigation request and such handler is in the
mentioned 'unconfirmed' state.
This change affects only to the Custom Handlers logic, implementing a
dummy logic for the Prompt Dialog functionality. This will be
implemented in a follow up patch.
[1]https://docs.google.com/document/d/1e6mSsbjLqBd1_4EAS_AX543vy53oq8SlycNwQ0mZ46g/edit?usp=sharing
- M chrome/browser/chrome_content_browser_client_navigation_throttles.cc
- A chrome/browser/custom_handlers/chrome_protocol_handler_navigation_throttle_browsertests.cc
- M chrome/browser/extensions/api/protocol_handlers/protocol_handlers_manager_browsertest.cc
- M chrome/test/BUILD.gn
- M components/custom_handlers/BUILD.gn
- M components/custom_handlers/protocol_handler.cc
- M components/custom_handlers/protocol_handler.h
- A components/custom_handlers/protocol_handler_navigation_throttle.cc
- A components/custom_handlers/protocol_handler_navigation_throttle.h
- A components/custom_handlers/protocol_handler_navigation_throttle_browsertest.cc
- M components/custom_handlers/protocol_handler_registry.cc
- M components/custom_handlers/protocol_handler_registry.h
- M components/custom_handlers/protocol_handler_registry_unittest.cc
- M components/custom_handlers/protocol_handler_throttle.cc
Code-Review: +1 by Balazs Engedy, +1 by Solomon Kinard
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula added 1 comment![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Battre (Gerrit)
Dominic Battre has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |