-
Notifications
You must be signed in to change notification settings - Fork 909
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Load magnet url instead of searching #903
Conversation
@@ -25,7 +25,8 @@ BraveAutocompleteSchemeClassifier::GetInputTypeForScheme( | |||
return metrics::OmniboxInputType::INVALID; | |||
} | |||
if (base::IsStringASCII(scheme) && | |||
base::LowerCaseEqualsASCII(scheme, kBraveUIScheme)) { | |||
(base::LowerCaseEqualsASCII(scheme, kBraveUIScheme) || | |||
base::LowerCaseEqualsASCII(scheme, kMagnetScheme))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you check whether WebTorrent extension is enabled here, probably through ExtensionRegistry?
We could just use chromium's result when WebTorrent is disabled, and only change it when it is enabled, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean IsWebtorrentEnabled(profile) && base::LowerCaseEqualsASCII(scheme, kMagnetScheme)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to include content_browser_client_helper.h
in this cc file but I got some compile error. Did you intend this header should be only included in brave_content_browser_client.cc
?
I'm curious why you implemented all methods in header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. IsWebtorrentEnabled()
changed to api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
Load magnet url instead of searching
Fix brave/brave-browser#1435
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests
) ongit rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
yarn test brave_browser_tests --filter=BraveContentBrowserClientTest.TypedMagnetURL
Reviewer Checklist: