Skip to content
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

Brave url scheme android #1007

Closed
wants to merge 3 commits into from
Closed

Conversation

AlexeyBarabash
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash commented Dec 3, 2018

Addresses https://github.com/brave/browser-android-tabs/issues/850.

Contains adaptation of PRs #584 and #164 for re-use with Android.

Requires PR from Android https://github.com/brave/browser-android-tabs/pull/912 .

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
    • Android
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

  1. Build and install apk.
  2. Type in address bar brave://version .
  3. Should be loaded originally known chrome://version page with brave://version in an url bar.
  4. Repete 2-3 with chrome://version

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

return (scheme1 == kBraveUIScheme && scheme2 == kChromeUIScheme) ||
(scheme1 == kChromeUIScheme && scheme2 == kBraveUIScheme) ||
(scheme1 == url::kAboutScheme && scheme2 == kBraveUIScheme) ||
(scheme1 == kBraveUIScheme && scheme2 == url::kAboutScheme);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe you need to compare about and chrome schemes here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I need this also, as I need to compare for equality all the pairs from <brave, chrome, about>:

brave<->chrome 
brave<->about
chrome<->about

But chrome<->about comparison is already done in IsEquivalentScheme_ChromiumImpl , https://cs.chromium.org/chromium/src/components/url_formatter/url_fixer.cc?type=cs&q=IsEquivalentScheme&g=0&l=683

bool IsEquivalentScheme(const std::string& scheme1,
                        const std::string& scheme2) {
  return scheme1 == scheme2 ||
         (scheme1 == url::kAboutScheme && scheme2 == kChromeUIScheme) ||
         (scheme1 == kChromeUIScheme && scheme2 == url::kAboutScheme);
}

So I used compare chrome<->about by invoking IsEquivalentScheme_ChromiumImp .

Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it currently work with typing in brave://about or another brave:// URL?

On desktop we use this instead of adding a standard scheme:
chromium_src/chrome/browser/browser_about_handler.cc

@AlexeyBarabash
Copy link
Contributor Author

@bbondy
Yes, it works with brave://about and about:version urls.

This PR is spread over the two repos, and it is also using browser_about_handler.cc at
https://github.com/brave/browser-android-tabs/pull/912/files#diff-8b62ac59d92e56ecc354070fc66774aeR155
.

@@ -10,7 +10,13 @@
class BraveToolbarModelImpl : public ToolbarModelImpl {
public:
using ToolbarModelImpl::ToolbarModelImpl;
#if defined(OS_ANDROID)
// On Android GetURLForDisplay is not invoked as on brave-coer,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: spelling of brave-core

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed.

@bsclifton
Copy link
Member

@AlexeyBarabash is this something that we'd still like to do? or can it be closed?

@AlexeyBarabash
Copy link
Contributor Author

@bsclifton I will check if brave://works on current Android-core build.
If it already works, then will close this PR.
Otherwise either close this PR or update.

@AlexeyBarabash
Copy link
Contributor Author

@bsclifton
Just finished build and I see brave://xxx works, but when the page gets loaded, I see chrome://xxx in url bar.
So the changes inherited from brave-core codebase work particularly.
I had met such situation while working on this PR and I will check which of these commits does fix of described about case.

@bsclifton
Copy link
Member

Closing as stale - let's create a new PR if we address this in the future 😄

@bsclifton bsclifton closed this Jun 18, 2020
@bsclifton bsclifton deleted the brave_url_scheme_android branch November 10, 2023 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants