PlzNavigate: Do not send synchronous navigations from the renderer to the browser.

PlzNavigate: Do not send synchronous navigations from the renderer to the browser.

This exposes ShouldMakeNetworkRequest to the renderer so it does not end up sending
the navigation to the browser for synchronous navigations.
This fixes WebRtcBrowserTest.CallInsideIframe.

BUG=475027
Committed: https://crrev.com/7fbe500b24aa0271f40b9c8e48c0cce365d9f33c
Cr-Commit-Position: refs/heads/master@{#337388}

[Fabrice (no longer in Chrome), 2015-07-03 10:05:40]

fdegans@chromium.org changed reviewers:
+ carlosk@chromium.org, clamy@chromium.org, nasko@chromium.org

[Fabrice (no longer in Chrome), 2015-07-03 10:05:41]

nasko@, clamy@: PTAL
carlosk@: FYI
Not sure this is the best place to move that function. Do we have some kind of
"utility" class with static methods? Suggestions welcome.

[clamy, 2015-07-03 11:41:45]

Thanks! A few comments. As discussed offline, the title and description of the
issue should reflect that the patch is about not sending synchronous navigations
from the renderer to the browser. (This has the additional effect of not
executing an onLoad event for the initial navigation to about:blank in a newly
created frame).

https://codereview.chromium.org/1222203004/diff/20001/content/common/navigati...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/1222203004/diff/20001/content/common/navigati...
content/common/navigation_params.cc:14: // TODO(clamy): same document
navigations should not make network requests.
Please add a DCHECK that browser-side navigation is enabled. We don't want other
people to use it right now.

https://codereview.chromium.org/1222203004/diff/20001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1222203004/diff/20001/content/common/navigati...
content/common/navigation_params.h:48: static bool
ShouldMakeNetworkRequest(const GURL& url);
A static method inside a struct strikes me as odd. I would rather have it
outside of any class/struct (it can remain in this file), with a name such as
ShouldMakeNetworkRequestForURL.

Also the comment should precise that this should ONLY be used with PlzNavigate.

https://codereview.chromium.org/1222203004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1222203004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4332: // PlzNavigate: send the request to
the browser if needed.
Could you rephrase the comment to something like "If the navigation is not
synchronous, send it to the browser"?

[Fabrice (no longer in Chrome), 2015-07-03 14:55:36]

Thanks! Edited the title and description.

https://codereview.chromium.org/1222203004/diff/20001/content/common/navigati...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/1222203004/diff/20001/content/common/navigati...
content/common/navigation_params.cc:14: // TODO(clamy): same document
navigations should not make network requests.
On 2015/07/03 11:41:44, clamy wrote:
> Please add a DCHECK that browser-side navigation is enabled. We don't want
other
> people to use it right now.

Similar checks in the code are CHECKS and not DCHECKS so I used that instead.
Done.

https://codereview.chromium.org/1222203004/diff/20001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1222203004/diff/20001/content/common/navigati...
content/common/navigation_params.h:48: static bool
ShouldMakeNetworkRequest(const GURL& url);
On 2015/07/03 11:41:44, clamy wrote:
> A static method inside a struct strikes me as odd. I would rather have it
> outside of any class/struct (it can remain in this file), with a name such as
> ShouldMakeNetworkRequestForURL.
> 
> Also the comment should precise that this should ONLY be used with
PlzNavigate.

Done.

https://codereview.chromium.org/1222203004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1222203004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4332: // PlzNavigate: send the request to
the browser if needed.
On 2015/07/03 11:41:44, clamy wrote:
> Could you rephrase the comment to something like "If the navigation is not
> synchronous, send it to the browser"?

Done.

[clamy, 2015-07-03 15:02:23]

Thanks! Lgtm with nits. Also I don't see an updated description :).

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
content/common/navigation_params.cc:16:
CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
nit: put the check above the comments, as they relate directly to the return
line.

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
content/common/navigation_params.h:26: // Helper function to determine if the
navigation request to |url| should be
nit: s/navigation request/navigation
nit: s/should be sent/should make a request

[carlosk, 2015-07-03 15:39:04]

One nit.

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
content/common/navigation_params.h:25: // This is not used outside of the
PlzNavigate project.
nit: This is generally indicated by having:
// PlzNavigate
As the first line of your method's comment documentation (see Navigator.h for
instance).

[nasko, 2015-07-06 11:06:17]

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
content/common/navigation_params.cc:14: // Data and Javascript urls should not
make network requests.
If the comment is aiming to list the cases for no network requests, then it is
missing about:blank URLs.

I think this should become more generic function, as we don't want to send
synchronous navigations to the browser process either. For example,
pushState/popState are not something we will generate an IPC for, just satisfy
the synchronous navigation in the renderer and only send a
DidCommitProvisionalLoad IPC.

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
content/common/navigation_params.h:29: bool ShouldMakeNetworkRequestForURL(const
GURL& url);
When this method is used on the renderer side, it isn't really deciding whether
to send a network request or not. It is deciding whether to even make the
request to the browser process.

https://codereview.chromium.org/1222203004/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1222203004/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:4332: // PlzNavigate: if the navigation is
not synchronous, send it to the browser.
The comment talks about synchronous navigations, yet the name is about network
requests. I think we should try to come up with a bit better naming for this.

[Fabrice (no longer in Chrome), 2015-07-06 12:45:09]

Thanks for the comments!
I rephrased the comments and actually pressed the button to update the commit
message.

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
content/common/navigation_params.cc:14: // Data and Javascript urls should not
make network requests.
On 2015/07/06 11:06:16, nasko (paris) wrote:
> If the comment is aiming to list the cases for no network requests, then it is
> missing about:blank URLs.
> 
> I think this should become more generic function, as we don't want to send
> synchronous navigations to the browser process either. For example,
> pushState/popState are not something we will generate an IPC for, just satisfy
> the synchronous navigation in the renderer and only send a
> DidCommitProvisionalLoad IPC.

I modified the comment to clarify.

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
content/common/navigation_params.cc:16:
CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/07/03 15:02:23, clamy wrote:
> nit: put the check above the comments, as they relate directly to the return
> line.

Done.

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
content/common/navigation_params.h:25: // This is not used outside of the
PlzNavigate project.
On 2015/07/03 15:39:03, carlosk wrote:
> nit: This is generally indicated by having:
> // PlzNavigate
> As the first line of your method's comment documentation (see Navigator.h for
> instance).

Done.

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
content/common/navigation_params.h:26: // Helper function to determine if the
navigation request to |url| should be
On 2015/07/03 15:02:23, clamy wrote:
> nit: s/navigation request/navigation
> nit: s/should be sent/should make a request

Done.

https://codereview.chromium.org/1222203004/diff/40001/content/common/navigati...
content/common/navigation_params.h:29: bool ShouldMakeNetworkRequestForURL(const
GURL& url);
On 2015/07/06 11:06:16, nasko (paris) wrote:
> When this method is used on the renderer side, it isn't really deciding
whether
> to send a network request or not. It is deciding whether to even make the
> request to the browser process.

Keeping the same name, per our offline chat. I clarified the comments.

[Fabrice (no longer in Chrome), 2015-07-06 12:45:41]

(missed that one, sorry)

https://codereview.chromium.org/1222203004/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1222203004/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:4332: // PlzNavigate: if the navigation is
not synchronous, send it to the browser.
On 2015/07/06 11:06:16, nasko (paris) wrote:
> The comment talks about synchronous navigations, yet the name is about network
> requests. I think we should try to come up with a bit better naming for this.

Updated this comment too, WDYT?

[nasko, 2015-07-06 12:58:16]

LGTM

[Fabrice (no longer in Chrome), 2015-07-06 13:00:59]

The CQ bit was checked by fdegans@chromium.org

[Fabrice (no longer in Chrome), 2015-07-06 13:01:00]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/1222203004/#ps60001
(title: "Comment refactoring")

[commit-bot: I haz the power, 2015-07-06 13:01:23]

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

[commit-bot: I haz the power, 2015-07-06 13:59:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-06 14:01:30]

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