Rewrite chrome:// in edit URLs too (closes brave/brave-browser#1616)

[fmarier]

Both GetFormattedFullURL() and GetURLForDisplay() need to be overridden in
order for the URL to be displayed properly in the URL bar and to be copy/pasted
properly when editing the URL.

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:

1. Open brave://settings.
2. Click on the URL bar to edit the URL.

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

[fmarier]

@yrliou This is part of the patch you have already reviewed in #1074.

It now only addresses brave/brave-browser#1616 since @bridiver suggested a different approach for the pdfjs bit.

[bridiver]

@fmarier I think the problem might be in HandleURLRewrite/HandleURLReverseRewrite in brave_content_browser_client.cc. Talking to @bbondy right now because those methods don't look right to me. Might also be related to other brave/chrome scheme issues cc @simonhong

[bridiver]

let's hold off on this until I can do a little investigation because I think the final fix for the issue @simonhong is working on will also take care of this

[simonhong]

I think this PR is valid because GetURL() fetches url from NavigationEntry.
NavigationEntry doesn't have brave scheme url.

[bridiver]

@simonhong that shouldn't matter with the URLHandler and I think I see what the problem might be

[bridiver]

I'm 95% sure this entire problem is the result of incorrectly changing the virtual url from brave to chrome. Please don't merge this until I get back to you.

[simonhong]

To start navigation of brave webui(brave scheme url), it should be mapped to chrome scheme.
So, NavigationEntry don't have brave scheme url at all.
I can't catch which point is incorrectly changing.
This is not the question about mapping timing. regardless of mapping timing, NavigationEntry should not use brave scheme.

[bridiver]

@simonhong because we're mapping from brave -> chrome in the fixup handler so we're modifying the virtual url to be chrome when we only want to modify the url_to_load to be chrome (which happens in the URLHandler pairs). I don't think BraveLocationBarModelImpl is needed at all. We want the virtual url to be brave because that's what get displayed in the URL bar

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);
}

[simonhong]

However, In upstream code, LocationBarModelImpl fetches url from NavigationEntry.
and it uses chrome url?
Ah, I didn't think about virtual..

[simonhong]

Hmm, I think if we pass brave url when instantiating NavigationEntry, we can miss some url checking. That checking is done only by chrome scheme.
To use that checking, we should make mapping timing earlier. but that means chrome url is passed when NavigationEntry is created.

[bridiver]

I actually think we want to do the exact opposite of what we're doing now in the fixup handler. We should be mapping chrome -> brave so we have the correct virtual url and then map brave -> chrome in the URLHandler pair so we actually load chrome

[bridiver]

@simonhong I don't think this will fix the issue you are working on because the mapping is still happening too late, but it should remove the need for this PR and BraveLocationBarModelImpl in general

[simonhong]

I actually think we want to do the exact opposite of what we're doing now in the fixup handler. We should be mapping chrome -> brave so we have the correct virtual url and then map brave -> chrome in the URLHandler pair so we actually load chrome

I'll think about this comment more.

[bridiver]

@simonhong you don't have to think about it because I just implemented it and seems to work correctly :)

[bbondy]

wouldn't this make various UI components which check against the chrome scheme exactly not work properly?

[bridiver]

@bbondy no, that is the entire purpose of virtual urls. The virtual urls are display-only

[bridiver]

replace by #1385 which maps the virtual url so we don't have to change the urlbar behavior