[Storage] Blob items are now shared between blobs. Ready for disk swap.

[Storage] Blob items are now shared between blobs. Ready for disk swap.
  * Added ShareableBlobDataItem for sharing items between blobs, and
    implemented blob item sharing.
  * Added InternalBlobData for the internal storage of blobs.
  * Removed CreateSnapshot ability from the old Builder, which is now
    exclusively used to tell the context to create a blob.
  * Fixed copying of the content_disposition header when
    programmatically creating a blob.
  * Fixed UMA histogram for files that have uint64max length value
    (which signifies 'end-of-file' without knowing the size), and
    added a histogram to track how often this happens.
  * Added more documentation and histograms for knowing the state of
    blobs on the web.

Design Doc: https://bit.ly/AutoBlobToDisk

R=michaeln
BUG=375297
Committed: https://crrev.com/8acaec3721c8f1b76004194be81612ee5ff08ecf
Cr-Commit-Position: refs/heads/master@{#315702}

[dmurph, 2015-02-04 02:15:05]

Hey Michael,

PTAL at the first draft of this patch.
Thanks!
Daniel

[michaeln, 2015-02-05 20:02:09]

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_da...
File storage/browser/blob/blob_data_builder.cc (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_da...
storage/browser/blob/blob_data_builder.cc:85: scoped_ptr<BlobDataBuilder>
BlobDataBuilder::Clone() {
Is there a use case for Clone()?

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_da...
storage/browser/blob/blob_data_builder.cc:94: new BlobDataItem(element.Pass(),
data_item->file_handle_));
Why make a new item instead of letting the new builder reference the same
collection of refcounted items? other->items_ = this->items_ ?

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_da...
File storage/browser/blob/blob_data_builder.h (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_da...
storage/browser/blob/blob_data_builder.h:27: // For builder-style use.
We generally don't use this pattern in chromium so it looked out of place to me.

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_da...
File storage/browser/blob/blob_data_item.h (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_da...
storage/browser/blob/blob_data_item.h:30: size_t offset() const { return
item_->offset(); }
Not sure size_t is the right type for offset & length? I think these can be
offsets into files so either int64 or uint64 would be a better fit.

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.cc:382: if (reusing_blob_item) {
woohooo!

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.cc:457:
std::transform(data.items_.begin(), data.items_.end(),
might be easier to read as a for (const auto& shareable_item : data.items_) 
snapshot_.items_.push_back(shareable_item->item(); loop

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.cc:459: [](const
scoped_refptr<ShareableBlobDataItem>& shareableItem) {
shareableItem s/b shareable_item

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.h:98: const storage::DataElement&
data_item);
storage:: not needed here

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/shareab...
File storage/browser/blob/shareable_blob_data_item.h (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/shareab...
storage/browser/blob/shareable_blob_data_item.h:36: virtual
~ShareableBlobDataItem();
does this need to be virtual?

[michaeln, 2015-02-05 22:58:48]

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.cc:102: if (entry->flags &
EXCEEDED_MEMORY)
not sure this test is needed since StartBuildingBlob can't result in an entry
with this flag set?

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.cc:167: if
(!CanFitDataElement(data_element)) {
it might make sense to hoist the one line body of the CanFit method inline into
the AppendBlobDataItem method body, its not used elsewhere and then it'd be real
easy to see just what it does

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.cc:172: TransformDataElement(uuid,
data_element);
maybe rename this method to AllocateBlobItem(id, element), you could use it in
the arg list in the line below

if (!AppendAllocatedBlobItem(uuid, target_blob_builder,
                             AllocateBlobItem(id, elem));

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.cc:299: size_t offset =
data_element.offset();
these can be len/offsets into files so int64 as the data type?

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.h:136: scoped_ptr<BlobDataSnapshot>
CreateSnapshot(const std::string& uuid,
just noticing this is a new CreateSnapshot overload, where is this used beside
the original CreateSnapshot method?

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/interna...
File storage/browser/blob/internal_blob_data.h (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/interna...
storage/browser/blob/internal_blob_data.h:1: // TODO: Insert description here.
(generated by dmurph)
copyright headers please

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/interna...
storage/browser/blob/internal_blob_data.h:22: class Builder {
It may be more clear to compose this differently? Either by derivation or
aggregation.

class InternalBD;
class MutableInternalBD : InternalBD;

class InternalBD;
class Builder {
  InternalBD* thing_being_built;
}

As coded InternalBlobData and its inner Builder as essentially the same, they
have the same data members and they have the same methods (with method bodies
being reused in a non-object oriented way in the .cc file). Also the two
different 'builder' classes tripped me up on my first read thru since both kinds
are used in certain methods (and a 2nd builder in this set of classes surprised
me).

[dmurph, 2015-02-06 01:32:31]

Hey Michael,

I tried to make this cleaner, please take a look.  I'm also now keeping track of
how many files blobs that are created with the 'unknown length' value. 
Histograms.xml change coming in the next patch (not next cl).

Daniel

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_da...
File storage/browser/blob/blob_data_builder.cc (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_da...
storage/browser/blob/blob_data_builder.cc:85: scoped_ptr<BlobDataBuilder>
BlobDataBuilder::Clone() {
Nope, removed.  Left over from having the context consume the builder.

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_da...
storage/browser/blob/blob_data_builder.cc:94: new BlobDataItem(element.Pass(),
data_item->file_handle_));
this was for a deep clone method, we don't need it anymore though.

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_da...
File storage/browser/blob/blob_data_builder.h (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_da...
storage/browser/blob/blob_data_builder.h:27: // For builder-style use.
Removed

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_da...
File storage/browser/blob/blob_data_item.h (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_da...
storage/browser/blob/blob_data_item.h:30: size_t offset() const { return
item_->offset(); }
Changed back to uint64

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.cc:102: if (entry->flags &
EXCEEDED_MEMORY)
True, removed.

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.cc:167: if
(!CanFitDataElement(data_element)) {
Done.

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.cc:172: TransformDataElement(uuid,
data_element);
Done.

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.cc:299: size_t offset =
data_element.offset();
Done

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.cc:382: if (reusing_blob_item) {
On 2015/02/05 at 20:02:09, michaeln wrote:
> woohooo!

:)

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.cc:457:
std::transform(data.items_.begin(), data.items_.end(),
Fine! :)

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.cc:459: [](const
scoped_refptr<ShareableBlobDataItem>& shareableItem) {
Done.

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.h:98: const storage::DataElement&
data_item);
Done

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/blob_st...
storage/browser/blob/blob_storage_context.h:136: scoped_ptr<BlobDataSnapshot>
CreateSnapshot(const std::string& uuid,
Inlined.

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/interna...
File storage/browser/blob/internal_blob_data.h (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/interna...
storage/browser/blob/internal_blob_data.h:1: // TODO: Insert description here.
(generated by dmurph)
Done.

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/interna...
storage/browser/blob/internal_blob_data.h:22: class Builder {
Done, let me know what you think.

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/shareab...
File storage/browser/blob/shareable_blob_data_item.h (right):

https://codereview.chromium.org/895933007/diff/1/storage/browser/blob/shareab...
storage/browser/blob/shareable_blob_data_item.h:36: virtual
~ShareableBlobDataItem();
Done.

[dmurph, 2015-02-06 21:57:17]

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

[dmurph, 2015-02-06 21:57:17]

+asvitkine for histogram.xml changes.

[michaeln, 2015-02-09 22:31:40]

lgtm!

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

https://codereview.chromium.org/895933007/diff/160001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:301: // 4) The Blob items are
expanded.
consider retaining the term 'Expand' in the method name that does this so the
comment and the corresonding code are more easily seen

[dmurph, 2015-02-10 01:30:20]

New patchsets have been uploaded after l-g-t-m from michaeln@chromium.org

[dmurph, 2015-02-10 01:36:31]

Hello Nasko,

Please take a look at the changes in these files:
 content/child/webblobregistry_impl.cc
 content/browser/loader/upload_data_stream_builder_unittest.cc
 content/public/test/mock_blob_url_request_context.cc

Thanks!
Daniel

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

https://codereview.chromium.org/895933007/diff/160001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.cc:301: // 4) The Blob items are
expanded.
On 2015/02/09 22:31:40, michaeln wrote:
> consider retaining the term 'Expand' in the method name that does this so the
> comment and the corresonding code are more easily seen

Done.

[dmurph, 2015-02-10 01:36:50]

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

[dmurph, 2015-02-10 01:36:51]

Hello Nasko,

Please take a look at the changes in these files:
 content/child/webblobregistry_impl.cc
 content/browser/loader/upload_data_stream_builder_unittest.cc
 content/public/test/mock_blob_url_request_context.cc

Thanks!
Daniel

[nasko, 2015-02-10 15:50:06]

content/child/webblobregistry_impl.cc
content/browser/loader/upload_data_stream_builder_unittest.cc
content/public/test/mock_blob_url_request_context.cc

LGTM

[Alexei Svitkine (slow), 2015-02-10 16:05:06]

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

https://codereview.chromium.org/895933007/diff/180001/content/browser/fileapi...
content/browser/fileapi/blob_storage_context_unittest.cc:152: {
Nit: Maybe make a helper function for this block.

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

https://codereview.chromium.org/895933007/diff/180001/content/browser/fileapi...
content/browser/fileapi/blob_url_request_job_unittest.cc:110: {  // Clean up for
ASAN
Nit: Inner scope is not necessary here.

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

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.h:123: InternalBlobData::Builder*
target_blob_data,
Modifiable parameters should be last per the style guide.

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/in...
File storage/browser/blob/internal_blob_data.h (right):

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/in...
storage/browser/blob/internal_blob_data.h:72: };
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/sh...
File storage/browser/blob/shareable_blob_data_item.h (right):

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/sh...
storage/browser/blob/shareable_blob_data_item.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
Nit: No (c) for new copyright messages - fix all the files being added.

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/sh...
storage/browser/blob/shareable_blob_data_item.h:14: namespace storage {
Nit: Add an empty line below this.

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/sh...
storage/browser/blob/shareable_blob_data_item.h:36: ~ShareableBlobDataItem();
Nit: Add an empty line below this.

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/sh...
storage/browser/blob/shareable_blob_data_item.h:43: }  // namespace storage
Nit: Add an empty line above.

https://codereview.chromium.org/895933007/diff/180001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/895933007/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:36914: +<histogram
name="Storage.Blob.ReusedItem" units="Boolean">
Please use enum="BooleanReused" (add a BooleanReused enum if there isn't one
already - e.g. similar to BooleanSuccess.

https://codereview.chromium.org/895933007/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:36917: +    True if we reuse an item for
a blob created from using Blob.slice.
Mention when this is logged, do so for all of these.

https://codereview.chromium.org/895933007/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:36960: +<histogram
name="Storage.BlobItemSize.File.Unknown" units="Boolean">
enum="BooleanUnknown", add it if it doesn't exist.

[dmurph, 2015-02-10 22:16:00]

New patchsets have been uploaded after l-g-t-m from nasko@chromium.org

[dmurph, 2015-02-10 22:17:47]

I added another histogram to show the distribution of unshared memory.

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

https://codereview.chromium.org/895933007/diff/180001/content/browser/fileapi...
content/browser/fileapi/blob_storage_context_unittest.cc:152: {
On 2015/02/10 at 16:05:05, Alexei Svitkine wrote:
> Nit: Maybe make a helper function for this block.

Done.

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

https://codereview.chromium.org/895933007/diff/180001/content/browser/fileapi...
content/browser/fileapi/blob_url_request_job_unittest.cc:110: {  // Clean up for
ASAN
On 2015/02/10 at 16:05:05, Alexei Svitkine wrote:
> Nit: Inner scope is not necessary here.

Done.

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

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_context.h:123: InternalBlobData::Builder*
target_blob_data,
On 2015/02/10 at 16:05:05, Alexei Svitkine wrote:
> Modifiable parameters should be last per the style guide.

Done.

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/in...
File storage/browser/blob/internal_blob_data.h (right):

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/in...
storage/browser/blob/internal_blob_data.h:72: };
On 2015/02/10 at 16:05:05, Alexei Svitkine wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/sh...
File storage/browser/blob/shareable_blob_data_item.h (right):

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/sh...
storage/browser/blob/shareable_blob_data_item.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/02/10 at 16:05:06, Alexei Svitkine wrote:
> Nit: No (c) for new copyright messages - fix all the files being added.

Done.

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/sh...
storage/browser/blob/shareable_blob_data_item.h:14: namespace storage {
On 2015/02/10 at 16:05:06, Alexei Svitkine wrote:
> Nit: Add an empty line below this.

Done.

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/sh...
storage/browser/blob/shareable_blob_data_item.h:36: ~ShareableBlobDataItem();
On 2015/02/10 at 16:05:05, Alexei Svitkine wrote:
> Nit: Add an empty line below this.

Done.

https://codereview.chromium.org/895933007/diff/180001/storage/browser/blob/sh...
storage/browser/blob/shareable_blob_data_item.h:43: }  // namespace storage
On 2015/02/10 at 16:05:06, Alexei Svitkine wrote:
> Nit: Add an empty line above.

Done.

https://codereview.chromium.org/895933007/diff/180001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/895933007/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:36914: +<histogram
name="Storage.Blob.ReusedItem" units="Boolean">
On 2015/02/10 at 16:05:06, Alexei Svitkine wrote:
> Please use enum="BooleanReused" (add a BooleanReused enum if there isn't one
already - e.g. similar to BooleanSuccess.

Done.

https://codereview.chromium.org/895933007/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:36917: +    True if we reuse an item for
a blob created from using Blob.slice.
On 2015/02/10 at 16:05:06, Alexei Svitkine wrote:
> Mention when this is logged, do so for all of these.

Done.

https://codereview.chromium.org/895933007/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:36960: +<histogram
name="Storage.BlobItemSize.File.Unknown" units="Boolean">
On 2015/02/10 at 16:05:06, Alexei Svitkine wrote:
> enum="BooleanUnknown", add it if it doesn't exist.

Done.

[Alexei Svitkine (slow), 2015-02-10 22:38:55]

lgtm

[dmurph, 2015-02-10 23:54:03]

The CQ bit was checked by dmurph@chromium.org

[commit-bot: I haz the power, 2015-02-10 23:54:47]

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

[commit-bot: I haz the power, 2015-02-11 01:23:32]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-02-11 01:23:56]

Patchset 11 (id:??) landed as
https://crrev.com/8acaec3721c8f1b76004194be81612ee5ff08ecf
Cr-Commit-Position: refs/heads/master@{#315702}