Bug 92278 - IndexedDB: Pass cursor continue results back in callback
Summary: IndexedDB: Pass cursor continue results back in callback
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 523.x (Safari 3)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on: 91123 92414
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-25 12:27 PDT by Joshua Bell
Modified: 2012-08-14 09:07 PDT (History)
9 users (show)

See Also:


Attachments
Patch (36.15 KB, patch)
2012-07-25 17:15 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (36.99 KB, patch)
2012-07-26 13:36 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (39.85 KB, patch)
2012-07-26 15:14 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (37.54 KB, patch)
2012-08-01 12:25 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (39.07 KB, patch)
2012-08-09 14:15 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-07-25 12:27:14 PDT
Right now the message signaling the front-end that the cursor has advanced is done via onSuccessWithContinuation(). An event is queued, and then at dispatch time the cursor is told to go fetch copies from the back end (three separate sync IPC calls).

We should instead have onSuccessWithContinuation() bring the key, primarykey, and (optionally) value along with it.
Comment 1 Joshua Bell 2012-07-25 12:54:51 PDT
Note that per https://bugs.webkit.org/show_bug.cgi?id=91123 this would actually have to do key injection.
Comment 2 Joshua Bell 2012-07-25 15:46:48 PDT
Also need to make sure this doesn't break prefetching (which at an IPC level is done in a similar fashion - key/primaryKey/value are all sent at once)
Comment 3 Joshua Bell 2012-07-25 16:02:50 PDT
Digging further, we're already clever on the Chromium side and send the key/primaryKey/value along in the onSuccess(cursor) and onSuccessWithContinuation(), and cache it in the renderer! So this is mostly cleanup, not a measurable performance win.
Comment 4 Joshua Bell 2012-07-25 17:15:12 PDT
Created attachment 154498 [details]
Patch
Comment 5 Joshua Bell 2012-07-25 17:18:46 PDT
Attached patch is the WebKit "end state". Landing will require the usual "add new methods but support the old ones" dance.

The Chromium "end state" is at: https://chromiumcodereview.appspot.com/10830028
Comment 6 Joshua Bell 2012-07-26 13:00:45 PDT
Full landing sequence is: 

(1) WebKit prep: Add new API stubs http://webkit.org/b/92414 
(2) Chromium changes: dispatch to old and new API https://chromiumcodereview.appspot.com/10830028
(3) WebKit changes: use the new API http://webkit.org/b/92278 (this patch)
(4) Chromium cleanup: delete old API usage 
(5) WebKit changes: delete old API
Comment 7 Joshua Bell 2012-07-26 13:36:35 PDT
Created attachment 154738 [details]
Patch
Comment 8 WebKit Review Bot 2012-07-26 13:40:19 PDT
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 9 Darin Fisher (:fishd, Google) 2012-07-26 13:47:51 PDT
Comment on attachment 154738 [details]
Patch

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

> Source/WebKit/chromium/public/WebIDBCallbacks.h:63
> +    virtual void onSuccess(const WebIDBKey&, const WebIDBKey&, const WebSerializedScriptValue&) { WEBKIT_ASSERT_NOT_REACHED(); }

nit: two key parameters... might help to have names to differentiate them.
Comment 10 Darin Fisher (:fishd, Google) 2012-07-26 13:48:45 PDT
Comment on attachment 154738 [details]
Patch

Whoops, back to R? ... should get someone more familiar with IDB to sign off first.
Comment 11 Joshua Bell 2012-07-26 15:00:42 PDT
(In reply to comment #9)
> (From update of attachment 154738 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154738&action=review
> 
> > Source/WebKit/chromium/public/WebIDBCallbacks.h:63
> > +    virtual void onSuccess(const WebIDBKey&, const WebIDBKey&, const WebSerializedScriptValue&) { WEBKIT_ASSERT_NOT_REACHED(); }
> 
> nit: two key parameters... might help to have names to differentiate them.

Thanks. :) I was wondering whether I should do that over in https://bugs.webkit.org/show_bug.cgi?id=92414 - I'll add them in both patches.
Comment 12 Joshua Bell 2012-07-26 15:14:07 PDT
Created attachment 154764 [details]
Patch
Comment 13 Joshua Bell 2012-07-30 16:29:50 PDT
dgrogan@, alecflett@ - can you take a look?
Comment 14 Alec Flett 2012-07-31 10:50:52 PDT
Comment on attachment 154764 [details]
Patch

LGTM, though I worry this will get confused easiliy with

 virtual void onSuccess(PassRefPtr<SerializedScriptValue>, PassRefPtr<IDBKey>, const IDBKeyPath&) = 0;

Can you document the cases in IDBCallbacks.h?

I really wish IDBCallbacks were split up at least a little bit.. ah well.
Comment 15 Joshua Bell 2012-08-01 12:25:33 PDT
Created attachment 155861 [details]
Patch
Comment 16 Joshua Bell 2012-08-01 12:26:23 PDT
(In reply to comment #15)
> Created an attachment (id=155861) [details]
> Patch

New patch rebases, fixes ChangeLogs, and adds comments to IDBCallbacks.h
Comment 17 Joshua Bell 2012-08-09 14:15:30 PDT
Created attachment 157544 [details]
Patch
Comment 18 Joshua Bell 2012-08-09 14:17:41 PDT
One more rebase. Part #2 has landed on the Chromium side so this is good to go.

@abarth or @fishd, r?
Comment 19 Joshua Bell 2012-08-13 11:53:19 PDT
Oh, fishd@ already gave it a thumbs up (#9), but kicked it back to r? (#10)

tony@ - r?
Comment 20 WebKit Review Bot 2012-08-14 09:07:00 PDT
Comment on attachment 157544 [details]
Patch

Clearing flags on attachment: 157544

Committed r125568: <http://trac.webkit.org/changeset/125568>
Comment 21 WebKit Review Bot 2012-08-14 09:07:06 PDT
All reviewed patches have been landed.  Closing bug.