Bug 92278 – IndexedDB: Pass cursor continue results back in callback

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joshua Bell]

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.

[Joshua Bell]

Note that per https://bugs.webkit.org/show_bug.cgi?id=91123 this would actually have to do key injection.

[Joshua Bell]

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)

[Joshua Bell]

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.

[Joshua Bell]

Created attachment 154498 [details]
Patch

[Joshua Bell]

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

[Joshua Bell]

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

[Joshua Bell]

Created attachment 154738 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

Comment on attachment 154738 [details]
Patch

Whoops, back to R? ... should get someone more familiar with IDB to sign off first.

[Joshua Bell]

(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.

[Joshua Bell]

Created attachment 154764 [details]
Patch

[Joshua Bell]

dgrogan@, alecflett@ - can you take a look?

[Alec Flett]

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.

[Joshua Bell]

Created attachment 155861 [details]
Patch

[Joshua Bell]

(In reply to comment #15)
> Created an attachment (id=155861) [details]
> Patch

New patch rebases, fixes ChangeLogs, and adds comments to IDBCallbacks.h

[Joshua Bell]

Created attachment 157544 [details]
Patch

[Joshua Bell]

One more rebase. Part #2 has landed on the Chromium side so this is good to go.

@abarth or @fishd, r?

[Joshua Bell]

Oh, fishd@ already gave it a thumbs up (#9), but kicked it back to r? (#10)

tony@ - r?

[WebKit Review Bot]

Comment on attachment 157544 [details]
Patch

Clearing flags on attachment: 157544

Committed r125568: <http://trac.webkit.org/changeset/125568>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.