Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8400061:
IndexedDB: Recycle cursor objects when calling continue(). (Closed)
|
Created:
9 years, 1 month ago by hans Modified:
9 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, dpranke-watch+content_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIndexedDB: Recycle curosr objects when calling continue().
This is the Chromium side of https://bugs.webkit.org/show_bug.cgi?id=71115
Instead of creating a new IDBCursor wrapper each time continue() is called,
we should instead send back a new onSuccessCursorContinue() callback, and the
IDBRequest will know which cursor to return.
This patch implements the new callback.
For performance, we piggy-pick the cursor's current key, primary key and
value with the callback. To be able to do this, the IndexedDBDispatcherHost
must keep track of which cursor corresponds to which pending callback.
The IndexedDBDispatcher keeps the key, primary key and value cached
for each cursor.
BUG=98685
TEST=all current tests pass
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108788
Patch Set 1 #
Total comments: 24
Patch Set 2 : New version #
Total comments: 10
Patch Set 3 : New version #
Total comments: 8
Patch Set 4 : Callback renamed to onSuccessWithContinuation, and style fixes #
Total comments: 10
Patch Set 5 : More style nits fixed #Messages
Total messages: 25 (0 generated)
Please take a look. I'd be especially grateful if you can help me look for threading issues. I was a little bit unsure about how IndexedDBDispatcher is accessed (on which thread the OnSuccessCursorContinue callback is called, and on which thread the RendererWebIDBCursorImpl::value() etc. are called). It seems the calls are always on the WebKit thread, but I couldn't provide any hard evidence.
http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... File content/renderer/indexed_db_dispatcher.cc (right): http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... content/renderer/indexed_db_dispatcher.cc:439: primaryKey, value)); You can remove the non-object_id parameters. http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... content/renderer/indexed_db_dispatcher.cc:441: cursor_key_[object_id] = key; If the onSuccess call is synchronous, shouldn't these be before it? I don't know if it is. http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... File content/renderer/indexed_db_dispatcher.h (right): http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... content/renderer/indexed_db_dispatcher.h:198: // FIXME: Double check these are only used on WebKit thread. You could use IDMap and AddWithID. IDMap ensures it's not called on multiple threads. But you don't have any threading problems. Incoming messages are already on the right thread by the time they get to IndexedDBDispatcher. http://codereview.chromium.org/8400061/diff/1/content/renderer/renderer_webid... File content/renderer/renderer_webidbcursor_impl.cc (left): http://codereview.chromium.org/8400061/diff/1/content/renderer/renderer_webid... content/renderer/renderer_webidbcursor_impl.cc:42: return key_; I think you can delete these data members.
http://codereview.chromium.org/8400061/diff/1/content/browser/in_process_webk... File content/browser/in_process_webkit/indexed_db_callbacks.cc (right): http://codereview.chromium.org/8400061/diff/1/content/browser/in_process_webk... content/browser/in_process_webkit/indexed_db_callbacks.cc:44: void IndexedDBCallbacks<WebKit::WebIDBCursor>::onSuccessCursorContinue() { Would it make sense to pass the WebIDBCursor as a parameter to this method instead of doing map look ups? http://codereview.chromium.org/8400061/diff/1/content/browser/in_process_webk... content/browser/in_process_webkit/indexed_db_callbacks.cc:49: What ensures that the idb_cursor isn't deleted between the continue call and the completion call? Maybe this is entirely synchronous such that onSuccessCursorContinue() is invoked prior to return from the continue() method? http://codereview.chromium.org/8400061/diff/1/content/browser/in_process_webk... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/8400061/diff/1/content/browser/in_process_webk... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:954: parent_->pending_cursor_continues_[response_id] = cursor_id; What if the continue call does not complete successfully? How is the map entry removed in that case? http://codereview.chromium.org/8400061/diff/1/content/common/indexed_db_messa... File content/common/indexed_db_messages.h (right): http://codereview.chromium.org/8400061/diff/1/content/common/indexed_db_messa... content/common/indexed_db_messages.h:150: IPC_MESSAGE_CONTROL4(IndexedDBMsg_CallbacksSuccessCursorContinue, Would it make sense to have cursor_id as a param here too http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... File content/renderer/indexed_db_dispatcher.cc (right): http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... content/renderer/indexed_db_dispatcher.cc:449: int32 response_id, cursor_id could be a useful here, i think you wouldn't need the pending_cursor_continues_ map http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... content/renderer/indexed_db_dispatcher.cc:454: Is there any chance the cursor was deleted prior to this completion callback? http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... content/renderer/indexed_db_dispatcher.cc:457: cursor_value_[cursor_id] = value; The RendererWebIDBCursorImpl has data members for each of these things. It might be cleaner to keep these values in an instance of that object instead of in these maps. I think what's missing is a way to lookup a RendererWebIDBCursorImpl by id. std::map<int32, RendererWebIDBCursorImpl*> cursors_;
http://codereview.chromium.org/8400061/diff/1/content/browser/in_process_webk... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/8400061/diff/1/content/browser/in_process_webk... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:954: parent_->pending_cursor_continues_[response_id] = cursor_id; On 2011/10/30 21:54:06, michaeln wrote: > What if the continue call does not complete successfully? How is the map entry > removed in that case? Looks like there are two ways in which continue does not succeed... - the method call below throws an exception by setting *ec to non-zero - instead of onSuccessCursorContinue() eventually being called, onSuccess(nullValue()) is called. Instead of holding this extra state in a separate map entry, have you considered stuffing the required state into the IndexedDBCallbacks<WebIDBCursor> instance so that it knows what cursor_id its dealing with and there are no extra map entries to cleanup on any of the possible return path.
Thanks very much for your comments, it's very helpful. New version coming up. http://codereview.chromium.org/8400061/diff/1/content/browser/in_process_webk... File content/browser/in_process_webkit/indexed_db_callbacks.cc (right): http://codereview.chromium.org/8400061/diff/1/content/browser/in_process_webk... content/browser/in_process_webkit/indexed_db_callbacks.cc:44: void IndexedDBCallbacks<WebKit::WebIDBCursor>::onSuccessCursorContinue() { On 2011/10/30 21:54:06, michaeln wrote: > Would it make sense to pass the WebIDBCursor as a parameter to this method > instead of doing map look ups? Can't change the function's signature since it's an override of WebIDBCallbacks::onSuccessCursorContinue(), but I can pass in the cursor id when creating the callbacks object. That lets us get rid of the pending_cursor_continues_ as you suggest in a later comment. http://codereview.chromium.org/8400061/diff/1/content/browser/in_process_webk... content/browser/in_process_webkit/indexed_db_callbacks.cc:49: On 2011/10/30 21:54:06, michaeln wrote: > What ensures that the idb_cursor isn't deleted between the continue call and the > completion call? Maybe this is entirely synchronous such that > onSuccessCursorContinue() is invoked prior to return from the continue() method? It's not synchronous. It's actually the idb_cursor object that's calling the onSuccessCursorContinue() method. So if it's been deleted, we would probably have already crashed. The deletion happens when the RendererWebIDBCursorImpl is destroyed in the renderer process, causing an IndexedDBHostMsg_CursorDestroyed to be sent to the browser process. There's a reassuring comment in ~RendererWebIDBCursorImpl(), saying "It's not possible for there to be pending callbacks that address this object since inside WebKit, they hold a reference to the object wich owns this object." So I think it's safe. http://codereview.chromium.org/8400061/diff/1/content/browser/in_process_webk... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/8400061/diff/1/content/browser/in_process_webk... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:954: parent_->pending_cursor_continues_[response_id] = cursor_id; On 2011/10/31 15:08:18, michaeln wrote: > On 2011/10/30 21:54:06, michaeln wrote: > > What if the continue call does not complete successfully? How is the map entry > > removed in that case? > > Looks like there are two ways in which continue does not succeed... > - the method call below throws an exception by setting *ec to non-zero > - instead of onSuccessCursorContinue() eventually being called, > onSuccess(nullValue()) is called. > > Instead of holding this extra state in a separate map entry, have you considered > stuffing the required state into the IndexedDBCallbacks<WebIDBCursor> instance > so that it knows what cursor_id its dealing with and there are no extra map > entries to cleanup on any of the possible return path. Right, passing the cursor_id into the callbacks object solves this problem nicely, so let's do that. http://codereview.chromium.org/8400061/diff/1/content/common/indexed_db_messa... File content/common/indexed_db_messages.h (right): http://codereview.chromium.org/8400061/diff/1/content/common/indexed_db_messa... content/common/indexed_db_messages.h:150: IPC_MESSAGE_CONTROL4(IndexedDBMsg_CallbacksSuccessCursorContinue, On 2011/10/30 21:54:06, michaeln wrote: > Would it make sense to have cursor_id as a param here too Yes. Doing that. http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... File content/renderer/indexed_db_dispatcher.cc (right): http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... content/renderer/indexed_db_dispatcher.cc:439: primaryKey, value)); On 2011/10/29 00:54:28, dgrogan wrote: > You can remove the non-object_id parameters. Done. http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... content/renderer/indexed_db_dispatcher.cc:441: cursor_key_[object_id] = key; On 2011/10/29 00:54:28, dgrogan wrote: > If the onSuccess call is synchronous, shouldn't these be before it? I don't > know if it is. It's not synchronous, but it certainly looks weird. Fixed. http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... content/renderer/indexed_db_dispatcher.cc:449: int32 response_id, On 2011/10/30 21:54:06, michaeln wrote: > cursor_id could be a useful here, i think you wouldn't need the > pending_cursor_continues_ map Done. http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... content/renderer/indexed_db_dispatcher.cc:454: On 2011/10/30 21:54:06, michaeln wrote: > Is there any chance the cursor was deleted prior to this completion callback? No, according to the comment in ~RendererWebIDBCursorImpl, the callback object holds a reference to the cursor. http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... content/renderer/indexed_db_dispatcher.cc:457: cursor_value_[cursor_id] = value; On 2011/10/30 21:54:06, michaeln wrote: > The RendererWebIDBCursorImpl has data members for each of these things. It might > be cleaner to keep these values in an instance of that object instead of in > these maps. Yes, doing that. > > I think what's missing is a way to lookup a RendererWebIDBCursorImpl by id. > > std::map<int32, RendererWebIDBCursorImpl*> cursors_; Doing that. http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... File content/renderer/indexed_db_dispatcher.h (right): http://codereview.chromium.org/8400061/diff/1/content/renderer/indexed_db_dis... content/renderer/indexed_db_dispatcher.h:198: // FIXME: Double check these are only used on WebKit thread. On 2011/10/29 00:54:28, dgrogan wrote: > You could use IDMap and AddWithID. IDMap ensures it's not called on multiple > threads. But you don't have any threading problems. Incoming messages are > already on the right thread by the time they get to IndexedDBDispatcher. OK, thanks. http://codereview.chromium.org/8400061/diff/1/content/renderer/renderer_webid... File content/renderer/renderer_webidbcursor_impl.cc (left): http://codereview.chromium.org/8400061/diff/1/content/renderer/renderer_webid... content/renderer/renderer_webidbcursor_impl.cc:42: return key_; On 2011/10/29 00:54:28, dgrogan wrote: > I think you can delete these data members. Letting them stay, and removing the maps in the dispatcher instead. It seems cleaner to let each cursor keep track of its values etc.
http://codereview.chromium.org/8400061/diff/1/content/browser/in_process_webk... File content/browser/in_process_webkit/indexed_db_callbacks.cc (right): http://codereview.chromium.org/8400061/diff/1/content/browser/in_process_webk... content/browser/in_process_webkit/indexed_db_callbacks.cc:49: On 2011/10/31 16:11:52, hans wrote: > On 2011/10/30 21:54:06, michaeln wrote: > > What ensures that the idb_cursor isn't deleted between the continue call and > the > > completion call? Maybe this is entirely synchronous such that > > onSuccessCursorContinue() is invoked prior to return from the continue() > method? > > It's not synchronous. > > It's actually the idb_cursor object that's calling the onSuccessCursorContinue() > method. So if it's been deleted, we would probably have already crashed. > > The deletion happens when the RendererWebIDBCursorImpl is destroyed in the > renderer process, causing an IndexedDBHostMsg_CursorDestroyed to be sent to the > browser process. > > There's a reassuring comment in ~RendererWebIDBCursorImpl(), saying "It's not > possible for there to be pending callbacks that address this object since inside > WebKit, they hold a reference to the object wich owns this object." So I think > it's safe. That comment isn't so reassuring since there are no pending callbacks that address RendererWebIDBCursorImpl at all. Thusfar, each step gets a new instance. Here's the case i'm thinking about. * cursor.continue(); cursor = null; // so there is no longer a js/v8 ref to the thing * return to event loop * gc happens // so the cursor js/v8 wrapper gets deleted * continue completion occurs // BOOM, id is not in maps at some point in the chain What actually prevents RendererWebIDBCursorImpl from being deleted in there (and then propagating that deletion back thru), and if nothing prevents that, how should that case be handled. I'm wondering if need to either take measures to 'keep it alive' or take measures to 'recreate as needed'. http://codereview.chromium.org/8400061/diff/8001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_callbacks.cc (right): http://codereview.chromium.org/8400061/diff/8001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_callbacks.cc:48: should we be more defensive here... DCHECK(idb_cursor); if (!idb_cursor) return; http://codereview.chromium.org/8400061/diff/8001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_callbacks.h (right): http://codereview.chromium.org/8400061/diff/8001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_callbacks.h:80: // onSuccessCUrsorContinue() when a continue() call has succeeded, or onSuccessCUrsorContinue typo http://codereview.chromium.org/8400061/diff/8001/content/renderer/indexed_db_... File content/renderer/indexed_db_dispatcher.cc (right): http://codereview.chromium.org/8400061/diff/8001/content/renderer/indexed_db_... content/renderer/indexed_db_dispatcher.cc:423: void IndexedDBDispatcher::OnSuccessSerializedScriptValue( When the cursor hits the end, what should it's key, value, and primaryKey attributes reflect? I'm wondering what values should be stuff into the RendererWebIDBCursorImpl instance at this time. http://codereview.chromium.org/8400061/diff/8001/content/renderer/indexed_db_... content/renderer/indexed_db_dispatcher.cc:451: RendererWebIDBCursorImpl* cursor = cursors_[cursor_id]; DCHECK(cursor) http://codereview.chromium.org/8400061/diff/8001/content/renderer/renderer_we... File content/renderer/renderer_webidbcursor_impl.cc (right): http://codereview.chromium.org/8400061/diff/8001/content/renderer/renderer_we... content/renderer/renderer_webidbcursor_impl.cc:76: void RendererWebIDBCursorImpl::setKeyAndValue(const IndexedDBKey& key, nit: args should all line up
On 2011/10/31 18:58:25, michaeln wrote: > That comment isn't so reassuring since there are no pending callbacks that > address RendererWebIDBCursorImpl at all. Thusfar, each step gets a new instance. > > Here's the case i'm thinking about. > > * cursor.continue(); cursor = null; // so there is no longer a js/v8 ref to > the thing > * return to event loop > * gc happens // so the cursor js/v8 wrapper gets deleted > * continue completion occurs // BOOM, id is not in maps at some point in the > chain > > What actually prevents RendererWebIDBCursorImpl from being deleted in there (and > then propagating that deletion back thru), and if nothing prevents that, how > should that case be handled. I'm wondering if need to either take measures to > 'keep it alive' or take measures to 'recreate as needed'. I've been spending some time proving to myself that there is something preventing RendererWebIDBCursorImpl from being deleted in the case you describe. But I can't convince myself, so let's assume it can be deleted. This means we should be defensive in the IndexedDBDispatcherHost, and assume it may not exist in the cursors_ map when we get the callback. I'm updating the patch to add that defensiveness. So do we need to keep it alive or recreate it? No, I think not. The target of the callback is an IDBRequest. And that IDBRequest holds a reference to the cursor (with my WebKit patch). So if the IDBRequest is still there, the cursor is too. If the IDBRequest is not there anymore, there's nowhere to send the callback anyway, so we can just return.
http://codereview.chromium.org/8400061/diff/8001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_callbacks.cc (right): http://codereview.chromium.org/8400061/diff/8001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_callbacks.cc:48: On 2011/10/31 18:58:25, michaeln wrote: > should we be more defensive here... > > DCHECK(idb_cursor); > if (!idb_cursor) > return; Yes, let's do this. http://codereview.chromium.org/8400061/diff/8001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_callbacks.h (right): http://codereview.chromium.org/8400061/diff/8001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_callbacks.h:80: // onSuccessCUrsorContinue() when a continue() call has succeeded, or On 2011/10/31 18:58:25, michaeln wrote: > onSuccessCUrsorContinue typo Done. http://codereview.chromium.org/8400061/diff/8001/content/renderer/indexed_db_... File content/renderer/indexed_db_dispatcher.cc (right): http://codereview.chromium.org/8400061/diff/8001/content/renderer/indexed_db_... content/renderer/indexed_db_dispatcher.cc:423: void IndexedDBDispatcher::OnSuccessSerializedScriptValue( On 2011/10/31 18:58:25, michaeln wrote: > When the cursor hits the end, what should it's key, value, and primaryKey > attributes reflect? I'm wondering what values should be stuff into the > RendererWebIDBCursorImpl instance at this time. That's a good question. The spec only says that the result when stepping a cursor hits the end should be "null", and we do that. It doesn't say what should happen with the old cursor object, though, if the user keeps a reference to it. I don't have any strong views here. http://codereview.chromium.org/8400061/diff/8001/content/renderer/indexed_db_... content/renderer/indexed_db_dispatcher.cc:451: RendererWebIDBCursorImpl* cursor = cursors_[cursor_id]; On 2011/10/31 18:58:25, michaeln wrote: > DCHECK(cursor) Done. http://codereview.chromium.org/8400061/diff/8001/content/renderer/renderer_we... File content/renderer/renderer_webidbcursor_impl.cc (right): http://codereview.chromium.org/8400061/diff/8001/content/renderer/renderer_we... content/renderer/renderer_webidbcursor_impl.cc:76: void RendererWebIDBCursorImpl::setKeyAndValue(const IndexedDBKey& key, On 2011/10/31 18:58:25, michaeln wrote: > nit: args should all line up Done.
> And that IDBRequest holds a reference to the cursor (with my WebKit patch). > So if the IDBRequest is still there, the cursor is too. Can you point that out to me in the patch? I see an m_cursor data member, but i only see it getting set on completion. Maybe the 'source' if the IDBRequest is the cursor object and that's what you mean? http://codereview.chromium.org/8400061/diff/15001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_callbacks.h (right): http://codereview.chromium.org/8400061/diff/15001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_callbacks.h:91: , cursor_id_(cursor_id) { } chrome-style initializers don't put the ',' up front http://codereview.chromium.org/8400061/diff/15001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/8400061/diff/15001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:1078: WebIDBCursor* IndexedDBDispatcherHost::getCursorFromId(int32 cursor_id) { nit: please you move this method up in the .cc file to match its location in the .h file http://codereview.chromium.org/8400061/diff/15001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_dispatcher_host.h (right): http://codereview.chromium.org/8400061/diff/15001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.h:68: WebKit::WebIDBCursor* getCursorFromId(int32 cursor_id); upper case method names http://codereview.chromium.org/8400061/diff/15001/content/renderer/renderer_w... File content/renderer/renderer_webidbcursor_impl.h (right): http://codereview.chromium.org/8400061/diff/15001/content/renderer/renderer_w... content/renderer/renderer_webidbcursor_impl.h:34: void setKeyAndValue(const IndexedDBKey& key, const IndexedDBKey& primary_key, since this isn't defined in the WebKitAPI, please use a chromium-style upper cased method name
On 2011/11/01 18:38:37, michaeln wrote: > > And that IDBRequest holds a reference to the cursor (with my WebKit patch). > > So if the IDBRequest is still there, the cursor is too. > > Can you point that out to me in the patch? I see an m_cursor data member, but i > only see it getting set on completion. Maybe the 'source' if the IDBRequest is > the cursor object and that's what you mean? In https://bugs.webkit.org/show_bug.cgi?id=71115, I'm adding the m_cursor member to IDBRequest. This gets set when the cursor is successfully opened (call to setCursor in IDBRequest::onSuccess(PassRefPtr<IDBCursorBackendInterface> backend)), and it holds on to this reference for the lifetime of IDBRequest. So as long as a continue call has an IDBRequest waiting for callbacks, there is at least one reference to the cursor object. I'll look at the style issues tomorrow, seems I've spent too much time in WebKit land :)
On 2011/11/01 19:19:50, hans wrote: > On 2011/11/01 18:38:37, michaeln wrote: > > > And that IDBRequest holds a reference to the cursor (with my WebKit patch). > > > So if the IDBRequest is still there, the cursor is too. > > > > Can you point that out to me in the patch? I see an m_cursor data member, but > i > > only see it getting set on completion. Maybe the 'source' if the IDBRequest is > > the cursor object and that's what you mean? > > In https://bugs.webkit.org/show_bug.cgi?id=71115, I'm adding the m_cursor member > to IDBRequest. This gets set when the cursor is successfully opened (call to > setCursor in IDBRequest::onSuccess(PassRefPtr<IDBCursorBackendInterface> > backend)), and it holds on to this reference for the lifetime of IDBRequest. > > So as long as a continue call has an IDBRequest waiting for callbacks, there is > at least one reference to the cursor object. I see, the 'request' from the open call serves as the 'request' for all subsequent continue calls. I was assuming each continue call resulted in a new request which i see now (after viewing some webcore code) is not the case. But this raises a different question about the webcore patch. Does the addition of the m_cursor data member add a reference cycle since the m_cursor also holds a reference to its m_request? How does that cycle get broken?
> When the cursor hits the end, what should it's key, value, and primaryKey > attributes reflect? I'm wondering what values should be stuff into the > RendererWebIDBCursorImpl instance at this time. This came up at TPAC today, we should expect a spec change to clarify it from Jonas. From the current draft it looks like the values should be 'undefined' once you run off then end. This came up in the context of a related spec change was to allow .key and .value accessors to work while a .continue() call is pending. In the current draft they don't work since the 'got value' flag is reset when continue() is called. Jonas intends to change things such that the current value is readable while in this state.
On 2011/11/02 01:08:59, michaeln wrote: > > When the cursor hits the end, what should it's key, value, and primaryKey > > attributes reflect? I'm wondering what values should be stuff into the > > RendererWebIDBCursorImpl instance at this time. > > This came up at TPAC today, we should expect a spec change to clarify it from > Jonas. From the current draft it looks like the values should be 'undefined' > once you run off then end. Great! I propose that we address this in a separate patch, though. (And in that bug we should also make DumpRenderTree not crash in this case.. :)
On 2011/11/01 21:16:46, michaeln wrote: > > So as long as a continue call has an IDBRequest waiting for callbacks, there > is > > at least one reference to the cursor object. > > I see, the 'request' from the open call serves as the 'request' for all > subsequent continue calls. I was assuming each continue call resulted in a new > request which i see now (after viewing some webcore code) is not the case. > > But this raises a different question about the webcore patch. Does the addition > of the m_cursor data member add a reference cycle since the m_cursor also holds > a reference to its m_request? How does that cycle get broken? I've been trying to untangle this today, but I don't have a great solution. The cursor holds a reference to the request. And it needs that reference both when a continue call is in progress, and otherwise. The request only really needs a reference to the cursor when a continue call is in progress. So this would mean there is a cycle when the continue call is in progress, but not otherwise. Would that be acceptable? Please chime in about this on the WebKit bug.
http://codereview.chromium.org/8400061/diff/15001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_callbacks.h (right): http://codereview.chromium.org/8400061/diff/15001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_callbacks.h:91: , cursor_id_(cursor_id) { } On 2011/11/01 18:38:37, michaeln wrote: > chrome-style initializers don't put the ',' up front Done. http://codereview.chromium.org/8400061/diff/15001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/8400061/diff/15001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:1078: WebIDBCursor* IndexedDBDispatcherHost::getCursorFromId(int32 cursor_id) { On 2011/11/01 18:38:37, michaeln wrote: > nit: please you move this method up in the .cc file to match its location in the > .h file Done. http://codereview.chromium.org/8400061/diff/15001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_dispatcher_host.h (right): http://codereview.chromium.org/8400061/diff/15001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.h:68: WebKit::WebIDBCursor* getCursorFromId(int32 cursor_id); On 2011/11/01 18:38:37, michaeln wrote: > upper case method names Done. http://codereview.chromium.org/8400061/diff/15001/content/renderer/renderer_w... File content/renderer/renderer_webidbcursor_impl.h (right): http://codereview.chromium.org/8400061/diff/15001/content/renderer/renderer_w... content/renderer/renderer_webidbcursor_impl.h:34: void setKeyAndValue(const IndexedDBKey& key, const IndexedDBKey& primary_key, On 2011/11/01 18:38:37, michaeln wrote: > since this isn't defined in the WebKitAPI, please use a chromium-style upper > cased method name Done.
On 2011/11/02 16:46:26, hans wrote: > On 2011/11/01 21:16:46, michaeln wrote: > > > So as long as a continue call has an IDBRequest waiting for callbacks, there > > is > > > at least one reference to the cursor object. > > > > I see, the 'request' from the open call serves as the 'request' for all > > subsequent continue calls. I was assuming each continue call resulted in a new > > request which i see now (after viewing some webcore code) is not the case. > > > > But this raises a different question about the webcore patch. Does the > addition > > of the m_cursor data member add a reference cycle since the m_cursor also > holds > > a reference to its m_request? How does that cycle get broken? > > I've been trying to untangle this today, but I don't have a great solution. > > The cursor holds a reference to the request. And it needs that reference both > when a continue call is in progress, and otherwise. > > The request only really needs a reference to the cursor when a continue call is > in progress. > > So this would mean there is a cycle when the continue call is in progress, but > not otherwise. Would that be acceptable? I think that would break the cycle. > Please chime in about this on the WebKit bug. Ok.
This change lg at this point, but lets wait on the wk patch review prior to landing things. What sequence do these need to go in?
On 2011/11/03 01:37:21, michaeln wrote: > This change lg at this point, but lets wait on the wk patch review prior to > landing things. What sequence do these need to go in? First the WebKit patch, then this, and then a follow-up in WebKit to actually start using the new callback. Thanks for the review work! I'll land this once the WebKit patch rolls in.
lgtm http://codereview.chromium.org/8400061/diff/20003/content/renderer/indexed_db... File content/renderer/indexed_db_dispatcher.cc (right): http://codereview.chromium.org/8400061/diff/20003/content/renderer/indexed_db... content/renderer/indexed_db_dispatcher.cc:450: nit: blank line not needed http://codereview.chromium.org/8400061/diff/20003/content/renderer/indexed_db... content/renderer/indexed_db_dispatcher.cc:497: void IndexedDBDispatcher::CursorDestroyed(int32 cursor_id) { please move this to a position in the .cc file that matches method declarations in the .h file http://codereview.chromium.org/8400061/diff/20003/content/renderer/indexed_db... File content/renderer/indexed_db_dispatcher.h (right): http://codereview.chromium.org/8400061/diff/20003/content/renderer/indexed_db... content/renderer/indexed_db_dispatcher.h:17: #include <map> we generally like to put std includes up top followed by a blank line, and then the other includes http://codereview.chromium.org/8400061/diff/20003/content/renderer/indexed_db... content/renderer/indexed_db_dispatcher.h:32: class RendererWebIDBCursorImpl; nit: this forward should be up by class IndexedDBKey forward http://codereview.chromium.org/8400061/diff/20003/content/renderer/indexed_db... content/renderer/indexed_db_dispatcher.h:159: void CursorDestroyed(int32 cursor_id); nit: since the previous method is 'static', can you put this instance method before to keep it with the other instance methods
http://codereview.chromium.org/8400061/diff/20003/content/renderer/indexed_db... File content/renderer/indexed_db_dispatcher.cc (right): http://codereview.chromium.org/8400061/diff/20003/content/renderer/indexed_db... content/renderer/indexed_db_dispatcher.cc:450: On 2011/11/03 18:10:04, michaeln wrote: > nit: blank line not needed Done. http://codereview.chromium.org/8400061/diff/20003/content/renderer/indexed_db... content/renderer/indexed_db_dispatcher.cc:497: void IndexedDBDispatcher::CursorDestroyed(int32 cursor_id) { On 2011/11/03 18:10:04, michaeln wrote: > please move this to a position in the .cc file that matches method declarations > in the .h file Done. http://codereview.chromium.org/8400061/diff/20003/content/renderer/indexed_db... File content/renderer/indexed_db_dispatcher.h (right): http://codereview.chromium.org/8400061/diff/20003/content/renderer/indexed_db... content/renderer/indexed_db_dispatcher.h:17: #include <map> On 2011/11/03 18:10:04, michaeln wrote: > we generally like to put std includes up top followed by a blank line, and then > the other includes Done. http://codereview.chromium.org/8400061/diff/20003/content/renderer/indexed_db... content/renderer/indexed_db_dispatcher.h:32: class RendererWebIDBCursorImpl; On 2011/11/03 18:10:04, michaeln wrote: > nit: this forward should be up by class IndexedDBKey forward Done. http://codereview.chromium.org/8400061/diff/20003/content/renderer/indexed_db... content/renderer/indexed_db_dispatcher.h:159: void CursorDestroyed(int32 cursor_id); On 2011/11/03 18:10:04, michaeln wrote: > nit: since the previous method is 'static', can you put this instance method > before to keep it with the other instance methods Done.
brettw: owners approval for content/renderer and content/common please.
On 2011/11/04 10:14:08, hans wrote: > brettw: owners approval for content/renderer and content/common please. jam: owners approval for content/renderer and content/common ? brettw doesn't seem to be online.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hans@chromium.org/8400061/26001
Change committed as 108788 |
