use brave:// for the virtual url and chrome:// for the internal u…

[bridiver]

…rl to load

fix brave/brave-browser#1616

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
• 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:

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

[bbondy]

nice cleanup

[simonhong]

It would be good to have a test that checks virtual url is brave url for chrome or brave webui navigation.

[bridiver]

yep, I'm going to look to see what tests already exist and see what others make sense. It looks like some tests are broken now, but haven't looked at them yet so not sure if it's a code or test issue

[simonhong]

I think only fixup handler should be executed when original url is chrome.

This PR adds unnecessary mapping from brave to chrome url in URLHandler when |original_url| is chrome url. When |original_url| is chrome url, we just want to set brave url as a virtual url and original chrome url should be passed to |load_to_url|.

In the near future, we want to delete mapping code in URLHandler because we want to move mapping timing from URLHandler to more earlier. So, URLHandler should not be executed when chrome url is original url.

void RewriteUrlForNavigation(const GURL& original_url,
                             BrowserContext* browser_context,
                             GURL* url_to_load,
                             GURL* virtual_url,
                             bool* reverse_on_redirect) {
  // Fix up the given URL before letting it be rewritten, so that any minor
  // cleanup (e.g., removing leading dots) will not lead to a virtual URL.
  *virtual_url = original_url;
  BrowserURLHandlerImpl::GetInstance()->FixupURLBeforeRewrite(virtual_url,
                                                              browser_context);

  // Allow the browser URL handler to rewrite the URL. This will, for example,
  // remove "view-source:" from the beginning of the URL to get the URL that
  // will actually be loaded. This real URL won't be shown to the user, just
  // used internally.
  *url_to_load = *virtual_url;
  BrowserURLHandlerImpl::GetInstance()->RewriteURLIfNecessary(
      url_to_load, browser_context, reverse_on_redirect);
}

[bridiver]

@simonhong what unnecessary mapping is happening? We are mapping the virtual url to brave:// and the url_to_load to chrome://

[simonhong]

@bridiver Yep, that's the result of URLHandler.
I mean URLHandler doesn't need to be involved when original url is chrome url.
If original url is chrome url, we just need to set brave url as a virtual url by fixup. I think that's all what we need when original url is chrome url.
However, load_to_url is initialized with fixed virtual url(brave url).
So, additional mapping from brave to chrome should be done.

[simonhong]

This point is what I worried.
When user types chrome url, load to url is initialized with brave url.
Then, I think brave url is the input of URLHandler before it is re-written our handler by below.
If so, that could cause skipping upstream URLHandler that checks chrome scheme.

[bridiver]

I see your point, but we may need to add two handers then because we have to run after ChromeContentBrowserClient to fix kChromeUISyncInternalsHost. I'll look at the upstream code to try to find something it breaks and update either way to convert back to chrome before the superclass method

[simonhong]

nit: I think this second condition in if clause can be removed.

[simonhong]

Q) Why do you make rest handler skip for these two hosts (sync and welcome) by returning true?
Do you think upstream url handler doesn't need to handle them?

[simonhong]

Ah! I saw upstream code tries to rewritten it. Ignore my above comment.

[simonhong]

Do we need to add new pair after adding super class' handlers?

[bridiver]

yes, the order is critical because of rewriting that the superclass does

[simonhong]

Just curiosity, any particular reason about excluding newtab host?

[bridiver]

yes, it breaks the extension newtab override functionality

[fmarier]

I'm going to remove myself from the list of reviewers since I don't think I'll have much useful feedback to provide here.