Allow URL requests for object/embed tags to be intercepted as streams.

Allow URL requests for object/embed tags to be intercepted as streams.

BrowserPlugins make URL requests on behalf of object tags. Requests on behalf of
object tags should be treated in a similar way to requests in behalf of frames
in that they are allowed to be intercepted as a stream request, however unlike frames they should never be downloaded. This CL allows
these requests to be intercepted in that way.

BUG=416310
TBR=mkosiba@chromium.org,benwells@chromium.org

Committed: https://crrev.com/9325d7ca3d712a1fcc58d4066984206da6b9e612
Cr-Commit-Position: refs/heads/master@{#305130}

[raymes, 2014-11-06 00:00:00]

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

[raymes, 2014-11-06 00:00:01]

mmenke: I was able to greatly simplify this and hopefully it makes much more
sense. Sorry for the bother if you still don't think you're the right person to
take a look :(

If you're ok with the changes to
content/browser/loader/buffered_resource_handler.cc I will get fsamuel@ to
review the other changes.

[mmenke, 2014-11-06 15:08:17]

I still have no idea what this is doing.  Object?  Stream?  Should we be
bypassing the "must download" logic?

Should also probably also have a test for this.  I guess layout tests are the
most common for content/ bugs in web-exposed features?

[raymes, 2014-11-12 05:44:17]

Hi mmenke,

Thanks for taking a look. Sorry for the delay. Please see below.

On 2014/11/06 15:08:17, mmenke wrote:
> I still have no idea what this is doing.  Object?  Stream? Should we be
> bypassing the "must download" logic?

Stream requests are used to intercept URL requests that are made by the browser
and pass them on to extensions. You reviewed a related CL 6 months ago which may
provide some more context (https://codereview.chromium.org/263513004) otherwise
zork@ can give more background.

Right now, streams are only intercepted as requests if they are made on behalf
of a frame. If you look a bit further down inside
BufferedResourceHandler::SelectNextHandler you will see a similar snippet of
code that does this. Note that info->allow_download() is only true if the
request is made on behalf of a frame.

However in this case, we are dealing with a situation where the request is for
an embedded plugin and the request isn't made on behalf of a frame. It is made
on behalf of an <object> tag (which is where the Object bit comes in). In these
cases, allow_download will never be true (I added a DCHECK for this) and we
never want the browser to download the request anyway.

> Should also probably also have a test for this.  I guess layout tests are the
> most common for content/ bugs in web-exposed features?

I added a unittest here for BufferedResourceLoader which tests stream
interception. This feature isn't exposed directly to the web so it's not
possible to add a layout test for it. However, I will be writing a browser test
soon which will exercise this code as well.

Thanks for taking a look!
Raymes

[mmenke, 2014-11-12 12:03:33]

On 2014/11/12 05:44:17, raymes wrote:
> Hi mmenke,
> 
> Thanks for taking a look. Sorry for the delay. Please see below.
> 
> On 2014/11/06 15:08:17, mmenke wrote:
> > I still have no idea what this is doing.  Object?  Stream? Should we be
> > bypassing the "must download" logic?
> 
> Stream requests are used to intercept URL requests that are made by the
browser
> and pass them on to extensions. You reviewed a related CL 6 months ago which
may
> provide some more context (https://codereview.chromium.org/263513004)
otherwise
> zork@ can give more background.
> 
> Right now, streams are only intercepted as requests if they are made on behalf
> of a frame. If you look a bit further down inside
> BufferedResourceHandler::SelectNextHandler you will see a similar snippet of
> code that does this. Note that info->allow_download() is only true if the
> request is made on behalf of a frame.
> 
> However in this case, we are dealing with a situation where the request is for
> an embedded plugin and the request isn't made on behalf of a frame. It is made
> on behalf of an <object> tag (which is where the Object bit comes in). In
these
> cases, allow_download will never be true (I added a DCHECK for this) and we
> never want the browser to download the request anyway.
> 
> > Should also probably also have a test for this.  I guess layout tests are
the
> > most common for content/ bugs in web-exposed features?
> 
> I added a unittest here for BufferedResourceLoader which tests stream
> interception. This feature isn't exposed directly to the web so it's not
> possible to add a layout test for it. However, I will be writing a browser
test
> soon which will exercise this code as well.
> 
> Thanks for taking a look!
> Raymes

Your other CL had another reviewer, who I assumed what was going on.

On this CL, I do not know enough to verify the code is correct, or even does
anything useful that needs to be done.  I did see the other chunk of code, but I
really have no idea how they interact.  The fact that I can't verify this CL is
correct, or that it doesn't seem to have any bugs, or unintended consequences,
means that I don't know enough to be the primary review of it.

It's also my judgment that learning enough to review it would involve more time
than I have to spend doing so.  I rarely refuse to do reviews, but in this case,
it seems warranted.

[raymes, 2014-11-12 22:50:47]

raymes@chromium.org changed reviewers:
+ zork@chromium.org

[raymes, 2014-11-12 22:50:47]

Thanks mmenke. Yes the other reviewer was zork@ (he was on the cc line here). He
implemented most of the streams code and would have the most context here.

zork@ could you please take a look? Thanks!

[Zachary Kuznia, 2014-11-13 23:18:08]

https://codereview.chromium.org/694773003/diff/40001/content/browser/loader/b...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/694773003/diff/40001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:316: if
(info->GetResourceType() == content::RESOURCE_TYPE_OBJECT) {
Why isn't this caught by line 333?

https://codereview.chromium.org/694773003/diff/40001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:321: return
UseAlternateNextHandler(handler.Pass(), payload);
Is there a reason not to test that handler is set?

[raymes, 2014-11-13 23:42:10]

Thanks for taking a look!

https://codereview.chromium.org/694773003/diff/40001/content/browser/loader/b...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/694773003/diff/40001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:316: if
(info->GetResourceType() == content::RESOURCE_TYPE_OBJECT) {
On 2014/11/13 23:18:08, Zachary Kuznia wrote:
> Why isn't this caught by line 333?

Because in these cases |allow_download| is always false. The reason is that
allow_download is only ever true if the resource types are frames (see
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...).

This makes sense because if the request isn't coming from a frame we don't want
the browser to download it.

https://codereview.chromium.org/694773003/diff/40001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:321: return
UseAlternateNextHandler(handler.Pass(), payload);
There is one other place where requests are made on behalf of objects which is
for pnacl pexes:
https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/re...

In those cases the request might not be intercepted as a stream.

[Zachary Kuznia, 2014-11-13 23:48:42]

https://codereview.chromium.org/694773003/diff/40001/content/browser/loader/b...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/694773003/diff/40001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:316: if
(info->GetResourceType() == content::RESOURCE_TYPE_OBJECT) {
On 2014/11/13 23:42:10, raymes wrote:
> On 2014/11/13 23:18:08, Zachary Kuznia wrote:
> > Why isn't this caught by line 333?
> 
> Because in these cases |allow_download| is always false. The reason is that
> allow_download is only ever true if the resource types are frames (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...).
> 
> This makes sense because if the request isn't coming from a frame we don't
want
> the browser to download it.

It feels like we should share this call with the other case, instead of copying
it.  However, that might made the code harder to read...  Can you think of a way
to re-arrange this so that MaybeInterceptAsStream has only one call site in this
function?

https://codereview.chromium.org/694773003/diff/40001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:321: return
UseAlternateNextHandler(handler.Pass(), payload);
On 2014/11/13 23:42:10, raymes wrote:
> There is one other place where requests are made on behalf of objects which is
> for pnacl pexes:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/re...
> 
> In those cases the request might not be intercepted as a stream.

In that case, shouldn't we maintain the current behavior of returning true, like
like 325?

[raymes, 2014-11-14 20:55:29]

Thanks Zach! PTAL

https://codereview.chromium.org/694773003/diff/40001/content/browser/loader/b...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/694773003/diff/40001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:316: if
(info->GetResourceType() == content::RESOURCE_TYPE_OBJECT) {
I did try to share the code in a few different ways but none of them seemed to
make it clearer. If we have a function, it needs to have a return value which is
a bool indicating whether the stream was intercepted and an out param which is a
bool which indicates the return value which felt a bit awkward. If you prefer
this, let me know and I can make the change :)

https://codereview.chromium.org/694773003/diff/40001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:321: return
UseAlternateNextHandler(handler.Pass(), payload);
Yes, good catch! I added a test for this.

[Zachary Kuznia, 2014-11-14 22:01:45]

LGTM

https://codereview.chromium.org/694773003/diff/40001/content/browser/loader/b...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/694773003/diff/40001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:316: if
(info->GetResourceType() == content::RESOURCE_TYPE_OBJECT) {
On 2014/11/14 20:55:29, raymes wrote:
> I did try to share the code in a few different ways but none of them seemed to
> make it clearer. If we have a function, it needs to have a return value which
is
> a bool indicating whether the stream was intercepted and an out param which is
a
> bool which indicates the return value which felt a bit awkward. If you prefer
> this, let me know and I can make the change :)

Fair enough, I thought that might be the case.

[raymes, 2014-11-17 00:58:54]

+OWNERS
mmenke: could you please take a look at content/browser/loader/*

jochen for:
content/public/browser/resource_request_info.h

[raymes, 2014-11-17 00:59:07]

raymes@chromium.org changed reviewers:
+ jochen@chromium.org

[raymes, 2014-11-17 00:59:07]

+OWNERS
mmenke: could you please take a look at content/browser/loader/*

jochen for:
content/public/browser/resource_request_info.h

[mmenke, 2014-11-17 18:57:08]

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:320:
host_->MaybeInterceptAsStream(request(), response_.get(), &payload));
The other call to this is bypassed if "net::IsSupportedMimeType" is false.  Do
we care about that here?

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
File content/browser/loader/buffered_resource_handler_unittest.cc (right):

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:5: #include
"content/browser/loader/buffered_resource_handler.h"
optional nit:  I believe the corresponding header file for code being tested
should appear first (as it already is) with a blank line after it.  Hrm...We're
not very consistent about that, even in this directory, that's done in only 2 of
4 test files.

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:19:
BufferedResourceHandlerTest() : intercepted_as_stream_(false) {}
nit:  Blank line after constructor.

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:39:
TestResourceHandler() : ResourceHandler(nullptr) {}
nit:  Blank lines between methods.

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:78:
scoped_ptr<ResourceHandler> MaybeInterceptAsStream(
nit:  blank line between methods.

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:108: bool
must_download_;
nit:  const?

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:120:
request_resource_type == RESOURCE_TYPE_SUB_FRAME);
Include base/logging.h, for CHECK...Actually, should these be switched to a
DCHECK, and moved into buffered_resource_handler.cc?

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:126:
GURL("http://www.google.com"), net::DEFAULT_PRIORITY, nullptr, nullptr));
include base/scoped_ptr.h and url/gurl.h

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:127:
ResourceRequestInfo::AllocateForTesting(request.get(),
request.get(), should be moved onto next line, aligned with other arguments.

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:143: bool defer =
true;
defer should always be initialized to false, to mimic behavior in production
code.

[raymes, 2014-11-17 23:44:25]

Thanks, ptal!

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:320:
host_->MaybeInterceptAsStream(request(), response_.get(), &payload));
Requests for those types should never get intercepted as streams. Added a DCHECK
here.

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
File content/browser/loader/buffered_resource_handler_unittest.cc (right):

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:5: #include
"content/browser/loader/buffered_resource_handler.h"
Yes, I've seen lots of inconsistency in that. I added the space.

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:19:
BufferedResourceHandlerTest() : intercepted_as_stream_(false) {}
On 2014/11/17 18:57:08, mmenke wrote:
> nit:  Blank line after constructor.

Done.

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:39:
TestResourceHandler() : ResourceHandler(nullptr) {}
On 2014/11/17 18:57:08, mmenke wrote:
> nit:  Blank lines between methods.

Done.

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:78:
scoped_ptr<ResourceHandler> MaybeInterceptAsStream(
On 2014/11/17 18:57:08, mmenke wrote:
> nit:  blank line between methods.

Done.

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:108: bool
must_download_;
On 2014/11/17 18:57:08, mmenke wrote:
> nit:  const?

Done.

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:120:
request_resource_type == RESOURCE_TYPE_SUB_FRAME);
Done, I moved it into buffered_resource_handler.cc

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:126:
GURL("http://www.google.com"), net::DEFAULT_PRIORITY, nullptr, nullptr));
On 2014/11/17 18:57:08, mmenke wrote:
> include base/scoped_ptr.h and url/gurl.h

Done.

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:127:
ResourceRequestInfo::AllocateForTesting(request.get(),
On 2014/11/17 18:57:08, mmenke wrote:
> request.get(), should be moved onto next line, aligned with other arguments.

Done.

https://codereview.chromium.org/694773003/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler_unittest.cc:143: bool defer =
true;
On 2014/11/17 18:57:08, mmenke wrote:
> defer should always be initialized to false, to mimic behavior in production
> code.

Done.

[jochen (gone - plz use gerrit), 2014-11-18 13:40:38]

content/public lgtm

[mmenke, 2014-11-18 16:32:17]

LGTM

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:320: info->GetResourceType()
== RESOURCE_TYPE_SUB_FRAME);
Can move this down below the "!info->allow_download()" check before, I think.

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:326:
DCHECK(!net::IsSupportedMimeType(mime_type));
On 2014/11/17 23:44:25, raymes wrote:
> Requests for those types should never get intercepted as streams. Added a
DCHECK
> here.

In addition to never getting intercepted as streams, they're also never of
RESOURCE_TYPE_OBJECT?

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
File content/browser/loader/buffered_resource_handler_unittest.cc (right):

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:12: #include
"content/public/test/test_browser_thread_bundle.h"
Header no longer used

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:13: #include
"content/public/test/test_utils.h"
Is this used?

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:43:
base::MessageLoop message_loop_;
Should include message loop header.

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:48:
TestResourceHandler() : ResourceHandler(nullptr) {}
nit:  Blank lines between methods.

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:78: bool
OnReadCompleted(int bytes_read, bool* defer) override {
Methods currently not being tested should probably have a NOTREACHED() to
indicate that fact (And include base/logging.h)

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:86: void
OnDataDownloaded(int bytes_downloaded) override {}
 private:
  DISALLOW_COPY_AND_ASSIGN(TestResourceHandler);

And include macros.h?

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:111:
test_->set_intercepted_as_stream();
nit:  Think this makes more sense as an accessor, and getting rid of the
dependency on the test fixture.  Having test classes depend on the fixture is
pretty uncommon, and not really needed, here.

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:226: ASSERT_FALSE(
I think these can all be EXPECTs.

[raymes, 2014-11-19 04:08:37]

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:320: info->GetResourceType()
== RESOURCE_TYPE_SUB_FRAME);
On 2014/11/18 16:32:17, mmenke wrote:
> Can move this down below the "!info->allow_download()" check before, I think.

Done.

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:326:
DCHECK(!net::IsSupportedMimeType(mime_type));
Arg no that's not necessarily true. I had already moved the DCHECK into the if
statement below but forgot to upload the patchset. Thanks for catching.

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
File content/browser/loader/buffered_resource_handler_unittest.cc (right):

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:12: #include
"content/public/test/test_browser_thread_bundle.h"
On 2014/11/18 16:32:17, mmenke wrote:
> Header no longer used

Done.

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:13: #include
"content/public/test/test_utils.h"
On 2014/11/18 16:32:17, mmenke wrote:
> Is this used?

Done.

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:43:
base::MessageLoop message_loop_;
On 2014/11/18 16:32:17, mmenke wrote:
> Should include message loop header.

Done.

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:48:
TestResourceHandler() : ResourceHandler(nullptr) {}
On 2014/11/18 16:32:17, mmenke wrote:
> nit:  Blank lines between methods.

Done.

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:78: bool
OnReadCompleted(int bytes_read, bool* defer) override {
On 2014/11/18 16:32:17, mmenke wrote:
> Methods currently not being tested should probably have a NOTREACHED() to
> indicate that fact (And include base/logging.h)

Done.

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:86: void
OnDataDownloaded(int bytes_downloaded) override {}
On 2014/11/18 16:32:17, mmenke wrote:
>  private:
>   DISALLOW_COPY_AND_ASSIGN(TestResourceHandler);
> 
> And include macros.h?

Done.

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:111:
test_->set_intercepted_as_stream();
Well spotted, thanks! Done.

https://codereview.chromium.org/694773003/diff/100001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:226: ASSERT_FALSE(
On 2014/11/18 16:32:17, mmenke wrote:
> I think these can all be EXPECTs.

Done.

[mmenke, 2014-11-19 16:44:59]

A couple more comments.  Feel free to land when you've addressed them.

https://codereview.chromium.org/694773003/diff/180001/content/browser/loader/...
File content/browser/loader/buffered_resource_handler_unittest.cc (right):

https://codereview.chromium.org/694773003/diff/180001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:21: class
BufferedResourceHandlerTest : public testing::Test {
This can now go below the other classes, which it depends one.  Can optionally
inline TestStreamIsIntercepted.

https://codereview.chromium.org/694773003/diff/180001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:41: class
TestResourceHandler : public ResourceHandler {
This class and the next one need to go in an anonymous namespace - actually, you
can just stick the test fixture and all tests in it as well.

https://codereview.chromium.org/694773003/diff/180001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:102:
TestResourceDispatcherHost(bool stream_has_handler)
explicit

https://codereview.chromium.org/694773003/diff/180001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:106: bool
intercepted_as_stream() { return intercepted_as_stream_; }
nit const

[raymes, 2014-11-19 23:34:34]

https://codereview.chromium.org/694773003/diff/180001/content/browser/loader/...
File content/browser/loader/buffered_resource_handler_unittest.cc (right):

https://codereview.chromium.org/694773003/diff/180001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:21: class
BufferedResourceHandlerTest : public testing::Test {
On 2014/11/19 16:44:59, mmenke wrote:
> This can now go below the other classes, which it depends one.  Can optionally
> inline TestStreamIsIntercepted.

Done.

https://codereview.chromium.org/694773003/diff/180001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:41: class
TestResourceHandler : public ResourceHandler {
On 2014/11/19 16:44:59, mmenke wrote:
> This class and the next one need to go in an anonymous namespace - actually,
you
> can just stick the test fixture and all tests in it as well.

Done.

https://codereview.chromium.org/694773003/diff/180001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:102:
TestResourceDispatcherHost(bool stream_has_handler)
On 2014/11/19 16:44:59, mmenke wrote:
> explicit

Done.

https://codereview.chromium.org/694773003/diff/180001/content/browser/loader/...
content/browser/loader/buffered_resource_handler_unittest.cc:106: bool
intercepted_as_stream() { return intercepted_as_stream_; }
On 2014/11/19 16:44:59, mmenke wrote:
> nit const

Done.

[raymes, 2014-11-19 23:45:11]

raymes@chromium.org changed reviewers:
+ benwells@chromium.org, mkosiba@chromium.org

[raymes, 2014-11-19 23:45:12]

There were a bunch of unittests that had to be updated as a result of this.
TBRing OWNERS

mkosiba for:                         

components/navigation_interception/intercept_navigation_resource_throttle_unittest.cc
    

benwells for:

extensions/browser/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc
 
 extensions/browser/api/declarative_webrequest/webrequest_condition_unittest.cc

[raymes, 2014-11-19 23:50:00]

Patchset #13 (id:240001) has been deleted

[raymes, 2014-11-20 03:01:46]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-11-20 03:03:46]

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

[raymes, 2014-11-20 03:04:12]

The CQ bit was unchecked by raymes@chromium.org

[raymes, 2014-11-20 03:04:36]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-11-20 03:06:33]

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

[commit-bot: I haz the power, 2014-11-20 03:59:18]

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

[commit-bot: I haz the power, 2014-11-20 03:59:20]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[raymes, 2014-11-20 04:14:49]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-11-20 04:16:12]

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

[commit-bot: I haz the power, 2014-11-20 13:58:04]

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

[commit-bot: I haz the power, 2014-11-20 13:58:05]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[raymes, 2014-11-20 22:38:25]

The CQ bit was checked by raymes@chromium.org

[raymes, 2014-11-20 22:38:39]

The CQ bit was unchecked by raymes@chromium.org

[raymes, 2014-11-20 23:33:02]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-11-20 23:34:41]

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

[raymes, 2014-11-21 00:35:25]

Patchset #19 (id:380001) has been deleted

[raymes, 2014-11-21 00:36:44]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-11-21 00:38:39]

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

[commit-bot: I haz the power, 2014-11-21 01:03:00]

Committed patchset #18 (id:360001)

[commit-bot: I haz the power, 2014-11-21 01:03:43]

Patchset 18 (id:??) landed as
https://crrev.com/9325d7ca3d712a1fcc58d4066984206da6b9e612
Cr-Commit-Position: refs/heads/master@{#305130}