blob hacking webcore style

WebCore blob hacking 

Added BlobDataHandle, a thread safe layer of indirection between Blob instances and the underying data.

Change blob processing/handling classes to work with BlobDataHandles insead of URLs. 
-- WebCore::Blob 
-- WebCore::BlobBuilder 
-- postMessage() plumbing 
-- WebSocket.send() plumbing 
-- fileWriter.write() plumbing 
-- SerializedScriptValue 
-- FormData 

Switched to using string uuids instead of urls in structs and params in the WebKit API. 
-- WebBlobData::Item 
-- WebHTTPBody::Element 
-- WebFileWriter::write() 
-- WebBlobRegistry 

Removed main thread bridging from BlobRegistry control flow for blobs (not for streams).

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

BUG=174200

[michaeln, 2013-09-25 21:35:01]

hi kinuko, can you help to review this one... thnx

[michaeln, 2013-09-25 21:43:56]

The corresponding chromium change is already committed...
https://chromiumcodereview.appspot.com/23223003

[jsbell, 2013-09-26 18:00:33]

https://codereview.chromium.org/23992003/diff/20001/Source/bindings/v8/Serial...
File Source/bindings/v8/SerializedScriptValue.cpp (right):

https://codereview.chromium.org/23992003/diff/20001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.cpp:245: static const uint32_t
wireFormatVersion = 3;
As an aside, we have a copy of this value living over in Chromium-land to detect
version skew from when we moved the IDB back-end.

We'll need to bump it there before this lands or, more correctly, expose this in
the Blink API somehow and consume it there.

[jsbell, 2013-09-26 19:12:45]

https://codereview.chromium.org/23992003/diff/20001/LayoutTests/fast/storage/...
File LayoutTests/fast/storage/serialized-script-value.html (right):

https://codereview.chromium.org/23992003/diff/20001/LayoutTests/fast/storage/...
LayoutTests/fast/storage/serialized-script-value.html:13: // we only test a few
cases of the "old" serialization format because
Nit: When you bump the SSV wire version you'll want to add tests that ensure the
old version is still parsed.

https://codereview.chromium.org/23992003/diff/20001/public/platform/WebBlobDa...
File public/platform/WebBlobData.h (right):

https://codereview.chromium.org/23992003/diff/20001/public/platform/WebBlobDa...
public/platform/WebBlobData.h:38: #define USE_BLOB_UUIDS
Is this used anywhere?

[michaeln, 2013-09-26 20:03:45]

> We'll need to bump it there before this lands or, more correctly, expose this
in
> the Blink API somehow and consume it there.

Yikes, thanx for looking and point that out, i hadn't stumbled across that in my
blob world travels :)

On 2013/09/26 19:12:45, jsbell wrote:
>
https://codereview.chromium.org/23992003/diff/20001/LayoutTests/fast/storage/...
> File LayoutTests/fast/storage/serialized-script-value.html (right):
> 
>
https://codereview.chromium.org/23992003/diff/20001/LayoutTests/fast/storage/...
> LayoutTests/fast/storage/serialized-script-value.html:13: // we only test a
few
> cases of the "old" serialization format because
> Nit: When you bump the SSV wire version you'll want to add tests that ensure
the
> old version is still parsed.

This test does some of that. It deserializes objects in v1 format. None of these
tests however do any thing with blobs. I should add some tests that do work with
blobs as part of this CL.

>
https://codereview.chromium.org/23992003/diff/20001/public/platform/WebBlobDa...
> File public/platform/WebBlobData.h (right):
> 
>
https://codereview.chromium.org/23992003/diff/20001/public/platform/WebBlobDa...
> public/platform/WebBlobData.h:38: #define USE_BLOB_UUIDS
> Is this used anywhere?

This could/should be used to toggle the value used for that version number in
chrome-land. Can you point me at the sources for that?

[michaeln, 2013-09-26 20:06:00]

woohooo.... green layout test try jobs... will have to try our larger test
suites

[michaeln, 2013-09-26 20:07:44]

> Yikes, thanx for looking and point that out, i hadn't stumbled across that in
my
> blob world travels :)

Haha... blob trotting :)

[jsbell, 2013-09-26 20:55:21]

On 2013/09/26 20:03:45, michaeln wrote:
> > We'll need to bump it there before this lands or, more correctly, expose
this
> in
> > the Blink API somehow and consume it there.
> 
> Yikes, thanx for looking and point that out, i hadn't stumbled across that in
my
> blob world travels :)

Yeah, leftover from our move and it wasn't hurting us yet so we ignored it...

> 
> On 2013/09/26 19:12:45, jsbell wrote:
> >
>
https://codereview.chromium.org/23992003/diff/20001/LayoutTests/fast/storage/...
> > File LayoutTests/fast/storage/serialized-script-value.html (right):
> > 
> >
>
https://codereview.chromium.org/23992003/diff/20001/LayoutTests/fast/storage/...
> > LayoutTests/fast/storage/serialized-script-value.html:13: // we only test a
> few
> > cases of the "old" serialization format because
> > Nit: When you bump the SSV wire version you'll want to add tests that ensure
> the
> > old version is still parsed.
> 
> This test does some of that. It deserializes objects in v1 format. None of
these
> tests however do any thing with blobs. I should add some tests that do work
with
> blobs as part of this CL.

Yeah, but it looks like you changed all the v2 cases to v3 and didn't add any v2
cases back in. Unless I missed them?

> >
>
https://codereview.chromium.org/23992003/diff/20001/public/platform/WebBlobDa...
> > File public/platform/WebBlobData.h (right):
> > 
> >
>
https://codereview.chromium.org/23992003/diff/20001/public/platform/WebBlobDa...
> > public/platform/WebBlobData.h:38: #define USE_BLOB_UUIDS
> > Is this used anywhere?
> 
> This could/should be used to toggle the value used for that version number in
> chrome-land. Can you point me at the sources for that?

Ah, I was grepping about in Blink, not Chromium. I see it in
content/child/webblobregistry_impl.cc and a few other places.

[michaeln, 2013-09-26 21:04:52]

https://codereview.chromium.org/23992003/diff/20001/LayoutTests/fast/storage/...
File LayoutTests/fast/storage/serialized-script-value.html (right):

https://codereview.chromium.org/23992003/diff/20001/LayoutTests/fast/storage/...
LayoutTests/fast/storage/serialized-script-value.html:117:
testSerialization(arrayObject,
i'll add one here to test v2 in the same way v1 is being tested here... in
addition to adding *something* for blobs

[michaeln, 2013-09-27 23:46:19]

and i'll bump version values as needed for...
https://codereview.chromium.org/24876004

[kinuko, 2013-09-30 11:31:52]

blob internal url -> uuid switching part looks good to me, just made some
mostly-nit comments.

https://codereview.chromium.org/23992003/diff/20001/Source/bindings/v8/Serial...
File Source/bindings/v8/SerializedScriptValue.cpp (right):

https://codereview.chromium.org/23992003/diff/20001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.cpp:1865: return false;
so it's assuming we should always get the same version of message unless it's
from wired, correct?

https://codereview.chromium.org/23992003/diff/20001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.cpp:1977: // FIXME: Maybe we should
require that it work that way where the ssv must have a BDH for any
Could we include the bug number to refer this FIXME in the comment? (not fully
sure if we do this too in blink code though)

https://codereview.chromium.org/23992003/diff/20001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.cpp:1981: // the blob in the src
process happens to still exists at the time the dest process is deserializing.
nit: exists -> exist

https://codereview.chromium.org/23992003/diff/20001/Source/core/fileapi/Blob.cpp
File Source/core/fileapi/Blob.cpp (right):

https://codereview.chromium.org/23992003/diff/20001/Source/core/fileapi/Blob....
Source/core/fileapi/Blob.cpp:42: class PublicBlobURLRegistry : public
URLRegistry {
nit: Now all BlobURLs are public (and this class is private)... do we still want
to add this 'Public' prefix?

https://codereview.chromium.org/23992003/diff/20001/Source/core/fileapi/FileR...
File Source/core/fileapi/FileReaderLoader.cpp (right):

https://codereview.chromium.org/23992003/diff/20001/Source/core/fileapi/FileR...
Source/core/fileapi/FileReaderLoader.cpp:85: void
FileReaderLoader::startInternal(ScriptExecutionContext* scriptExecutionContext,
const Stream* stream, PassRefPtr<BlobDataHandle> blobData)
No need to be done in this CL, but this method should be probably placed much
below together with other private methods (at least below start())

https://codereview.chromium.org/23992003/diff/20001/Source/core/fileapi/FileR...
Source/core/fileapi/FileReaderLoader.cpp:97:
BlobRegistry::registerStreamURL(scriptExecutionContext->securityOrigin(),
m_urlForReading, stream->url());
Can we assert somewhere that we must have either blobData or stream?

https://codereview.chromium.org/23992003/diff/20001/Source/modules/websockets...
File Source/modules/websockets/NewWebSocketChannelImpl.h (right):

https://codereview.chromium.org/23992003/diff/20001/Source/modules/websockets...
Source/modules/websockets/NewWebSocketChannelImpl.h:109: RefPtr<BlobDataHandle>
blobData;
nit: We also have a class named BlobData, and calling this (of BlobDataHandle)
'blobData' could be possibly confusing.. ?

https://codereview.chromium.org/23992003/diff/20001/public/platform/WebBlobDa...
File public/platform/WebBlobData.h (right):

https://codereview.chromium.org/23992003/diff/20001/public/platform/WebBlobDa...
public/platform/WebBlobData.h:38: #define USE_BLOB_UUIDS
On 2013/09/26 19:12:46, jsbell wrote:
> Is this used anywhere?

(on behalf of michaeln) it's for chromium-side impl switch, used only in
chromium code.

https://codereview.chromium.org/23992003/diff/20001/public/platform/WebBlobDa...
public/platform/WebBlobData.h:52: // TypeURL is DEPRECATED
Can you remove this comment too?

https://codereview.chromium.org/23992003/diff/20001/public/platform/WebHTTPBo...
File public/platform/WebHTTPBody.h (right):

https://codereview.chromium.org/23992003/diff/20001/public/platform/WebHTTPBo...
public/platform/WebHTTPBody.h:34: #include "WebBlobData.h"
Is this change needed?

https://codereview.chromium.org/23992003/diff/20001/public/web/WebBlob.h
File public/web/WebBlob.h (right):

https://codereview.chromium.org/23992003/diff/20001/public/web/WebBlob.h#newc...
public/web/WebBlob.h:34: #include "../platform/WebBlobData.h"
ditto. Is this change necessary?

[michaeln, 2013-09-30 20:50:23]

thnx for looking, i'll upload a new cl soon

https://codereview.chromium.org/23992003/diff/20001/Source/bindings/v8/Serial...
File Source/bindings/v8/SerializedScriptValue.cpp (right):

https://codereview.chromium.org/23992003/diff/20001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.cpp:245: static const uint32_t
wireFormatVersion = 3;
On 2013/09/26 18:00:36, jsbell wrote:
> As an aside, we have a copy of this value living over in Chromium-land to
detect
> version skew from when we moved the IDB back-end.
> 
> We'll need to bump it there before this lands or, more correctly, expose this
in
> the Blink API somehow and consume it there.

i'll sync up with the changes to fix that (thnx for those changes)

https://codereview.chromium.org/23992003/diff/20001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.cpp:1865: return false;
On 2013/09/30 11:31:53, kinuko wrote:
> so it's assuming we should always get the same version of message unless it's
> from wired, correct?

deserialization will fail if we get an older format that contains an old style
blob url

https://codereview.chromium.org/23992003/diff/20001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.cpp:1977: // FIXME: Maybe we should
require that it work that way where the ssv must have a BDH for any
On 2013/09/30 11:31:53, kinuko wrote:
> Could we include the bug number to refer this FIXME in the comment? (not fully
> sure if we do this too in blink code though)

Do you think it's a good idea to do whats described here?

https://codereview.chromium.org/23992003/diff/20001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.cpp:1981: // the blob in the src
process happens to still exists at the time the dest process is deserializing.
On 2013/09/30 11:31:53, kinuko wrote:
> nit: exists -> exist

done (or will do on next upload)

https://codereview.chromium.org/23992003/diff/20001/Source/core/fileapi/Blob.cpp
File Source/core/fileapi/Blob.cpp (right):

https://codereview.chromium.org/23992003/diff/20001/Source/core/fileapi/Blob....
Source/core/fileapi/Blob.cpp:42: class PublicBlobURLRegistry : public
URLRegistry {
On 2013/09/30 11:31:53, kinuko wrote:
> nit: Now all BlobURLs are public (and this class is private)... do we still
want
> to add this 'Public' prefix?

I guess we don't need to. Maybe this was overzealous on my part about
eradicating the idea of using urls as the identifier for these things.

https://codereview.chromium.org/23992003/diff/20001/Source/core/fileapi/FileR...
File Source/core/fileapi/FileReaderLoader.cpp (right):

https://codereview.chromium.org/23992003/diff/20001/Source/core/fileapi/FileR...
Source/core/fileapi/FileReaderLoader.cpp:97:
BlobRegistry::registerStreamURL(scriptExecutionContext->securityOrigin(),
m_urlForReading, stream->url());
On 2013/09/30 11:31:53, kinuko wrote:
> Can we assert somewhere that we must have either blobData or stream?

Will do (or done depending on when you look next).

https://codereview.chromium.org/23992003/diff/20001/Source/modules/websockets...
File Source/modules/websockets/NewWebSocketChannelImpl.h (right):

https://codereview.chromium.org/23992003/diff/20001/Source/modules/websockets...
Source/modules/websockets/NewWebSocketChannelImpl.h:109: RefPtr<BlobDataHandle>
blobData;
On 2013/09/30 11:31:53, kinuko wrote:
> nit: We also have a class named BlobData, and calling this (of BlobDataHandle)
> 'blobData' could be possibly confusing.. ?

renamed to blobDataHandle

https://codereview.chromium.org/23992003/diff/20001/public/platform/WebBlobDa...
File public/platform/WebBlobData.h (right):

https://codereview.chromium.org/23992003/diff/20001/public/platform/WebBlobDa...
public/platform/WebBlobData.h:52: // TypeURL is DEPRECATED
On 2013/09/30 11:31:53, kinuko wrote:
> Can you remove this comment too?

done

https://codereview.chromium.org/23992003/diff/20001/public/platform/WebHTTPBo...
File public/platform/WebHTTPBody.h (right):

https://codereview.chromium.org/23992003/diff/20001/public/platform/WebHTTPBo...
public/platform/WebHTTPBody.h:34: #include "WebBlobData.h"
On 2013/09/30 11:31:53, kinuko wrote:
> Is this change needed?

Yes, this ensures code compiled against this .h file see the USE_BLOB_UUIDS
flag. And it allows us to define that flag w/o additional DEPS rules. We (piman
and i) chose to not alter DEPS to make that flag available when compiling
chromium browser process code, transitively including this file here is needed
for that.

https://codereview.chromium.org/23992003/diff/20001/public/web/WebBlob.h
File public/web/WebBlob.h (right):

https://codereview.chromium.org/23992003/diff/20001/public/web/WebBlob.h#newc...
public/web/WebBlob.h:34: #include "../platform/WebBlobData.h"
On 2013/09/30 11:31:53, kinuko wrote:
> ditto. Is this change necessary?

Yes, this ensures code compiled against this .h file see the USE_BLOB_UUIDS
flag.

Once the USE_BLOB_UUID flag is removed and effectively always on, i could come
back and remove this (and the other one too), but these are needed in the
interim.

[michaeln, 2013-10-01 23:51:51]

ptal, rebased and made changes per the last round of comments.

There's a small block of code in the layout test file that's not quite done yet,
but otherwise i think this is ready. I'll kick off larger tryjobs to see what
they think, and finish work on the layout test so it serializes/deserializes
blob values too.

[michaeln, 2013-10-01 23:57:55]

https://codereview.chromium.org/23992003/diff/46001/Source/core/fileapi/BlobR...
File Source/core/fileapi/BlobRegistry.h (right):

https://codereview.chromium.org/23992003/diff/46001/Source/core/fileapi/BlobR...
Source/core/fileapi/BlobRegistry.h:49: // if necessary.
comment is stale, will update...

https://codereview.chromium.org/23992003/diff/46001/Source/core/fileapi/BlobR...
Source/core/fileapi/BlobRegistry.h:52: static void init();
this method shouldn't be here, removing...

[michaeln, 2013-10-02 01:31:38]

ok, updated the blink layout test to "do something with blobs", i think this cl
is ready

... but... i have to fix page_state_serialization_unittest.cc in chromium prior
to landing based on tryruns for snapshot #10

[kinuko, 2013-10-02 15:41:38]

Serialization tests feel slightly unsure, but the change lgtm

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
File LayoutTests/fast/storage/resources/serialized-script-value.js (right):

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
LayoutTests/fast/storage/resources/serialized-script-value.js:97: function
_testBlobSerialization() {
nit: Unlike _testSerialization we're calling this function without any
indirection in .html... I doubt we need preceding '_' in this function name.

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
LayoutTests/fast/storage/resources/serialized-script-value.js:101:
shouldBeTrue("areValuesIdentical(blobObj1, blobObj2)");
areValuesIdentical only checks object properties, and for Blob it's only 'size'
and 'type'.  I feel ideally we should have a test which checks its contents too

https://codereview.chromium.org/23992003/diff/54001/Source/core/fileapi/Blob.h
File Source/core/fileapi/Blob.h (right):

https://codereview.chromium.org/23992003/diff/54001/Source/core/fileapi/Blob....
Source/core/fileapi/Blob.h:71: Blob(PassRefPtr<BlobDataHandle>);
explicit

https://codereview.chromium.org/23992003/diff/54001/Source/core/fileapi/File.cpp
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/23992003/diff/54001/Source/core/fileapi/File....
Source/core/fileapi/File.cpp:129: 
nit: extra empty line

https://codereview.chromium.org/23992003/diff/54001/Source/core/platform/netw...
File Source/core/platform/network/BlobData.cpp (right):

https://codereview.chromium.org/23992003/diff/54001/Source/core/platform/netw...
Source/core/platform/network/BlobData.cpp:35: #include "core/platform/UUID.h"
Looks like this one's just moved to platform/UUID.h

[michaeln, 2013-10-02 20:09:00]

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
File LayoutTests/fast/storage/resources/serialized-script-value.js (right):

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
LayoutTests/fast/storage/resources/serialized-script-value.js:97: function
_testBlobSerialization() {
On 2013/10/02 15:41:39, kinuko wrote:
> nit: Unlike _testSerialization we're calling this function without any
> indirection in .html... I doubt we need preceding '_' in this function name.

Do you know the reasons for that layer of indirection in the .html for
_testSerialization?

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
LayoutTests/fast/storage/resources/serialized-script-value.js:101:
shouldBeTrue("areValuesIdentical(blobObj1, blobObj2)");
On 2013/10/02 15:41:39, kinuko wrote:
> areValuesIdentical only checks object properties, and for Blob it's only
'size'
> and 'type'.  I feel ideally we should have a test which checks its contents
too

i could use sync xhr with public blob urls to test the contents without making
the test async

https://codereview.chromium.org/23992003/diff/54001/Source/core/fileapi/Blob.h
File Source/core/fileapi/Blob.h (right):

https://codereview.chromium.org/23992003/diff/54001/Source/core/fileapi/Blob....
Source/core/fileapi/Blob.h:71: Blob(PassRefPtr<BlobDataHandle>);
On 2013/10/02 15:41:39, kinuko wrote:
> explicit

Done.

https://codereview.chromium.org/23992003/diff/54001/Source/core/fileapi/File.cpp
File Source/core/fileapi/File.cpp (right):

https://codereview.chromium.org/23992003/diff/54001/Source/core/fileapi/File....
Source/core/fileapi/File.cpp:129: 
On 2013/10/02 15:41:39, kinuko wrote:
> nit: extra empty line

Done.

https://codereview.chromium.org/23992003/diff/54001/Source/core/platform/netw...
File Source/core/platform/network/BlobData.cpp (right):

https://codereview.chromium.org/23992003/diff/54001/Source/core/platform/netw...
Source/core/platform/network/BlobData.cpp:35: #include "core/platform/UUID.h"
On 2013/10/02 15:41:39, kinuko wrote:
> Looks like this one's just moved to platform/UUID.h

Thnx, was wondering what was up with those compilation errors on the bots? I'll
rebase and fix this header up after i pick those changes up.

[michaeln, 2013-10-02 20:30:40]

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
File LayoutTests/fast/storage/resources/serialized-script-value.js (right):

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
LayoutTests/fast/storage/resources/serialized-script-value.js:101:
shouldBeTrue("areValuesIdentical(blobObj1, blobObj2)");
On 2013/10/02 20:09:01, michaeln wrote:
> On 2013/10/02 15:41:39, kinuko wrote:
> > areValuesIdentical only checks object properties, and for Blob it's only
> 'size'
> > and 'type'.  I feel ideally we should have a test which checks its contents
> too
> 
> i could use sync xhr with public blob urls to test the contents without making
> the test async

Done.

[jsbell, 2013-10-02 22:24:41]

Noticed a more couple nits.

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
File LayoutTests/fast/storage/serialized-script-value.html (right):

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
LayoutTests/fast/storage/serialized-script-value.html:91: [0x03ff, 0x003f,
0x3f6f, 0x5301, 0x6101,
To be pedantic here, you want to add a v3 case, not replace the v2 cases
(otherwise we lose v2 coverage). Maybe testSerialization should take an array
for oldFormat, so it would be called with testSerialization(obj, v3, [v2, v1]).

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
LayoutTests/fast/storage/serialized-script-value.html:124: [0x02ff, 0x003f,
0x0041, 0x013f, 0x0153, // deserialize from v2
This is losing v1 coverage?

https://codereview.chromium.org/23992003/diff/54001/Source/bindings/v8/Serial...
File Source/bindings/v8/SerializedScriptValue.cpp (right):

https://codereview.chromium.org/23992003/diff/54001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.cpp:1909:
fileList->append(doReadFileHelper());
If doReadFileHelper() returns 0, should this return false?

The (new) FileList serialization code doesn't handle a FileList with null
entries, I'm guessing that's the case elsewhere too.

https://codereview.chromium.org/23992003/diff/54001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.cpp:1972: // FIXME: Maybe we should
require that it work that way where the ssv must have a BDH for any
+1 to this.

Do any of our current tests rely on the 'get lucky' behavior?

https://codereview.chromium.org/23992003/diff/54001/Source/bindings/v8/Serial...
File Source/bindings/v8/SerializedScriptValue.h (right):

https://codereview.chromium.org/23992003/diff/54001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.h:102: // Only reflects the truth if
the SSV was created by walking a v8 value, not reliable
I think the only callers of this basically do:

SSV ssv = myvalue.serialize();
if (ssv.containsBlobs()) { throw; }

Perhaps the API should be that callers pass in the BlobDataHandleMap? I think I
made a similar comment on ericu@'s patch, since in that case the array is
promptly grabbed and passed data in parallel with the SSV wire data.

It's probably safe to wait until ericu@'s changes go in, since that will clarify
how this is used.

[michaeln, 2013-10-03 00:22:24]

https://codereview.chromium.org/23992003/diff/54001/Source/bindings/v8/Serial...
File Source/bindings/v8/SerializedScriptValue.cpp (right):

https://codereview.chromium.org/23992003/diff/54001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.cpp:1909:
fileList->append(doReadFileHelper());
On 2013/10/02 22:24:41, jsbell wrote:
> If doReadFileHelper() returns 0, should this return false?

Ooops... thnx... done!

> The (new) FileList serialization code doesn't handle a FileList with null
> entries, I'm guessing that's the case elsewhere too.

Hmmm... the old code doesn't either? I'm gonna stay away from altering that in
this CL.

https://codereview.chromium.org/23992003/diff/54001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.cpp:1972: // FIXME: Maybe we should
require that it work that way where the ssv must have a BDH for any
On 2013/10/02 22:24:41, jsbell wrote:
> +1 to this.
> 
> Do any of our current tests rely on the 'get lucky' behavior?

I pretty sure they do. Any test that involves sending blobs to/from shared
workers definitely do.

To make this change means making changes to any code paths that carry blobs in
serialized values to explicitly handle a separate collection of blob uuids or
handles in parallel... and do that from end-to-end. I'll open a bug about this
task in particular.

[michaeln, 2013-10-03 22:08:51]

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
File LayoutTests/fast/storage/serialized-script-value.html (right):

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
LayoutTests/fast/storage/serialized-script-value.html:91: [0x03ff, 0x003f,
0x3f6f, 0x5301, 0x6101,
The first value is expected to be in the format that blink currently produces.
All of these must be changed to 0x03ff or the tests won't run properly.

I'm following the precedent set by an earlier cl...

 // we only test a few cases of the "old" serialization format...

The format change i'm making only affects blob, file, filelist types.

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
LayoutTests/fast/storage/serialized-script-value.html:124: [0x02ff, 0x003f,
0x0041, 0x013f, 0x0153, // deserialize from v2
On 2013/10/02 22:24:41, jsbell wrote:
> This is losing v1 coverage?

Not really. The v1 and v2 (and v3) representations are the same for this case.
We're making sure v2 flags at the front of the stream are accepted as input. The
test below ensures that v1 flags at the front of the stream are also accepted as
input.

[jsbell, 2013-10-07 18:44:51]

So lgtm although I'm not familiar with the Chromium side of the change.

If you want me to page the whole thing into my brain I can do it, but it sounds
like Kinuko has that covered.

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
File LayoutTests/fast/storage/serialized-script-value.html (right):

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
LayoutTests/fast/storage/serialized-script-value.html:91: [0x03ff, 0x003f,
0x3f6f, 0x5301, 0x6101,
On 2013/10/03 22:08:52, michaeln wrote:
> The first value is expected to be in the format that blink currently produces.
> All of these must be changed to 0x03ff or the tests won't run properly.
> 
> I'm following the precedent set by an earlier cl...
> 
>  // we only test a few cases of the "old" serialization format...

Okay - SGTM.

> The format change i'm making only affects blob, file, filelist types.

Maybe add something like this to the comment and clarify the strategy i.e. that
new cases should be added when the serialization format changes, but only bump
the format number if the contents are the same.

https://codereview.chromium.org/23992003/diff/54001/LayoutTests/fast/storage/...
LayoutTests/fast/storage/serialized-script-value.html:124: [0x02ff, 0x003f,
0x0041, 0x013f, 0x0153, // deserialize from v2
On 2013/10/03 22:08:52, michaeln wrote:
> On 2013/10/02 22:24:41, jsbell wrote:
> > This is losing v1 coverage?
> 
> Not really. The v1 and v2 (and v3) representations are the same for this case.
> We're making sure v2 flags at the front of the stream are accepted as input.
The
> test below ensures that v1 flags at the front of the stream are also accepted
as
> input.
> 

Ok.

[michaeln, 2013-10-07 21:11:34]

thnx for looking. i need to poke at some chromuim unittest prior to landing
this. i'll put a link that that cl here when i have it prepped.

[abarth-chromium, 2013-10-07 21:21:12]

@michaeln: Just to let you know, this CL is related to some work I'm doing to
break the dependency from core/platform/network to the rest of core.  My current
plan is to wait for this CL to land and then break the dependency.  It sounds
like you're getting close to landing, which is great!

[michaeln, 2013-10-07 21:38:59]

On 2013/10/07 21:21:12, abarth wrote:
> @michaeln: Just to let you know, this CL is related to some work I'm doing to
> break the dependency from core/platform/network to the rest of core.  My
current
> plan is to wait for this CL to land and then break the dependency.  It sounds
> like you're getting close to landing, which is great!

@abarth, thnx for the heads up and even more for not changing the world out from
underneath me :)

[michaeln, 2013-10-08 01:18:55]

tiny chromium cl blocking this can be found here...
https://codereview.chromium.org/26405003/

[michaeln, 2013-10-08 23:12:25]

On 2013/10/08 01:18:55, michaeln wrote:
> tiny chromium cl blocking this can be found here...
> https://codereview.chromium.org/26405003/

^^^ is committed so i'll 'try' this again now

[michaeln, 2013-10-09 00:11:40]

bummer, have to wait longer for the change to land to make its way to the bots
page_state_serialization_unittest.cc(55) :error C2039: 'deprecated_blob_url'

[michaeln, 2013-10-09 19:50:50]

Hooray... looks like all blink and chromium targets build now with the flag
defined!

I'll sync/rebase/run it by the trybots again/take one last look thru... and then
dump it into the CQ.

[michaeln, 2013-10-09 20:19:46]

Where is the DEPS file that describes the include rules for WebKit\public\web?

** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  public\web\WebBlob.h
      Illegal include: "../platform/WebBlobData.h"
    Because of no rule applying.

[michaeln, 2013-10-09 20:25:19]

On 2013/10/09 20:19:46, michaeln wrote:
> Where is the DEPS file that describes the include rules for WebKit\public\web?
> 
> ** Presubmit ERRORS **
> You added one or more #includes that violate checkdeps rules.
>   public\web\WebBlob.h
>       Illegal include: "../platform/WebBlobData.h"
>     Because of no rule applying.

also @adam, would you please do the owners review

[alecflett, 2013-10-09 21:13:55]

On 2013/10/09 20:25:19, michaeln wrote:
> On 2013/10/09 20:19:46, michaeln wrote:
> > Where is the DEPS file that describes the include rules for
WebKit\public\web?
> > 
> > ** Presubmit ERRORS **
> > You added one or more #includes that violate checkdeps rules.
> >   public\web\WebBlob.h
> >       Illegal include: "../platform/WebBlobData.h"
> >     Because of no rule applying.
> 
> also @adam, would you please do the owners review

pretty sure you can just do "public/platform/WebBlobData.h" rather then using
relative paths

[abarth-chromium, 2013-10-09 22:07:57]

lgtm

https://codereview.chromium.org/23992003/diff/93001/public/web/WebBlob.h
File public/web/WebBlob.h (right):

https://codereview.chromium.org/23992003/diff/93001/public/web/WebBlob.h#newc...
public/web/WebBlob.h:38: #include "../platform/WebURL.h"
Yeah, sorry.  These should be like:

#include "public/platform/WebBlobData.h"

[abarth-chromium, 2013-10-09 22:09:50]

Maybe the DEPS file is missing?

public/web/DEPS

should have +public/platform

[michaeln, 2013-10-09 23:45:47]

> "public/platform/WebFoo.h"

That did it, thnx, I switched them all over.

#include "../platform/WebBlobData.h"
#include "../platform/WebCommon.h"
#include "../platform/WebPrivatePtr.h"
#include "../platform/WebString.h"
#include "../platform/WebURL.h"

[commit-bot: I haz the power, 2013-10-09 23:47:33]

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

[commit-bot: I haz the power, 2013-10-10 03:55:32]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[michaeln, 2013-10-10 21:48:32]

On 2013/10/10 03:55:32, I haz the power (commit-bot) wrote:
> Sorry for I got bad news for ya.
> Compile failed with a clobber build on mac_layout.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
> Your code is likely broken or HEAD is junk. Please ensure your
> code is not broken then alert the build sheriffs.
> Look at the try server FAQ for more details.

FAILED: clang++ -MMD -MF
obj/third_party/WebKit/Source/core/xml/webcore_remaining.XMLHttpRequest.o.d
../../third_party/WebKit/Source/core/xml/XMLHttpRequest.cpp:651:68:error: no
member named 'url' in 'WebCore::Blob'
    LOG(Network, "XMLHttpRequest %p send() Blob '%s'", this,
body->url().elidedString().utf8().data());

Um... yo clang... what the heck source files are you compiling exactly? That
line of code is no where that i can see. Although now that i've switched to git
it is more challenging to actually view revision history ahead of my local
checkout? Maybe its a newly introduced line of code and until i rebase it's hard
to actually the history or view the diffs.

Yup, after some inscrutable machinations i was able to view history w/o having
to first rebase. The logging was added in revision
0d21e575337ccbb5708738ede5f6722dd613b25d (that's easy work with isn't it, just
think like git).
Review URL: https://codereview.chromium.org/26231002

[michaeln, 2013-10-10 22:09:34]

bummer, i have to wait for tyoshio's change to be rolled into deps since i'm
working in a chromium client that is synced to deps. with svn, i'd just update
that single file and get on with it.

[commit-bot: I haz the power, 2013-10-11 02:41:10]

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

[commit-bot: I haz the power, 2013-10-11 08:04:01]

Failed to apply patch for Source/core/fileapi/FileReader.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/fileapi/FileReader.cpp
  Hunk #6 FAILED at 163.
  1 out of 6 hunks FAILED -- saving rejects to file
Source/core/fileapi/FileReader.cpp.rej

Patch:       Source/core/fileapi/FileReader.cpp
Index: Source/core/fileapi/FileReader.cpp
diff --git a/Source/core/fileapi/FileReader.cpp
b/Source/core/fileapi/FileReader.cpp
index
b3a3327099b4b3e93b665c49bf1644f8bfec3c87..e55174a07a6d361d6bd4f82829e9a665bf0dd529
100644
--- a/Source/core/fileapi/FileReader.cpp
+++ b/Source/core/fileapi/FileReader.cpp
@@ -46,9 +46,9 @@ namespace WebCore {
 
 namespace {
 
-const CString utf8BlobURL(Blob* blob)
+const CString utf8BlobUUID(Blob* blob)
 {
-    return blob->url().string().utf8();
+    return blob->uuid().utf8();
 }
 
 const CString utf8FilePath(Blob* blob)
@@ -103,7 +103,7 @@ void FileReader::readAsArrayBuffer(Blob* blob,
ExceptionState& es)
     if (!blob)
         return;
 
-    LOG(FileAPI, "FileReader: reading as array buffer: %s %s\n",
utf8BlobURL(blob).data(), utf8FilePath(blob).data());
+    LOG(FileAPI, "FileReader: reading as array buffer: %s %s\n",
utf8BlobUUID(blob).data(), utf8FilePath(blob).data());
 
     readInternal(blob, FileReaderLoader::ReadAsArrayBuffer, es);
 }
@@ -113,7 +113,7 @@ void FileReader::readAsBinaryString(Blob* blob,
ExceptionState& es)
     if (!blob)
         return;
 
-    LOG(FileAPI, "FileReader: reading as binary: %s %s\n",
utf8BlobURL(blob).data(), utf8FilePath(blob).data());
+    LOG(FileAPI, "FileReader: reading as binary: %s %s\n",
utf8BlobUUID(blob).data(), utf8FilePath(blob).data());
 
     readInternal(blob, FileReaderLoader::ReadAsBinaryString, es);
 }
@@ -123,7 +123,7 @@ void FileReader::readAsText(Blob* blob, const String&
encoding, ExceptionState&
     if (!blob)
         return;
 
-    LOG(FileAPI, "FileReader: reading as text: %s %s\n",
utf8BlobURL(blob).data(), utf8FilePath(blob).data());
+    LOG(FileAPI, "FileReader: reading as text: %s %s\n",
utf8BlobUUID(blob).data(), utf8FilePath(blob).data());
 
     m_encoding = encoding;
     readInternal(blob, FileReaderLoader::ReadAsText, es);
@@ -139,7 +139,7 @@ void FileReader::readAsDataURL(Blob* blob, ExceptionState&
es)
     if (!blob)
         return;
 
-    LOG(FileAPI, "FileReader: reading as data URL: %s %s\n",
utf8BlobURL(blob).data(), utf8FilePath(blob).data());
+    LOG(FileAPI, "FileReader: reading as data URL: %s %s\n",
utf8BlobUUID(blob).data(), utf8FilePath(blob).data());
 
     readInternal(blob, FileReaderLoader::ReadAsDataURL, es);
 }
@@ -163,7 +163,7 @@ void FileReader::readInternal(Blob* blob,
FileReaderLoader::ReadType type, Excep
     m_loader = adoptPtr(new FileReaderLoader(m_readType, this));
     m_loader->setEncoding(m_encoding);
     m_loader->setDataType(m_blob->type());
-    m_loader->start(scriptExecutionContext(), *m_blob);
+    m_loader->start(scriptExecutionContext(), m_blob->blobDataHandle());
 }
 
 static void delayedAbort(ScriptExecutionContext*, FileReader* reader)

[michaeln, 2013-10-14 23:32:24]

Trying again now that ScriptExecutionContext has been renamed in countless
number of files and lines of code.

[commit-bot: I haz the power, 2013-10-14 23:33:28]

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

[commit-bot: I haz the power, 2013-10-14 23:33:44]

Failed to apply patch for Source/core/fileapi/FileReader.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/fileapi/FileReader.cpp
  Hunk #6 FAILED at 157.
  1 out of 6 hunks FAILED -- saving rejects to file
Source/core/fileapi/FileReader.cpp.rej

Patch:       Source/core/fileapi/FileReader.cpp
Index: Source/core/fileapi/FileReader.cpp
diff --git a/Source/core/fileapi/FileReader.cpp
b/Source/core/fileapi/FileReader.cpp
index
b654d9aa63e0093994a3a38b043ca24783fa2e3f..2ff2020c0d69010c47361ff2ea8aa35f2cc5413f
100644
--- a/Source/core/fileapi/FileReader.cpp
+++ b/Source/core/fileapi/FileReader.cpp
@@ -46,9 +46,9 @@ namespace WebCore {
 
 namespace {
 
-const CString utf8BlobURL(Blob* blob)
+const CString utf8BlobUUID(Blob* blob)
 {
-    return blob->url().string().utf8();
+    return blob->uuid().utf8();
 }
 
 const CString utf8FilePath(Blob* blob)
@@ -97,7 +97,7 @@ void FileReader::readAsArrayBuffer(Blob* blob, ExceptionState&
es)
     if (!blob)
         return;
 
-    LOG(FileAPI, "FileReader: reading as array buffer: %s %s\n",
utf8BlobURL(blob).data(), utf8FilePath(blob).data());
+    LOG(FileAPI, "FileReader: reading as array buffer: %s %s\n",
utf8BlobUUID(blob).data(), utf8FilePath(blob).data());
 
     readInternal(blob, FileReaderLoader::ReadAsArrayBuffer, es);
 }
@@ -107,7 +107,7 @@ void FileReader::readAsBinaryString(Blob* blob,
ExceptionState& es)
     if (!blob)
         return;
 
-    LOG(FileAPI, "FileReader: reading as binary: %s %s\n",
utf8BlobURL(blob).data(), utf8FilePath(blob).data());
+    LOG(FileAPI, "FileReader: reading as binary: %s %s\n",
utf8BlobUUID(blob).data(), utf8FilePath(blob).data());
 
     readInternal(blob, FileReaderLoader::ReadAsBinaryString, es);
 }
@@ -117,7 +117,7 @@ void FileReader::readAsText(Blob* blob, const String&
encoding, ExceptionState&
     if (!blob)
         return;
 
-    LOG(FileAPI, "FileReader: reading as text: %s %s\n",
utf8BlobURL(blob).data(), utf8FilePath(blob).data());
+    LOG(FileAPI, "FileReader: reading as text: %s %s\n",
utf8BlobUUID(blob).data(), utf8FilePath(blob).data());
 
     m_encoding = encoding;
     readInternal(blob, FileReaderLoader::ReadAsText, es);
@@ -133,7 +133,7 @@ void FileReader::readAsDataURL(Blob* blob, ExceptionState&
es)
     if (!blob)
         return;
 
-    LOG(FileAPI, "FileReader: reading as data URL: %s %s\n",
utf8BlobURL(blob).data(), utf8FilePath(blob).data());
+    LOG(FileAPI, "FileReader: reading as data URL: %s %s\n",
utf8BlobUUID(blob).data(), utf8FilePath(blob).data());
 
     readInternal(blob, FileReaderLoader::ReadAsDataURL, es);
 }
@@ -157,7 +157,7 @@ void FileReader::readInternal(Blob* blob,
FileReaderLoader::ReadType type, Excep
     m_loader = adoptPtr(new FileReaderLoader(m_readType, this));
     m_loader->setEncoding(m_encoding);
     m_loader->setDataType(m_blob->type());
-    m_loader->start(executionContext(), *m_blob);
+    m_loader->start(executionContext(), m_blob->blobDataHandle());
 }
 
 static void delayedAbort(ExecutionContext*, FileReader* reader)

[michaeln, 2013-10-15 00:01:50]

Oh no... looks like ExecutionContext has been renamed back... hooray for the
churn :/

[michaeln, 2013-10-15 00:30:58]

On 2013/10/15 00:01:50, michaeln wrote:
> Oh no... looks like ExecutionContext has been renamed back... hooray for the
> churn :/

No doubt i can expect it to toggle the other way again real soon now, this is no
fun trying to commit a blink change from within a chromium checkout whose
webkit/master branch is in the past. I've setup another git branch to track
origin/master and have cherry-picked this change into it and will (d)commit from
there.

[michaeln, 2013-10-15 00:37:10]

here's the other CL from the origin/master tracking branch for commit
https://codereview.chromium.org/26435003/

> No doubt i can expect it to toggle the other way again real soon now, this is
no
> fun trying to commit a blink change from within a chromium checkout whose
> webkit/master branch is in the past. I've setup another git branch to track
> origin/master and have cherry-picked this change into it and will (d)commit
from
> there.

[michaeln, 2013-10-15 20:17:36]

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