Bug 71115 - IndexedDB: Recycle cursor objects when calling continue()
Summary: IndexedDB: Recycle cursor objects when calling continue()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Wennborg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-28 06:37 PDT by Hans Wennborg
Modified: 2011-11-03 09:32 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.18 KB, patch)
2011-10-28 06:49 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (9.96 KB, patch)
2011-10-31 09:22 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (12.35 KB, patch)
2011-11-02 10:12 PDT, Hans Wennborg
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Wennborg 2011-10-28 06:37:07 PDT
IndexedDB: Recycle cursor objects when calling continue()
Comment 1 Hans Wennborg 2011-10-28 06:49:35 PDT
Created attachment 112858 [details]
Patch
Comment 2 Hans Wennborg 2011-10-28 06:59:58 PDT
Comment on attachment 112858 [details]
Patch

(For reference, the Chromium side is here: http://codereview.chromium.org/8400061)
Comment 3 WebKit Review Bot 2011-10-28 07:02:36 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 David Grogan 2011-10-28 17:04:06 PDT
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 5 Darin Fisher (:fishd, Google) 2011-10-30 22:51:47 PDT
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?
Comment 6 Hans Wennborg 2011-10-31 09:21:59 PDT
(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?
Comment 7 Hans Wennborg 2011-10-31 09:22:25 PDT
Created attachment 113058 [details]
Patch
Comment 8 Darin Fisher (:fishd, Google) 2011-10-31 11:26:22 PDT
(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.
Comment 9 Darin Fisher (:fishd, Google) 2011-10-31 11:27:18 PDT
(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 10 WebKit Review Bot 2011-10-31 12:01:32 PDT
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
Comment 11 Hans Wennborg 2011-11-01 09:14:29 PDT
(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.
Comment 12 Hans Wennborg 2011-11-01 09:15:41 PDT
(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.
Comment 13 Darin Fisher (:fishd, Google) 2011-11-01 16:08:57 PDT
(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.
Comment 14 Hans Wennborg 2011-11-02 10:12:03 PDT
Created attachment 113325 [details]
Patch
Comment 15 Hans Wennborg 2011-11-02 10:13:09 PDT
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 16 Darin Fisher (:fishd, Google) 2011-11-02 16:47:54 PDT
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 17 David Grogan 2011-11-02 17:46:30 PDT
Comment on attachment 113325 [details]
Patch

LGTM

New stuff looks fine.
Comment 18 Darin Fisher (:fishd, Google) 2011-11-02 21:28:26 PDT
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?
Comment 19 Hans Wennborg 2011-11-03 04:52:19 PDT
(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.
Comment 20 Hans Wennborg 2011-11-03 05:04:16 PDT
Committed r99169: <http://trac.webkit.org/changeset/99169>
Comment 21 Darin Fisher (:fishd, Google) 2011-11-03 09:32:56 PDT
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.