Move URL fixup to a preliminary phase that doesn't affect virtual URLs.

Move URL fixup to a preliminary phase that doesn't affect virtual URLs.

Uses BrowserURLHandler since this depends on components and can't
live in content.

BUG=449829
TEST=See bug for repro steps.

Committed: https://crrev.com/94a977f6dc4714f49841db1c2d831ab53e557f15
Cr-Commit-Position: refs/heads/master@{#316923}

[Charlie Reis, 2015-02-17 17:24:45]

creis@chromium.org changed reviewers:
+ msw@chromium.org, zea@chromium.org

[Charlie Reis, 2015-02-17 17:24:46]

msw@: Can you review?  We've been discussing the approach on the bug, but I
thought I'd get the patch working to show what I mean.

zea@: Can you review the chrome/browser/sync changes?  It looks like there's a
bug in one of the sync tests, which navigates to a chrome:// URL even though
those don't get synced.

https://codereview.chromium.org/923183003/diff/40001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/synced_session.cc (left):

https://codereview.chromium.org/923183003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/synced_session.cc:59: //
SessionModelAssociator::ShouldSyncTab to ensure the logic matches.
SessionModelAssociator no longer exists.  However, both of that method and this
one show why the Sync tests should not be using chrome: URLs (or their about:
equivalents).

https://codereview.chromium.org/923183003/diff/40001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
(left):

https://codereview.chromium.org/923183003/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc:73:
const GURL url("about:version");
Sync doesn't allow chrome:// URLs to be synced, so this was not a good choice of
test page.  It just happened to work because of a quirk in the way we handled
virtual URLs.

[Nicolas Zea, 2015-02-17 18:23:54]

sync LGTM, thanks for fixing that test

[msw, 2015-02-17 22:51:38]

https://codereview.chromium.org/923183003/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/923183003/diff/60001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl.cc:170: GURL dest_url =
GetContentClient()->browser()->FixupURL(url);
Why can't this just call url_fixer::FixupURL directly?

[Charlie Reis, 2015-02-17 22:57:27]

https://codereview.chromium.org/923183003/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/923183003/diff/60001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl.cc:170: GURL dest_url =
GetContentClient()->browser()->FixupURL(url);
On 2015/02/17 22:51:38, msw wrote:
> Why can't this just call url_fixer::FixupURL directly?

content/ can't depend on components/.

[msw, 2015-02-17 23:06:41]

lgtm with a comment nit

https://codereview.chromium.org/923183003/diff/60001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/923183003/diff/60001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:2286: // Make sure this cleanup
happens before the rewriting phase that determines
nit: it's a little presumptuous of this comment to note that this function is
called before something else.

https://codereview.chromium.org/923183003/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/923183003/diff/60001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl.cc:170: GURL dest_url =
GetContentClient()->browser()->FixupURL(url);
On 2015/02/17 22:57:27, Charlie Reis wrote:
> On 2015/02/17 22:51:38, msw wrote:
> > Why can't this just call url_fixer::FixupURL directly?
> 
> content/ can't depend on components/.

Acknowledged.

[Charlie Reis, 2015-02-17 23:29:33]

New patchsets have been uploaded after l-g-t-m from
zea@chromium.org,msw@chromium.org

[Charlie Reis, 2015-02-17 23:31:48]

creis@chromium.org changed reviewers:
+ jam@chromium.org

[Charlie Reis, 2015-02-17 23:31:49]

Thanks-- new patch uploaded.

@jam: Can you take a look at this as well?  My reasoning for this approach is in
comment 21 on the bug.

https://codereview.chromium.org/923183003/diff/60001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/923183003/diff/60001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:2286: // Make sure this cleanup
happens before the rewriting phase that determines
On 2015/02/17 23:06:41, msw wrote:
> nit: it's a little presumptuous of this comment to note that this function is
> called before something else.

I can see how it reads that way.  Then again, it's the reason for this change. 
I've renamed this to CBC::FixupURLBeforeRewriting so that it's explicit.

[jam, 2015-02-18 06:04:57]

On 2015/02/17 23:31:49, Charlie Reis wrote:
> Thanks-- new patch uploaded.
> 
> @jam: Can you take a look at this as well?  My reasoning for this approach is
in
> comment 21 on the bug.

Is there anyway we can fix this entirely in content? It seems preferable that
any content embedder would have the fix without needing to implement this method
in CBC.

[Charlie Reis, 2015-02-18 17:13:51]

On 2015/02/18 06:04:57, jam wrote:
> On 2015/02/17 23:31:49, Charlie Reis wrote:
> > Thanks-- new patch uploaded.
> > 
> > @jam: Can you take a look at this as well?  My reasoning for this approach
is
> in
> > comment 21 on the bug.
> 
> Is there anyway we can fix this entirely in content? It seems preferable that
> any content embedder would have the fix without needing to implement this
method
> in CBC.

The bug itself lives outside content.  Only content/ + chrome/ has this bug
because browser_about_handler.cc calls FixupURL in its rewriting phase.  Other
content embedders don't have this problem unless they do some kind of fixup of
their own.

[Charlie Reis, 2015-02-18 21:39:38]

After chatting with John, it sounds like it's worthwhile to put this fix closer
to the BrowserURLHandler rewriting logic, rather than cluttering
ContentBrowserClient.  I've moved it to a preliminary fixup URLHandler on
BrowserURLHandler.

@msw, can you take a look at the diff to patch set 6?  The fix works the same
way but is just managed in a different place.  Thanks!

[jam, 2015-02-18 21:56:12]

lgtm

[msw, 2015-02-18 22:01:24]

https://codereview.chromium.org/923183003/diff/100001/chrome/browser/browser_...
File chrome/browser/browser_about_handler.cc (right):

https://codereview.chromium.org/923183003/diff/100001/chrome/browser/browser_...
chrome/browser/browser_about_handler.cc:33: *url =
url_fixer::FixupURL(url->possibly_invalid_spec(), std::string());
Should this call FixupBrowserAboutURL instead?

https://codereview.chromium.org/923183003/diff/100001/chrome/browser/browser_...
File chrome/browser/browser_about_handler.h (right):

https://codereview.chromium.org/923183003/diff/100001/chrome/browser/browser_...
chrome/browser/browser_about_handler.h:16: bool FixupBrowserAboutURL(GURL* url,
content::BrowserContext* browser_context);
Looking through all the URLHandler code, I don't see any actual handlers that
use |browser_context|. If it's not actually used, we should just nix the arg.

https://codereview.chromium.org/923183003/diff/100001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/923183003/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2266:
handler->SetFixupHandler(&FixupBrowserAboutURL);
Is it reasonable to just call AddHandlerPair with FixupBrowserAboutURL before
WillHandleBrowserAboutURL? I don't know the code well, but maybe fixup could
just fit in as a standard handler, rather than a special new fixup handler.

Do you need this exceptional fixup handler notion just for CreateNavigationEntry
to call fixup before other rewriting?

[Charlie Reis, 2015-02-18 22:15:10]

New patchsets have been uploaded after l-g-t-m from jam@chromium.org

[Charlie Reis, 2015-02-18 22:16:18]

https://codereview.chromium.org/923183003/diff/100001/chrome/browser/browser_...
File chrome/browser/browser_about_handler.cc (right):

https://codereview.chromium.org/923183003/diff/100001/chrome/browser/browser_...
chrome/browser/browser_about_handler.cc:33: *url =
url_fixer::FixupURL(url->possibly_invalid_spec(), std::string());
On 2015/02/18 22:01:23, msw wrote:
> Should this call FixupBrowserAboutURL instead?

Good idea.  Done.

https://codereview.chromium.org/923183003/diff/100001/chrome/browser/browser_...
File chrome/browser/browser_about_handler.h (right):

https://codereview.chromium.org/923183003/diff/100001/chrome/browser/browser_...
chrome/browser/browser_about_handler.h:16: bool FixupBrowserAboutURL(GURL* url,
content::BrowserContext* browser_context);
On 2015/02/18 22:01:23, msw wrote:
> Looking through all the URLHandler code, I don't see any actual handlers that
> use |browser_context|. If it's not actually used, we should just nix the arg.

Sadly, HandleNewTabURLRewrite and HandleNewTabURLReverseRewrite use it.  (Would
have been nice!)

https://codereview.chromium.org/923183003/diff/100001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/923183003/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2266:
handler->SetFixupHandler(&FixupBrowserAboutURL);
On 2015/02/18 22:01:23, msw wrote:
> Is it reasonable to just call AddHandlerPair with FixupBrowserAboutURL before
> WillHandleBrowserAboutURL? I don't know the code well, but maybe fixup could
> just fit in as a standard handler, rather than a special new fixup handler.

I don't think so, since all the handlers added by AddHandlerPair will get called
during the rewrite phase and thus affect the virtual URL.  We need something
that runs in advance.

> Do you need this exceptional fixup handler notion just for
CreateNavigationEntry
> to call fixup before other rewriting?

Yes.  (There are other calls to BrowserURLHandler::RewriteURLIfNecessary, but
only the CreateNavigationEntry one has this URL spoof bug.)

[msw, 2015-02-18 22:17:47]

Okay, lgtm.

[Charlie Reis, 2015-02-18 22:18:09]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-02-18 22:20:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/923183003/120001

[commit-bot: I haz the power, 2015-02-18 23:51:14]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-18 23:51:51]

Patchset 7 (id:??) landed as
https://crrev.com/94a977f6dc4714f49841db1c2d831ab53e557f15
Cr-Commit-Position: refs/heads/master@{#316923}