Bug 95409

Summary: IndexedDB: Use ScriptValue instead of SerializedScriptValue for get/openCursor
Product: WebKit Reporter: Alec Flett <alecflett>
Component: WebCore Misc.Assignee: Alec Flett <alecflett>
Severity: Normal CC: abarth, donggwan.kim, haraken, japhet, michael, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 94023, 95385    
Bug Blocks: 88287    
Description Flags
Patch for landing
Patch for landing none

Description Alec Flett 2012-08-29 17:36:55 PDT
This is the second half of bug 94023, and also involves a lot of code removal that will be easier after bug 95385 lands.
Comment 1 Alec Flett 2012-09-14 12:30:54 PDT
Created attachment 164206 [details]
Comment 2 Alec Flett 2012-09-14 12:31:45 PDT
haraken@ - this is the second half of bug 94023 - mind taking a look at this?
Comment 3 Kentaro Hara 2012-09-14 14:13:21 PDT
Comment on attachment 164206 [details]

LGTM at a first quick look. I'm now leaving, let me take a detailed look tomorrow.
Comment 4 Kentaro Hara 2012-09-15 06:30:02 PDT
Comment on attachment 164206 [details]

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

LGTM with some nits.

> Source/WebCore/Modules/indexeddb/IDBAny.h:63
> +    static PassRefPtr<IDBAny> create(const T& idbObject)

Are both create(const T& idbObject) and create(PassRefPtr<T> idbObject) needed? You might want to remove create(PassRefPtr<T> idbObject) in favor of create(const T& idbObject).

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:276
> +            ASSERT_UNUSED(injected, injected);

Given that the current code has ASSERT(valueAfterInjection), shouldn't this be ASSERT(injected)?

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:277
>              // FIXME: There is no way to report errors here. Move this into onSuccessWithContinuation so that we can abort the transaction there. See: https://bugs.webkit.org/show_bug.cgi?id=92278

You can move this comment to just before the ASSERT().

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:376
> +    ASSERT(injected);
> +    if (!injected) {

This code looks a bit strange:

- If 'injected' can be 0, 'ASSERT(injected)' should be removed.
- If 'injected' shouldn't be 0, 'if (!injected)' should be removed.
Comment 5 Alec Flett 2012-09-17 11:25:08 PDT
Created attachment 164423 [details]
Patch for landing
Comment 6 WebKit Review Bot 2012-09-17 11:31:02 PDT
Comment on attachment 164423 [details]
Patch for landing

Rejecting attachment 164423 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
oid WebCore::IDBRequest::onSuccess(WTF::PassRefPtr<WebCore::SerializedScriptValue>, WTF::PassRefPtr<WebCore::IDBKey>, const WebCore::IDBKeyPath&)':
Source/WebCore/Modules/indexeddb/IDBRequest.cpp:374: error: unused variable 'injected'
  CXX(target) out/Release/obj.target/webcore_remaining/Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.o
make: *** [out/Release/obj.target/webcore_remaining/Source/WebCore/Modules/indexeddb/IDBRequest.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/13860975
Comment 7 Alec Flett 2012-09-17 11:41:13 PDT
Created attachment 164425 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-09-17 12:03:16 PDT
Comment on attachment 164425 [details]
Patch for landing

Clearing flags on attachment: 164425

Committed r128789: <http://trac.webkit.org/changeset/128789>
Comment 9 WebKit Review Bot 2012-09-17 12:03:20 PDT
All reviewed patches have been landed.  Closing bug.