Chromium Blob hacking

Chromium Blob hacking

Start using BlobStorageContext (and family) as a replacement for BlobStorageController. The new class manages blob data by uuid and seperately maintains a mapping from of public blob urls to uuids.

Added a methods to create an additional mapping of old-style 'private urls' to new-style uuids to help with transitioning, blink needs to be retrofitted too, but that has to happen separately.

Changed blob processing classes to take BlobDataHandles as input instead of GURLs. Not long after IPC deserizliation, get a BlobDataHandle and pass that around. 
- FileSystemOperation.Write (and famlity) for FileWriter support 
- ResourceDispatcherHost (and famility) for blob url requests 
- (PostMessage handling and WebIntent plumbing ultimately needs to be treated in this way too) 


Switched to using string uuids instead of urls in IPC messages (and structs carried in ipc messages).
Until blink is retrofitted, the chromiums side supports both forms of identifying blobs (old-style private urls and new-style uuids).
- FileSystemHostMsg_Write 
- webkit_base::DataElement (and consumers: BlobData, ResourceRequestBody) 
- blob registry/building ipc messages 

Implemented the WebKit::WebBlobRegistryImpl such that it can be invoked on any renderer/worker thread. 

Got more explicit about naming: FileSystemURL vs BlobUUID vs PublicBlobURL.

Note: There are corresponding changes to be made in Blink, after which, the things annotated as deprecated in this CL can be removed.

TBR=inferno
BUG=174200
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220754

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221886

[michaeln, 2013-08-20 23:59:29]

Hi, this is mostly mind numbing changes to replace BlobStorageController with
BlobStorageContext and family. Maybe kinuko can take a look at the top half of
the files in the CL and eric can take a look at the bottom half as a way to
split the review up a little.

[kinuko, 2013-08-21 10:22:19]

Looking good. I'll take another look, but sending out first round comments

https://codereview.chromium.org/23223003/diff/43001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/23223003/diff/43001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:624: // TODO(before): actually
check the return values
Will you fix this TODO before submit or in another CL?

https://codereview.chromium.org/23223003/diff/43001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/23223003/diff/43001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:156: // transition period.
nit: can you add a link to the issue here?

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/fil...
File content/child/fileapi/file_system_dispatcher.h (right):

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/fil...
content/child/fileapi/file_system_dispatcher.h:109: const StatusCallback&
error_callback);
nit: indent looks weird

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/web...
File content/child/fileapi/webfilewriter_base.cc (right):

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/web...
content/child/fileapi/webfilewriter_base.cc:45: DCHECK(kCancelNotInProgress ==
cancel_state_);
nit: DCHECK_EQ would give better output

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
File webkit/browser/blob/blob_storage_host.cc (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_storage_host.cc:120: // 1. During
serialization/deserialization to 'clone' and existing blob.
nit: and -> an

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
File webkit/browser/blob/blob_url_request_job_factory.cc (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_url_request_job_factory.cc:22: return
reinterpret_cast<BlobDataHandle*>(
Doesn't static_cast work here??

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_url_request_job_factory.cc:25: }  // namespace
nit: can you insert empty lines after line 18 and before line 25?

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_url_request_job_factory.cc:31: // The request takes
ownership
nit: please end comments with period

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_operation_runner.cc (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_operation_runner.cc:233:
GURL("blob://see_user_data/"), writer_delegate.get()));
nit: This URL string is a bit distracting... can this be a constant or a helper
method to return URLRequest defined in blob_url_request_job_factory.h where
readers can easily find the comment about we don't look up data based on the
URL?

[michaeln, 2013-08-21 17:57:00]

thnx for looking, i'll take care of the nits in the next upload

https://codereview.chromium.org/23223003/diff/43001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/23223003/diff/43001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:624: // TODO(before): actually
check the return values
On 2013/08/21 10:22:20, kinuko wrote:
> Will you fix this TODO before submit or in another CL?

That's a good question, another CL.

I don't think I can make this checks just yet as a test for valid inputs.
Unfortunately, it's "expected" that in some cases the values provided will not
be found. I'll have to come back after blink has been updated and history/page
state serialization has been massaged in some way.

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_operation_runner.cc (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_operation_runner.cc:233:
GURL("blob://see_user_data/"), writer_delegate.get()));
On 2013/08/21 10:22:20, kinuko wrote:
> nit: This URL string is a bit distracting... can this be a constant or a
helper
> method to return URLRequest defined in blob_url_request_job_factory.h where
> readers can easily find the comment about we don't look up data based on the
> URL?

Yes, thanx! That's a really good idea, also gives me a place to put a TODO about
providing a BlobReader : public FileReader derivative so callsites like this
don't need to use URLRequest as the means of reading the bytes out.

[michaeln, 2013-08-21 21:43:00]

I also have to wade thru the try results?

[ericu, 2013-08-21 23:26:08]

https://codereview.chromium.org/23223003/diff/43001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/canned_syncable_file_system.cc
(right):

https://codereview.chromium.org/23223003/diff/43001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/canned_syncable_file_system.cc:648: const
std::string blob_id(std::string("blob:") + data);
What's this blob_id for?  Why are you putting the data in there?  That looks
wrong.

https://codereview.chromium.org/23223003/diff/43001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/23223003/diff/43001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:156: // transition period.
On 2013/08/21 10:22:20, kinuko wrote:
> nit: can you add a link to the issue here?
And perhaps a TODO(michaeln) just to let folks know who's responsible?

https://codereview.chromium.org/23223003/diff/43001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:243: // Keeps track of blobs
utilized in this process and cleans up
Nit: s/utilized/used ?

https://codereview.chromium.org/23223003/diff/43001/content/browser/loader/up...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/23223003/diff/43001/content/browser/loader/up...
content/browser/loader/upload_data_stream_builder.cc:86: // Append the elements
in the referred blob data.
s/referred/referenced/ ?

https://codereview.chromium.org/23223003/diff/43001/content/browser/loader/up...
content/browser/loader/upload_data_stream_builder.cc:95:
body->SetUserData(handle.get(), handle.release());
handle.release() could be called before handle.get() here; C++ explicitly
disavows any guarantee on parameter evaluation order.
Also, do we really want to use the handle as both key and value?
Where does it get cleaned up?

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/web...
File content/child/fileapi/webfilewriter_base_unittest.cc (right):

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/web...
content/child/fileapi/webfilewriter_base_unittest.cc:232: // TODO(michaeln):
update the tests to once webkit api is updated.
to what?

https://codereview.chromium.org/23223003/diff/43001/content/child/webblobregi...
File content/child/webblobregistry_impl.cc (left):

https://codereview.chromium.org/23223003/diff/43001/content/child/webblobregi...
content/child/webblobregistry_impl.cc:75: DCHECK(ChildThread::current());
Why remove this DCHECK?

https://codereview.chromium.org/23223003/diff/43001/content/child/webblobregi...
File content/child/webblobregistry_impl.cc (right):

https://codereview.chromium.org/23223003/diff/43001/content/child/webblobregi...
content/child/webblobregistry_impl.cc:165: sender_->Send(new
BlobHostMsg_DecrementRefCount(uuid));
Why decrement the refcount here?

https://codereview.chromium.org/23223003/diff/43001/content/child/webblobregi...
File content/child/webblobregistry_impl.h (right):

https://codereview.chromium.org/23223003/diff/43001/content/child/webblobregi...
content/child/webblobregistry_impl.h:35: // DEPRECATED, almost
OK, that almost is funny, but needs to have your name on it.

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
File webkit/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_storage_context.cc:236: return;
Should this be an error or a DCHECK(false)?

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_storage_context.cc:242: if
(deprecated_blob_urls_.find(url) == deprecated_blob_urls_.end())
Same here?

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
File webkit/browser/blob/blob_storage_host.cc (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_storage_host.cc:139: private_blob_urls_.insert(url);
Why insert the url into private_blob_urls here?

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
File webkit/browser/blob/blob_url_request_job_factory.cc (left):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_url_request_job_factory.cc:26:
DCHECK(file_system_context_.get());
Why remove these DCHECKs?

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
File webkit/browser/blob/blob_url_request_job_factory.cc (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_url_request_job_factory.cc:31: // The request takes
ownership
In this case, perhaps remove the comment?  It's pretty obvious that ownership of
blob_data_handle is passed, given that the function's only one line long.

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
File webkit/browser/blob/blob_url_request_job_factory.h (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_url_request_job_factory.h:36: // request by utilizing
this method.
Remove comment?  It's confusing, but once understood, seems trivial.

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/view_...
File webkit/browser/blob/view_blob_internals_job.cc (left):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/view_...
webkit/browser/blob/view_blob_internals_job.cc:143:
blob_storage_controller_->RemoveBlob(GURL(blob_url));
Can we no longer remove blobs, or what?

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/view_...
File webkit/browser/blob/view_blob_internals_job.cc (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/view_...
webkit/browser/blob/view_blob_internals_job.cc:221: 
Nit: extra whitespace?

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_operation_impl_write_unittest.cc
(right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_operation_impl_write_unittest.cc:207: 
Nit: remove extra line

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_operation_runner.cc (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_operation_runner.cc:9: #include
"webkit/browser/blob/blob_url_request_job_factory.h"
Is this needed?

[michaeln, 2013-08-22 22:32:34]

Did some local testing and fixing, but haven't responded to comments just yet.
Starting some new try jobs.

[michaeln, 2013-08-27 23:24:06]

Ptal. There's one test that i have to look more closely at,
DownloadDangerousBlobData.

https://codereview.chromium.org/23223003/diff/43001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/canned_syncable_file_system.cc
(right):

https://codereview.chromium.org/23223003/diff/43001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/canned_syncable_file_system.cc:648: const
std::string blob_id(std::string("blob:") + data);
On 2013/08/21 23:26:09, ericu wrote:
> What's this blob_id for?  Why are you putting the data in there?  That looks
> wrong.

Done.

https://codereview.chromium.org/23223003/diff/43001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/23223003/diff/43001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:156: // transition period.
On 2013/08/21 23:26:09, ericu wrote:
> On 2013/08/21 10:22:20, kinuko wrote:
> > nit: can you add a link to the issue here?
> And perhaps a TODO(michaeln) just to let folks know who's responsible?

Done.

https://codereview.chromium.org/23223003/diff/43001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:243: // Keeps track of blobs
utilized in this process and cleans up
On 2013/08/21 23:26:09, ericu wrote:
> Nit: s/utilized/used ?

Done.

https://codereview.chromium.org/23223003/diff/43001/content/browser/loader/up...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/23223003/diff/43001/content/browser/loader/up...
content/browser/loader/upload_data_stream_builder.cc:86: // Append the elements
in the referred blob data.
On 2013/08/21 23:26:09, ericu wrote:
> s/referred/referenced/ ?

Done.

https://codereview.chromium.org/23223003/diff/43001/content/browser/loader/up...
content/browser/loader/upload_data_stream_builder.cc:95:
body->SetUserData(handle.get(), handle.release());
On 2013/08/21 23:26:09, ericu wrote:
> handle.release() could be called before handle.get() here; C++ explicitly
> disavows any guarantee on parameter evaluation order.
> Also, do we really want to use the handle as both key and value?
> Where does it get cleaned up?

Yikes, thnx, changed things to not be dependent on param eval order.

The |body| owns this handle now. It will get cleaned up when body is deleted.
Multiple handles can be attached to the body in this way, using the ptr value as
the key ensures there are no collisions.

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/fil...
File content/child/fileapi/file_system_dispatcher.h (right):

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/fil...
content/child/fileapi/file_system_dispatcher.h:109: const StatusCallback&
error_callback);
On 2013/08/21 10:22:20, kinuko wrote:
> nit: indent looks weird

Done.

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/web...
File content/child/fileapi/webfilewriter_base.cc (right):

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/web...
content/child/fileapi/webfilewriter_base.cc:45: DCHECK(kCancelNotInProgress ==
cancel_state_);
On 2013/08/21 10:22:20, kinuko wrote:
> nit: DCHECK_EQ would give better output

Done.

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/web...
File content/child/fileapi/webfilewriter_base_unittest.cc (right):

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/web...
content/child/fileapi/webfilewriter_base_unittest.cc:232: // TODO(michaeln):
update the tests to once webkit api is updated.
> to what?
added a link to the bug

https://codereview.chromium.org/23223003/diff/43001/content/child/webblobregi...
File content/child/webblobregistry_impl.cc (left):

https://codereview.chromium.org/23223003/diff/43001/content/child/webblobregi...
content/child/webblobregistry_impl.cc:75: DCHECK(ChildThread::current());
On 2013/08/21 23:26:09, ericu wrote:
> Why remove this DCHECK?

Because soon, blink will be invoking these methods on background threads too.

https://codereview.chromium.org/23223003/diff/43001/content/child/webblobregi...
File content/child/webblobregistry_impl.cc (right):

https://codereview.chromium.org/23223003/diff/43001/content/child/webblobregi...
content/child/webblobregistry_impl.cc:165: sender_->Send(new
BlobHostMsg_DecrementRefCount(uuid));
On 2013/08/21 23:26:09, ericu wrote:
> Why decrement the refcount here?

BlobHostMsg_DeprecatedRegisterBlobURL implicitly adds a ref. That ref is
balanced with the eventual BlobHostMsg_DeprecatedRevokeBlobURL two methods down.

https://codereview.chromium.org/23223003/diff/43001/content/child/webblobregi...
File content/child/webblobregistry_impl.h (right):

https://codereview.chromium.org/23223003/diff/43001/content/child/webblobregi...
content/child/webblobregistry_impl.h:35: // DEPRECATED, almost
On 2013/08/21 23:26:09, ericu wrote:
> OK, that almost is funny, but needs to have your name on it.

Done.

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
File webkit/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_storage_context.cc:236: return;
On 2013/08/21 23:26:09, ericu wrote:
> Should this be an error or a DCHECK(false)?

Error checking/handling with the old BlobStorageController class is very lax.
These additions are here to support its old way of doing the bookkeeping. I
can't really make any assertions about what's an error vs whats expected in
them.

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
File webkit/browser/blob/blob_storage_host.cc (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_storage_host.cc:139: private_blob_urls_.insert(url);
On 2013/08/21 23:26:09, ericu wrote:
> Why insert the url into private_blob_urls here?

Done (duh)

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
File webkit/browser/blob/blob_url_request_job_factory.cc (left):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_url_request_job_factory.cc:26:
DCHECK(file_system_context_.get());
On 2013/08/21 23:26:09, ericu wrote:
> Why remove these DCHECKs?

Good question. Higher level code (storagepartition-ware) used to create a nested
ownership hierarchy of handlers. I've flattened that out because i saw no real
reason for 3 levels of indirection. In some test cases not all of these values
are provided. In the old world, a containing 'handler' shielded this class from
instantiation in those cases. Now that this handler is being instanced directly
it can get instanced w/o these values in those tests cases.

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
File webkit/browser/blob/blob_url_request_job_factory.cc (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_url_request_job_factory.cc:22: return
reinterpret_cast<BlobDataHandle*>(
On 2013/08/21 10:22:20, kinuko wrote:
> Doesn't static_cast work here??

Done.

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_url_request_job_factory.cc:25: }  // namespace
On 2013/08/21 10:22:20, kinuko wrote:
> nit: can you insert empty lines after line 18 and before line 25?

Done.

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_url_request_job_factory.cc:31: // The request takes
ownership
On 2013/08/21 23:26:09, ericu wrote:
> In this case, perhaps remove the comment?  It's pretty obvious that ownership
of
> blob_data_handle is passed, given that the function's only one line long.

Done.

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
File webkit/browser/blob/blob_url_request_job_factory.h (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_url_request_job_factory.h:36: // request by utilizing
this method.
On 2013/08/21 23:26:09, ericu wrote:
> Remove comment?  It's confusing, but once understood, seems trivial.

I think a comment that explains that this class does not lookup based on url is
helpful. I've reduced the comment to just that.

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/view_...
File webkit/browser/blob/view_blob_internals_job.cc (left):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/view_...
webkit/browser/blob/view_blob_internals_job.cc:143:
blob_storage_controller_->RemoveBlob(GURL(blob_url));
On 2013/08/21 23:26:09, ericu wrote:
> Can we no longer remove blobs, or what?

It's not really needed afaict and makes checking the validity of renderer
provided inputs a lot more difficult (suppose the user removes something a
renderer is still referring to).

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/view_...
File webkit/browser/blob/view_blob_internals_job.cc (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/view_...
webkit/browser/blob/view_blob_internals_job.cc:221: 
On 2013/08/21 23:26:09, ericu wrote:
> Nit: extra whitespace?

Done.

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_operation_runner.cc (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_operation_runner.cc:9: #include
"webkit/browser/blob/blob_url_request_job_factory.h"
> Is this needed?

yes, see line 234

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_operation_runner.cc:233:
GURL("blob://see_user_data/"), writer_delegate.get()));
On 2013/08/21 10:22:20, kinuko wrote:
> nit: This URL string is a bit distracting... can this be a constant or a
helper
> method to return URLRequest defined in blob_url_request_job_factory.h where
> readers can easily find the comment about we don't look up data based on the
> URL?

added a helper method to construct the URLRequest

[michaeln, 2013-08-28 01:27:36]

> There's one test that i have to look more closely at,
DownloadDangerousBlobData.

Done

[kinuko, 2013-08-28 17:24:31]

https://codereview.chromium.org/23223003/diff/101001/content/child/fileapi/we...
File content/child/fileapi/webfilewriter_impl.h (right):

https://codereview.chromium.org/23223003/diff/101001/content/child/fileapi/we...
content/child/fileapi/webfilewriter_impl.h:37: int64 offset) OVERRIDE;
nit: indent

https://codereview.chromium.org/23223003/diff/101001/webkit/browser/blob/blob...
File webkit/browser/blob/blob_storage_host.cc (right):

https://codereview.chromium.org/23223003/diff/101001/webkit/browser/blob/blob...
webkit/browser/blob/blob_storage_host.cc:139:
ignore_result(DecrementBlobRefCount(uuid));
I'm lost here... do we need to surround RegisterPublicBlobURL between increment
and decrement ref?  RegisterPublicBlobURL seems to increment ref, so we're doing
+1, +1 and then -1?

[michaeln, 2013-08-28 20:21:34]

https://codereview.chromium.org/23223003/diff/101001/webkit/browser/blob/blob...
File webkit/browser/blob/blob_storage_host.cc (right):

https://codereview.chromium.org/23223003/diff/101001/webkit/browser/blob/blob...
webkit/browser/blob/blob_storage_host.cc:139:
ignore_result(DecrementBlobRefCount(uuid));
On 2013/08/28 17:24:31, kinuko wrote:
> I'm lost here... do we need to surround RegisterPublicBlobURL between
increment and decrement ref?

Sorry for the confusion. Yes it is needed, but not for refcounting reasons.

The RegisterPublicBlobURL method has runtime checks to assert that the uuid
being mapped is actually "in use" in the invoking renderer. Those checks are not
valid with renderers still using the old deprecated methods. The extra
increment/decrement calls here work around that (the uuid will be "in use" for
the duration of the RegisterPublicBlobURL call).

[michaeln, 2013-08-28 20:48:02]

https://codereview.chromium.org/23223003/diff/101001/content/child/fileapi/we...
File content/child/fileapi/webfilewriter_impl.h (right):

https://codereview.chromium.org/23223003/diff/101001/content/child/fileapi/we...
content/child/fileapi/webfilewriter_impl.h:37: int64 offset) OVERRIDE;
On 2013/08/28 17:24:31, kinuko wrote:
> nit: indent

Done.

[ericu, 2013-08-28 22:01:27]

https://codereview.chromium.org/23223003/diff/43001/content/browser/loader/up...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/23223003/diff/43001/content/browser/loader/up...
content/browser/loader/upload_data_stream_builder.cc:86: // Append the elements
in the referred blob data.
On 2013/08/27 23:24:06, michaeln wrote:
> On 2013/08/21 23:26:09, ericu wrote:
> > s/referred/referenced/ ?
> 
> Done.

Nope.

https://codereview.chromium.org/23223003/diff/43001/content/browser/loader/up...
content/browser/loader/upload_data_stream_builder.cc:95:
body->SetUserData(handle.get(), handle.release());
On 2013/08/27 23:24:06, michaeln wrote:
> On 2013/08/21 23:26:09, ericu wrote:
> > handle.release() could be called before handle.get() here; C++ explicitly
> > disavows any guarantee on parameter evaluation order.
> > Also, do we really want to use the handle as both key and value?
> > Where does it get cleaned up?
> 
> Yikes, thnx, changed things to not be dependent on param eval order.
> 
> The |body| owns this handle now. It will get cleaned up when body is deleted.
> Multiple handles can be attached to the body in this way, using the ptr value
as
> the key ensures there are no collisions.

OK, so it's not going to be looked up by key later, just via an iterator?  If
so, that seems fine.

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/web...
File content/child/fileapi/webfilewriter_base_unittest.cc (right):

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/web...
content/child/fileapi/webfilewriter_base_unittest.cc:232: // TODO(michaeln):
update the tests to once webkit api is updated.
On 2013/08/27 23:24:06, michaeln wrote:
> > to what?
> added a link to the bug

Still "update the tests to"...to what?
Oh, do you mean "too"?

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
File webkit/browser/blob/blob_url_request_job_factory.h (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_url_request_job_factory.h:36: // request by utilizing
this method.
On 2013/08/27 23:24:06, michaeln wrote:
> On 2013/08/21 23:26:09, ericu wrote:
> > Remove comment?  It's confusing, but once understood, seems trivial.
> 
> I think a comment that explains that this class does not lookup based on url
is
> helpful. I've reduced the comment to just that.

How about "This class ignores the request's URL and uses the value given to
SetRequestedBlobDataHandle instead."?

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_operation_runner.cc (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_operation_runner.cc:233:
GURL("blob://see_user_data/"), writer_delegate.get()));
On 2013/08/27 23:24:06, michaeln wrote:
> On 2013/08/21 10:22:20, kinuko wrote:
> > nit: This URL string is a bit distracting... can this be a constant or a
> helper
> > method to return URLRequest defined in blob_url_request_job_factory.h where
> > readers can easily find the comment about we don't look up data based on the
> > URL?
> 
> added a helper method to construct the URLRequest

Much nicer!

https://codereview.chromium.org/23223003/diff/119001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/23223003/diff/119001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:516: 
Why does this now require a special case?
What did it do beforehand?

[michaeln, 2013-08-28 22:54:47]

https://codereview.chromium.org/23223003/diff/43001/content/browser/loader/up...
File content/browser/loader/upload_data_stream_builder.cc (right):

https://codereview.chromium.org/23223003/diff/43001/content/browser/loader/up...
content/browser/loader/upload_data_stream_builder.cc:86: // Append the elements
in the referred blob data.
On 2013/08/28 22:01:28, ericu wrote:
> On 2013/08/27 23:24:06, michaeln wrote:
> > On 2013/08/21 23:26:09, ericu wrote:
> > > s/referred/referenced/ ?
> > 
> > Done.
> 
> Nope.

done, really now :?)

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/web...
File content/child/fileapi/webfilewriter_base_unittest.cc (right):

https://codereview.chromium.org/23223003/diff/43001/content/child/fileapi/web...
content/child/fileapi/webfilewriter_base_unittest.cc:232: // TODO(michaeln):
update the tests to once webkit api is updated.
Yes, i meant "too" (or just botched the sentence). The write() method call that
takes a blobUrl as input is going away soon.

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
File webkit/browser/blob/blob_url_request_job_factory.h (right):

https://codereview.chromium.org/23223003/diff/43001/webkit/browser/blob/blob_...
webkit/browser/blob/blob_url_request_job_factory.h:36: // request by utilizing
this method.
> How about "This class ignores the request's URL and uses the value given to
> SetRequestedBlobDataHandle instead."?

Sgtm!

https://codereview.chromium.org/23223003/diff/119001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/23223003/diff/119001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:516: 
On 2013/08/28 22:01:28, ericu wrote:
> Why does this now require a special case?
> What did it do beforehand?

Good question.

This alternate code path for initiating a request is buggy in that it neglected
to ensure the requested blob is not deleted prior to the request actually
getting underway. BeginRequestInternal() can delay and defer starting real work.
In the interval, the blob url may be revoked and/or the blob data may be freed
up... bugs waiting to happen. The old code would also lookup by url if there was
no blob explicitly associated with a request, so it mostly/sometimes/kinda
worked.

Now that the BlobProtocolHandler never looksup by URL, this failed right off the
bat without the extra logic to associate the blob to the request.

Looks like other things are skipped in this alternate code path too, appcached
resources probably cannot be "downloaded" via this method. I haven't verified
that but it sure looks like it won't work.

[ericu, 2013-08-28 23:02:04]

lgtm

https://codereview.chromium.org/23223003/diff/119001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/23223003/diff/119001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:516: 
On 2013/08/28 22:54:47, michaeln wrote:
> On 2013/08/28 22:01:28, ericu wrote:
> > Why does this now require a special case?
> > What did it do beforehand?
> 
> Good question.
> 
> This alternate code path for initiating a request is buggy in that it
neglected
> to ensure the requested blob is not deleted prior to the request actually
> getting underway. BeginRequestInternal() can delay and defer starting real
work.
> In the interval, the blob url may be revoked and/or the blob data may be freed
> up... bugs waiting to happen. The old code would also lookup by url if there
was
> no blob explicitly associated with a request, so it mostly/sometimes/kinda
> worked.
> 
> Now that the BlobProtocolHandler never looksup by URL, this failed right off
the
> bat without the extra logic to associate the blob to the request.
> 
> Looks like other things are skipped in this alternate code path too, appcached
> resources probably cannot be "downloaded" via this method. I haven't verified
> that but it sure looks like it won't work.

As discussed, add a TODO or log a bug for this.

[michaeln, 2013-08-28 23:40:33]

+darin for content/ owners review

[kinuko, 2013-08-29 02:33:03]

lgtm2

https://codereview.chromium.org/23223003/diff/101001/webkit/browser/blob/blob...
File webkit/browser/blob/blob_storage_host.cc (right):

https://codereview.chromium.org/23223003/diff/101001/webkit/browser/blob/blob...
webkit/browser/blob/blob_storage_host.cc:139:
ignore_result(DecrementBlobRefCount(uuid));
On 2013/08/28 20:21:34, michaeln wrote:
> On 2013/08/28 17:24:31, kinuko wrote:
> > I'm lost here... do we need to surround RegisterPublicBlobURL between
> increment and decrement ref?
> 
> Sorry for the confusion. Yes it is needed, but not for refcounting reasons.
> 
> The RegisterPublicBlobURL method has runtime checks to assert that the uuid
> being mapped is actually "in use" in the invoking renderer. Those checks are
not
> valid with renderers still using the old deprecated methods. The extra
> increment/decrement calls here work around that (the uuid will be "in use" for
> the duration of the RegisterPublicBlobURL call).

I see, thanks for the explanation. Could you note a brief comment about that? 
I'm afraid I'll be confused and will bother you again

[michaeln, 2013-08-29 21:30:57]

On 2013/08/29 02:33:03, kinuko wrote:
> lgtm2
> 
>
https://codereview.chromium.org/23223003/diff/101001/webkit/browser/blob/blob...
> File webkit/browser/blob/blob_storage_host.cc (right):
> 
>
https://codereview.chromium.org/23223003/diff/101001/webkit/browser/blob/blob...
> webkit/browser/blob/blob_storage_host.cc:139:
> ignore_result(DecrementBlobRefCount(uuid));
> On 2013/08/28 20:21:34, michaeln wrote:
> > On 2013/08/28 17:24:31, kinuko wrote:
> > > I'm lost here... do we need to surround RegisterPublicBlobURL between
> > increment and decrement ref?
> > 
> > Sorry for the confusion. Yes it is needed, but not for refcounting reasons.
> > 
> > The RegisterPublicBlobURL method has runtime checks to assert that the uuid
> > being mapped is actually "in use" in the invoking renderer. Those checks are
> not
> > valid with renderers still using the old deprecated methods. The extra
> > increment/decrement calls here work around that (the uuid will be "in use"
for
> > the duration of the RegisterPublicBlobURL call).
> 
> I see, thanks for the explanation. Could you note a brief comment about that? 
> I'm afraid I'll be confused and will bother you again

I'll add a short comment...
// Temporarily bump the refcount so the uuid passes the InUse
// check inside the RegisterPublicBlobURL method.

It is confusing. I don't want to leave these two parallel sets of ipc messages
in place for too long but I need to slog thru the blink changes before the
deprecated code paths can be yanked.

[michaeln, 2013-08-30 01:10:34]

The next small blink CL is on deck... https://codereview.chromium.org/23703003/

[darin (slow to review), 2013-08-30 19:55:32]

LGTM

https://codereview.chromium.org/23223003/diff/149001/content/browser/net/view...
File content/browser/net/view_blob_internals_job_factory.cc (right):

https://codereview.chromium.org/23223003/diff/149001/content/browser/net/view...
content/browser/net/view_blob_internals_job_factory.cc:16: return
url.SchemeIs(chrome::kChromeUIScheme) &&
nit: indentation

https://codereview.chromium.org/23223003/diff/149001/content/browser/net/view...
content/browser/net/view_blob_internals_job_factory.cc:25: return new
webkit_blob::ViewBlobInternalsJob(
nit: indentation

https://codereview.chromium.org/23223003/diff/149001/webkit/browser/blob/blob...
File webkit/browser/blob/blob_storage_host.cc (right):

https://codereview.chromium.org/23223003/diff/149001/webkit/browser/blob/blob...
webkit/browser/blob/blob_storage_host.cc:107: bool IsPrivateBlobURL(const GURL&
url) {
nit: I usually recommend putting small helper functions like this at the top of
the file so as not to break up the flow of class methods.

[michaeln, 2013-08-30 21:08:43]

thnx!

https://codereview.chromium.org/23223003/diff/149001/content/browser/net/view...
File content/browser/net/view_blob_internals_job_factory.cc (right):

https://codereview.chromium.org/23223003/diff/149001/content/browser/net/view...
content/browser/net/view_blob_internals_job_factory.cc:16: return
url.SchemeIs(chrome::kChromeUIScheme) &&
On 2013/08/30 19:55:32, darin wrote:
> nit: indentation

Done.

https://codereview.chromium.org/23223003/diff/149001/content/browser/net/view...
content/browser/net/view_blob_internals_job_factory.cc:25: return new
webkit_blob::ViewBlobInternalsJob(
On 2013/08/30 19:55:32, darin wrote:
> nit: indentation

Done.

https://codereview.chromium.org/23223003/diff/149001/webkit/browser/blob/blob...
File webkit/browser/blob/blob_storage_host.cc (right):

https://codereview.chromium.org/23223003/diff/149001/webkit/browser/blob/blob...
webkit/browser/blob/blob_storage_host.cc:107: bool IsPrivateBlobURL(const GURL&
url) {
On 2013/08/30 19:55:32, darin wrote:
> nit: I usually recommend putting small helper functions like this at the top
of
> the file so as not to break up the flow of class methods.

i usually like that too, but i want to delete this particular helper when the
following too deprecated (at birth) methods are removed. having it in my face
right here will help to remember to do that.

[commit-bot: I haz the power, 2013-08-30 21:56:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/23223003/163001

[commit-bot: I haz the power, 2013-09-01 23:06:09]

Change committed as 220754

[michaeln, 2013-09-06 22:25:03]

oh well... just today discovered that it was reverted shortly after having
landed.

i'm not a fan of the commit queue. i want to be around when my changes are being
built and tested for real.

[commit-bot: I haz the power, 2013-09-07 01:22:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/23223003/85001

[commit-bot: I haz the power, 2013-09-07 04:20:26]

Change committed as 221886