Bug 110398 - IndexedDB: Implement SharedBuffer version of put() / onSuccess()
Summary: IndexedDB: Implement SharedBuffer version of put() / onSuccess()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alec Flett
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-20 15:41 PST by Alec Flett
Modified: 2013-02-21 23:41 PST (History)
13 users (show)

See Also:


Attachments
Patch (43.70 KB, patch)
2013-02-20 15:46 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (42.60 KB, patch)
2013-02-21 11:29 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (45.55 KB, patch)
2013-02-21 14:46 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (45.94 KB, patch)
2013-02-21 15:53 PST, Alec Flett
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alec Flett 2013-02-20 15:41:18 PST
IndexedDB: Implement SharedBuffer version of put() / onSuccess()
Comment 1 Alec Flett 2013-02-20 15:46:07 PST
Created attachment 189396 [details]
Patch
Comment 2 Alec Flett 2013-02-20 15:46:36 PST
jsbell/dgrogan: quick review?
Comment 3 WebKit Review Bot 2013-02-20 15:49:07 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 Joshua Bell 2013-02-20 16:39:31 PST
Comment on attachment 189396 [details]
Patch

Overall lgtm...


View in context: https://bugs.webkit.org/attachment.cgi?id=189396&action=review

> Source/WebCore/ChangeLog:15
> +        (WebCore::IDBBackingStore::getRecord):

Add notes to these methods or delete them.

> Source/WebCore/ChangeLog:25
> +        (IDBBackingStore):

Especially delete these - they're not helpful.

> Source/WebCore/ChangeLog:42
> +        (WebCore):

Ditto.

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:1687
> +    value.append(t, result.end() - t);

Although you're not introducing it in this patch, can you rename |t| to |valuePosition| to match the other implementation of this function, for readability?

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:286
> +void IDBRequest::onSuccess(PassRefPtr<IDBCursorBackendInterface> backend, PassRefPtr<IDBKey> key, PassRefPtr<IDBKey> primaryKey, PassRefPtr<SharedBuffer> rawValue)

Rename rawValue to valueBuffer or just buffer?

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:384
> +    return onSuccessInternal(deserializeIDBValue(requestState(), value));

Ugh... need to add ScriptValue::createString/Number/Undefined/Null, so we can get SSV out of the loop and simplify IDBAny. But that's wkbug.com/100292

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:397
> +void IDBRequest::onSuccess(PassRefPtr<IDBKey> key, PassRefPtr<IDBKey> primaryKey, PassRefPtr<SharedBuffer> rawValue)

Rename rawValue to valueBuffer or just buffer?

> Source/WebKit/chromium/tests/IDBAbortOnCorruptTest.cpp:70
> +    virtual void onSuccessWithPrefetch(const Vector<RefPtr<IDBKey> >&, const Vector<RefPtr<IDBKey> >&, const Vector<RefPtr<SharedBuffer> >&) { }

OVERRIDE here?
Comment 5 Alec Flett 2013-02-21 11:29:43 PST
Created attachment 189565 [details]
Patch
Comment 6 Alec Flett 2013-02-21 11:30:54 PST
tony@ - r? for WebCore/storage/indexeddb/ ?
fishd@ or abarth@ - r? for WebKit/chromium ?
Comment 7 Alec Flett 2013-02-21 11:54:26 PST
(also note that this will likely fail the cr-* EWS bots until https://codereview.chromium.org/12326023/ lands on the chromium side)
Comment 8 Tony Chang 2013-02-21 12:26:03 PST
Comment on attachment 189565 [details]
Patch

GTK build failures look real.
Comment 9 Alec Flett 2013-02-21 14:46:19 PST
Created attachment 189605 [details]
Patch
Comment 10 Alec Flett 2013-02-21 15:53:57 PST
Created attachment 189624 [details]
Patch
Comment 11 WebKit Review Bot 2013-02-21 23:41:37 PST
Comment on attachment 189624 [details]
Patch

Clearing flags on attachment: 189624

Committed r143694: <http://trac.webkit.org/changeset/143694>
Comment 12 WebKit Review Bot 2013-02-21 23:41:41 PST
All reviewed patches have been landed.  Closing bug.