IndexedDB: Recycle cursor objects when calling continue()
Created attachment 112858 [details] Patch
Comment on attachment 112858 [details] Patch (For reference, the Chromium side is here: http://codereview.chromium.org/8400061)
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment on attachment 112858 [details] Patch LGTM (some comments on the chromium patch) So you're going to land this whole patch, then the chromium side, then a webcore patch for the FIXME in IDBCursorBackendImpl.cpp?
Comment on attachment 112858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112858&action=review > Source/WebCore/ChangeLog:13 > + The Chromium code needs an update before we can actually start using nit: this probably doesn't need to be in the ChangeLog. this won't help someone who is looking at the ChangeLog to understand this patch. maybe this information can just be in the bug? > Source/WebCore/storage/IDBCallbacks.h:58 > + virtual void onSuccessCursorContinue() = 0; is there perhaps a better name for this method? i'm struggling to understand this method based on its name. it seems like all of the other onSuccess variants provide a result, but this one is about not supplying a result but still reporting success?
(In reply to comment #5) > (From update of attachment 112858 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=112858&action=review > > > Source/WebCore/ChangeLog:13 > > + The Chromium code needs an update before we can actually start using > > nit: this probably doesn't need to be in the ChangeLog. this won't help > someone who is looking at the ChangeLog to understand this patch. maybe > this information can just be in the bug? You're right. Done. > > > Source/WebCore/storage/IDBCallbacks.h:58 > > + virtual void onSuccessCursorContinue() = 0; > > is there perhaps a better name for this method? i'm struggling to understand > this method based on its name. it seems like all of the other onSuccess variants > provide a result, but this one is about not supplying a result but still reporting > success? Yeah, naming is hard. This onSuccess method is special in that it doesn't provide a result, as you say. Instead, it just indicates that the cursor continue was successful, and then the object which implements the interface will know what to do with it. We're generally very restrictive with comments in WebKit code, but maybe this case warrants one?
Created attachment 113058 [details] Patch
(In reply to comment #6) > This onSuccess method is special in that it doesn't provide a result, as you say. Instead, it just indicates that the cursor continue was successful, and then the object which implements the interface will know what to do with it. Is "cursor continue" really the right term for this? onSuccessWithCursorContinuation? onSuccessWithContinuation? > We're generally very restrictive with comments in WebKit code, but maybe this case warrants one? I think a better name would be helpful. If we find the right name, then yes, comments probably wouldn't be needed.
(In reply to comment #8) > (In reply to comment #6) > > This onSuccess method is special in that it doesn't provide a result, as you say. Instead, it just indicates that the cursor continue was successful, and then the object which implements the interface will know what to do with it. > > Is "cursor continue" really the right term for this? onSuccessWithCursorContinuation? onSuccessWithContinuation? By the way, the "on" prefix is not very helpful here. Could this also be called didContinue?
Comment on attachment 113058 [details] Patch Attachment 113058 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10245377 New failing tests: inspector/debugger/bind-script-to-resource.html media/media-blocked-by-willsendrequest.html storage/indexeddb/cursor-inconsistency.html
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #6) > > > This onSuccess method is special in that it doesn't provide a result, as you say. Instead, it just indicates that the cursor continue was successful, and then the object which implements the interface will know what to do with it. > > > > Is "cursor continue" really the right term for this? onSuccessWithCursorContinuation? onSuccessWithContinuation? > > By the way, the "on" prefix is not very helpful here. Could this also be called didContinue? The other callbacks are called onSuccess because they correspond directly to onSuccess in the IndexedDB spec. I think onSuccessWithContinuation would be a good name. Let me know if you think that's ok, and I'll upload a new version of the patch.
(In reply to comment #10) > storage/indexeddb/cursor-inconsistency.html This failed because I uploaded the wrong version of the patch. I'll re-upload once we settle on the name issue.
(In reply to comment #11) > I think onSuccessWithContinuation would be a good name. That works grammatically. OK by me. > > Let me know if you think that's ok, and I'll upload a new version of the patch.
Created attachment 113325 [details] Patch
New version uploaded that renames the callback. This also tries to break the reference cycle between the IDBRequest and the IDBCursor. IDBRequest will now only hold a reference to the IDBCursor when a continue() call is pending. Please re-review.
Comment on attachment 113325 [details] Patch WebKit API changes LGTM. If you could get David or Josh to LGTM the rest, I'll R+ the patch.
Comment on attachment 113325 [details] Patch LGTM New stuff looks fine.
Comment on attachment 113325 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113325&action=review > Source/WebCore/storage/IDBRequest.cpp:375 > + RefPtr<IDBCursorWithValue> cursorWithValue(static_cast<IDBCursorWithValue*>(prpCursor.get())); I think this would be much nicer if you had a function like this: static PassRefPtr<IDBCursorWithValue> IDBCursorWithValue::fromCursor(PassRefPtr<IDBCursor>) Then, this callsite would turn into: m_result = IDBAny::create(IDBCursorWithValue::fromCursor(prpCursor)); Nice, right?
(In reply to comment #18) > (From update of attachment 113325 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113325&action=review > > > Source/WebCore/storage/IDBRequest.cpp:375 > > + RefPtr<IDBCursorWithValue> cursorWithValue(static_cast<IDBCursorWithValue*>(prpCursor.get())); > > I think this would be much nicer if you had a function like this: > > static PassRefPtr<IDBCursorWithValue> IDBCursorWithValue::fromCursor(PassRefPtr<IDBCursor>) > > Then, this callsite would turn into: > > m_result = IDBAny::create(IDBCursorWithValue::fromCursor(prpCursor)); > > Nice, right? Thanks Darin, that makes it nicer. Will do that and land.
Committed r99169: <http://trac.webkit.org/changeset/99169>
FWIW, an alternative would have been a toIDBCursorWithValue(PassRefPtr<IDBCursor>) function. That might actually be a more canonical form of a casting function in WebCore. Either way is fine by me, but just thought I'd mention this for completeness.