Summary: | IndexedDB: Pass cursor continue results back in callback | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joshua Bell <jsbell> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Joshua Bell <jsbell> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, alecflett, dglazkov, dgrogan, fishd, jamesr, tkent+wkapi, tony, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 523.x (Safari 3) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 91123, 92414 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Joshua Bell
2012-07-25 12:27:14 PDT
Note that per https://bugs.webkit.org/show_bug.cgi?id=91123 this would actually have to do key injection. 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) 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. Created attachment 154498 [details]
Patch
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 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 Created attachment 154738 [details]
Patch
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 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 on attachment 154738 [details]
Patch
Whoops, back to R? ... should get someone more familiar with IDB to sign off first.
(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. Created attachment 154764 [details]
Patch
dgrogan@, alecflett@ - can you take a look? 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.
Created attachment 155861 [details]
Patch
(In reply to comment #15) > Created an attachment (id=155861) [details] > Patch New patch rebases, fixes ChangeLogs, and adds comments to IDBCallbacks.h Created attachment 157544 [details]
Patch
One more rebase. Part #2 has landed on the Chromium side so this is good to go. @abarth or @fishd, r? Oh, fishd@ already gave it a thumbs up (#9), but kicked it back to r? (#10) tony@ - r? Comment on attachment 157544 [details] Patch Clearing flags on attachment: 157544 Committed r125568: <http://trac.webkit.org/changeset/125568> All reviewed patches have been landed. Closing bug. |