[Storage] Blob Storage Refactoring pt 1

[Storage] Blob Storage Refactoring pt 1:
* Renaming classes to be more descriptive.
* Changing smart pointers to reflect strict ownership model.
* Adding pointers to facilitate future resource swapping.
* Remove renderer-side dependency on blob_data.h

This patch makes all of the far-reaching changes that effect
everyone that uses the blob storage context.  Subsequent
changes should only effect the blob infrastructure.

https://bit.ly/AutoBlobToDisk

BUG=375297
Committed: https://crrev.com/bff2e53fb3e210be45d264797036ec59005ebdd6
Cr-Commit-Position: refs/heads/master@{#312800}

[dmurph, 2015-01-15 00:47:22]

dmurph@chromium.org changed reviewers:
+ michaeln@chromium.org

[dmurph, 2015-01-15 00:47:23]

Hi Michael,

This is the first patch in the blob storage refactor.  I haven't updated the doc
yet, but I would love for you to take an initial look.  I already found a couple
bugs related to not keeping around the BlobDataHandle, which is nice (and fixed
here).

I'll ping again when the doc is done.

Daniel

[michaeln, 2015-01-16 00:09:09]

not a full review, but a first pass at it

it looks like the blob_data.h/cc files maybe could be moved into the
browser/storage dir (and sharablefileref too)? is any of that used in the
renderer anymore?

https://codereview.chromium.org/810403004/diff/180001/content/browser/fileapi...
File content/browser/fileapi/blob_storage_host.cc (right):

https://codereview.chromium.org/810403004/diff/180001/content/browser/fileapi...
content/browser/fileapi/blob_storage_host.cc:13: using storage::BlobDataBuilder;
needed?

https://codereview.chromium.org/810403004/diff/180001/content/browser/fileapi...
File content/browser/fileapi/chrome_blob_storage_context.cc (right):

https://codereview.chromium.org/810403004/diff/180001/content/browser/fileapi...
content/browser/fileapi/chrome_blob_storage_context.cc:34:
scoped_ptr<storage::BlobDataSnapshotHandle> handle_;
for this particular class we only want to keep the uuid around, we want to allow
the snapshot can to change out from underneath

https://codereview.chromium.org/810403004/diff/180001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/810403004/diff/180001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:245:
blob_data_handle_map_[uuid] = std::make_pair(blob_data_handle.release(), 1);
also for this use case, we really only want to retain the uuid and allow the
data underneath to be swapped out

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

https://codereview.chromium.org/810403004/diff/180001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:17: using
storage::BlobDataBuilder;
i dont see where this is needed?

https://codereview.chromium.org/810403004/diff/180001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:19: using
storage::BlobStorageContext;
i think all usages (except one which is easily changed) are qualified with the
namespace, i'd vote to drop the using declarations

https://codereview.chromium.org/810403004/diff/180001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:93: BlobStorageContext*
blob_context,
storage::

https://codereview.chromium.org/810403004/diff/180001/content/child/webblobre...
File content/child/webblobregistry_impl.cc (right):

https://codereview.chromium.org/810403004/diff/180001/content/child/webblobre...
content/child/webblobregistry_impl.cc:15: #include
"storage/common/blob/blob_data.h"
is this include needed any more?

https://codereview.chromium.org/810403004/diff/180001/storage/browser/blob/bl...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/810403004/diff/180001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.h:44:
scoped_ptr<BlobDataSnapshotHandle> GetBlobDataFromUUID(
we may want to distinguish between retaining the snapshot and just keeping the
uuid valid, there are places where the handle is intended to be held for an
indefinite amount of time that has nothing todo with actively reading the data,
we don't need to datasnapshot for the duration is these cases

https://codereview.chromium.org/810403004/diff/180001/storage/common/blob/blo...
File storage/common/blob/blob_data.h (right):

https://codereview.chromium.org/810403004/diff/180001/storage/common/blob/blo...
storage/common/blob/blob_data.h:165: inline bool operator==(const
BlobDataSnapshot& a, const BlobDataBuilder& b) {
an odd operator

[dmurph, 2015-01-16 23:45:57]

Done!  Updated the doc to add new architecture information as well as cl
descriptions:
bit.ly/AutoBlobToDisk

Please do a full review :)

https://codereview.chromium.org/810403004/diff/180001/content/browser/fileapi...
File content/browser/fileapi/blob_storage_host.cc (right):

https://codereview.chromium.org/810403004/diff/180001/content/browser/fileapi...
content/browser/fileapi/blob_storage_host.cc:13: using storage::BlobDataBuilder;
On 2015/01/16 00:09:09, michaeln wrote:
> needed?

Done.

https://codereview.chromium.org/810403004/diff/180001/content/browser/fileapi...
File content/browser/fileapi/chrome_blob_storage_context.cc (right):

https://codereview.chromium.org/810403004/diff/180001/content/browser/fileapi...
content/browser/fileapi/chrome_blob_storage_context.cc:34:
scoped_ptr<storage::BlobDataSnapshotHandle> handle_;
On 2015/01/16 00:09:09, michaeln wrote:
> for this particular class we only want to keep the uuid around, we want to
allow
> the snapshot can to change out from underneath

Right, we want to keep the handle around though, right?

https://codereview.chromium.org/810403004/diff/180001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/810403004/diff/180001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:245:
blob_data_handle_map_[uuid] = std::make_pair(blob_data_handle.release(), 1);
On 2015/01/16 00:09:09, michaeln wrote:
> also for this use case, we really only want to retain the uuid and allow the
> data underneath to be swapped out

I think this is fixed from the decoupling of the snapshot and the handle.

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

https://codereview.chromium.org/810403004/diff/180001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:17: using
storage::BlobDataBuilder;
On 2015/01/16 00:09:09, michaeln wrote:
> i dont see where this is needed?

Done.

https://codereview.chromium.org/810403004/diff/180001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:19: using
storage::BlobStorageContext;
On 2015/01/16 00:09:09, michaeln wrote:
> i think all usages (except one which is easily changed) are qualified with the
> namespace, i'd vote to drop the using declarations

Done.

https://codereview.chromium.org/810403004/diff/180001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:93: BlobStorageContext*
blob_context,
On 2015/01/16 00:09:09, michaeln wrote:
> storage::

Done.

https://codereview.chromium.org/810403004/diff/180001/content/child/webblobre...
File content/child/webblobregistry_impl.cc (right):

https://codereview.chromium.org/810403004/diff/180001/content/child/webblobre...
content/child/webblobregistry_impl.cc:15: #include
"storage/common/blob/blob_data.h"
On 2015/01/16 00:09:09, michaeln wrote:
> is this include needed any more?

Nope, done.

https://codereview.chromium.org/810403004/diff/180001/storage/browser/blob/bl...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/810403004/diff/180001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.h:44:
scoped_ptr<BlobDataSnapshotHandle> GetBlobDataFromUUID(
On 2015/01/16 00:09:09, michaeln wrote:
> we may want to distinguish between retaining the snapshot and just keeping the
> uuid valid, there are places where the handle is intended to be held for an
> indefinite amount of time that has nothing todo with actively reading the
data,
> we don't need to datasnapshot for the duration is these cases

Sounds good, I decoupled them, so you create a snapshot with a handle.

https://codereview.chromium.org/810403004/diff/180001/storage/common/blob/blo...
File storage/common/blob/blob_data.h (right):

https://codereview.chromium.org/810403004/diff/180001/storage/common/blob/blo...
storage/common/blob/blob_data.h:165: inline bool operator==(const
BlobDataSnapshot& a, const BlobDataBuilder& b) {
On 2015/01/16 00:09:09, michaeln wrote:
> an odd operator

It's to make testing easier.  We grab a snapshot and compare it with the
original builder.

[dmurph, 2015-01-20 21:36:20]

dmurph@chromium.org changed reviewers:
+ mmenke@chromium.org, qinmin@chromium.org

[dmurph, 2015-01-20 21:36:21]

+qinmin@chromium.org for
 content/browser/media/android/media_resource_getter_impl.cc

+mmenke@chromium.org for
 content/browser/loader/*

+nasko@chromium.org for:
 content/child/webblobregistry_impl.cc
 content/common/fileapi/webblob_messages.h
 content/public/test/mock_blob_url_request_context.cc
(These are purely rename changes.)

Hello everyone,
Please take a look.  This is a refactor of the blob storage system to fix some
memory issues.  Michaeln is the main reviewer of the meaty changes, and I need
approvals for the refactor renames in the rest of the system.

Thanks!
Daniel

[mmenke, 2015-01-20 22:03:57]

loader/ LGTM (Largely deferring to michaeln to verify correctness)

https://codereview.chromium.org/810403004/diff/200001/content/browser/loader/...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/810403004/diff/200001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:74: // TODO(dmurph): store
this snapshot.  This works for now, as the items
nit:  Capitalize S

https://codereview.chromium.org/810403004/diff/200001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:75: // that the snapshot
points to are refcounted and kept in memory.  But as
Should mention that they're attached to the request by the RDH.  (+1 to cleaning
that up - the current ownership model is weird and ugly)

https://codereview.chromium.org/810403004/diff/200001/storage/common/blob/blo...
File storage/common/blob/blob_data.h (right):

https://codereview.chromium.org/810403004/diff/200001/storage/common/blob/blo...
storage/common/blob/blob_data.h:89: class STORAGE_COMMON_EXPORT BlobDataBuilder
{
This file should be renamed...Or are you planning on doing that in a followup
CL?

[michaeln, 2015-01-21 01:46:42]

this is looking pretty good to me, i mostly just have nits, there are two things
i think would be good for this CL

* move/rename the 'blob_data' files to final destinations
* ensure snapshot lifetimes are managed via the snapshot class in
upload_data_stream_builder too

https://codereview.chromium.org/810403004/diff/200001/content/browser/fileapi...
File content/browser/fileapi/blob_storage_host.h (right):

https://codereview.chromium.org/810403004/diff/200001/content/browser/fileapi...
content/browser/fileapi/blob_storage_host.h:32: class CONTENT_EXPORT
BlobStorageHost {
not for this cl, but this file should probably in storage/browser/blob too

https://codereview.chromium.org/810403004/diff/200001/content/browser/fileapi...
File content/browser/fileapi/chrome_blob_storage_context.cc (right):

https://codereview.chromium.org/810403004/diff/200001/content/browser/fileapi...
content/browser/fileapi/chrome_blob_storage_context.cc:71:
scoped_ptr<storage::BlobDataBuilder> blob_data(
maybe rename the local to blob_data_builder

https://codereview.chromium.org/810403004/diff/200001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/810403004/diff/200001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:229:
scoped_ptr<storage::BlobDataBuilder> blob_data(
instead of heap allocaing it, could BlobDataBuilder be sitting on the stack?

https://codereview.chromium.org/810403004/diff/200001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:234: blob_data_handle =
context->AddFinishedBlob(*blob_data.get());
so readers don't have to decrypt *smart.get()

https://codereview.chromium.org/810403004/diff/200001/content/browser/loader/...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/810403004/diff/200001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:78: // decomposition.
It would be really good to store snapshots in this CL so spilling over to disk
isn't gated on a reader type.

I think if the snapshot class derived from base::SupportsUserData::Data, it
could be attached to the ResourceRequestBody*, which SupportsUserData, passed
into Build(). Having that class derive from userdata makes sense sense to me
it's intended as a 'scoping' device for various operations (same reason it makes
sense for BDH to derived from that class).

https://codereview.chromium.org/810403004/diff/200001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/810403004/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:440:
scoped_ptr<storage::BlobDataBuilder> blob_data(
i think BlobDataBuilder can be on the stack here too and
might be nice to use blob_builder as a name for locals like this

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:91: for (const auto& blob_item :
data.items_) {
nit {}'s not needed

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:126: auto* entry = found->second;
Probably writing BlobMapEntry* instead of auto* would be more readable here
since the definition of blob_map_ is not nearby.

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:143: auto* entry = found->second;
ditto BlobMapEntry*

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:189: auto* entry =
blob_map_.find(item.blob_uuid())->second;\
Ditto BlobMapEntry* vs auto*, and you probably didn't mean to have a trailing \
here.

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:220: auto* entry = found->second;
ditto BlobMapEntry* vs auto*

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:264: auto& items =
src_blob_data.items();
i'm wondering what type items is? 

In addition to local readability, another downside of auto is that grepping srcs
for usage of a data type doesn't work as well as it used to.

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:349: return (found->second->flags &
BEING_BUILT) && found->second->data_builder;
BEING_BUILT seems redundant now that we have the mutually exclusive data vs
data_builder members

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.h:69: scoped_ptr<BlobDataBuilder>
data_builder;
maybe comment that |data| and |data_builder| are mutually exclusive

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_url_request_job.cc:202: auto& items =
blob_data_->items();
does const auto work here? i was wondering about the const'ness

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_url_request_job.cc:206: const BlobDataItem& item =
*items.at(i);
it's seems a little odd to have a & ref type to a refcounted data type

https://codereview.chromium.org/810403004/diff/200001/storage/common/blob/blo...
File storage/common/blob/blob_data.h (right):

https://codereview.chromium.org/810403004/diff/200001/storage/common/blob/blo...
storage/common/blob/blob_data.h:89: class STORAGE_COMMON_EXPORT BlobDataBuilder
{
On 2015/01/20 22:03:57, mmenke wrote:
> This file should be renamed...Or are you planning on doing that in a followup
> CL?

renamed and moved into /browser/storage too

[qinmin, 2015-01-21 18:21:09]

content/browser/media/android/ lgtm

[dmurph, 2015-01-21 22:40:10]

dmurph@chromium.org changed reviewers:
+ jam@chromium.org, kinuko@chromium.org, nasko@chromium.org

[dmurph, 2015-01-21 22:40:12]

Hello eveyone!

This is a refactor of the blob storage in the browser.
(https://bit.ly/AutoBlobToDisk)

+nasko@chromium.org for:
 content/browser/shareable_file_reference_unittest.c
 content/browser/storage_partition_impl_map.cc
 content/child/webblobregistry_impl.cc
 content/common/fileapi/webblob_messages.h
 content/public/test/mock_blob_url_request_context.cc
(These are purely rename changes.)

+kinuko@chromium.org for:
 chrome/browser/sync_file_system/*
 chrome/browser/media_galleries/fileapi/*
(These are purely rename changes.)

+jam@chromium.org for:
 chrome/browser/chromeos/*
 chrome/browser/extensions/api/*
(These are purely rename changes.)

Sorry for the large change, this is the file-rename portion that hits the most
external code. michaeln is the primary reviewer for the design/architecture.

https://codereview.chromium.org/810403004/diff/200001/content/browser/fileapi...
File content/browser/fileapi/blob_storage_host.h (right):

https://codereview.chromium.org/810403004/diff/200001/content/browser/fileapi...
content/browser/fileapi/blob_storage_host.h:32: class CONTENT_EXPORT
BlobStorageHost {
On 2015/01/21 01:46:41, michaeln wrote:
> not for this cl, but this file should probably in storage/browser/blob too

Acknowledged.

https://codereview.chromium.org/810403004/diff/200001/content/browser/fileapi...
File content/browser/fileapi/chrome_blob_storage_context.cc (right):

https://codereview.chromium.org/810403004/diff/200001/content/browser/fileapi...
content/browser/fileapi/chrome_blob_storage_context.cc:71:
scoped_ptr<storage::BlobDataBuilder> blob_data(
On 2015/01/21 01:46:41, michaeln wrote:
> maybe rename the local to blob_data_builder

Done.

https://codereview.chromium.org/810403004/diff/200001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/810403004/diff/200001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:229:
scoped_ptr<storage::BlobDataBuilder> blob_data(
On 2015/01/21 01:46:41, michaeln wrote:
> instead of heap allocaing it, could BlobDataBuilder be sitting on the stack?

Done.

https://codereview.chromium.org/810403004/diff/200001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:234: blob_data_handle =
context->AddFinishedBlob(*blob_data.get());
On 2015/01/21 01:46:41, michaeln wrote:
> so readers don't have to decrypt *smart.get() 

Done.

https://codereview.chromium.org/810403004/diff/200001/content/browser/loader/...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/810403004/diff/200001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:74: // TODO(dmurph): store
this snapshot.  This works for now, as the items
On 2015/01/20 22:03:57, mmenke wrote:
> nit:  Capitalize S

Done.

https://codereview.chromium.org/810403004/diff/200001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:75: // that the snapshot
points to are refcounted and kept in memory.  But as
On 2015/01/20 22:03:57, mmenke wrote:
> Should mention that they're attached to the request by the RDH.  (+1 to
cleaning
> that up - the current ownership model is weird and ugly)

Done.

https://codereview.chromium.org/810403004/diff/200001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:78: // decomposition.
On 2015/01/21 01:46:41, michaeln wrote:
> It would be really good to store snapshots in this CL so spilling over to disk
> isn't gated on a reader type.
> 
> I think if the snapshot class derived from base::SupportsUserData::Data, it
> could be attached to the ResourceRequestBody*, which SupportsUserData, passed
> into Build(). Having that class derive from userdata makes sense sense to me
> it's intended as a 'scoping' device for various operations (same reason it
makes
> sense for BDH to derived from that class).

Done.

https://codereview.chromium.org/810403004/diff/200001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/810403004/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:440:
scoped_ptr<storage::BlobDataBuilder> blob_data(
On 2015/01/21 01:46:41, michaeln wrote:
> i think BlobDataBuilder can be on the stack here too and
> might be nice to use blob_builder as a name for locals like this

Done.

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:91: for (const auto& blob_item :
data.items_) {
On 2015/01/21 01:46:41, michaeln wrote:
> nit {}'s not needed

Done.

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:126: auto* entry = found->second;
On 2015/01/21 01:46:41, michaeln wrote:
> Probably writing BlobMapEntry* instead of auto* would be more readable here
> since the definition of blob_map_ is not nearby.

Done.

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:143: auto* entry = found->second;
On 2015/01/21 01:46:41, michaeln wrote:
> ditto BlobMapEntry*

Done.

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:189: auto* entry =
blob_map_.find(item.blob_uuid())->second;\
On 2015/01/21 01:46:41, michaeln wrote:
> Ditto BlobMapEntry* vs auto*, and you probably didn't mean to have a trailing
\
> here.

Done.

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:220: auto* entry = found->second;
On 2015/01/21 01:46:41, michaeln wrote:
> ditto BlobMapEntry* vs auto*

Done.

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:264: auto& items =
src_blob_data.items();
On 2015/01/21 01:46:41, michaeln wrote:
> i'm wondering what type items is? 
> 
> In addition to local readability, another downside of auto is that grepping
srcs
> for usage of a data type doesn't work as well as it used to.

It's a long type, which is why I omitted it.  I put it in now (const
std::vector<scoped_refptr<BlobDataItem>>&)

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:349: return (found->second->flags &
BEING_BUILT) && found->second->data_builder;
On 2015/01/21 01:46:41, michaeln wrote:
> BEING_BUILT seems redundant now that we have the mutually exclusive data vs
> data_builder members

Sounds good.  I'll keep 'flags' around, as we'll probably use that to keep track
of 'IsMovingToDisk' or other states.

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.h:69: scoped_ptr<BlobDataBuilder>
data_builder;
On 2015/01/21 01:46:41, michaeln wrote:
> maybe comment that |data| and |data_builder| are mutually exclusive

Done.

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_url_request_job.cc:202: auto& items =
blob_data_->items();
On 2015/01/21 01:46:41, michaeln wrote:
> does const auto work here? i was wondering about the const'ness

Ah, yeah, sorry, that was a mistake.

https://codereview.chromium.org/810403004/diff/200001/storage/browser/blob/bl...
storage/browser/blob/blob_url_request_job.cc:206: const BlobDataItem& item =
*items.at(i);
On 2015/01/21 01:46:41, michaeln wrote:
> it's seems a little odd to have a & ref type to a refcounted data type

It avoids copying a the refptr, and it's pretty clear.  Also, the refptr is
staying alive.  I could do a "const scoped_refptr<BlobDataItem>&" so there isn't
the copy, but that seems less clear.

https://codereview.chromium.org/810403004/diff/200001/storage/common/blob/blo...
File storage/common/blob/blob_data.h (right):

https://codereview.chromium.org/810403004/diff/200001/storage/common/blob/blo...
storage/common/blob/blob_data.h:89: class STORAGE_COMMON_EXPORT BlobDataBuilder
{
On 2015/01/20 22:03:57, mmenke wrote:
> This file should be renamed...Or are you planning on doing that in a followup
> CL?

Done.

https://codereview.chromium.org/810403004/diff/200001/storage/common/blob/blo...
storage/common/blob/blob_data.h:89: class STORAGE_COMMON_EXPORT BlobDataBuilder
{
On 2015/01/21 01:46:41, michaeln wrote:
> On 2015/01/20 22:03:57, mmenke wrote:
> > This file should be renamed...Or are you planning on doing that in a
followup
> > CL?
> 
> renamed and moved into /browser/storage too

Done.

[mmenke, 2015-01-22 00:04:47]

https://codereview.chromium.org/810403004/diff/260001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/810403004/diff/260001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:381:
storage::BlobStorageContext* blob_context) {
Is this still needed, now that you're attaching the blobs directly to the
request?

[dmurph, 2015-01-22 00:18:29]

https://codereview.chromium.org/810403004/diff/260001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/810403004/diff/260001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:381:
storage::BlobStorageContext* blob_context) {
On 2015/01/22 00:04:47, mmenke wrote:
> Is this still needed, now that you're attaching the blobs directly to the
> request?

Yes.  The Handle and Snapshot are two different things.  A handle guarentees
that the blob will not be destroyed from the blob storage context (basically
keeps a refcount of the whole blob).  The Snapshot holds refcounted data
elements, which represent the backing 'items' of a blob (which can be memory or
file backed).  The storage context can swap these items interally between ram
and disk backed representations, so we need to hold refptrs of the data elements
wherever we want to do reading from a blob. (also, items can be in multiple
blobs). See the design doc here for more info:
https://bit.ly/AutoBlobToDisk

[michaeln, 2015-01-22 02:18:09]

https://codereview.chromium.org/810403004/diff/260001/content/browser/fileapi...
File content/browser/fileapi/chrome_blob_storage_context.cc (right):

https://codereview.chromium.org/810403004/diff/260001/content/browser/fileapi...
content/browser/fileapi/chrome_blob_storage_context.cc:72:
scoped_ptr<storage::BlobDataBuilder> blob_data_builder(
i think this could be on the stack too

https://codereview.chromium.org/810403004/diff/260001/content/browser/loader/...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/810403004/diff/260001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:89:
body->SetUserData(snapshot.get(), snapshot.release());
i'm don't think this is safe, release() can get called prior to get, elsewhere
this is done as 

    const void* key = bar.get();
    foo->SetUserData(key, bar.release());

https://codereview.chromium.org/810403004/diff/260001/storage/browser/blob/bl...
File storage/browser/blob/blob_data_item.h (right):

https://codereview.chromium.org/810403004/diff/260001/storage/browser/blob/bl...
storage/browser/blob/blob_data_item.h:24: : public
base::RefCountedThreadSafe<BlobDataItem> {
Hmmm, does this class need to be threadsafe? The ShareableFileRef class is not
threadsafe.

Until we have a usecase for iterating the items array on different threads, it
might be good to not declare this as threadsafe to avoid complications for
if/when BlobDataItems become more abstract to accomodate a things like a blobs
backed by data in a ServiceWorker Cache object.

https://codereview.chromium.org/810403004/diff/260001/storage/browser/blob/bl...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/810403004/diff/260001/storage/browser/blob/bl...
storage/browser/blob/blob_url_request_job.cc:262: auto& items =
blob_data_->items();
const auto here too and below? (not sure the locals are worth it unless it
avoids a line wrap somewhere)

[michaeln, 2015-01-22 02:26:34]

https://codereview.chromium.org/810403004/diff/260001/storage/browser/blob/bl...
File storage/browser/blob/blob_data_snapshot.h (right):

https://codereview.chromium.org/810403004/diff/260001/storage/browser/blob/bl...
storage/browser/blob/blob_data_snapshot.h:26: const std::string& uuid() const {
return uuid_; }
A snapshot may outlive the registered blob, so this uuid isn't guaranteed to be
good. It may make sense for this class to internally own BlobDataHandle ensure
the uuid doesn't go away either?

[kinuko, 2015-01-22 11:14:31]

>+kinuko@chromium.org for:
> chrome/browser/sync_file_system/*
> chrome/browser/media_galleries/fileapi/*

lgtm

[jam, 2015-01-22 17:20:07]

lgtm

[nasko, 2015-01-22 22:01:15]

LGTM on requested files:

 content/browser/shareable_file_reference_unittest.c
 content/browser/storage_partition_impl_map.cc
 content/child/webblobregistry_impl.cc
 content/common/fileapi/webblob_messages.h
 content/public/test/mock_blob_url_request_context.cc

https://codereview.chromium.org/810403004/diff/260001/content/browser/shareab...
File content/browser/shareable_file_reference_unittest.cc (right):

https://codereview.chromium.org/810403004/diff/260001/content/browser/shareab...
content/browser/shareable_file_reference_unittest.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2015, as this is a "new" file

[dmurph, 2015-01-23 00:10:19]

https://codereview.chromium.org/810403004/diff/260001/content/browser/loader/...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/810403004/diff/260001/content/browser/loader/...
content/browser/loader/upload_data_stream_builder.cc:89:
body->SetUserData(snapshot.get(), snapshot.release());
On 2015/01/22 02:18:09, michaeln wrote:
> i'm don't think this is safe, release() can get called prior to get, elsewhere
> this is done as 
> 
>     const void* key = bar.get();
>     foo->SetUserData(key, bar.release());

Done.

https://codereview.chromium.org/810403004/diff/260001/content/browser/shareab...
File content/browser/shareable_file_reference_unittest.cc (right):

https://codereview.chromium.org/810403004/diff/260001/content/browser/shareab...
content/browser/shareable_file_reference_unittest.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/01/22 22:01:15, nasko wrote:
> 2015, as this is a "new" file

Done.

https://codereview.chromium.org/810403004/diff/260001/storage/browser/blob/bl...
File storage/browser/blob/blob_data_item.h (right):

https://codereview.chromium.org/810403004/diff/260001/storage/browser/blob/bl...
storage/browser/blob/blob_data_item.h:24: : public
base::RefCountedThreadSafe<BlobDataItem> {
On 2015/01/22 02:18:09, michaeln wrote:
> Hmmm, does this class need to be threadsafe? The ShareableFileRef class is not
> threadsafe.
> 
> Until we have a usecase for iterating the items array on different threads, it
> might be good to not declare this as threadsafe to avoid complications for
> if/when BlobDataItems become more abstract to accomodate a things like a blobs
> backed by data in a ServiceWorker Cache object.

changed, and clarified in BlobDataHandle

https://codereview.chromium.org/810403004/diff/260001/storage/browser/blob/bl...
File storage/browser/blob/blob_data_snapshot.h (right):

https://codereview.chromium.org/810403004/diff/260001/storage/browser/blob/bl...
storage/browser/blob/blob_data_snapshot.h:26: const std::string& uuid() const {
return uuid_; }
On 2015/01/22 02:26:34, michaeln wrote:
> A snapshot may outlive the registered blob, so this uuid isn't guaranteed to
be
> good. It may make sense for this class to internally own BlobDataHandle ensure
> the uuid doesn't go away either?

As discussed, removed the uuid here, and clarified responsibilites.

https://codereview.chromium.org/810403004/diff/260001/storage/browser/blob/bl...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/810403004/diff/260001/storage/browser/blob/bl...
storage/browser/blob/blob_url_request_job.cc:262: auto& items =
blob_data_->items();
On 2015/01/22 02:18:09, michaeln wrote:
> const auto here too and below? (not sure the locals are worth it unless it
> avoids a line wrap somewhere)

Changed here and elsewhere.

[michaeln, 2015-01-23 01:21:46]

lgtm!

[dmurph, 2015-01-23 06:36:45]

The CQ bit was checked by dmurph@chromium.org

[commit-bot: I haz the power, 2015-01-23 06:37:44]

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

[commit-bot: I haz the power, 2015-01-23 06:45:53]

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

[commit-bot: I haz the power, 2015-01-23 06:45:54]

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

[dmurph, 2015-01-23 07:55:43]

The CQ bit was checked by dmurph@chromium.org

[commit-bot: I haz the power, 2015-01-23 07:55:58]

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

[commit-bot: I haz the power, 2015-01-23 09:19:30]

Committed patchset #20 (id:360001)

[commit-bot: I haz the power, 2015-01-23 09:20:34]

Patchset 20 (id:??) landed as
https://crrev.com/bff2e53fb3e210be45d264797036ec59005ebdd6
Cr-Commit-Position: refs/heads/master@{#312800}

[brucedawson, 2015-01-27 19:05:27]

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

[brucedawson, 2015-01-27 19:05:29]

https://codereview.chromium.org/810403004/diff/360001/storage/browser/blob/bl...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/810403004/diff/360001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:190: BlobMapEntry* entry =
blob_map_.find(item.blob_uuid())->second;
This line is confusing, and possibly a bug. The declaration of 'entry' shadows
the variable of the same name and type declared at the top of the function. It
is not clear whether this nested declaration was intentional.

Specifically, I'm not sure whether the setting of entry->flags below is supposed
to be affected by this clause.

I recommend either renaming the inner-scoped variable to avoid confusion, or not
declaring it at all if it is a bug.

This was found by /analyze on Windows,
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Windows%20Analyz...,
see bug 427616.

[dmurph, 2015-01-29 17:43:48]

https://codereview.chromium.org/810403004/diff/360001/storage/browser/blob/bl...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/810403004/diff/360001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:190: BlobMapEntry* entry =
blob_map_.find(item.blob_uuid())->second;
On 2015/01/27 19:05:29, brucedawson wrote:
> This line is confusing, and possibly a bug. The declaration of 'entry' shadows
> the variable of the same name and type declared at the top of the function. It
> is not clear whether this nested declaration was intentional.
> 
> Specifically, I'm not sure whether the setting of entry->flags below is
supposed
> to be affected by this clause.
> 
> I recommend either renaming the inner-scoped variable to avoid confusion, or
not
> declaring it at all if it is a bug.
> 
> This was found by /analyze on Windows,
>
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Windows%20Analyz...,
> see bug 427616.

Thankfully this isn't a bug, I'll rename the inner variable in my next patch. 
Thanks for pointing this out!