Update InterceptingResourceHandler tests to use MockResourceLoader.

Update InterceptingResourceHandler tests to use MockResourceLoader.

This will make it easier to land an upcoming ResourceHandler refactor,
and also gives us some more ASSERTs shared with other tests.

Also prevent next ResourceHandler in the chain from being called
re-entrantly, and do a couple other test cleanups (Removing duplicated
code, particularly), and move a lot of redundant initialization into
the test fixture.

BUG=659317
Review-Url: https://codereview.chromium.org/2623383004
Cr-Commit-Position: refs/heads/master@{#444429}
Committed: https://chromium.googlesource.com/chromium/src/+/b38c6a55bbbaa4d7fa714f1ceb8cb0099c0d3ae2

[mmenke, 2017-01-12 21:17:47]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 21:18:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-01-12 22:02:50]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 22:03:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 22:27:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 22:27:56]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mmenke, 2017-01-12 22:34:13]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 22:35:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 23:41:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 23:41:33]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-13 16:18:32]

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

[mmenke, 2017-01-13 16:18:34]

Sorry for this CL - review of it seems like it will be more than a little on the
mind numbing side.  On the plus side, the test file is 335 lines shorter,
without removing any tests(!), though I did remove a couple of the sillier
EXPECTs.

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
File content/browser/loader/intercepting_resource_handler_unittest.cc (left):

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:77:
PREVIEWS_OFF);  // previews_state
All calls to this method through this entire file are identical.

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:98:
EXPECT_NE(kData, std::string(old_test_handler->buffer()->data()));
Some of these expects seem a bit silly, and make even less sense in the new code
than in the old code.

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
File content/browser/loader/intercepting_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:102: const
std::string kData = "The data";
I've left these declarations where they are, instead of moving them to just
before first use (Or to the top of the methods) to make review marginally
simpler.

[mmenke, 2017-01-13 16:19:31]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 16:19:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-01-13 16:51:56]

Description was changed from

==========
Update InterceptingResourceHandler tests to use MockResourceLoader.

This will make it easier to land an upcoming ResourceHandler refactor,
and also gives us some more ASSERTs shared with other tests.

Also prevent next ResourceHandler in the chain from being called
re-entrantly, and do a couple other test cleanups (Removing duplicated
code, particularly).

BUG=659317
==========

to

==========
Update InterceptingResourceHandler tests to use MockResourceLoader.

This will make it easier to land an upcoming ResourceHandler refactor,
and also gives us some more ASSERTs shared with other tests.

Also prevent next ResourceHandler in the chain from being called
re-entrantly, and do a couple other test cleanups (Removing duplicated
code, particularly), and move a lot of redundant initialization into
the test fixture.

BUG=659317
==========

[commit-bot: I haz the power, 2017-01-13 17:16:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 17:16:37]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-17 21:48:11]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 21:48:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 23:05:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 23:05:42]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2017-01-18 00:36:33]

LGTM; notes below are at your judgement.

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
File content/browser/loader/intercepting_resource_handler_unittest.cc (left):

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:98:
EXPECT_NE(kData, std::string(old_test_handler->buffer()->data()));
On 2017/01/13 16:18:33, mmenke wrote:
> Some of these expects seem a bit silly, and make even less sense in the new
code
> than in the old code.

I take it by implication you removed the ones that seemed silly to you?

(Also, just so I'm not missing anything: the string "The data" was never shown
to the RH stack before this point in the test, so there's no reason other than
very lucky random uninitialize memory why the above "NE" would ever be false?)

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
File content/browser/loader/intercepting_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:76:
intercepting_handler_.reset(new InterceptingResourceHandler(
Isn't std::make_unique the new hotness?

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:149: // The
response is received. The new ResourceHandler should be used handle
nit: "*to* handle"?

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:359:
intercepting_handler_->UseNewHandler(std::move(new_handler), std::string());
Thought (i.e. not even a suggestion): This code seems like it happens often
enough that it could be turned into a helper routine.

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:421:
EXPECT_EQ("The long l", old_handler_body_);
nit, suggestion: Compute "The long l" as per the comment rather than hardcoding
it?

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:443:
EXPECT_EQ(net::URLRequestStatus::SUCCESS, old_handler_status_.status());
suggestion: Find a way to assert that the old hander's been torn down?

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:482:
EXPECT_EQ(net::URLRequestStatus::IO_PENDING, old_handler_status_.status());
Thought: Not sure this EXPECT makes a lot of sense except for documentation. 
NE(CANCELED) might be a more appropriate test.

[mmenke, 2017-01-18 16:27:31]

Thanks for the review!

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
File content/browser/loader/intercepting_resource_handler_unittest.cc (left):

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:98:
EXPECT_NE(kData, std::string(old_test_handler->buffer()->data()));
On 2017/01/18 00:36:33, Randy Smith - Not in Mondays wrote:
> On 2017/01/13 16:18:33, mmenke wrote:
> > Some of these expects seem a bit silly, and make even less sense in the new
> code
> > than in the old code.
> 
> I take it by implication you removed the ones that seemed silly to you?

Yes, these seemed silly, and don't really have an analog when using the
MockResourceLoader (Though suppose we could put similar tests in there instead).

> (Also, just so I'm not missing anything: the string "The data" was never shown
> to the RH stack before this point in the test, so there's no reason other than
> very lucky random uninitialize memory why the above "NE" would ever be false?)

Correct.

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
File content/browser/loader/intercepting_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:76:
intercepting_handler_.reset(new InterceptingResourceHandler(
On 2017/01/18 00:36:33, Randy Smith - Not in Mondays wrote:
> Isn't std::make_unique the new hotness?

I guess?  Switched to base::MakeUnique, though it's more characters.  :(

I tend to prefer new because I hate the associated pattern:
auto foo = base::MakeUnique<Foo>(...), which I feel obscures the type of foo.

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:149: // The
response is received. The new ResourceHandler should be used handle
On 2017/01/18 00:36:33, Randy Smith - Not in Mondays wrote:
> nit: "*to* handle"?

Done.

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:359:
intercepting_handler_->UseNewHandler(std::move(new_handler), std::string());
On 2017/01/18 00:36:33, Randy Smith - Not in Mondays wrote:
> Thought (i.e. not even a suggestion): This code seems like it happens often
> enough that it could be turned into a helper routine.

The problem is that in every test we set different properties on the new
handler.  We could keep an unowned pointer to it and set stuff on it after
passing it along - think that may actually be a little prettier than using
new_handler_status/new_handler_body if we removed them, too...  But think that's
a bit too much for this CL.  Also not sure if it's worth doing.

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:421:
EXPECT_EQ("The long l", old_handler_body_);
On 2017/01/18 00:36:33, Randy Smith - Not in Mondays wrote:
> nit, suggestion: Compute "The long l" as per the comment rather than
hardcoding
> it?

Done.  Also took the opportunity to move all consts to the start of each test,
which I think makes the tests easier to figure out.

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:443:
EXPECT_EQ(net::URLRequestStatus::SUCCESS, old_handler_status_.status());
On 2017/01/18 00:36:33, Randy Smith - Not in Mondays wrote:
> suggestion: Find a way to assert that the old hander's been torn down?

Done.  Made TestResourceHandler expose a WeakPtr, and use that instead of
unowned pointers everywhere in this test.  Also removed the old_handler status
checks below this line, since now we know it's been deleted.

https://codereview.chromium.org/2623383004/diff/80001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler_unittest.cc:482:
EXPECT_EQ(net::URLRequestStatus::IO_PENDING, old_handler_status_.status());
On 2017/01/18 00:36:33, Randy Smith - Not in Mondays wrote:
> Thought: Not sure this EXPECT makes a lot of sense except for documentation. 
> NE(CANCELED) might be a more appropriate test.

Done.  You're right, it doesn't, just blindly kept it from the old test.  Added
error code checks below all net::URLRequestStatus::CANCELED checks.

[mmenke, 2017-01-18 16:27:39]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-01-18 16:27:40]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/2623383004/#ps140001
(title: "Response to comments")

[commit-bot: I haz the power, 2017-01-18 16:28:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 18:06:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 18:06:15]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mmenke, 2017-01-18 18:08:12]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-01-18 18:08:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 18:56:24]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484762892082730,
"parent_rev": "acc729512852c66d25b86ebf3533c1531d331983", "commit_rev":
"b38c6a55bbbaa4d7fa714f1ceb8cb0099c0d3ae2"}

[commit-bot: I haz the power, 2017-01-18 18:56:54]

Description was changed from

==========
Update InterceptingResourceHandler tests to use MockResourceLoader.

This will make it easier to land an upcoming ResourceHandler refactor,
and also gives us some more ASSERTs shared with other tests.

Also prevent next ResourceHandler in the chain from being called
re-entrantly, and do a couple other test cleanups (Removing duplicated
code, particularly), and move a lot of redundant initialization into
the test fixture.

BUG=659317
==========

to

==========
Update InterceptingResourceHandler tests to use MockResourceLoader.

This will make it easier to land an upcoming ResourceHandler refactor,
and also gives us some more ASSERTs shared with other tests.

Also prevent next ResourceHandler in the chain from being called
re-entrantly, and do a couple other test cleanups (Removing duplicated
code, particularly), and move a lot of redundant initialization into
the test fixture.

BUG=659317

Review-Url: https://codereview.chromium.org/2623383004
Cr-Commit-Position: refs/heads/master@{#444429}
Committed:
https://chromium.googlesource.com/chromium/src/+/b38c6a55bbbaa4d7fa714f1ceb8c...
==========

[commit-bot: I haz the power, 2017-01-18 18:56:55]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/b38c6a55bbbaa4d7fa714f1ceb8c...