Don't refer browser-initiated navigations to web-safe URLs to delegate.

Don't refer browser-initiated navigations to web-safe URLs to delegate.

In a previous CL (https://codereview.chromium.org/890183002) it was
decided to refer browser-initiated navigations to the owner WebContents'
delegate, on the assumption that the navigation was to a non-web-safe
url (e.g. a "chrome"-scheme url). However, this change can block
guest navigations to web-safe URLs just because they originated, for
example, from an extension.

This CL ensures that navigations to web-safe URLs are not referred to
the delegate in order to allow them to succeed.

BUG=488053
Committed: https://crrev.com/a03c23d32d176daf1e95cfffbfa333316e1eb1a6
Cr-Commit-Position: refs/heads/master@{#339204}

[wjmaclean, 2015-07-16 19:18:51]

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

[wjmaclean, 2015-07-16 19:19:07]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[wjmaclean, 2015-07-16 19:19:49]

creis@ - it seems that the safest way to address this bug is in WebViewGuest ...
please take a look? (Small CL.)

[commit-bot: I haz the power, 2015-07-16 19:20:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1234403005/1

[commit-bot: I haz the power, 2015-07-16 20:29:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-16 20:29:48]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2015-07-17 00:05:15]

I'm sad that we aren't fixing the strangeness that extension navigations get
branded as browser-initiated, since that leaves big open questions about whether
they should be allowed to go to chrome:// URLs or be treated in the same way as
context menus here.  I suppose that's a bigger issue to discuss on the other CL
(https://codereview.chromium.org/1227793006/), so I'll bring it up there.

LGTM with nits.

https://codereview.chromium.org/1234403005/diff/1/chrome/browser/apps/guest_v...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/1234403005/diff/1/chrome/browser/apps/guest_v...
chrome/browser/apps/guest_view/web_view_browsertest.cc:1606: // Navigating to a
file URL is forbidden inside a <webview>.
This comment is stale.  The navigation is supposed to be allowed, right?

https://codereview.chromium.org/1234403005/diff/1/chrome/browser/apps/guest_v...
chrome/browser/apps/guest_view/web_view_browsertest.cc:1617: // Verify that the
<webview> ends up at about:blank.
This comment is stale.

https://codereview.chromium.org/1234403005/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1234403005/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.cc:1227: // urls should
not be referred to the delegate, which may block them.
I feel like this code is getting very difficult to follow or reason about. 
After spending some time refreshing myself on
https://codereview.chromium.org/890183002, I think I've concluded that this
change is safe, since it takes the exception we added (for allowing chrome://
URLs in context menus) and only makes it narrower.  It's pretty hard to conclude
that from reading this whole comment block, so I have a suggested revision
below:

"Most navigations should be handled by WebViewGuest::LoadURLWithParams, which
takes care of blocking chrome:// URLs and other web-unsafe schemes. 
(NavigateGuest and CreateNewGuestWebViewWindow also go through
LoadURLWithParams.)

We make an exception here for context menu items, since the Language Settings
item uses a browser-initiated navigation to a chrome:// URL.  These can be
passed to the embedder's WebContentsDelegate so that the browser performs the
action for the <webview>."

[wjmaclean, 2015-07-17 01:29:46]

Updated comments as requested ... thanks for the comment suggestions, they
explain things well!

https://codereview.chromium.org/1234403005/diff/1/chrome/browser/apps/guest_v...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/1234403005/diff/1/chrome/browser/apps/guest_v...
chrome/browser/apps/guest_view/web_view_browsertest.cc:1606: // Navigating to a
file URL is forbidden inside a <webview>.
On 2015/07/17 00:05:15, Charlie Reis wrote:
> This comment is stale.  The navigation is supposed to be allowed, right?

Done.

https://codereview.chromium.org/1234403005/diff/1/chrome/browser/apps/guest_v...
chrome/browser/apps/guest_view/web_view_browsertest.cc:1617: // Verify that the
<webview> ends up at about:blank.
On 2015/07/17 00:05:15, Charlie Reis wrote:
> This comment is stale.

Done.

https://codereview.chromium.org/1234403005/diff/1/extensions/browser/guest_vi...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1234403005/diff/1/extensions/browser/guest_vi...
extensions/browser/guest_view/web_view/web_view_guest.cc:1227: // urls should
not be referred to the delegate, which may block them.
On 2015/07/17 00:05:15, Charlie Reis wrote:
> I feel like this code is getting very difficult to follow or reason about. 
> After spending some time refreshing myself on
> https://codereview.chromium.org/890183002, I think I've concluded that this
> change is safe, since it takes the exception we added (for allowing chrome://
> URLs in context menus) and only makes it narrower.  

Yes, that was my conclusion, though I didn't think to describe it in those
terms.

> It's pretty hard to conclude
> that from reading this whole comment block, so I have a suggested revision
> below:
> 
> "Most navigations should be handled by WebViewGuest::LoadURLWithParams, which
> takes care of blocking chrome:// URLs and other web-unsafe schemes. 
> (NavigateGuest and CreateNewGuestWebViewWindow also go through
> LoadURLWithParams.)
> 
> We make an exception here for context menu items, since the Language Settings
> item uses a browser-initiated navigation to a chrome:// URL.  These can be
> passed to the embedder's WebContentsDelegate so that the browser performs the
> action for the <webview>."

Sure, this seems reasonable ... done.

[lazyboy, 2015-07-17 01:44:24]

lazyboy@chromium.org changed reviewers:
+ lazyboy@chromium.org

[lazyboy, 2015-07-17 01:44:24]

lgtm

[wjmaclean, 2015-07-17 01:44:45]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2015-07-17 01:44:45]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1234403005/#ps20001
(title: "Improve comments.")

[commit-bot: I haz the power, 2015-07-17 01:46:32]

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

[commit-bot: I haz the power, 2015-07-17 02:37:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-07-17 02:38:47]

Patchset 2 (id:??) landed as
https://crrev.com/a03c23d32d176daf1e95cfffbfa333316e1eb1a6
Cr-Commit-Position: refs/heads/master@{#339204}

[wjmaclean, 2015-09-16 20:15:46]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1350893002/ by wjmaclean@chromium.org.

The reason for reverting is: Since this CL was landed something has changed that
causes clinking on links in the PDF viewer (embedded inside another WebView,
e.g. the Chrome app "Browser Sample") to open in the wrong window, or in the
case of "Open in a new tab", not be opened at all.

Reverting this CL fixes
https://code.google.com/p/chromium/issues/detail?id=529187 and
https://code.google.com/p/chromium/issues/detail?id=521573

A new bug has been filed to capture the error where the links are not opened in
the correct target window:

https://code.google.com/p/chromium/issues/detail?id=532621

.