Also filter the URLs from incoming non-provisional navigation IPCs.

Also filter the URLs from incoming non-provisional navigation IPCs.

Without this, e.g. a navigation to about:srcdoc looks like a navigation to
about:blank during the provisional phase, and suddendly the internal
about:srcdoc URL is leaked to the embedder.

BUG=168611
TEST=WebContentsImplTest.FilterURLs
R=creis@chromium.org

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

[Charlie Reis, 2013-01-07 18:49:03]

Yep, looks right.  This will translate about:srcdoc to about:blank in the
DidFinishLoad / DidFailLoad cases.

Can I trouble you for a test?  We wouldn't want it to regress if the FilterURL
logic changed.

https://codereview.chromium.org/11801011/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/11801011/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:2138: const GURL&
validated_url,
Wow, why was that called validated?  It's coming straight from the renderer.  I
agree that it should go through FilterURL.

[jochen (gone - plz use gerrit), 2013-01-07 19:17:03]

On 2013/01/07 18:49:03, creis wrote:
> Yep, looks right.  This will translate about:srcdoc to about:blank in the
> DidFinishLoad / DidFailLoad cases.
> 
> Can I trouble you for a test?  We wouldn't want it to regress if the FilterURL
> logic changed.
> 
Sure, a test should never be trouble!


>
https://codereview.chromium.org/11801011/diff/1/content/browser/web_contents/...
> File content/browser/web_contents/web_contents_impl.cc (left):
> 
>
https://codereview.chromium.org/11801011/diff/1/content/browser/web_contents/...
> content/browser/web_contents/web_contents_impl.cc:2138: const GURL&
> validated_url,
> Wow, why was that called validated?  It's coming straight from the renderer. 
I
> agree that it should go through FilterURL.

I didn't check, but I might be to blame for this misnomer :(

[jochen (gone - plz use gerrit), 2013-01-07 20:41:32]

added a test

[Charlie Reis, 2013-01-07 20:58:45]

Thanks.  LGTM with some minor comments.

https://codereview.chromium.org/11801011/diff/6001/content/browser/web_conten...
File content/browser/web_contents/test_web_contents.h (right):

https://codereview.chromium.org/11801011/diff/6001/content/browser/web_conten...
content/browser/web_contents/test_web_contents.h:93: void
TestDidFinishLoad(int64 frame_id, const GURL& url, bool is_main_frame);
You can probably skip this with a FRIEND_TEST_ALL_PREFIXES line in
web_contents_impl.h, since there's several of those already.  That will let your
test call OnDidFinishLoad directly.

https://codereview.chromium.org/11801011/diff/6001/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/11801011/diff/6001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl_unittest.cc:1988: // A navigation
to about:whatever should always look like a navigation to
I got a bit confused by this comment, since you're actually navigating to
about:blank, not about:whatever.

[jochen (gone - plz use gerrit), 2013-01-07 21:11:59]

https://codereview.chromium.org/11801011/diff/6001/content/browser/web_conten...
File content/browser/web_contents/test_web_contents.h (right):

https://codereview.chromium.org/11801011/diff/6001/content/browser/web_conten...
content/browser/web_contents/test_web_contents.h:93: void
TestDidFinishLoad(int64 frame_id, const GURL& url, bool is_main_frame);
On 2013/01/07 20:58:46, creis wrote:
> You can probably skip this with a FRIEND_TEST_ALL_PREFIXES line in
> web_contents_impl.h, since there's several of those already.  That will let
your
> test call OnDidFinishLoad directly.

As discussed, I think emulating an IPC is a cleaner approach here, because
otherwise I'd need to setup the message_source_ variable which is otherwise set
by OnMessageReceived. It feels like this hardcodes unnecessarily much knowledge
about the internal implementation

https://codereview.chromium.org/11801011/diff/6001/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/11801011/diff/6001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl_unittest.cc:1988: // A navigation
to about:whatever should always look like a navigation to
On 2013/01/07 20:58:46, creis wrote:
> I got a bit confused by this comment, since you're actually navigating to
> about:blank, not about:whatever.

Clarified the comment

[Charlie Reis, 2013-01-07 21:14:33]

LGTM

[commit-bot: I haz the power, 2013-01-07 21:15:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/11801011/11001

[commit-bot: I haz the power, 2013-01-07 23:59:57]

Retried try job too often on win_aura for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2013-01-08 08:17:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/11801011/11001