<webview>: Do not show blocked_by_client error page for guest.

<webview>: Do not show blocked_by_client error page for guest.

When a request to load a top level frame inside webview is cancelled by
using <webview>'s WebRequest API, by default we load
"This webpage is blocked by an extension" error page.
This CL avoids loading error page's html in guest render view.
We already fire loadabort event in this case.

BUG=284741
Test=With webrequest API, a webview containing:
<a href="http://foo">Foo</a>

When clicked on the link, if embedder wants to prevent navigating the <webview>
to foo, does so using webRequest API to cancel:

<webview>.request.onBeforeRequest.addListener(function(details) {
  if (details.type == 'main_frame' && details.url == 'http://foo' {
    window.open('...');
    return {cancel: true};
  }
  return {cancel: 'false'};
},
{urls: ['<all_urls>']},
['blocking']);

The "return {cancel:true}" won't load the mentioned error page anymore in
the webview.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223769

[lazyboy, 2013-09-03 22:53:55]

Hi Charlie, Can you take a look at the starter approach? Thanks.

[Charlie Reis, 2013-09-03 23:14:31]

https://chromiumcodereview.appspot.com/23856006/diff/1/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/23856006/diff/1/content/renderer/rende...
content/renderer/render_view_impl.cc:3672: // 'replace' load.  This is necessary
to avoid messing up session history.
I'm having trouble understanding why you want to diverge from the normal
didFailProvisionalLoad behavior.  This comment in particular makes me concerned
that the session history will be affected.

Is it just the "blocked by an extension" wording that's a problem?  Maybe
there's a way to change the wording?  Or is there a reason you can't show an
error page?

[lazyboy, 2013-09-04 20:55:37]

On 2013/09/03 23:14:31, creis wrote:
>
https://chromiumcodereview.appspot.com/23856006/diff/1/content/renderer/rende...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/23856006/diff/1/content/renderer/rende...
> content/renderer/render_view_impl.cc:3672: // 'replace' load.  This is
necessary
> to avoid messing up session history.
> I'm having trouble understanding why you want to diverge from the normal
> didFailProvisionalLoad behavior.  This comment in particular makes me
concerned
> that the session history will be affected.
I have to look into detail about session history, I saw the code above that
early returns for "error.reason == net::ERR_ABORTED", which made me try this
approach.
If the session history is incorrect with these changes, what would be the way to
check this?
Would it be enough to check if reload() and go(-1), go(1) behaves correctly? 
> 
> Is it just the "blocked by an extension" wording that's a problem?  Maybe
> there's a way to change the wording?  Or is there a reason you can't show an
> error page?
I've noted the use case in the Test section of CL description, 
Preventing a link click via webview's webRequest API results in showing this
error
page which is not ideal.

[lazyboy, 2013-09-13 20:09:25]

On 2013/09/04 20:55:37, lazyboy wrote:
> On 2013/09/03 23:14:31, creis wrote:
> >
>
https://chromiumcodereview.appspot.com/23856006/diff/1/content/renderer/rende...
> > File content/renderer/render_view_impl.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/23856006/diff/1/content/renderer/rende...
> > content/renderer/render_view_impl.cc:3672: // 'replace' load.  This is
> necessary
> > to avoid messing up session history.
> > I'm having trouble understanding why you want to diverge from the normal
> > didFailProvisionalLoad behavior.  This comment in particular makes me
> concerned
> > that the session history will be affected.
> I have to look into detail about session history, I saw the code above that
> early returns for "error.reason == net::ERR_ABORTED", which made me try this
> approach.
> If the session history is incorrect with these changes, what would be the way
to
> check this?
> Would it be enough to check if reload() and go(-1), go(1) behaves correctly? 
> > 
> > Is it just the "blocked by an extension" wording that's a problem?  Maybe
> > there's a way to change the wording?  Or is there a reason you can't show an
> > error page?
> I've noted the use case in the Test section of CL description, 
> Preventing a link click via webview's webRequest API results in showing this
> error
> page which is not ideal.

Hey Charlie, can you take another look and suggest based on our discussion on
last weekly mtg?

[Fady Samuel, 2013-09-13 20:12:22]

https://chromiumcodereview.appspot.com/23856006/diff/1/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/23856006/diff/1/content/renderer/rende...
content/renderer/render_view_impl.cc:3672: // 'replace' load.  This is necessary
to avoid messing up session history.
On 2013/09/03 23:14:31, creis wrote:
> I'm having trouble understanding why you want to diverge from the normal
> didFailProvisionalLoad behavior.  This comment in particular makes me
concerned
> that the session history will be affected.
> 
> Is it just the "blocked by an extension" wording that's a problem?  Maybe
> there's a way to change the wording?  Or is there a reason you can't show an
> error page?

There is a desire to give <webview> developers the ability to block navigations.
Currently, only the WebRequest API can do that for <webview> (unless we
introduce an alternative mechanism).

[Charlie Reis, 2013-09-13 21:44:56]

I feel like it's worth confirming this is a sane behavior with someone more
familiar with the Web Request API (and that they weren't relying on the error
page for some reason), but otherwise this seems ok.  LGTM.

https://chromiumcodereview.appspot.com/23856006/diff/1/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/23856006/diff/1/content/renderer/rende...
content/renderer/render_view_impl.cc:3672: // 'replace' load.  This is necessary
to avoid messing up session history.
On 2013/09/03 23:14:31, creis wrote:
> I'm having trouble understanding why you want to diverge from the normal
> didFailProvisionalLoad behavior.  This comment in particular makes me
concerned
> that the session history will be affected.

I suppose this comment mainly applies to the LoadNavigationErrorPage call below,
so this change is making it behave more like the ERR_ABORTED case.

[lazyboy, 2013-09-13 22:11:18]

On 2013/09/13 21:44:56, creis wrote:
> I feel like it's worth confirming this is a sane behavior with someone more
> familiar with the Web Request API (and that they weren't relying on the error
> page for some reason), but otherwise this seems ok.  LGTM.
+mpcomplete for commenting on this.
Matt, can you take a look at the Test= section of the CL description? The intent
of this CL is to not show BLOCKED_BY_CLIENT error page inside <webview>, we
already throw a 'loadabort' event in this case on the <webview> element.
Thanks.
> 
>
https://chromiumcodereview.appspot.com/23856006/diff/1/content/renderer/rende...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/23856006/diff/1/content/renderer/rende...
> content/renderer/render_view_impl.cc:3672: // 'replace' load.  This is
necessary
> to avoid messing up session history.
> On 2013/09/03 23:14:31, creis wrote:
> > I'm having trouble understanding why you want to diverge from the normal
> > didFailProvisionalLoad behavior.  This comment in particular makes me
> concerned
> > that the session history will be affected.
> 
> I suppose this comment mainly applies to the LoadNavigationErrorPage call
below,
> so this change is making it behave more like the ERR_ABORTED case.

[lazyboy, 2013-09-13 22:50:07]

On 2013/09/13 22:11:18, lazyboy wrote:
> On 2013/09/13 21:44:56, creis wrote:
> > I feel like it's worth confirming this is a sane behavior with someone more
> > familiar with the Web Request API (and that they weren't relying on the
error
> > page for some reason), but otherwise this seems ok.  LGTM.
> +mpcomplete for commenting on this.
> Matt, can you take a look at the Test= section of the CL description? The
intent
> of this CL is to not show BLOCKED_BY_CLIENT error page inside <webview>, we
> already throw a 'loadabort' event in this case on the <webview> element.
> Thanks.
Actually adding Matt this time.
> > 
> >
>
https://chromiumcodereview.appspot.com/23856006/diff/1/content/renderer/rende...
> > File content/renderer/render_view_impl.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/23856006/diff/1/content/renderer/rende...
> > content/renderer/render_view_impl.cc:3672: // 'replace' load.  This is
> necessary
> > to avoid messing up session history.
> > On 2013/09/03 23:14:31, creis wrote:
> > > I'm having trouble understanding why you want to diverge from the normal
> > > didFailProvisionalLoad behavior.  This comment in particular makes me
> > concerned
> > > that the session history will be affected.
> > 
> > I suppose this comment mainly applies to the LoadNavigationErrorPage call
> below,
> > so this change is making it behave more like the ERR_ABORTED case.

[Matt Perry, 2013-09-16 18:53:01]

LGTM.

What do you display instead? A blank page?

[lazyboy, 2013-09-16 18:56:25]

On 2013/09/16 18:53:01, Matt Perry wrote:
> LGTM.
> 
> What do you display instead? A blank page?

We keep showing the page, i.e. do nothing.
The loadabort event gets fired on <webview> to notify the fact.

[lazyboy, 2013-09-16 23:00:20]

+cdn for Security review:
content/public/common/view_messages.h

[Cris Neckar, 2013-09-17 18:06:16]

IPC changes LGTM

[commit-bot: I haz the power, 2013-09-17 19:20:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/23856006/1

[commit-bot: I haz the power, 2013-09-18 02:11:03]

Change committed as 223769