PlzNavigate: make all ResourceDispatcherHostTests pass

PlzNavigate: make all ResourceDispatcherHostTests pass

This CL rewrites 4 ResourceDispatcherHostTests so that they properly test
events when browser side navigation is enabled. The way they are written right
now corresponds to the current navigation architecture, and does not correctly
simulates navigation events when PlzNavigate is enabled.

BUG=439423
Committed: https://crrev.com/9fb38cde5f792e54eee8e32625520f989f20ff2a
Cr-Commit-Position: refs/heads/master@{#386376}

[clamy, 2016-04-05 14:10:53]

Description was changed from

==========
PlzNavigate: make all ResourceDispatcherHostTests pass

This CL rewrites 4 ResourceDispatcherHostTests so that they properly test
events when browser side navigation is enabled. The way they are written right
now corresponds to the current navigation architecture, and does not correctly
simulates navigation events when PlzNavigate is enabled.

BUG=439423
==========

to

==========
PlzNavigate: make all ResourceDispatcherHostTests pass

This CL rewrites 4 ResourceDispatcherHostTests so that they properly test
events when browser side navigation is enabled. The way they are written right
now corresponds to the current navigation architecture, and does not correctly
simulates navigation events when PlzNavigate is enabled.

BUG=439423
==========

[clamy, 2016-04-05 14:10:53]

clamy@chromium.org changed reviewers:
+ mmenke@chromium.org

[clamy, 2016-04-05 14:11:07]

@mmenke: PTAL

[carlosk, 2016-04-05 15:28:45]

carlosk@chromium.org changed reviewers:
+ carlosk@chromium.org

[carlosk, 2016-04-05 15:28:46]

Thanks. Just a couple of minor things.

https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_unittest.cc:1148: // The
navigation should fail with the error code of ERR_INVALID_RESPONSE.
As the expected error code is an input parameter this comment seems incorrect.

https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_unittest.cc:2537: if
(IsBrowserSideNavigationEnabled())
nit: use the SUCCEED() << "Message" operator (which is still a nop BTW).

[mmenke, 2016-04-05 19:40:04]

Disclaimer:  I'm unfamiliar with navigation_url_loader_unittest, so this may
take me a while to do through.  Hope to do a first pass tomorrow, but may not
have time, depending on review load.

On 2016/04/05 15:28:46, carlosk wrote:
> Thanks. Just a couple of minor things.
> 
>
https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
> File content/browser/loader/resource_dispatcher_host_unittest.cc (right):
> 
>
https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
> content/browser/loader/resource_dispatcher_host_unittest.cc:1148: // The
> navigation should fail with the error code of ERR_INVALID_RESPONSE.
> As the expected error code is an input parameter this comment seems incorrect.
> 
>
https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
> content/browser/loader/resource_dispatcher_host_unittest.cc:2537: if
> (IsBrowserSideNavigationEnabled())
> nit: use the SUCCEED() << "Message" operator (which is still a nop BTW).

[mmenke, 2016-04-07 21:42:59]

Sorry for slowness - been deluged with reviews and docs to comment on this week.
 First thing on my schedule tomorrow.

[mmenke, 2016-04-08 15:19:13]

loader/ LGTM.  Do not have enough knowledge (Or time) to review the
test_navigation_url_loader_delegate for correctness.

https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_unittest.cc:1129: // Creates and
drive a main resource request until completion. Then asserts
nit:  "Create and drive" or "creates and drives".  Whichever you use should
agree in tense with the next sentence as well.

https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_unittest.cc:1139: false,
REQUEST_CONTEXT_TYPE_LOCATION);
I assume "REQUEST_CONTEXT_TYPE_LOCATION" is the general case?  None of the
RequestContextTypes are documented.  :(

https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_unittest.cc:2524: const GURL
forbidden_download_url = GURL("http:bla");
consts are pretty rare, unless you use kConstNamingStyle.  I'd just make thes
enon-const, to match the style of the std::strings above, but I'm happy, either
way.

https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_unittest.cc:2618:
EXPECT_EQ(delegate.net_error(), net::ERR_ABORTED);
Think this requires a comment.  I assume this is because the navigation should
be aborted once the request is identified as a download (but the request itself
should continue).

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
File content/test/test_navigation_url_loader_delegate.cc (right):

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
content/test/test_navigation_url_loader_delegate.cc:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
--(c)

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
File content/test/test_navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
content/test/test_navigation_url_loader_delegate.h:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
new files shouldn't use the (c)

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
content/test/test_navigation_url_loader_delegate.h:10: #include
"base/run_loop.h"
Forward declare this, and move the include into the cc file?

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
content/test/test_navigation_url_loader_delegate.h:19: // PlzNavigate
nit:  Add blank line before comment.

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
content/test/test_navigation_url_loader_delegate.h:36: // Waits for various
navigation events.
Think it's important to note that the event we're waiting on must not have
happened yet (RunLoop normally allows that, but since we only create it when
these methods are called...)

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
content/test/test_navigation_url_loader_delegate.h:57: scoped_ptr<StreamHandle>
body_;
"Streams" make me sad.  None of the classes in content/public actually what a
stream actually is.  Anyhow, going to go ahead and not spend the time to
understand all this, just going to review loader/ based on assuming this class
does what I hope it does.

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
content/test/test_navigation_url_loader_delegate.h:64: scoped_ptr<base::RunLoop>
request_started_;
DISALLOW_COPY_AND_ASSIGN, include base/macros.h

[clamy, 2016-04-08 16:02:16]

clamy@chromium.org changed reviewers:
+ nasko@chromium.org

[clamy, 2016-04-08 16:03:18]

Thanks!
@nasko: PTAL at the changes in content/test. I'm pulling code out of
navigation_url_loader_unittests.cc to use in other unit tests.

https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_unittest.cc:1129: // Creates and
drive a main resource request until completion. Then asserts
On 2016/04/08 15:19:12, mmenke wrote:
> nit:  "Create and drive" or "creates and drives".  Whichever you use should
> agree in tense with the next sentence as well.

Done.

https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_unittest.cc:1139: false,
REQUEST_CONTEXT_TYPE_LOCATION);
On 2016/04/08 15:19:12, mmenke wrote:
> I assume "REQUEST_CONTEXT_TYPE_LOCATION" is the general case?  None of the
> RequestContextTypes are documented.  :(

I think so. It's the clicking on a link case.

https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_unittest.cc:1148: // The
navigation should fail with the error code of ERR_INVALID_RESPONSE.
On 2016/04/05 15:28:46, carlosk wrote:
> As the expected error code is an input parameter this comment seems incorrect.

Done. Indeed.

https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_unittest.cc:2524: const GURL
forbidden_download_url = GURL("http:bla");
On 2016/04/08 15:19:12, mmenke wrote:
> consts are pretty rare, unless you use kConstNamingStyle.  I'd just make thes
> enon-const, to match the style of the std::strings above, but I'm happy,
either
> way.

Done.

https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_unittest.cc:2537: if
(IsBrowserSideNavigationEnabled())
On 2016/04/05 15:28:46, carlosk wrote:
> nit: use the SUCCEED() << "Message" operator (which is still a nop BTW).

Done.

https://codereview.chromium.org/1863573003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_unittest.cc:2618:
EXPECT_EQ(delegate.net_error(), net::ERR_ABORTED);
On 2016/04/08 15:19:12, mmenke wrote:
> Think this requires a comment.  I assume this is because the navigation should
> be aborted once the request is identified as a download (but the request
itself
> should continue).

Done.

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
File content/test/test_navigation_url_loader_delegate.cc (right):

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
content/test/test_navigation_url_loader_delegate.cc:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
On 2016/04/08 15:19:12, mmenke wrote:
> --(c)

Done.

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
File content/test/test_navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
content/test/test_navigation_url_loader_delegate.h:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
On 2016/04/08 15:19:13, mmenke wrote:
> new files shouldn't use the (c)

Done.

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
content/test/test_navigation_url_loader_delegate.h:10: #include
"base/run_loop.h"
On 2016/04/08 15:19:13, mmenke wrote:
> Forward declare this, and move the include into the cc file?

Done.

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
content/test/test_navigation_url_loader_delegate.h:19: // PlzNavigate
On 2016/04/08 15:19:12, mmenke wrote:
> nit:  Add blank line before comment.

Done.

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
content/test/test_navigation_url_loader_delegate.h:36: // Waits for various
navigation events.
On 2016/04/08 15:19:13, mmenke wrote:
> Think it's important to note that the event we're waiting on must not have
> happened yet (RunLoop normally allows that, but since we only create it when
> these methods are called...)

Done.

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
content/test/test_navigation_url_loader_delegate.h:57: scoped_ptr<StreamHandle>
body_;
On 2016/04/08 15:19:13, mmenke wrote:
> "Streams" make me sad.  None of the classes in content/public actually what a
> stream actually is.  Anyhow, going to go ahead and not spend the time to
> understand all this, just going to review loader/ based on assuming this class
> does what I hope it does.

Acknowledged.

https://codereview.chromium.org/1863573003/diff/1/content/test/test_navigatio...
content/test/test_navigation_url_loader_delegate.h:64: scoped_ptr<base::RunLoop>
request_started_;
On 2016/04/08 15:19:13, mmenke wrote:
> DISALLOW_COPY_AND_ASSIGN, include base/macros.h

Done.

[nasko, 2016-04-08 17:13:05]

content/test/test_navigation_url_loader_delegate* LGTM with a couple of nits.

https://codereview.chromium.org/1863573003/diff/40001/content/test/test_navig...
File content/test/test_navigation_url_loader_delegate.cc (right):

https://codereview.chromium.org/1863573003/diff/40001/content/test/test_navig...
content/test/test_navigation_url_loader_delegate.cc:68: if (request_failed_)
nit: Is this optional now? It wasn't before the move.

https://codereview.chromium.org/1863573003/diff/40001/content/test/test_navig...
File content/test/test_navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/1863573003/diff/40001/content/test/test_navig...
content/test/test_navigation_url_loader_delegate.h:42: // Note: if the event
already happened, this will fail.
nit: "this will fail" < what is "this"? How would we know "this" failed?

[clamy, 2016-04-11 11:47:38]

Thanks!

https://codereview.chromium.org/1863573003/diff/40001/content/test/test_navig...
File content/test/test_navigation_url_loader_delegate.cc (right):

https://codereview.chromium.org/1863573003/diff/40001/content/test/test_navig...
content/test/test_navigation_url_loader_delegate.cc:68: if (request_failed_)
On 2016/04/08 17:13:05, nasko (very slow Apr 8-18) wrote:
> nit: Is this optional now? It wasn't before the move.

Yes. Because one of the ResourceDispatcherHost methods does not use the message
loops in this object but external ones, we can get to OnRequestFailed without
having created the request_failed_ message loop.

https://codereview.chromium.org/1863573003/diff/40001/content/test/test_navig...
File content/test/test_navigation_url_loader_delegate.h (right):

https://codereview.chromium.org/1863573003/diff/40001/content/test/test_navig...
content/test/test_navigation_url_loader_delegate.h:42: // Note: if the event
already happened, this will fail.
On 2016/04/08 17:13:05, nasko (very slow Apr 8-18) wrote:
> nit: "this will fail" < what is "this"? How would we know "this" failed?

The functions will hang. Corrected the comment.

[clamy, 2016-04-11 11:47:45]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-04-11 11:47:45]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1863573003/#ps60001
(title: "Rebase + addressed nit")

[commit-bot: I haz the power, 2016-04-11 11:47:57]

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

[commit-bot: I haz the power, 2016-04-11 12:57:56]

Description was changed from

==========
PlzNavigate: make all ResourceDispatcherHostTests pass

This CL rewrites 4 ResourceDispatcherHostTests so that they properly test
events when browser side navigation is enabled. The way they are written right
now corresponds to the current navigation architecture, and does not correctly
simulates navigation events when PlzNavigate is enabled.

BUG=439423
==========

to

==========
PlzNavigate: make all ResourceDispatcherHostTests pass

This CL rewrites 4 ResourceDispatcherHostTests so that they properly test
events when browser side navigation is enabled. The way they are written right
now corresponds to the current navigation architecture, and does not correctly
simulates navigation events when PlzNavigate is enabled.

BUG=439423
==========

[commit-bot: I haz the power, 2016-04-11 12:57:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-11 13:00:44]

Description was changed from

==========
PlzNavigate: make all ResourceDispatcherHostTests pass

This CL rewrites 4 ResourceDispatcherHostTests so that they properly test
events when browser side navigation is enabled. The way they are written right
now corresponds to the current navigation architecture, and does not correctly
simulates navigation events when PlzNavigate is enabled.

BUG=439423
==========

to

==========
PlzNavigate: make all ResourceDispatcherHostTests pass

This CL rewrites 4 ResourceDispatcherHostTests so that they properly test
events when browser side navigation is enabled. The way they are written right
now corresponds to the current navigation architecture, and does not correctly
simulates navigation events when PlzNavigate is enabled.

BUG=439423

Committed: https://crrev.com/9fb38cde5f792e54eee8e32625520f989f20ff2a
Cr-Commit-Position: refs/heads/master@{#386376}
==========

[commit-bot: I haz the power, 2016-04-11 13:00:45]

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