Blobs: Mechanism for creating Blobs in browser process, then transferring to renderer.

Blobs: Mechanism for creating Blobs in browser process, then transferring to renderer.

This is in support of https://codereview.chromium.org/250143002, so see the comments there for motivation.

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

[tommycli, 2014-05-07 21:35:58]

michaeln: This patch is a spinoff of all the Blob-passing logic we discussed in
the attached-image extraction patch.

See https://codereview.chromium.org/250143002

The latest patchset of that patch shows using this code 'in-action'. I've
verified that the Blob is correctly held and released using log messages and
manual testing.

[michaeln, 2014-05-08 21:11:56]

this is looking pretty good

https://codereview.chromium.org/266373006/diff/90001/content/public/browser/f...
File content/public/browser/fileapi/blob_context.h (right):

https://codereview.chromium.org/266373006/diff/90001/content/public/browser/f...
content/public/browser/fileapi/blob_context.h:6: #define
CONTENT_PUBLIC_BROWSER_FILEAPI_BLOB_CONTEXT_H_
It may make more sense to put this in public/browser directly. The only reason
the impl is in fileapi is because of the close relationship betweek Blobs and
Files in javascript land. That doesn't matter at the content api layer.

https://codereview.chromium.org/266373006/diff/90001/content/public/browser/f...
content/public/browser/fileapi/blob_context.h:11: namespace webkit_blob {
I think we'll also need a content api wrapper for the
webkit_blob::BlodDataHandle class. Everything under /webkit is being migrated to
/content, that class is an impl detail of the content lib and shouldn't be
directly accessed by /chrome.

[tommycli, 2014-05-08 22:43:02]

michaeln: Thanks! Here's some changes.

https://codereview.chromium.org/266373006/diff/90001/content/public/browser/f...
File content/public/browser/fileapi/blob_context.h (right):

https://codereview.chromium.org/266373006/diff/90001/content/public/browser/f...
content/public/browser/fileapi/blob_context.h:6: #define
CONTENT_PUBLIC_BROWSER_FILEAPI_BLOB_CONTEXT_H_
On 2014/05/08 21:11:57, michaeln wrote:
> It may make more sense to put this in public/browser directly. The only reason
> the impl is in fileapi is because of the close relationship betweek Blobs and
> Files in javascript land. That doesn't matter at the content api layer.

Done.

https://codereview.chromium.org/266373006/diff/90001/content/public/browser/f...
content/public/browser/fileapi/blob_context.h:11: namespace webkit_blob {
On 2014/05/08 21:11:57, michaeln wrote:
> I think we'll also need a content api wrapper for the
> webkit_blob::BlodDataHandle class. Everything under /webkit is being migrated
to
> /content, that class is an impl detail of the content lib and shouldn't be
> directly accessed by /chrome.

Done.

[michaeln, 2014-05-08 23:03:18]

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

https://codereview.chromium.org/266373006/diff/150001/content/browser/fileapi...
content/browser/fileapi/chrome_blob_storage_context.cc:19: class
ChromeBlobHandle : public BlobHandle {
The ChromeXXX naming convention is really archaic and doesn't fit at all
anymore. Lets call this BlobHandleImpl or something like that. wdyt?

Also this class should be in an annon namespace.

https://codereview.chromium.org/266373006/diff/150001/content/public/browser/...
File content/public/browser/blob_context.h (right):

https://codereview.chromium.org/266373006/diff/150001/content/public/browser/...
content/public/browser/blob_context.h:26: class CONTENT_EXPORT BlobContext {
i'm not sure when CONTENT_EXPORT is needed and when it's not on these
interfaces. i see a mix of usage in the /public/browser directory. if it's not
needed definitely prefer to not use the macro.

[tommycli, 2014-05-09 16:10:17]

michaeln: Addressed your comments and associated the Blobs with the lifetime of
the RenderViewHosts instead of the WebContents.

This should be more tightly scoped and handle cases like page navigation better.

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

https://codereview.chromium.org/266373006/diff/150001/content/browser/fileapi...
content/browser/fileapi/chrome_blob_storage_context.cc:19: class
ChromeBlobHandle : public BlobHandle {
On 2014/05/08 23:03:18, michaeln wrote:
> The ChromeXXX naming convention is really archaic and doesn't fit at all
> anymore. Lets call this BlobHandleImpl or something like that. wdyt?
> 
> Also this class should be in an annon namespace.

Done.

https://codereview.chromium.org/266373006/diff/150001/content/public/browser/...
File content/public/browser/blob_context.h (right):

https://codereview.chromium.org/266373006/diff/150001/content/public/browser/...
content/public/browser/blob_context.h:26: class CONTENT_EXPORT BlobContext {
On 2014/05/08 23:03:18, michaeln wrote:
> i'm not sure when CONTENT_EXPORT is needed and when it's not on these
> interfaces. i see a mix of usage in the /public/browser directory. if it's not
> needed definitely prefer to not use the macro.

Done.

[tommycli, 2014-05-12 18:59:27]

michaeln: ping

+jam, since this has substantial content/ portion.

[jam, 2014-05-12 22:35:02]

https://codereview.chromium.org/266373006/diff/190001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/266373006/diff/190001/content/public/browser/...
content/public/browser/browser_context.h:82: static content::BlobContext*
GetBlobContext(BrowserContext* browser_context);
why expose this "blob context" concept to embedders? all you need is one method
on this class. same for BlobHandle, just use string

[michaeln, 2014-05-12 22:43:16]

The blob impl parts of this look good and I think the new public api being
presented is also good.

The extension specific changes is this CL might be better off in the other CL.

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

https://codereview.chromium.org/266373006/diff/190001/content/browser/fileapi...
content/browser/fileapi/chrome_blob_storage_context.cc:90: new
BlobHandleImpl(context_->AddFinishedBlob(blob_data.get()).Pass()));
AddFinishedBlob can return NULL is some cases, i think the public content API
method probably should when that happens as case well.

[tommycli, 2014-05-12 23:35:04]

michaeln, jam: Addressed your comments.

I also stripped out the extension-specific stuff, and will put that in a
separate CL. Thanks!

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

https://codereview.chromium.org/266373006/diff/190001/content/browser/fileapi...
content/browser/fileapi/chrome_blob_storage_context.cc:90: new
BlobHandleImpl(context_->AddFinishedBlob(blob_data.get()).Pass()));
On 2014/05/12 22:43:17, michaeln wrote:
> AddFinishedBlob can return NULL is some cases, i think the public content API
> method probably should when that happens as case well.

Done.

https://codereview.chromium.org/266373006/diff/190001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/266373006/diff/190001/content/public/browser/...
content/public/browser/browser_context.h:82: static content::BlobContext*
GetBlobContext(BrowserContext* browser_context);
On 2014/05/12 22:35:03, jam wrote:
> why expose this "blob context" concept to embedders? all you need is one
method
> on this class. same for BlobHandle, just use string

BlobHandle's content-private implementing class (See
chrome_blob_storage_context.cc) holds a reference to the underlying
BlobDataHandle and thus controls its lifetime. I've updated the comments to
BlobHandle.

As to why BlobContext is a separate class? See
https://codereview.chromium.org/250143002/ comments for discussion leading to
current design.

I suspect we didn't want to put add a CreateMemoryBackedBlob method in every
class that implements the BrowserContext interface. And - we might add more Blob
methods in the future.

[jam, 2014-05-13 02:44:23]

https://codereview.chromium.org/266373006/diff/190001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/266373006/diff/190001/content/public/browser/...
content/public/browser/browser_context.h:82: static content::BlobContext*
GetBlobContext(BrowserContext* browser_context);
On 2014/05/12 23:35:05, tommycli wrote:
> On 2014/05/12 22:35:03, jam wrote:
> > why expose this "blob context" concept to embedders? all you need is one
> method
> > on this class. same for BlobHandle, just use string
> 
> BlobHandle's content-private implementing class (See
> chrome_blob_storage_context.cc) holds a reference to the underlying
> BlobDataHandle and thus controls its lifetime. I've updated the comments to
> BlobHandle.

ah, I missed that sorry. ok please put this in a separate file per
http://www.chromium.org/developers/content-module/content-api. also get rid of
the DISALLOW_COPY_AND_ASSIGN as an abstract class can't be copied.
> 
> As to why BlobContext is a separate class? See
> https://codereview.chromium.org/250143002/ comments for discussion leading to
> current design.
> 
> I suspect we didn't want to put add a CreateMemoryBackedBlob method in every
> class that implements the BrowserContext interface. And - we might add more
Blob
> methods in the future.

I haven't read through all the comments in that link; but I don't understand
your sentence above. how is a static method that returns a class which has a
method foo different from having foo be a static method directly? the one
implementation would still be in content

[michaeln, 2014-05-13 03:34:43]

> I haven't read through all the comments in that link; but I don't understand
> your sentence above. how is a static method that returns a class which has a
> method foo different from having foo be a static method directly? the one
> implementation would still be in content

They're different at test time, a static method is more difficult to mock up
than a virtual method of an abstract class.

I'm pretty sure we'll want to add at least one more method to
CreateFileBackedBlob() and probably a introduce an abstrct BlobReader class
which takes a blob handle as input, so a factory method for that class too. Flip
a coin, 3 static methods vs an abstract class with 3 virtual methods. The coin I
flipped came down on the side of the abstract class.

[tommycli, 2014-05-13 18:27:42]

jam: moved BlobHandle to a separate file. Thanks!

https://codereview.chromium.org/266373006/diff/190001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/266373006/diff/190001/content/public/browser/...
content/public/browser/browser_context.h:82: static content::BlobContext*
GetBlobContext(BrowserContext* browser_context);
On 2014/05/13 02:44:23, jam wrote:
> On 2014/05/12 23:35:05, tommycli wrote:
> > On 2014/05/12 22:35:03, jam wrote:
> > > why expose this "blob context" concept to embedders? all you need is one
> > method
> > > on this class. same for BlobHandle, just use string
> > 
> > BlobHandle's content-private implementing class (See
> > chrome_blob_storage_context.cc) holds a reference to the underlying
> > BlobDataHandle and thus controls its lifetime. I've updated the comments to
> > BlobHandle.
> 
> ah, I missed that sorry. ok please put this in a separate file per
> http://www.chromium.org/developers/content-module/content-api. also get rid of
> the DISALLOW_COPY_AND_ASSIGN as an abstract class can't be copied.
> > 
> > As to why BlobContext is a separate class? See
> > https://codereview.chromium.org/250143002/ comments for discussion leading
to
> > current design.
> > 
> > I suspect we didn't want to put add a CreateMemoryBackedBlob method in every
> > class that implements the BrowserContext interface. And - we might add more
> Blob
> > methods in the future.
> 
> I haven't read through all the comments in that link; but I don't understand
> your sentence above. how is a static method that returns a class which has a
> method foo different from having foo be a static method directly? the one
> implementation would still be in content

Done.

[michaeln, 2014-05-13 20:59:03]

This lgtm. If jam would rather the CreateMemoryBackedBlob be a static method or
instance method on BrowserContext, that would be fine by me too.

[jam, 2014-05-13 21:23:26]

On 2014/05/13 03:34:43, michaeln wrote:
> > I haven't read through all the comments in that link; but I don't understand
> > your sentence above. how is a static method that returns a class which has a
> > method foo different from having foo be a static method directly? the one
> > implementation would still be in content
> 
> They're different at test time, a static method is more difficult to mock up
> than a virtual method of an abstract class.
> 
> I'm pretty sure we'll want to add at least one more method to
> CreateFileBackedBlob() and probably a introduce an abstrct BlobReader class
> which takes a blob handle as input, so a factory method for that class too.
Flip
> a coin, 3 static methods vs an abstract class with 3 virtual methods. The coin
I
> flipped came down on the side of the abstract class.

re BlobReader, I imagine that would be a separate class in
content/public/browser/blob_reader.h with its own factory, so i think it doesn't
concern this api discussion. regarding CreateFileBackedBlob, it's fine to add
more (static) methods; the arguments should be the same whether there are one or
two imo. so then it comes down to testing: in general for content api, i prefer
waiting to see an actual use case before adding interfaces. more importantly
though, it seems that the tests should be inside content, and so whatever
mechanism is used to mock this method should not be exposed in the content api,
as this BlobContext class does. that would argue for having a static fortesting
method inside content/browser which the tests call.


On 2014/05/13 20:59:03, michaeln wrote:
> This lgtm. If jam would rather the CreateMemoryBackedBlob be a static method
or
> instance method on BrowserContext, that would be fine by me too.

the convention (i agree it's confusing, we probably need to rejigger this to
make it less so) is that the interface methods are implemented by embedders,
while the static ones are in content. so this would be a static method

[jam, 2014-05-13 21:24:08]

https://codereview.chromium.org/266373006/diff/170002/content/public/browser/...
File content/public/browser/blob_handle.h (right):

https://codereview.chromium.org/266373006/diff/170002/content/public/browser/...
content/public/browser/blob_handle.h:17: virtual std::string uuid() = 0;
nit: per style guide, this should be GetUUID. also please document.

[michaeln, 2014-05-13 22:04:51]

having this be a static BrowserContext::CreateFileBackedBlob method sgtm2

> re BlobReader, I imagine that would be a separate class in
> content/public/browser/blob_reader.h with its own factory, so i think it
doesn't
> concern this api discussion.

true

> regarding CreateFileBackedBlob, it's fine to add
> more (static) methods; the arguments should be the same whether there are one
or
> two imo.

ok

> so then it comes down to testing: in general for content api, i prefer
> waiting to see an actual use case before adding interfaces. more importantly
> though, it seems that the tests should be inside content

I was referring to unittesting the extension system code that calls the
CreateMemoryBackedBlob method, but we have TestingProfile classes to help with
the setup for stuff like that.

> the convention (i agree it's confusing, we probably need to rejigger this to
> make it less so) is that the interface methods are implemented by embedders,
> while the static ones are in content. so this would be a static method

That makes sense although we have lots of classes that don't follow that pattern
so i wasn't aware of that convention.
DOMStorageContext, IndexedDBContext, StoragePartition, StreamHandle, others.

[tommycli, 2014-05-14 00:15:28]

jam: Removed BlobContext and rejiggered things per your comments. thx

https://codereview.chromium.org/266373006/diff/170002/content/public/browser/...
File content/public/browser/blob_handle.h (right):

https://codereview.chromium.org/266373006/diff/170002/content/public/browser/...
content/public/browser/blob_handle.h:17: virtual std::string uuid() = 0;
On 2014/05/13 21:24:08, jam wrote:
> nit: per style guide, this should be GetUUID. also please document.

Done.

[jam, 2014-05-14 17:14:00]

lgtm

https://codereview.chromium.org/266373006/diff/320001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/266373006/diff/320001/content/public/browser/...
content/public/browser/browser_context.h:49: typedef
base::Callback<void(scoped_ptr<BlobHandle>)> BlobCallback;
nit: convention in content api is to put the callback right above the method
that uses it, so move this to line 83

[tommycli, 2014-05-14 18:07:04]

jam: thanks!

https://codereview.chromium.org/266373006/diff/320001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/266373006/diff/320001/content/public/browser/...
content/public/browser/browser_context.h:49: typedef
base::Callback<void(scoped_ptr<BlobHandle>)> BlobCallback;
On 2014/05/14 17:14:00, jam wrote:
> nit: convention in content api is to put the callback right above the method
> that uses it, so move this to line 83

Done.

[tommycli, 2014-05-14 18:07:11]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-05-14 18:07:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/266373006/340001

[commit-bot: I haz the power, 2014-05-14 21:01:59]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-14 21:18:48]

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

[commit-bot: I haz the power, 2014-05-14 21:18:49]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[tommycli, 2014-05-14 21:34:44]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-05-14 21:36:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/266373006/340001

[commit-bot: I haz the power, 2014-05-14 22:37:34]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-14 22:56:17]

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

[commit-bot: I haz the power, 2014-05-14 22:56:18]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[tommycli, 2014-05-14 23:55:59]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-05-14 23:56:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/266373006/340001

[commit-bot: I haz the power, 2014-05-15 00:55:16]

Change committed as 270549