[Blob] Dependencies for blob storage testing

[Blob] Dependencies for blob storage testing

* Added a way to populate the memory in a WebThreadSafeData
  for testing support in content/
* Added new blob registration method using the Builder pattern in the WebBlobRegistry class.

BUG=375297
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197142

[dmurph, 2015-06-02 17:58:31]

dmurph@chromium.org changed reviewers:
+ jochen@chromium.org, jsbell@chromium.org

[dmurph, 2015-06-02 17:58:32]

Hey Jochen & Josh

Can you please take a quick look at this CL?  It's a webkit patch I need to
finish creating the renderer side of my blob work.  This is specifically for
testing.

Note: WebThreadSafeData is only used in our blob system, so there isn't much
side-effect here.

Thanks!
Daniel

patch gif:
http://i.imgur.com/r13NAse.gif

[jsbell, 2015-06-02 19:30:56]

https://codereview.chromium.org/1162773003/diff/1/Source/platform/exported/We...
File Source/platform/exported/WebThreadSafeData.cpp (right):

https://codereview.chromium.org/1162773003/diff/1/Source/platform/exported/We...
Source/platform/exported/WebThreadSafeData.cpp:51:
raw->mutableData()->appendRange(data, data + length);
could also just be: append(data, length)

https://codereview.chromium.org/1162773003/diff/1/Source/platform/exported/We...
Source/platform/exported/WebThreadSafeData.cpp:52: m_private =
WebPrivatePtr<RawData>(raw.release().leakRef());
m_private = static_cast<RawData*>( ... ) seems to be more common

https://codereview.chromium.org/1162773003/diff/1/public/platform/WebBlobData.h
File public/platform/WebBlobData.h (right):

https://codereview.chromium.org/1162773003/diff/1/public/platform/WebBlobData...
public/platform/WebBlobData.h:51: enum Type { TypeData, TypeFile, TypeBlob,
TypeFileSystemURL } type;
Why introduce an enum name here?

https://codereview.chromium.org/1162773003/diff/1/public/platform/WebBlobData...
public/platform/WebBlobData.h:73: // the same index will result in null data.
Maybe amend with a TODO to rework the API?

https://codereview.chromium.org/1162773003/diff/1/public/platform/WebThreadSa...
File public/platform/WebThreadSafeData.h (right):

https://codereview.chromium.org/1162773003/diff/1/public/platform/WebThreadSa...
public/platform/WebThreadSafeData.h:53: // For testing use only. This allows us
to populate this object in content/
Nit: probably shouldn't refer to content/ as blink does have other embedders.
I'd just keep the first sentence.

https://codereview.chromium.org/1162773003/diff/1/public/platform/WebThreadSa...
public/platform/WebThreadSafeData.h:54: BLINK_PLATFORM_EXPORT void assign(char*
data, size_t length);
const char*

[dmurph, 2015-06-02 20:17:41]

https://codereview.chromium.org/1162773003/diff/1/Source/platform/exported/We...
File Source/platform/exported/WebThreadSafeData.cpp (right):

https://codereview.chromium.org/1162773003/diff/1/Source/platform/exported/We...
Source/platform/exported/WebThreadSafeData.cpp:51:
raw->mutableData()->appendRange(data, data + length);
On 2015/06/02 at 19:30:55, jsbell wrote:
> could also just be: append(data, length)

Looks better, done.

https://codereview.chromium.org/1162773003/diff/1/Source/platform/exported/We...
Source/platform/exported/WebThreadSafeData.cpp:52: m_private =
WebPrivatePtr<RawData>(raw.release().leakRef());
On 2015/06/02 at 19:30:55, jsbell wrote:
> m_private = static_cast<RawData*>( ... ) seems to be more common

I'm confused, I don't see how this would work.  I can change the order so I'm
not creating a temporary variable though.

Note that .leakRef() is how we populate the m_private field below.

https://codereview.chromium.org/1162773003/diff/1/public/platform/WebBlobData.h
File public/platform/WebBlobData.h (right):

https://codereview.chromium.org/1162773003/diff/1/public/platform/WebBlobData...
public/platform/WebBlobData.h:51: enum Type { TypeData, TypeFile, TypeBlob,
TypeFileSystemURL } type;
On 2015/06/02 19:30:56, jsbell wrote:
> Why introduce an enum name here?

I want to be able to use this type as a return type for a function, and without
the name I can't reference it at all.

https://codereview.chromium.org/1162773003/diff/1/public/platform/WebBlobData...
public/platform/WebBlobData.h:73: // the same index will result in null data.
On 2015/06/02 19:30:56, jsbell wrote:
> Maybe amend with a TODO to rework the API?

Done.

https://codereview.chromium.org/1162773003/diff/1/public/platform/WebThreadSa...
File public/platform/WebThreadSafeData.h (right):

https://codereview.chromium.org/1162773003/diff/1/public/platform/WebThreadSa...
public/platform/WebThreadSafeData.h:53: // For testing use only. This allows us
to populate this object in content/
On 2015/06/02 at 19:30:56, jsbell wrote:
> Nit: probably shouldn't refer to content/ as blink does have other embedders.
I'd just keep the first sentence.

Done.

https://codereview.chromium.org/1162773003/diff/1/public/platform/WebThreadSa...
public/platform/WebThreadSafeData.h:54: BLINK_PLATFORM_EXPORT void assign(char*
data, size_t length);
On 2015/06/02 at 19:30:56, jsbell wrote:
> const char*

Done.

[dmurph, 2015-06-02 20:19:48]

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

[dmurph, 2015-06-02 20:19:48]

dmurph@chromium.org changed required reviewers:
+ jsbell@chromium.org

[dmurph, 2015-06-02 20:19:49]

+pdr as reviewer as he is in the same time zone :)

[pdr., 2015-06-02 20:26:16]

https://codereview.chromium.org/1162773003/diff/20001/public/platform/WebThre...
File public/platform/WebThreadSafeData.h (right):

https://codereview.chromium.org/1162773003/diff/20001/public/platform/WebThre...
public/platform/WebThreadSafeData.h:53: // For testing use only.
Other than two instances in WebLayer.h, I don't see other cases where we've
exposed things in platform for testing. How are similar apis tested today?

[dmurph, 2015-06-02 22:50:24]

This patch is probably going to be redone to do:

* Make WebBlobData pure virtual
  * accessor for each item, WebBlobDataItem
* Make WebBlobDataItem, which will be pure virtual
  * methods to claim ownership of the data inside.

[jochen (gone - plz use gerrit), 2015-06-03 10:43:30]

https://codereview.chromium.org/1162773003/diff/40001/public/platform/WebBlob...
File public/platform/WebBlobData.h (right):

https://codereview.chromium.org/1162773003/diff/40001/public/platform/WebBlob...
public/platform/WebBlobData.h:51: enum Type { TypeData, TypeFile, TypeBlob,
TypeFileSystemURL } type;
why this change?

[dmurph, 2015-06-03 17:54:30]

https://codereview.chromium.org/1162773003/diff/40001/public/platform/WebBlob...
File public/platform/WebBlobData.h (right):

https://codereview.chromium.org/1162773003/diff/40001/public/platform/WebBlob...
public/platform/WebBlobData.h:51: enum Type { TypeData, TypeFile, TypeBlob,
TypeFileSystemURL } type;
On 2015/06/03 at 10:43:30, jochen wrote:
> why this change?

I would like to be able to reference this enum value, and without the name here
I cannot do that cleanly (I would have to use the C-style int)

[jochen (gone - plz use gerrit), 2015-06-05 12:09:09]

On 2015/06/03 at 17:54:30, dmurph wrote:
>
https://codereview.chromium.org/1162773003/diff/40001/public/platform/WebBlob...
> File public/platform/WebBlobData.h (right):
> 
>
https://codereview.chromium.org/1162773003/diff/40001/public/platform/WebBlob...
> public/platform/WebBlobData.h:51: enum Type { TypeData, TypeFile, TypeBlob,
TypeFileSystemURL } type;
> On 2015/06/03 at 10:43:30, jochen wrote:
> > why this change?
> 
> I would like to be able to reference this enum value, and without the name
here I cannot do that cleanly (I would have to use the C-style int)

but you don't reference it?

and in the brave new world, we'd use an enum class

[dmurph, 2015-06-11 23:42:32]

Hey guys,

PTAL again.  I changed the WebBlobRegistry interface to have a construction
pattern.  This would let us get rid of WebBlobData entirely and just have the
builder.

Second, I made WebThreadSafeData constructable and copyable outside of blink so
I can test the BlobRegistry implemenation that I'm creating (the current one is
not tested at all).

Thanks!
Daniel

Patch selfie: http://i.imgur.com/1BrAWkm.gif

[jochen (gone - plz use gerrit), 2015-06-12 08:39:23]

lgtm once jsbell is happy

https://codereview.chromium.org/1162773003/diff/60001/public/platform/WebBlob...
File public/platform/WebBlobRegistry.h (right):

https://codereview.chromium.org/1162773003/diff/60001/public/platform/WebBlob...
public/platform/WebBlobRegistry.h:60: virtual ~Builder() { }
not needed

[jsbell, 2015-06-12 16:43:43]

lgtm

https://codereview.chromium.org/1162773003/diff/60001/public/platform/WebBlob...
File public/platform/WebBlobRegistry.h (right):

https://codereview.chromium.org/1162773003/diff/60001/public/platform/WebBlob...
public/platform/WebBlobRegistry.h:60: virtual ~Builder() { }
On 2015/06/12 08:39:23, jochen wrote:
> not needed

Is this for planned subclasses in unit tests on the Chromium side?

[dmurph, 2015-06-12 19:06:48]

jsbell: PTAL, I added a 'build' method instead of relying on the destructor.  I
also updated comments accordingly.

https://codereview.chromium.org/1162773003/diff/60001/public/platform/WebBlob...
File public/platform/WebBlobRegistry.h (right):

https://codereview.chromium.org/1162773003/diff/60001/public/platform/WebBlob...
public/platform/WebBlobRegistry.h:60: virtual ~Builder() { }
On 2015/06/12 at 16:43:43, jsbell wrote:
> On 2015/06/12 08:39:23, jochen wrote:
> > not needed
> 
> Is this for planned subclasses in unit tests on the Chromium side?

Yes, there will be a mock subclass of this as well as the production one.

According to style guide, I need this:
http://google.github.io/styleguide/cppguide.html#Interfaces

[dmurph, 2015-06-15 20:33:17]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2015-06-15 20:33:17]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1162773003/#ps80001
(title: "Added build() method")

[commit-bot: I haz the power, 2015-06-15 20:33:38]

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

[commit-bot: I haz the power, 2015-06-15 22:12:35]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197142