Issue 18590006: Blob support for IDB [Blink]

Issue 18590006: Blob support for IDB [Blink] (Closed)

Created:
7 years, 5 months ago by ericu

Modified:
6 years, 6 months ago

Reviewers:
haraken, jsbell, cmumford

CC:
blink-reviews, jamesr, jsbell+bindings_chromium.org, alecflett, eae+blinkwatch, abarth-chromium, marja+watch_chromium.org, dglazkov+blink, dgrogan, adamk+blink_chromium.org, Nate Chapin, do-not-use, jsbell

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

This is the master CL that contains the whole remaining Blink side. It turns on Blob support in IDB, so it has to wait until the Chromium side has completely landed. See https://codereview.chromium.org/18023022/ for the Chromium side. See https://docs.google.com/a/chromium.org/document/d/1Kdr4pcFt4QBDLLQn-fY4kZgw6ptmK23lthGZdQMVh2Y/edit for the big picture document [chromium.org account required]. Read-only view at https://docs.google.com/document/d/1Kdr4pcFt4QBDLLQn-fY4kZgw6ptmK23lthGZdQMVh2Y/pub [no account required]. BUG=108012 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175802

Patch Set 1 #

Patch Set 2 : Merged out #

Patch Set 3 : some compile fixes #

Patch Set 4 : It builds, with lots stubbed out. #

Patch Set 5 : WIP on serialization/deserialization #

Patch Set 6 : Blobs work again; Files still WIP. #

Patch Set 7 : Progress on writing files. #

Patch Set 8 : Progress on passing blob data out from IPCs. #

Patch Set 9 : Got the rest of the data passed through. #

Patch Set 10 : Working blob round-trip. #

Patch Set 11 : Clean up minor TODOs and comments. #

Patch Set 12 : Prefetch file metadata #

Patch Set 13 : Merged out [untested] #

Patch Set 14 : Merge fixes [builds, untested] #

Total comments: 15

Patch Set 15 : merged out paternity leave changes--untested, uncompiled #

Patch Set 16 : two merges no compile #

Patch Set 17 : Checkpoint work toward compiling. #

Patch Set 18 : compiles with hack #

Patch Set 19 : working but leaking blobs #

Patch Set 20 : Leak fixed via backchannel to ack blobs. #

Patch Set 21 : Fix deps, move files, clean up. #

Patch Set 22 : More code review feedback. #

Patch Set 23 : Tweaked a comment. #

Patch Set 24 : Make sure blobs aren't GCed while in use; fix a resource leak. #

Patch Set 25 : Remove some fprintfs #

Patch Set 26 : Merged out #

Patch Set 27 : small cleanup #

Patch Set 28 : webkit_unit_tests passing #

Patch Set 29 : bugfix for layout tests #

Patch Set 30 : All layout tests pass, including basic functionlity test. #

Patch Set 31 : Add missing layout test files. #

Patch Set 32 : Add cursor test. #

Patch Set 33 : Did one small TODO. #

Patch Set 34 : Update blobs.html expected output. #

Patch Set 35 : Add layout test for blob lifetime and content #

Patch Set 36 : Fix file extension #

Patch Set 37 : Layout test for deleting an objectstore and a database containing blobs. #

Patch Set 38 : small cleanup #

Total comments: 7

Patch Set 39 : merged out--haven't compiled yet #

Patch Set 40 : build fixes #

Patch Set 41 : Small code review fixes. #

Patch Set 42 : Fix a bad assert. #

Patch Set 43 : Tiny style tweak #

Patch Set 44 : Layout test for blob property in index key path. #

Patch Set 45 : Merged out #

Patch Set 46 : Merged out #

Patch Set 47 : Small tweaks #

Patch Set 48 : Merged out #

Patch Set 49 : Remove dead code. #

Patch Set 50 : Marged out #

Patch Set 51 : Merged out #

Patch Set 52 : Test reading blobs before they're committed. #

Patch Set 53 : Merged out #

Patch Set 54 : Fix merge #

Patch Set 55 : Move to new repo. #

Patch Set 56 : Merged out #

Patch Set 57 : Rebasing issue? #

Total comments: 21

Patch Set 58 : Roll in most of Josh's feedback. #

Total comments: 12

Patch Set 59 : More code review tweaks. #

Patch Set 60 : Punctuation typo fix. #

Patch Set 61 : Fix expectations too. #

Total comments: 6

Patch Set 62 : Fix a few last nits; I need to rerun try jobs anyway. #

Patch Set 63 : aaaaaaaand one more nit. #

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+560 lines, -188 lines)			Patch
A	LayoutTests/storage/indexeddb/blob-basics-metadata.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57	1 chunk	+137 lines, -0 lines	0 comments	Download
A	LayoutTests/storage/indexeddb/blob-basics-metadata-expected.txt	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57	1 chunk	+68 lines, -0 lines	0 comments	Download
A	LayoutTests/storage/indexeddb/blob-delete-objectstore-db.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59	1 chunk	+71 lines, -0 lines	0 comments	Download
A	LayoutTests/storage/indexeddb/blob-delete-objectstore-db-expected.txt	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60	1 chunk	+32 lines, -0 lines	0 comments	Download
A	LayoutTests/storage/indexeddb/blob-valid-after-deletion.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62	1 chunk	+86 lines, -0 lines	0 comments	Download
A	LayoutTests/storage/indexeddb/blob-valid-after-deletion-expected.txt	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36	1 chunk	+27 lines, -0 lines	0 comments	Download
A	LayoutTests/storage/indexeddb/blob-valid-before-commit.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61	1 chunk	+65 lines, -0 lines	0 comments	Download
A	LayoutTests/storage/indexeddb/blob-valid-before-commit-expected.txt	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51	1 chunk	+20 lines, -0 lines	0 comments	Download
M	LayoutTests/storage/indexeddb/keypath-intrinsic-properties-expected.txt	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57	2 chunks	+19 lines, -5 lines	0 comments	Download
D	LayoutTests/storage/indexeddb/noblobs.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29	1 chunk	+0 lines, -86 lines	0 comments	Download
D	LayoutTests/storage/indexeddb/noblobs-expected.txt	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38	1 chunk	+0 lines, -81 lines	0 comments	Download
M	LayoutTests/storage/indexeddb/resources/keypath-intrinsic-properties.js	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57	3 chunks	+35 lines, -4 lines	0 comments	Download
M	Source/bindings/v8/SerializedScriptValue.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51	1 chunk	+0 lines, -4 lines	0 comments	Download
M	Source/modules/indexeddb/IDBObjectStore.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57	1 chunk	+0 lines, -8 lines	0 comments	Download

Messages

Total messages: 18 (0 generated)

Expand Messages | Collapse Messages

jsbell

https://codereview.chromium.org/18590006/diff/34001/Source/bindings/v8/SerializedScriptValue.h File Source/bindings/v8/SerializedScriptValue.h (right): https://codereview.chromium.org/18590006/diff/34001/Source/bindings/v8/SerializedScriptValue.h#newcode63 Source/bindings/v8/SerializedScriptValue.h:63: static PassRefPtr<SerializedScriptValue> create(v8::Handle<v8::Value>, MessagePortArray*, ArrayBufferArray*, Vector<BlobInfo>*, bool& didThrow, v8::Isolate*); ...

7 years, 3 months ago (2013-09-12 22:52:17 UTC) #1

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

https://codereview.chromium.org/18590006/diff/34001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.h:63: static
PassRefPtr<SerializedScriptValue> create(v8::Handle<v8::Value>,
MessagePortArray*, ArrayBufferArray*, Vector<BlobInfo>*, bool& didThrow,
v8::Isolate*);
For symmetry, a with MessagePortArray and ArrayBufferArray, how about typedef
Vector<BlobInfo> BlobInfoArray ?

Within SSV at least, the use pattern is pretty much exactly the same.

https://codereview.chromium.org/18590006/diff/34001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.h:101: const Vector<BlobInfo>*
blobInfo() const { return m_blobInfo; }
So far as I can tell, this is only used in a DCHECK in the new code? 
Can we remove m_blobInfo entirely, so just the ephemeral Reader/Writer has it as
a member?

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBCursor.cpp:339: m_blobInfo.swap(blobInfo);
I'm waffling on OwnPtr<Vector> vs. swapping here. Blink seems to use both
patterns about equally.

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBObjectStore.cpp:224:
backendDB()->put(m_transaction->id(), id(), valueBuffer,
serializedValue->blobInfo(), key.release(),
static_cast<IDBDatabaseBackendInterface::PutMode>(putMode), request, indexIds,
indexKeys);
If we remove m_blobInfo from SSV, this can just the the local &blobInfo; to me
that's clearer since we've already asked the SSV to dump the blob metadata into
our local stash.

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBObjectStore.cpp:225: // TODO(ericu): Hold a
reference to each blob in serializedValue->blobInfo()
Adding a vector of blob refs to IDBTransaction sounds fine. The IDBRequest
itself may be collected earlier.

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBObjectStore.cpp:342:
generateIndexKeysForValue(request->requestState(), m_indexMetadata, value,
&indexKeys);
Although it's not obvious in the spec, this needs to handle a few Blob/File
properties:

https://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#key-path-construct

The scenario is that data containing Blobs already exists in a store. A new
index is created store.createIndex("mime-type-index", "blob.type");

When creating the index, this class will be used to fetch the records to index
them. We should ensure we're getting back the Blob metadata (which should be in
the serialized value) and that we can deserialize the SSV (which happens in the
IDBRequest which invokes this), but that we're not doing too much extra work
since the blob data itself will never be used here.

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBRequest.h (right):

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBRequest.h:83: virtual void
onSuccess(PassRefPtr<IDBCursorBackendInterface>, PassRefPtr<IDBKey>,
PassRefPtr<IDBKey> primaryKey, PassRefPtr<SharedBuffer>, const
Vector<BlobInfo>&);
I think this becomes more readable if the tuple being schlepped around is
(PassRefPtr<SharedBuffer>, PassOwnPtr<BlobInfoArray>), and the current owner
holds then in RefPtr/OwnPtr rather than one being "Pass"ed and the other being
"swap()"ed

ericu

As with the other CL, I've repaired the bit rot and cleaned it up a ...

7 years, 1 month ago (2013-11-20 23:06:08 UTC) #2

As with the other CL, I've repaired the bit rot and cleaned it up a bit.  Still
not completely baked, but I've responded to all the comments.

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

https://codereview.chromium.org/18590006/diff/34001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.h:63: static
PassRefPtr<SerializedScriptValue> create(v8::Handle<v8::Value>,
MessagePortArray*, ArrayBufferArray*, Vector<BlobInfo>*, bool& didThrow,
v8::Isolate*);
On 2013/09/12 22:52:17, jsbell wrote:
> For symmetry, a with MessagePortArray and ArrayBufferArray, how about typedef
> Vector<BlobInfo> BlobInfoArray ?
> 
> Within SSV at least, the use pattern is pretty much exactly the same.

Huh.  That won't compile, despite being identical.  Something to do with
overloaded methods wanting to know the full definition of BlobInfo?  I'm going
to put that on the low-priority list.

https://codereview.chromium.org/18590006/diff/34001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.h:101: const Vector<BlobInfo>*
blobInfo() const { return m_blobInfo; }
On 2013/09/12 22:52:17, jsbell wrote:
> So far as I can tell, this is only used in a DCHECK in the new code? 
> Can we remove m_blobInfo entirely, so just the ephemeral Reader/Writer has it
as
> a member?

Yup!  Done.

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBCursor.cpp:339: m_blobInfo.swap(blobInfo);
On 2013/09/12 22:52:17, jsbell wrote:
> I'm waffling on OwnPtr<Vector> vs. swapping here. Blink seems to use both
> patterns about equally.

Switched to OwnPtr.

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBObjectStore.cpp:224:
backendDB()->put(m_transaction->id(), id(), valueBuffer,
serializedValue->blobInfo(), key.release(),
static_cast<IDBDatabaseBackendInterface::PutMode>(putMode), request, indexIds,
indexKeys);
On 2013/09/12 22:52:17, jsbell wrote:
> If we remove m_blobInfo from SSV, this can just the the local &blobInfo; to me
> that's clearer since we've already asked the SSV to dump the blob metadata
into
> our local stash.

Done.

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBObjectStore.cpp:225: // TODO(ericu): Hold a
reference to each blob in serializedValue->blobInfo()
On 2013/09/12 22:52:17, jsbell wrote:
> Adding a vector of blob refs to IDBTransaction sounds fine. The IDBRequest
> itself may be collected earlier.

Updated TODO.

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBObjectStore.cpp:342:
generateIndexKeysForValue(request->requestState(), m_indexMetadata, value,
&indexKeys);
On 2013/09/12 22:52:17, jsbell wrote:
> Although it's not obvious in the spec, this needs to handle a few Blob/File
> properties:
> 
> https://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#key-path-construct
> 
> The scenario is that data containing Blobs already exists in a store. A new
> index is created store.createIndex("mime-type-index", "blob.type");
> 
> When creating the index, this class will be used to fetch the records to index
> them. We should ensure we're getting back the Blob metadata (which should be
in
> the serialized value) and that we can deserialize the SSV (which happens in
the
> IDBRequest which invokes this), but that we're not doing too much extra work
> since the blob data itself will never be used here.

Ah, I hadn't noticed that the blob properties were fair game for indexing. 
We'll have to talk about this some more.  I've got the metadata on the browser
side already, I just need to understand where I need to pipe it.

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBRequest.h (right):

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBRequest.h:83: virtual void
onSuccess(PassRefPtr<IDBCursorBackendInterface>, PassRefPtr<IDBKey>,
PassRefPtr<IDBKey> primaryKey, PassRefPtr<SharedBuffer>, const
Vector<BlobInfo>&);
On 2013/09/12 22:52:17, jsbell wrote:
> I think this becomes more readable if the tuple being schlepped around is
> (PassRefPtr<SharedBuffer>, PassOwnPtr<BlobInfoArray>), and the current owner
> holds then in RefPtr/OwnPtr rather than one being "Pass"ed and the other being
> "swap()"ed
> 

Done.

ericu

https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/IDBObjectStore.cpp File Source/modules/indexeddb/IDBObjectStore.cpp (right): https://codereview.chromium.org/18590006/diff/34001/Source/modules/indexeddb/IDBObjectStore.cpp#newcode342 Source/modules/indexeddb/IDBObjectStore.cpp:342: generateIndexKeysForValue(request->requestState(), m_indexMetadata, value, &indexKeys); On 2013/11/20 23:06:08, ericu wrote: ...

7 years ago (2013-11-26 00:26:13 UTC) #3

jsbell

Just some initial notes. Initial thought: I'd be nice if we could immediately stuff the ...

7 years ago (2013-12-20 19:13:53 UTC) #4

ericu

Regarding IDBAny: It's tempting to try to stuff the blob info in there earlier, but ...

6 years, 10 months ago (2014-02-12 19:12:54 UTC) #5

ericu

OK, the last of the Chromium side's in the commit queue. Once it lands, I'll ...

6 years, 6 months ago (2014-05-29 00:05:59 UTC) #6

jsbell

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/indexeddb/blob-basics-metadata-expected.txt File LayoutTests/storage/indexeddb/blob-basics-metadata-expected.txt (right): https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/indexeddb/blob-basics-metadata-expected.txt#newcode64 LayoutTests/storage/indexeddb/blob-basics-metadata-expected.txt:64: WARN: shouldBe() expects string arguments this is due to ...

6 years, 6 months ago (2014-05-29 00:40:05 UTC) #7

ericu

6 years, 6 months ago (2014-06-02 22:18:47 UTC) #8

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/ind...
File LayoutTests/storage/indexeddb/blob-basics-metadata-expected.txt (right):

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/ind...
LayoutTests/storage/indexeddb/blob-basics-metadata-expected.txt:64: WARN:
shouldBe() expects string arguments
On 2014/05/29 00:40:06, jsbell wrote:
> this is due to shouldBeNull(cursor) w/o quotes

Done.

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/ind...
File LayoutTests/storage/indexeddb/blob-basics-metadata.html (right):

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/ind...
LayoutTests/storage/indexeddb/blob-basics-metadata.html:92:
shouldBeNull(cursor);
On 2014/05/29 00:40:06, jsbell wrote:
> should be: shouldBeNull("cursor")

Done.

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/ind...
LayoutTests/storage/indexeddb/blob-basics-metadata.html:112: doRead(keyName,
readTransactionOnComplete);
On 2014/05/29 00:40:06, jsbell wrote:
> Personally, I find the use of getValidator here makes this harder to follow
than
> just inlining the callback function definition inside this doRead call.

Done; I suspect that was left over from more complex logic I had in a previous
version--I agree with you that this is more readable.

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/ind...
File LayoutTests/storage/indexeddb/blob-delete-objectstore-db.html (right):

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/ind...
LayoutTests/storage/indexeddb/blob-delete-objectstore-db.html:9:
description("Test deleting an object store and a database containing blobs.");
On 2014/05/29 00:40:06, jsbell wrote:
> Since this test doesn't have any shouldXXXX() assertions, maybe add a note
that
> it's just watching for crashes.

Done.

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/ind...
LayoutTests/storage/indexeddb/blob-delete-objectstore-db.html:19: blobA = new
Blob([blobAContent], {"type" : "text\/plain"});
On 2014/05/29 00:40:06, jsbell wrote:
> No need to escape the forward slash here (and below)

Done.

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/ind...
LayoutTests/storage/indexeddb/blob-delete-objectstore-db.html:22: delete blobA;
On 2014/05/29 00:40:06, jsbell wrote:
> This is unusual; it's unclear why you'd write this with the delete operator,
> other than to "scope" the variable while still exposing it to the
> evalAndLog()-family of helpers. We don't do that anywhere else in our tests.
> 
> The variable isn't referenced elsewhere, either, so it's not even worth doing
> `blobA = null` or `blobA = undefined`. It makes it read like an attempt to
> release the blob, but that won't happen without e.g. an explicit gc() call,
and
> then the idiomatic form in tests would be `blobA = null; gc();`

Fixed; I'd misremembered how delete worked.
As it turns out, there's no reason we even need to drop the reference here.

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/ind...
File LayoutTests/storage/indexeddb/blob-valid-after-deletion.html (right):

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/ind...
LayoutTests/storage/indexeddb/blob-valid-after-deletion.html:33:
debug("doRead():");
On 2014/05/29 00:40:06, jsbell wrote:
> FYI, we have a `preamble();` helper in shared.js that logs the current
function
> name automagically, which makes maintenance easier.

Done.

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/ind...
LayoutTests/storage/indexeddb/blob-valid-after-deletion.html:72: delete record;
On 2014/05/29 00:40:06, jsbell wrote:
> This use of the delete operator is even weirder. What's the intent?

It's a brain-o, and furthermore, completely superfluous.  Removed.

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/ind...
File LayoutTests/storage/indexeddb/resources/keypath-intrinsic-properties.js
(right):

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/ind...
LayoutTests/storage/indexeddb/resources/keypath-intrinsic-properties.js:16:
evalAndLog("store.createIndex('blob size', 'blob.size')");
On 2014/05/29 00:40:06, jsbell wrote:
> The IDB spec also has Blob.size, File.name and File.lastModifiedDate - is
there
> a way to test those as well?
> 
> https://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#key-path-construct

I added Blob.type. File.name and File.lastModifiedDate are a little more
cumbersome due to the lack of a File constructor.  I've tested elsewhere that
File and FileList, including their intrinsic properties, generally work. 
There's no specific code to handle indexes, so it should be the case that if any
one Blob-related intrinsic property works with keypaths, they all do.  Given
that, do you think we need a specific test for the File ones?

https://codereview.chromium.org/18590006/diff/1450001/LayoutTests/storage/ind...
LayoutTests/storage/indexeddb/resources/keypath-intrinsic-properties.js:35:
evalAndLog("store.put("+JSON.stringify(datum)+")");
On 2014/05/29 00:40:06, jsbell wrote:
> Note that the put() call is being constructed from a string here; the Blob is
> getting stringified, so this doesn't actually serialize a Blob, just an object
> that's blob-shaped like {"type":"","size":0}

Ah, good catch--I missed that.  Array makes the translation back and forth
seamlessly, but Blob doesn't.  Switched to remove the stringification for the
actual put call; I've kept it for progress output.

jsbell

lgtm! None of the suggestions are very important. You'll need an OWNERS review for SerializedScriptValue.h ...

6 years, 6 months ago (2014-06-02 23:05:06 UTC) #9

ericu

https://codereview.chromium.org/18590006/diff/1470001/LayoutTests/storage/indexeddb/blob-basics-metadata.html File LayoutTests/storage/indexeddb/blob-basics-metadata.html (right): https://codereview.chromium.org/18590006/diff/1470001/LayoutTests/storage/indexeddb/blob-basics-metadata.html#newcode12 LayoutTests/storage/indexeddb/blob-basics-metadata.html:12: fileInput = document.getElementById("fileInput"); On 2014/06/02 23:05:06, jsbell wrote: > ...

6 years, 6 months ago (2014-06-02 23:53:07 UTC) #10

ericu

+haraken as owner, for trivial deletion in SerializedScriptValue API. I'll kick off a try run ...

6 years, 6 months ago (2014-06-02 23:56:07 UTC) #11

cmumford

lgtm https://codereview.chromium.org/18590006/diff/1520015/LayoutTests/storage/indexeddb/blob-valid-after-deletion.html File LayoutTests/storage/indexeddb/blob-valid-after-deletion.html (right): https://codereview.chromium.org/18590006/diff/1520015/LayoutTests/storage/indexeddb/blob-valid-after-deletion.html#newcode22 LayoutTests/storage/indexeddb/blob-valid-after-deletion.html:22: var blobA = new Blob([blobAContent], {"type" : "text\/plain"}); ...

6 years, 6 months ago (2014-06-04 21:01:18 UTC) #13

jsbell

Just answering for Eric since he's away... https://codereview.chromium.org/18590006/diff/1520015/LayoutTests/storage/indexeddb/blob-valid-after-deletion.html File LayoutTests/storage/indexeddb/blob-valid-after-deletion.html (right): https://codereview.chromium.org/18590006/diff/1520015/LayoutTests/storage/indexeddb/blob-valid-after-deletion.html#newcode22 LayoutTests/storage/indexeddb/blob-valid-after-deletion.html:22: var blobA ...

6 years, 6 months ago (2014-06-04 21:56:25 UTC) #14

ericu

https://codereview.chromium.org/18590006/diff/1520015/LayoutTests/storage/indexeddb/blob-valid-after-deletion.html File LayoutTests/storage/indexeddb/blob-valid-after-deletion.html (right): https://codereview.chromium.org/18590006/diff/1520015/LayoutTests/storage/indexeddb/blob-valid-after-deletion.html#newcode22 LayoutTests/storage/indexeddb/blob-valid-after-deletion.html:22: var blobA = new Blob([blobAContent], {"type" : "text\/plain"}); On ...

6 years, 6 months ago (2014-06-06 22:57:00 UTC) #15

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericu@chromium.org/18590006/1550001

6 years, 6 months ago (2014-06-09 16:18:37 UTC) #17

Message was sent while issue was closed.

Change committed as 175802

Expand Messages | Collapse Messages