Merge TestResourceHandlers used by two different test fixtures.

Merge TestResourceHandlers used by two different test fixtures.

The MimeSniffingResourceHandler and InterceptingResourceHandler unit
tests use two different TestResourceHandlers with very similar feature
sets. This CL merges them into single class that supports all the
features both text fixtures need.

BUG=658760
Committed: https://crrev.com/2b6880fff5eb65c0d7f12fa3b755aaac22bf13a9
Cr-Commit-Position: refs/heads/master@{#427981}

[mmenke, 2016-10-24 16:32:36]

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

[commit-bot: I haz the power, 2016-10-24 16:32:56]

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, 2016-10-24 16:43:44]

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

[commit-bot: I haz the power, 2016-10-24 16:43:45]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[mmenke, 2016-10-24 19:06:49]

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

[commit-bot: I haz the power, 2016-10-24 19:07:36]

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, 2016-10-24 20:41:07]

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

[commit-bot: I haz the power, 2016-10-24 20:41:08]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-10-25 14:55:53]

Patchset #3 (id:40001) has been deleted

[mmenke, 2016-10-25 14:59:06]

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

[mmenke, 2016-10-25 14:59:06]

https://codereview.chromium.org/2441293003/diff/20001/content/browser/loader/...
File content/browser/loader/test_resource_handler.h (right):

https://codereview.chromium.org/2441293003/diff/20001/content/browser/loader/...
content/browser/loader/test_resource_handler.h:32: class TestResourceHandler :
public ResourceHandler {
Note that this is just the TestResourceHandler from
intercepting_resource_handler_unittest.cc, with a couple changes (ints instead
of bools, request_state/body are optional, some getters added, on_read calls
counted, may be leaving out one or two).

I'd suggest just comparing this with the old one side-by-side.  Reducing the
similarity threshold myself doesn't work, unfortunately, it ends up with the
MojoAsyncResourceHandler header as being more similar.

[Charlie Harrison, 2016-10-26 14:05:19]

Great cleanup! The code is so much more readable without constructors like new
Foo(false, true, true, false, false) :P

LGTM w/ minor nits.

https://codereview.chromium.org/2441293003/diff/20001/content/browser/loader/...
File content/browser/loader/test_resource_handler.cc (right):

https://codereview.chromium.org/2441293003/diff/20001/content/browser/loader/...
content/browser/loader/test_resource_handler.cc:30: NOTREACHED();
Can you NOTREACHED() << "Redirects are not supported by the
TestResourceHandler"?

Or, NOTIMPLEMENTED(), which I forget what it does.

https://codereview.chromium.org/2441293003/diff/20001/content/browser/loader/...
content/browser/loader/test_resource_handler.cc:48: bool
TestResourceHandler::OnWillStart(const GURL& url, bool* defer) {
include gurl.h

https://codereview.chromium.org/2441293003/diff/20001/content/browser/loader/...
content/browser/loader/test_resource_handler.cc:102: NOTREACHED();
ditto

https://codereview.chromium.org/2441293003/diff/20001/content/browser/loader/...
File content/browser/loader/test_resource_handler.h (right):

https://codereview.chromium.org/2441293003/diff/20001/content/browser/loader/...
content/browser/loader/test_resource_handler.h:15: class GURL;
ResourceHandler forward declares this already, so you can probably omit this as
a subclass.

[mmenke, 2016-10-26 15:27:30]

Going to go ahead and CQ, but if disagree with my responses, I'm happy to de-CQ,
discuss further, or handle in a followup CL.

https://codereview.chromium.org/2441293003/diff/20001/content/browser/loader/...
File content/browser/loader/test_resource_handler.cc (right):

https://codereview.chromium.org/2441293003/diff/20001/content/browser/loader/...
content/browser/loader/test_resource_handler.cc:30: NOTREACHED();
On 2016/10/26 14:05:19, Charlie Harrison wrote:
> Can you NOTREACHED() << "Redirects are not supported by the
> TestResourceHandler"?
> 
> Or, NOTIMPLEMENTED(), which I forget what it does.

Added the text, kept NOTREACHED().  Experimenting locally, NOTIMPLEMENTED() just
prints out "NOTIMPLMENTED" and a line number, while NOTREACHED() acts like a
failed DCHECK.

https://codereview.chromium.org/2441293003/diff/20001/content/browser/loader/...
content/browser/loader/test_resource_handler.cc:48: bool
TestResourceHandler::OnWillStart(const GURL& url, bool* defer) {
On 2016/10/26 14:05:19, Charlie Harrison wrote:
> include gurl.h 

I don't think it's needed - we never dereference it / copy it / delete it / etc.
 This is basically equivalent to taking a pointer we never use.

https://codereview.chromium.org/2441293003/diff/20001/content/browser/loader/...
content/browser/loader/test_resource_handler.cc:102: NOTREACHED();
On 2016/10/26 14:05:19, Charlie Harrison wrote:
> ditto

Done.

https://codereview.chromium.org/2441293003/diff/20001/content/browser/loader/...
File content/browser/loader/test_resource_handler.h (right):

https://codereview.chromium.org/2441293003/diff/20001/content/browser/loader/...
content/browser/loader/test_resource_handler.h:15: class GURL;
On 2016/10/26 14:05:19, Charlie Harrison wrote:
> ResourceHandler forward declares this already, so you can probably omit this
as
> a subclass.

I'd argue it's better to be comprehensive, and forward declare it.  Imagine, for
instance, we remove it, and then add some other dependency on GURL being forward
declared to this file.  Then we decide not to make this a ResourceHandler any
more, and get rid of that include.  We're suddenly just removing code, but have
to add a forward declaration because we're moving a not-clearly-related header.

That's not likely to happen in this case, but in general, it does happen, so
think it's best to be explicit about forward declaration to avoid IWYU issues
going forward.

[mmenke, 2016-10-26 15:27:38]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-10-26 15:27:38]

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

[commit-bot: I haz the power, 2016-10-26 15:28:04]

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

[Charlie Harrison, 2016-10-26 15:28:55]

still lgtm your reasoning makes sense to me.

[commit-bot: I haz the power, 2016-10-26 15:35:02]

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

[commit-bot: I haz the power, 2016-10-26 15:35:03]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mmenke, 2016-10-26 16:25:31]

mmenke@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[mmenke, 2016-10-26 16:25:32]

[+phajdan.jr]:  Please review content/test/BUILD.gn.  Should we have a wildcard
owners rule for that file?

[Paweł Hajdan Jr., 2016-10-27 08:14:44]

LGTM

Feel free to make a change to add wildcard per-file OWNERS.

[mmenke, 2016-10-27 09:37:56]

On 2016/10/27 08:14:44, Paweł Hajdan Jr. wrote:
> LGTM
> 
> Feel free to make a change to add wildcard per-file OWNERS.

Thanks, I'll do that.

[mmenke, 2016-10-27 09:38:02]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2016-10-27 09:38:16]

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, 2016-10-27 09:42:31]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-10-27 09:45:26]

Description was changed from

==========
Merge TestResourceHandlers used by two different test fixtures.

The MimeSniffingResourceHandler and InterceptingResourceHandler unit
tests use two different TestResourceHandlers with very similar feature
sets. This CL merges them into single class that supports all the
features both text fixtures need.

BUG=658760
==========

to

==========
Merge TestResourceHandlers used by two different test fixtures.

The MimeSniffingResourceHandler and InterceptingResourceHandler unit
tests use two different TestResourceHandlers with very similar feature
sets. This CL merges them into single class that supports all the
features both text fixtures need.

BUG=658760

Committed: https://crrev.com/2b6880fff5eb65c0d7f12fa3b755aaac22bf13a9
Cr-Commit-Position: refs/heads/master@{#427981}
==========

[commit-bot: I haz the power, 2016-10-27 09:45:27]

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