Use about:blank as the failback URL if the filter denies a navigation.

Use about:blank as the failback URL if the filter denies a navigation.

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

[Chris Evans, 2012-03-21 00:02:34]

I'm actually going to wait on the trybot runs on this one, just in case any of
the unittests are relying on the old behaviour.

[Charlie Reis, 2012-03-21 00:06:36]

LGTM.  CC'ing abarth@ as an FYI.

[Chris Evans, 2012-03-21 02:54:16]

On 2012/03/21 00:06:36, creis wrote:
> LGTM.  CC'ing abarth@ as an FYI.

(one very small unit test expectation change added)

[commit-bot: I haz the power, 2012-03-21 02:54:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cevans@chromium.org/9794009/6001

[commit-bot: I haz the power, 2012-03-21 02:54:39]

Presubmit check for 9794009-6001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser

[Chris Evans, 2012-03-21 05:09:48]

On 2012/03/21 00:06:36, creis wrote:
> LGTM.  CC'ing abarth@ as an FYI.

Actually, I see a possible additional problem. Let's discuss tomorrow.

[Chris Evans, 2012-03-21 13:52:42]

On 2012/03/21 05:09:48, Chris Evans wrote:
> On 2012/03/21 00:06:36, creis wrote:
> > LGTM.  CC'ing abarth@ as an FYI.
> 
> Actually, I see a possible additional problem. Let's discuss tomorrow.

I added some notes in the bug and this CL is updated with various fixes for
omissions.

[Charlie Reis, 2012-03-21 16:23:03]

https://chromiumcodereview.appspot.com/9794009/diff/13001/content/browser/ren...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/9794009/diff/13001/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:1043: if
(!validated_params.referrer.url.is_empty())
Your comment on the bug said that we should allow empty URLs for referrer but
not for navigations.  What if we added a is_navigation boolean flag to FilterURL
rather than having to do this check everywhere?

https://chromiumcodereview.appspot.com/9794009/diff/13001/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:1458: GURL* url) {
Can we add a comment saying why an empty URL is dangerous for navigations?

[Chris Evans, 2012-03-21 16:57:41]

On 2012/03/21 16:23:03, creis wrote:
>
https://chromiumcodereview.appspot.com/9794009/diff/13001/content/browser/ren...
> File content/browser/renderer_host/render_view_host_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9794009/diff/13001/content/browser/ren...
> content/browser/renderer_host/render_view_host_impl.cc:1043: if
> (!validated_params.referrer.url.is_empty())
> Your comment on the bug said that we should allow empty URLs for referrer but
> not for navigations.  What if we added a is_navigation boolean flag to
FilterURL
> rather than having to do this check everywhere?
> 
>
https://chromiumcodereview.appspot.com/9794009/diff/13001/content/browser/ren...
> content/browser/renderer_host/render_view_host_impl.cc:1458: GURL* url) {
> Can we add a comment saying why an empty URL is dangerous for navigations?

Both done.

[jam, 2012-03-21 17:02:17]

lgtm

[Charlie Reis, 2012-03-21 17:02:45]

https://chromiumcodereview.appspot.com/9794009/diff/17002/content/browser/ren...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/9794009/diff/17002/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:1142: FilterURL(policy,
renderer_id, true, &validated_params.link_url);
Why is it ok for these to be empty?

https://chromiumcodereview.appspot.com/9794009/diff/17002/content/browser/ren...
content/browser/renderer_host/render_view_host_impl.cc:1451: bool empty_allowed,
You'll need to update the .h file, right?  Please add a comment there guiding
people whether to pass true or false.

[cevans, 2012-03-21 17:57:12]

On Wed, Mar 21, 2012 at 10:02 AM,  <creis@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/9794009/diff/17002/content/browser/ren...
> File content/browser/renderer_host/render_view_host_impl.cc (right):
>
>
https://chromiumcodereview.appspot.com/9794009/diff/17002/content/browser/ren...
> content/browser/renderer_host/render_view_host_impl.cc:1142:
> FilterURL(policy, renderer_id, true, &validated_params.link_url);
> Why is it ok for these to be empty?

They are currently mostly empty. I've gone for a conservative approach
here for now (i.e. don't rewrite to about:blank in case it breaks
things). I'm not worried about this particular case because it
involves a context menu and browser-mediated user interaction.

>
>
https://chromiumcodereview.appspot.com/9794009/diff/17002/content/browser/ren...
> content/browser/renderer_host/render_view_host_impl.cc:1451: bool
> empty_allowed,
> You'll need to update the .h file, right?  Please add a comment there
> guiding people whether to pass true or false.

Ah, already had this, it just wasn't in the change list :)

PTAL?

>
> https://chromiumcodereview.appspot.com/9794009/

[Charlie Reis, 2012-03-21 18:02:51]

LGTM

[commit-bot: I haz the power, 2012-03-21 18:13:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cevans@chromium.org/9794009/15004