Bug 54102 - IndexedDB: Update the semantics of put()
Summary: IndexedDB: Update the semantics of put()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Hans Wennborg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-09 07:15 PST by Hans Wennborg
Modified: 2011-02-10 11:42 PST (History)
3 users (show)

See Also:


Attachments
Patch (13.67 KB, patch)
2011-02-09 07:28 PST, Hans Wennborg
jorlow: 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-02-09 07:15:29 PST
IndexedDB: Update the semantics of put()
Comment 1 Hans Wennborg 2011-02-09 07:28:00 PST
Created attachment 81811 [details]
Patch
Comment 2 Hans Wennborg 2011-02-09 07:32:59 PST
As usual, I'm a bit uncertain about the PassRefPtr stuff. In the case of the selectKeyForPut function, I figured it should take all parameters as raw pointers since it doesn't take ownership of anything, and it returns a PassRefPtr because the result is expected to go in a RefPtr. Does that make sense?
Comment 3 Jeremy Orlow 2011-02-09 13:00:37 PST
Comment on attachment 81811 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81811&action=review

r=me

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:229
> +        // FIXME: Generate auto increment key, and inject it through the key path.

Can you do this in a follow up patch?

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:260
> +    RefPtr<SerializedScriptValue> value = prpValue;

The only time we need to do the whole prp and then save it to a RefPtr bit is when we pass the PassRefPtr to something that takes a PassRefPtr.  If we're just doing ->'s and .get()s then we don't actually need to do it.  Not sure if you do here, but just something to note.  (We use it more than we probably should.)

Does stuff below reference prpValue or value?  If it's the former, then you have a bug.
Comment 4 Jeremy Orlow 2011-02-09 13:06:18 PST
(In reply to comment #2)
> As usual, I'm a bit uncertain about the PassRefPtr stuff. In the case of the selectKeyForPut function, I figured it should take all parameters as raw pointers since it doesn't take ownership of anything, and it returns a PassRefPtr because the result is expected to go in a RefPtr. Does that make sense?

Yup, that's perfect.
Comment 5 Jeremy Orlow 2011-02-09 13:07:56 PST
(In reply to comment #3)
> (From update of attachment 81811 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81811&action=review
> 
> r=me
> 
> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:229
> > +        // FIXME: Generate auto increment key, and inject it through the key path.
> 
> Can you do this in a follow up patch?
> 
> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:260
> > +    RefPtr<SerializedScriptValue> value = prpValue;
> 
> The only time we need to do the whole prp and then save it to a RefPtr bit is when we pass the PassRefPtr to something that takes a PassRefPtr.  If we're just doing ->'s and .get()s then we don't actually need to do it.  Not sure if you do here, but just something to note.  (We use it more than we probably should.)
> 
> Does stuff below reference prpValue or value?  If it's the former, then you have a bug.

To be clear: the reason for this is that when a PassRefPtr is assigned to another PassRefPtr or a RefPt, it gives up its reference and is zeroed out.  But if you just do a .get() or a -> that doesn't happen.
Comment 6 Hans Wennborg 2011-02-10 03:02:26 PST
Committed r78187: <http://trac.webkit.org/changeset/78187>
Comment 7 Hans Wennborg 2011-02-10 03:09:46 PST
(In reply to comment #3)
> (From update of attachment 81811 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81811&action=review
> 
> r=me
> 
> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:229
> > +        // FIXME: Generate auto increment key, and inject it through the key path.
> 
> Can you do this in a follow up patch?
Will do.

> 
> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:260
> > +    RefPtr<SerializedScriptValue> value = prpValue;
> 
> The only time we need to do the whole prp and then save it to a RefPtr bit is when we pass the PassRefPtr to something that takes a PassRefPtr.  If we're just doing ->'s and .get()s then we don't actually need to do it.  Not sure if you do here, but just something to note.  (We use it more than we probably should.)
Hmm, we just do -> and .get() on value, so we could get rid of that RefPtr.

But this makes me wonder why so many of the parameters to putInternal() are PassRefPtr, even though we don't take ownership. Is this because of the mechanism with transaction and callback tasks?

> 
> Does stuff below reference prpValue or value?  If it's the former, then you have a bug.
Just value.
Comment 8 Jeremy Orlow 2011-02-10 11:42:04 PST
(In reply to comment #7)
> (In reply to comment #3)
> > (From update of attachment 81811 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=81811&action=review
> > 
> > r=me
> > 
> > > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:229
> > > +        // FIXME: Generate auto increment key, and inject it through the key path.
> > 
> > Can you do this in a follow up patch?
> Will do.
> 
> > 
> > > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:260
> > > +    RefPtr<SerializedScriptValue> value = prpValue;
> > 
> > The only time we need to do the whole prp and then save it to a RefPtr bit is when we pass the PassRefPtr to something that takes a PassRefPtr.  If we're just doing ->'s and .get()s then we don't actually need to do it.  Not sure if you do here, but just something to note.  (We use it more than we probably should.)
> Hmm, we just do -> and .get() on value, so we could get rid of that RefPtr.
> 
> But this makes me wonder why so many of the parameters to putInternal() are PassRefPtr, even though we don't take ownership. Is this because of the mechanism with transaction and callback tasks?

Hmm.  Actually, that might be it.  Maybe see if you can remove them and report back?

> > 
> > Does stuff below reference prpValue or value?  If it's the former, then you have a bug.
> Just value.