PlzNavigate: make RenderViewtest helper functions work

PlzNavigate: make RenderViewtest helper functions work

This CL makes it so that the helper functions used in RenderViewImpltest work
with browser-side navigation enabled. This makes 20 browser tests pass.

BUG=475027
Committed: https://crrev.com/5c7ede5ffccfba15c2d0752d03bc6979f1d81bae
Cr-Commit-Position: refs/heads/master@{#324414}

[clamy, 2015-04-08 14:19:10]

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

[clamy, 2015-04-08 14:19:30]

@creis, carlosk: PTAL
@fdegans: FYI

https://codereview.chromium.org/1064413003/diff/1/content/child/web_url_loade...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1064413003/diff/1/content/child/web_url_loade...
content/child/web_url_loader_impl.cc:468:
DCHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
Several tests load HTML through a data url, requiring to move this CHECK after
deciding that we cannot handle the data url locally. Since its goal was to make
sure that we would not try to make normal resource requests to the browser for
frames if PlzNavigate, I think it is fine to move it there.

https://codereview.chromium.org/1064413003/diff/1/content/public/test/render_...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/1064413003/diff/1/content/public/test/render_...
content/public/test/render_view_test.cc:129:
request.setCheckForBrowserSideNavigation(false);
This ensure we will not attempt to go to the browser for this request (not
needed since we're getting the data through a data url).

https://codereview.chromium.org/1064413003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1064413003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1067:
CHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
I added the CHECK to catch tests that manually call OnNavigate, and where we
don't properly exercise the PlzNavigate path.

https://codereview.chromium.org/1064413003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1064413003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.h:567: friend class RenderViewImplTest;
This is to not make the OnCommitNavigation handler public.

[carlosk, 2015-04-08 15:16:42]

Looking good.

https://chromiumcodereview.appspot.com/1064413003/diff/1/content/child/web_ur...
File content/child/web_url_loader_impl.cc (right):

https://chromiumcodereview.appspot.com/1064413003/diff/1/content/child/web_ur...
content/child/web_url_loader_impl.cc:471: request.frameType() ==
WebURLRequest::FrameTypeNone);
For clarity this should be updated to use (the recently discovered)
DCHECK_IMPLIES:

DCHECK_IMPLIES(base::CommandLine::ForCurrentProcess()->HasSwitch( 
                   switches::kEnableBrowserSideNavigation),
               stream_override_.get() || 
               request.frameType() == WebURLRequest::FrameTypeNone);

https://chromiumcodereview.appspot.com/1064413003/diff/1/content/public/test/...
File content/public/test/render_view_test.cc (right):

https://chromiumcodereview.appspot.com/1064413003/diff/1/content/public/test/...
content/public/test/render_view_test.cc:129:
request.setCheckForBrowserSideNavigation(false);
On 2015/04/08 14:19:30, clamy wrote:
> This ensure we will not attempt to go to the browser for this request (not
> needed since we're getting the data through a data url).

Acknowledged.

https://chromiumcodereview.appspot.com/1064413003/diff/1/content/renderer/ren...
File content/renderer/render_frame_impl.cc (right):

https://chromiumcodereview.appspot.com/1064413003/diff/1/content/renderer/ren...
content/renderer/render_frame_impl.cc:1067:
CHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/04/08 14:19:30, clamy wrote:
> I added the CHECK to catch tests that manually call OnNavigate, and where we
> don't properly exercise the PlzNavigate path.

Good idea! Will make it way easier to find those many cases.

https://chromiumcodereview.appspot.com/1064413003/diff/1/content/renderer/ren...
File content/renderer/render_frame_impl.h (right):

https://chromiumcodereview.appspot.com/1064413003/diff/1/content/renderer/ren...
content/renderer/render_frame_impl.h:567: friend class RenderViewImplTest;
On 2015/04/08 14:19:30, clamy wrote:
> This is to not make the OnCommitNavigation handler public.

Instead of friending the test you could call OnMessageReceived, which is public,
passing a FrameMsg_CommitNavigation. I saw that being done in
RenderFrameHostManagerTest.DetachPendingChild.

[Charlie Reis, 2015-04-09 00:47:40]

LGTM with nits.  Glad to see more tests will pass!  Please enable them on the
FYI bot once this lands to help catch regressions.

https://codereview.chromium.org/1064413003/diff/1/content/child/web_url_loade...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1064413003/diff/1/content/child/web_url_loade...
content/child/web_url_loader_impl.cc:468:
DCHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/04/08 14:19:30, clamy wrote:
> Several tests load HTML through a data url, requiring to move this CHECK after
> deciding that we cannot handle the data url locally. Since its goal was to
make
> sure that we would not try to make normal resource requests to the browser for
> frames if PlzNavigate, I think it is fine to move it there.

Sure.  Please update the comment, though, since there are now some navigation
requests going through here other than those created by CommitNavigation.

https://codereview.chromium.org/1064413003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1064413003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1067:
CHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/04/08 15:16:42, carlosk wrote:
> On 2015/04/08 14:19:30, clamy wrote:
> > I added the CHECK to catch tests that manually call OnNavigate, and where we
> > don't properly exercise the PlzNavigate path.
> 
> Good idea! Will make it way easier to find those many cases.

Sounds good.  Can you make this a DCHECK instead?  That will still run on both
the debug and release bots, but it won't get compiled into release builds for
users.

[clamy, 2015-04-09 10:00:33]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-04-09 10:00:33]

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/1064413003/#ps20001
(title: "Addressed comments")

[commit-bot: I haz the power, 2015-04-09 10:01:03]

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

[commit-bot: I haz the power, 2015-04-09 11:32:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-09 11:33:25]

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

[clamy, 2015-04-09 15:55:07]

Thanks! (Just realized I forgot to send the mail regarding the comments in the
review)

https://codereview.chromium.org/1064413003/diff/1/content/child/web_url_loade...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1064413003/diff/1/content/child/web_url_loade...
content/child/web_url_loader_impl.cc:468:
DCHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/04/09 00:47:40, Charlie Reis (slow until 4-15) wrote:
> On 2015/04/08 14:19:30, clamy wrote:
> > Several tests load HTML through a data url, requiring to move this CHECK
after
> > deciding that we cannot handle the data url locally. Since its goal was to
> make
> > sure that we would not try to make normal resource requests to the browser
for
> > frames if PlzNavigate, I think it is fine to move it there.
> 
> Sure.  Please update the comment, though, since there are now some navigation
> requests going through here other than those created by CommitNavigation.

Done.

https://codereview.chromium.org/1064413003/diff/1/content/child/web_url_loade...
content/child/web_url_loader_impl.cc:471: request.frameType() ==
WebURLRequest::FrameTypeNone);
On 2015/04/08 15:16:42, carlosk wrote:
> For clarity this should be updated to use (the recently discovered)
> DCHECK_IMPLIES:
> 
> DCHECK_IMPLIES(base::CommandLine::ForCurrentProcess()->HasSwitch( 
>                    switches::kEnableBrowserSideNavigation),
>                stream_override_.get() || 
>                request.frameType() == WebURLRequest::FrameTypeNone);

Done. Clearer indeed.

https://codereview.chromium.org/1064413003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1064413003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1067:
CHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/04/09 00:47:40, Charlie Reis (slow until 4-15) wrote:
> On 2015/04/08 15:16:42, carlosk wrote:
> > On 2015/04/08 14:19:30, clamy wrote:
> > > I added the CHECK to catch tests that manually call OnNavigate, and where
we
> > > don't properly exercise the PlzNavigate path.
> > 
> > Good idea! Will make it way easier to find those many cases.
> 
> Sounds good.  Can you make this a DCHECK instead?  That will still run on both
> the debug and release bots, but it won't get compiled into release builds for
> users.

Done.

https://codereview.chromium.org/1064413003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1064413003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.h:567: friend class RenderViewImplTest;
On 2015/04/08 15:16:42, carlosk wrote:
> On 2015/04/08 14:19:30, clamy wrote:
> > This is to not make the OnCommitNavigation handler public.
> 
> Instead of friending the test you could call OnMessageReceived, which is
public,
> passing a FrameMsg_CommitNavigation. I saw that being done in
> RenderFrameHostManagerTest.DetachPendingChild.

Considering Nasko's comment at line 539, I will have it as a friend here (this
will eventually allow to move OnNavigate back as a private member).

[Charlie Reis, 2015-04-09 16:04:51]

https://codereview.chromium.org/1064413003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1064413003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.h:567: friend class RenderViewImplTest;
On 2015/04/09 15:55:06, clamy wrote:
> On 2015/04/08 15:16:42, carlosk wrote:
> > On 2015/04/08 14:19:30, clamy wrote:
> > > This is to not make the OnCommitNavigation handler public.
> > 
> > Instead of friending the test you could call OnMessageReceived, which is
> public,
> > passing a FrameMsg_CommitNavigation. I saw that being done in
> > RenderFrameHostManagerTest.DetachPendingChild.
> 
> Considering Nasko's comment at line 539, I will have it as a friend here (this
> will eventually allow to move OnNavigate back as a private member).

Actually, Nasko was referring to the FRIEND_TEST_ALL_PREFIXES lines below.  I'm
not sure if those will work for your new function?

We shouldn't have both the friend class and FRIEND_TEST_ALL_PREFIXES lines.  Can
you send a followup CL to choose one or the other?