Bug 71115 – IndexedDB: Recycle cursor objects when calling continue()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hans Wennborg]

IndexedDB: Recycle cursor objects when calling continue()

[Hans Wennborg]

Created attachment 112858 [details]
Patch

[Hans Wennborg]

Comment on attachment 112858 [details]
Patch

(For reference, the Chromium side is here: http://codereview.chromium.org/8400061)

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[David Grogan]

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?

[Darin Fisher (:fishd, Google)]

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?

[Hans Wennborg]

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

[Hans Wennborg]

Created attachment 113058 [details]
Patch

[Darin Fisher (:fishd, Google)]

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

[Darin Fisher (:fishd, Google)]

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

[WebKit Review Bot]

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

[Hans Wennborg]

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

[Hans Wennborg]

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

[Darin Fisher (:fishd, Google)]

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

[Hans Wennborg]

Created attachment 113325 [details]
Patch

[Hans Wennborg]

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.

[Darin Fisher (:fishd, Google)]

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.

[David Grogan]

Comment on attachment 113325 [details]
Patch

LGTM

New stuff looks fine.

[Darin Fisher (:fishd, Google)]

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?

[Hans Wennborg]

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

[Hans Wennborg]

Committed r99169: <http://trac.webkit.org/changeset/99169>

[Darin Fisher (:fishd, Google)]

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.