Add support for reading blobs when using the network service.

Add support for reading blobs when using the network service.

For now this only handles frame requests. A future change will add support

BUG=715677
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2906543002
Cr-Commit-Position: refs/heads/master@{#476903}
Committed: https://chromium.googlesource.com/chromium/src/+/9354af8d90801db9a5fa621f969dcbf3b185e979

[jam, 2017-05-24 21:05:35]

Description was changed from

==========
Add support for reading blobs when using the network service.

For now this only handles frame requests. A future change will add support

BUG=715677
==========

to

==========
Add support for reading blobs when using the network service.

For now this only handles frame requests. A future change will add support

BUG=715677
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-05-24 21:05:39]

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

[commit-bot: I haz the power, 2017-05-24 21:06:28]

Dry run: CQ is trying da patch.

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

[jam, 2017-05-24 21:25:25]

Description was changed from

==========
Add support for reading blobs when using the network service.

For now this only handles frame requests. A future change will add support

BUG=715677
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add support for reading blobs when using the network service.

For now this only handles frame requests. A future change will add support

BUG=715677
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-05-24 21:25:25]

jam@chromium.org changed reviewers:
+ dmurph@google.com

[jam, 2017-05-24 22:10:41]

I tried to strike a balance between sharing code with the URLRequestJob
implementation vs making the code unreadable. There's still some small
duplication though that I didn't think could be unwound cleanly. Given this code
hasn't really changed in a long time, I thought it was ok.

The other thing is that I'd like to make it easier to implement
mojom::URLLoaders with data pipes, so after this lands I'll look into sharing
code with the network implementation.

[dmurph, 2017-05-24 22:45:52]

dmurph@chromium.org changed reviewers:
+ dmurph@chromium.org
- dmurph@google.com

[commit-bot: I haz the power, 2017-05-25 00:57:55]

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

[commit-bot: I haz the power, 2017-05-25 00:57:56]

Dry run: This issue passed the CQ dry run.

[jam, 2017-05-25 17:53:31]

jam@chromium.org changed reviewers:
+ mek@chromium.org

[jam, 2017-05-25 17:53:45]

+Marijn as well

[dmurph, 2017-05-25 21:37:11]

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
File content/browser/blob_storage/blob_url_loader_factory.cc (right):

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:37: class BlobURLLoader
: public mojom::URLLoader {
Can you write threading expectations? I think we're constructed on the UI
thread, and then operated on by the IO thread.

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:210: mojo::DataPipe
data_pipe(kDefaultAllocationSize);
This part seems to make sense to me - I don't know mojo setup well though.

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:241: // TODO: How should
this be communicated to our client?
NotifyComplete(error)? Or are you unsure what the error code would be?

Can you put UMAs on all of the error possibilities here? I'm thinking
specifically about mojo-related errors (not http request errors). But that could
be in a follow up cl, and I could do that.

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:259: DeleteIfNeeded();
Redundant line - NotifyCompleted calls this. Or, maybe you need to destroy the
handles and pending write?

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:302: // TODO: Handle a
bad |result| value.
I'm imagining this would:
1. Close the stream
2. CallNotifyComplete(error)

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:333:
DISALLOW_COPY_AND_ASSIGN(BlobURLLoader);
I double checked that the logic/contents for this class matches
blob_url_request_job.

Are you able to take the test for blob_url_request_job and apply it to this new
handler? Those are pretty fully-featured tests, and with those I would be really
confident that the code here is correct.

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
File content/browser/blob_storage/blob_url_loader_factory.h (right):

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.h:25: : public
base::RefCountedThreadSafe<BlobURLLoaderFactory,
Can we make this not refcounted?

[Marijn Kruisselbrink, 2017-05-25 22:12:22]

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
File content/browser/blob_storage/blob_url_loader_factory.cc (right):

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:37: class BlobURLLoader
: public mojom::URLLoader {
On 2017/05/25 at 21:37:10, dmurph wrote:
> Can you write threading expectations? I think we're constructed on the UI
thread, and then operated on by the IO thread.

I'm pretty sure it's created on and fully runs on the IO thread (see the
DCHECK_CURRENTLY_ON in CreateLoaderAndStart)

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:70:
BrowserThread::GetTaskRunnerForThread(BrowserThread::FILE).get();
nit: BrowserThread::FILE is deprecated, and you probably should be using
base::CreateSequencedTaskRunnerWithTraits({base::MayBlock(),
base::TaskPriority::BACKGROUND}) or something similar. Although on the other
hand I can see how having minimal changes with the existing code (which is using
BrowserThread::FILE) might be nice too, especially since I'm not sure which
tasks BlobReader actually expects to be sequenced with one another. So yeah, I
guess just using the same sequenced task runner as the old code for now is
definitely the safest thing to do.

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:282:
std::move(response_body_consumer_handle_));
nit: mojo::ScopedHandleBase is not documented as clearing itself on being moved
from, so even if it might currently always do so it is possibly not safe to rely
on that? Of course it does seem reasonable for ScopedHandleBase to behave
similar to std::unique_ptr in this case, so maybe the correct fix is to just
document that behavior?

[jam, 2017-05-26 04:35:08]

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

[commit-bot: I haz the power, 2017-05-26 04:35:44]

Dry run: CQ is trying da patch.

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

[jam, 2017-05-26 04:46:47]

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
File content/browser/blob_storage/blob_url_loader_factory.cc (right):

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:37: class BlobURLLoader
: public mojom::URLLoader {
On 2017/05/25 22:12:22, Marijn Kruisselbrink wrote:
> On 2017/05/25 at 21:37:10, dmurph wrote:
> > Can you write threading expectations? I think we're constructed on the UI
> thread, and then operated on by the IO thread.
> 
> I'm pretty sure it's created on and fully runs on the IO thread (see the
> DCHECK_CURRENTLY_ON in CreateLoaderAndStart)

Correct, I added a dcheck as well.

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:70:
BrowserThread::GetTaskRunnerForThread(BrowserThread::FILE).get();
On 2017/05/25 22:12:22, Marijn Kruisselbrink wrote:
> nit: BrowserThread::FILE is deprecated, and you probably should be using
> base::CreateSequencedTaskRunnerWithTraits({base::MayBlock(),
> base::TaskPriority::BACKGROUND}) or something similar. Although on the other
> hand I can see how having minimal changes with the existing code (which is
using
> BrowserThread::FILE) might be nice too, especially since I'm not sure which
> tasks BlobReader actually expects to be sequenced with one another. So yeah, I
> guess just using the same sequenced task runner as the old code for now is
> definitely the safest thing to do.

Right, I wanted the same behavior. They can both be changed at the same time.

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:210: mojo::DataPipe
data_pipe(kDefaultAllocationSize);
On 2017/05/25 21:37:10, dmurph wrote:
> This part seems to make sense to me - I don't know mojo setup well though.

This is pretty much copied from content/common/url_loader_impl.cc. What I was
hinting at in the cl message is that I'll combine these into a helper class
after this lands. It's too much boilerplate.

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:241: // TODO: How should
this be communicated to our client?
On 2017/05/25 21:37:10, dmurph wrote:
> NotifyComplete(error)? Or are you unsure what the error code would be?

Done

> 
> Can you put UMAs on all of the error possibilities here? I'm thinking
> specifically about mojo-related errors (not http request errors). But that
could
> be in a follow up cl, and I could do that.

Do we really need it on all codepaths? i.e. the URLRequestJob doesn't do this,
and in general URLRequestJob implementations don't.

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:259: DeleteIfNeeded();
On 2017/05/25 21:37:10, dmurph wrote:
> Redundant line - NotifyCompleted calls this. Or, maybe you need to destroy the
> handles and pending write?

Done.

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:282:
std::move(response_body_consumer_handle_));
On 2017/05/25 22:12:22, Marijn Kruisselbrink wrote:
> nit: mojo::ScopedHandleBase is not documented as clearing itself on being
moved
> from, so even if it might currently always do so it is possibly not safe to
rely
> on that? Of course it does seem reasonable for ScopedHandleBase to behave
> similar to std::unique_ptr in this case, so maybe the correct fix is to just
> document that behavior?

The header says
"// Scoper for the actual handle types defined further below. It's move-only,
// like the C++11 |unique_ptr|."

https://cs.chromium.org/chromium/src/mojo/public/cpp/system/handle.h?rcl=8a5e...

is that what you're looking for?

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:302: // TODO: Handle a
bad |result| value.
On 2017/05/25 21:37:10, dmurph wrote:
> I'm imagining this would:
> 1. Close the stream
> 2. CallNotifyComplete(error)

Yep (this TODO is copied from the other implementation). Do you want me to
address the TODOs now or is later ok?

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.cc:333:
DISALLOW_COPY_AND_ASSIGN(BlobURLLoader);
On 2017/05/25 21:37:10, dmurph wrote:
> I double checked that the logic/contents for this class matches
> blob_url_request_job.
> 
> Are you able to take the test for blob_url_request_job and apply it to this
new
> handler? Those are pretty fully-featured tests, and with those I would be
really
> confident that the code here is correct.

In general, I prefer higher level tests so that they don't have to change when
the implementation does. Are the browser tests and layout tests not sufficient?
If not, I'd prefer to find ways of adding tests there, so that they can run on
both implementations.

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
File content/browser/blob_storage/blob_url_loader_factory.h (right):

https://codereview.chromium.org/2906543002/diff/1/content/browser/blob_storag...
content/browser/blob_storage/blob_url_loader_factory.h:25: : public
base::RefCountedThreadSafe<BlobURLLoaderFactory,
On 2017/05/25 21:37:10, dmurph wrote:
> Can we make this not refcounted?

It's used on StoragePartition and passed to the IO thread, hence the ref
counting. All the member classes there are similarly refcounted.

[commit-bot: I haz the power, 2017-05-26 06:20:00]

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

[commit-bot: I haz the power, 2017-05-26 06:20:01]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-05-26 09:48:11]

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

[kinuko, 2017-05-26 09:48:12]

A few drive-by's, looking pretty good in general (mod some concerns for code
duplication)

https://codereview.chromium.org/2906543002/diff/20001/content/browser/blob_st...
File content/browser/blob_storage/blob_url_loader_factory.cc (right):

https://codereview.chromium.org/2906543002/diff/20001/content/browser/blob_st...
content/browser/blob_storage/blob_url_loader_factory.cc:36: // Note: some of
this code is duplicated from storage::BlobURLRequestJob.
It feels we could share some more implementation with it..

https://codereview.chromium.org/2906543002/diff/20001/content/browser/blob_st...
content/browser/blob_storage/blob_url_loader_factory.cc:245: DeleteIfNeeded();
nit: not needed

[kinuko, 2017-05-26 09:48:29]

kinuko@chromium.org changed reviewers:
- kinuko@chromium.org

[kinuko, 2017-05-26 09:52:02]

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

[jam, 2017-05-26 14:13:56]

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

[commit-bot: I haz the power, 2017-05-26 14:14:13]

Dry run: CQ is trying da patch.

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

[jam, 2017-05-26 14:14:53]

https://codereview.chromium.org/2906543002/diff/20001/content/browser/blob_st...
File content/browser/blob_storage/blob_url_loader_factory.cc (right):

https://codereview.chromium.org/2906543002/diff/20001/content/browser/blob_st...
content/browser/blob_storage/blob_url_loader_factory.cc:36: // Note: some of
this code is duplicated from storage::BlobURLRequestJob.
On 2017/05/26 09:48:12, kinuko wrote:
> It feels we could share some more implementation with it..

There are some scattered lines of code that are shared, but they're closely
tired to URLRequestJob/IOBuffer. I looked into sharing them, but then I'd have
to define an interface to abstract these jobs. So given that this code has
hardly changed in the last year, and once PlzNavigate/MojoLoading ship we can
use this as the one code path, I thought it's better to not add extra
abstractions here that I would have to undo later.

https://codereview.chromium.org/2906543002/diff/20001/content/browser/blob_st...
content/browser/blob_storage/blob_url_loader_factory.cc:245: DeleteIfNeeded();
On 2017/05/26 09:48:12, kinuko wrote:
> nit: not needed

Done.

[commit-bot: I haz the power, 2017-05-26 15:42:02]

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

[commit-bot: I haz the power, 2017-05-26 15:42:03]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[kinuko, 2017-05-29 04:27:16]

lgtm

https://codereview.chromium.org/2906543002/diff/20001/content/browser/blob_st...
File content/browser/blob_storage/blob_url_loader_factory.cc (right):

https://codereview.chromium.org/2906543002/diff/20001/content/browser/blob_st...
content/browser/blob_storage/blob_url_loader_factory.cc:36: // Note: some of
this code is duplicated from storage::BlobURLRequestJob.
On 2017/05/26 14:14:53, jam wrote:
> On 2017/05/26 09:48:12, kinuko wrote:
> > It feels we could share some more implementation with it..
> 
> There are some scattered lines of code that are shared, but they're closely
> tired to URLRequestJob/IOBuffer. I looked into sharing them, but then I'd have
> to define an interface to abstract these jobs. So given that this code has
> hardly changed in the last year, and once PlzNavigate/MojoLoading ship we can
> use this as the one code path, I thought it's better to not add extra
> abstractions here that I would have to undo later.

I wondered we might be able to have some static helpers like you did for
headers, but might not be worth doing. (The code around has been stable for
sometime now)

https://codereview.chromium.org/2906543002/diff/40001/content/browser/blob_st...
File content/browser/blob_storage/blob_url_loader_factory.cc (right):

https://codereview.chromium.org/2906543002/diff/40001/content/browser/blob_st...
content/browser/blob_storage/blob_url_loader_factory.cc:129: NOTREACHED();
nit: I think there's a chance that this could be called (and no-op is fine)

[jam, 2017-05-30 04:38:42]

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

[commit-bot: I haz the power, 2017-05-30 04:38:56]

Dry run: CQ is trying da patch.

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

[jam, 2017-05-30 05:23:26]

Kinuko: ptal at the changes to navigation_url_loader_network_service.cc, I
updated it after your cl landed.

https://codereview.chromium.org/2906543002/diff/40001/content/browser/blob_st...
File content/browser/blob_storage/blob_url_loader_factory.cc (right):

https://codereview.chromium.org/2906543002/diff/40001/content/browser/blob_st...
content/browser/blob_storage/blob_url_loader_factory.cc:129: NOTREACHED();
On 2017/05/29 04:27:16, kinuko wrote:
> nit: I think there's a chance that this could be called (and no-op is fine)

Done.

[jam, 2017-05-30 05:23:34]

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

[commit-bot: I haz the power, 2017-05-30 05:23:53]

Dry run: CQ is trying da patch.

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

[kinuko, 2017-05-30 06:27:32]

still lgtm

https://codereview.chromium.org/2906543002/diff/80001/content/browser/blob_st...
File content/browser/blob_storage/blob_url_loader_factory.cc (right):

https://codereview.chromium.org/2906543002/diff/80001/content/browser/blob_st...
content/browser/blob_storage/blob_url_loader_factory.cc:36: // Note: some of
this code is duplicated from storage::BlobURLRequestJob.
Could you also add a short comment about the lifetime? (E.g. this is a
self-destruct object and deletes itself when handling a request completes (see
DeleteIfNeeded))

https://codereview.chromium.org/2906543002/diff/80001/content/browser/blob_st...
content/browser/blob_storage/blob_url_loader_factory.cc:118: }
nit: NOTREACHED() here

https://codereview.chromium.org/2906543002/diff/80001/content/browser/blob_st...
content/browser/blob_storage/blob_url_loader_factory.cc:242:
NotifyCompleted(net::ERR_UNEXPECTED);
return here

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

https://codereview.chromium.org/2906543002/diff/80001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:110: }
Should we have blob url handling case here? ...hm, but given that there's the
observation that in rare extension cases redirects to blob from other scheme
could happen having it down there might be still safer. (Thinking out loud)

[commit-bot: I haz the power, 2017-05-30 06:42:12]

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

[commit-bot: I haz the power, 2017-05-30 06:42:13]

Dry run: This issue passed the CQ dry run.

[jam, 2017-05-30 15:54:18]

https://codereview.chromium.org/2906543002/diff/80001/content/browser/blob_st...
File content/browser/blob_storage/blob_url_loader_factory.cc (right):

https://codereview.chromium.org/2906543002/diff/80001/content/browser/blob_st...
content/browser/blob_storage/blob_url_loader_factory.cc:36: // Note: some of
this code is duplicated from storage::BlobURLRequestJob.
On 2017/05/30 06:27:32, kinuko wrote:
> Could you also add a short comment about the lifetime? (E.g. this is a
> self-destruct object and deletes itself when handling a request completes (see
> DeleteIfNeeded))

Done.

https://codereview.chromium.org/2906543002/diff/80001/content/browser/blob_st...
content/browser/blob_storage/blob_url_loader_factory.cc:118: }
On 2017/05/30 06:27:32, kinuko wrote:
> nit: NOTREACHED() here

Done.

https://codereview.chromium.org/2906543002/diff/80001/content/browser/blob_st...
content/browser/blob_storage/blob_url_loader_factory.cc:242:
NotifyCompleted(net::ERR_UNEXPECTED);
On 2017/05/30 06:27:32, kinuko wrote:
> return here

Done.

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

https://codereview.chromium.org/2906543002/diff/80001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:110: }
On 2017/05/30 06:27:32, kinuko wrote:
> Should we have blob url handling case here? ...hm, but given that there's the
> observation that in rare extension cases redirects to blob from other scheme
> could happen having it down there might be still safer. (Thinking out loud)

yep, that's why I didn't put it here

[jam, 2017-05-30 16:01:53]

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

[commit-bot: I haz the power, 2017-05-30 16:02:11]

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-05-30 17:32:50]

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

[commit-bot: I haz the power, 2017-05-30 17:32:52]

Dry run: This issue passed the CQ dry run.

[Marijn Kruisselbrink, 2017-05-30 19:42:20]

lgtm

[dmurph, 2017-05-30 20:42:03]

last couple comments. let me know your thoughts about testing.

https://codereview.chromium.org/2906543002/diff/100001/content/browser/blob_s...
File content/browser/blob_storage/blob_url_loader_factory.cc (right):

https://codereview.chromium.org/2906543002/diff/100001/content/browser/blob_s...
content/browser/blob_storage/blob_url_loader_factory.cc:39: class BlobURLLoader
: public mojom::URLLoader {
There are not integration tests for edge cases for url-specific blob loading
code in the blob_url_request_job. All of these live in the
blob_url_request_job_unittest.cc. I'm mostly concerned around error cases, like
where files don't exist. It looks like we're using the old code for header
parsing, so that should be tested already.

So I'm a little hesitant with this not having unittests. If you see a way this
is tested already, or have different ideas, let me know.

https://codereview.chromium.org/2906543002/diff/100001/content/browser/blob_s...
content/browser/blob_storage/blob_url_loader_factory.cc:259:
NotifyCompleted(blob_reader_->net_error());
Are we sure we get deleted here? response_body_stream_ or pending_write_ will be
set from the call to NetToMojoPendingBuffer::BeginWrite above, correct?

Or are we depending on the OnResponseBodyStreamClosed /
OnResponseBodyStreamReady to be called? If so, it would be nice to have a
comment here about how the object will be destroyed.

[jam, 2017-06-02 18:10:59]

https://codereview.chromium.org/2906543002/diff/100001/content/browser/blob_s...
File content/browser/blob_storage/blob_url_loader_factory.cc (right):

https://codereview.chromium.org/2906543002/diff/100001/content/browser/blob_s...
content/browser/blob_storage/blob_url_loader_factory.cc:39: class BlobURLLoader
: public mojom::URLLoader {
On 2017/05/30 20:42:03, dmurph wrote:
> There are not integration tests for edge cases for url-specific blob loading
> code in the blob_url_request_job. All of these live in the
> blob_url_request_job_unittest.cc. I'm mostly concerned around error cases,
like
> where files don't exist. It looks like we're using the old code for header
> parsing, so that should be tested already.
> 
> So I'm a little hesitant with this not having unittests. If you see a way this
> is tested already, or have different ideas, let me know.

Good points. I've changed blob_url_request_job_unittest.cc so that it can work
with both code paths.

https://codereview.chromium.org/2906543002/diff/100001/content/browser/blob_s...
content/browser/blob_storage/blob_url_loader_factory.cc:259:
NotifyCompleted(blob_reader_->net_error());
On 2017/05/30 20:42:03, dmurph wrote:
> Are we sure we get deleted here? response_body_stream_ or pending_write_ will
be
> set from the call to NetToMojoPendingBuffer::BeginWrite above, correct?
> 
> Or are we depending on the OnResponseBodyStreamClosed /
> OnResponseBodyStreamReady to be called? 

Correct

> If so, it would be nice to have a
> comment here about how the object will be destroyed.

I'm hesitant to document how deletion happens at each callsite of
NotifyCompleted, so I've moved a comment to that method instead

[jam, 2017-06-02 18:17:37]

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

[commit-bot: I haz the power, 2017-06-02 18:18:04]

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-06-02 18:29:01]

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

[commit-bot: I haz the power, 2017-06-02 18:29:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jam, 2017-06-02 19:35:43]

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

[jam, 2017-06-02 19:35:44]

Patchset #8 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-06-02 19:36:16]

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-06-02 19:56:34]

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

[commit-bot: I haz the power, 2017-06-02 19:56:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmurph, 2017-06-02 20:10:33]

Looks great, lgtm with one nit.

https://codereview.chromium.org/2906543002/diff/160001/content/browser/blob_s...
File content/browser/blob_storage/blob_url_loader_factory.h (right):

https://codereview.chromium.org/2906543002/diff/160001/content/browser/blob_s...
content/browser/blob_storage/blob_url_loader_factory.h:57: // Safe to have a raw
pointer since the IO thread
nit: update comment, no longer raw pointer.

[jam, 2017-06-03 01:03:11]

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

[commit-bot: I haz the power, 2017-06-03 01:03:37]

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-06-03 02:35:35]

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

[commit-bot: I haz the power, 2017-06-03 02:35:37]

Dry run: This issue passed the CQ dry run.

[jam, 2017-06-03 17:45:38]

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

[commit-bot: I haz the power, 2017-06-03 17:45:43]

Dry run: CQ is trying da patch.

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

[jam, 2017-06-03 17:45:53]

Patchset #9 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-06-03 19:50:01]

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

[commit-bot: I haz the power, 2017-06-03 19:50:02]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jam, 2017-06-03 21:12:02]

The CQ bit was checked by jam@chromium.org

[jam, 2017-06-03 21:12:02]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
mek@chromium.org, dmurph@chromium.org
Link to the patchset: https://codereview.chromium.org/2906543002/#ps200001
(title: "fix threading issue with weakptr")

[commit-bot: I haz the power, 2017-06-03 21:12:12]

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-06-03 21:59:44]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1496524322140390,
"parent_rev": "bb6532a9810ffdf9cd8b5f47824e06609a48aba4", "commit_rev":
"9354af8d90801db9a5fa621f969dcbf3b185e979"}

[commit-bot: I haz the power, 2017-06-03 21:59:55]

Description was changed from

==========
Add support for reading blobs when using the network service.

For now this only handles frame requests. A future change will add support

BUG=715677
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add support for reading blobs when using the network service.

For now this only handles frame requests. A future change will add support

BUG=715677
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2906543002
Cr-Commit-Position: refs/heads/master@{#476903}
Committed:
https://chromium.googlesource.com/chromium/src/+/9354af8d90801db9a5fa621f969d...
==========

[commit-bot: I haz the power, 2017-06-03 21:59:57]

Committed patchset #9 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/9354af8d90801db9a5fa621f969d...