Bug 53421 – IndexedDB: Implement support for cursor updates

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

[Hans Wennborg]

IndexedDB: Implement support for cursor updates

[Hans Wennborg]

Created attachment 80638 [details]
Patch

[Jeremy Orlow]

Comment on attachment 80638 [details]
Patch

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

> Source/WebCore/storage/IDBCursorBackendImpl.cpp:81
>  void IDBCursorBackendImpl::update(PassRefPtr<SerializedScriptValue> prpValue, PassRefPtr<IDBCallbacks> prpCallbacks, ExceptionCode& ec)

remove the prp...only use that when we're about to assign to a RefPtr

> Source/WebCore/storage/IDBCursorBackendImpl.cpp:88
> +    RefPtr<IDBKey> key = m_currentIDBKeyValue ? m_currentIDBKeyValue : m_currentKey;

Can you please fix up the naming a bit.  It's kind of confusing.  And/or add a comment in the header.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:213
> +    if (objectStore->autoIncrement() && key && putMode != CursorUpdate) {

Hmm...if we implement autoIncrement properly, will we even need to distinguish between CursorUpdate and not?

[Hans Wennborg]

(In reply to comment #2)
> (From update of attachment 80638 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80638&action=review
> 
> > Source/WebCore/storage/IDBCursorBackendImpl.cpp:81
> >  void IDBCursorBackendImpl::update(PassRefPtr<SerializedScriptValue> prpValue, PassRefPtr<IDBCallbacks> prpCallbacks, ExceptionCode& ec)
> 
> remove the prp...only use that when we're about to assign to a RefPtr
Will fix.

> 
> > Source/WebCore/storage/IDBCursorBackendImpl.cpp:88
> > +    RefPtr<IDBKey> key = m_currentIDBKeyValue ? m_currentIDBKeyValue : m_currentKey;
> 
> Can you please fix up the naming a bit.  It's kind of confusing.  And/or add a comment in the header.
Will do.

> 
> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:213
> > +    if (objectStore->autoIncrement() && key && putMode != CursorUpdate) {
> 
> Hmm...if we implement autoIncrement properly, will we even need to distinguish between CursorUpdate and not?

Yeah, if we change the auto increment semantics, those checks could go away.

We still need the CursorUpdate mode for handling object stores with key paths, though.

[Jeremy Orlow]

Comment on attachment 80638 [details]
Patch

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

r=me assuming you fix these issues and/or start on the next patch right away

> LayoutTests/storage/indexeddb/cursor-update.html:79
> +    keyRange = webkitIDBKeyRange.lowerBound("myKey1");

maybe blank newline before?

> LayoutTests/storage/indexeddb/cursor-update.html:80
> +    result = evalAndLog("trans.objectStore('basicStore').openCursor({range: keyRange})");

Wait...did we not revert the options object for cursors...?

> LayoutTests/storage/indexeddb/cursor-update.html:125
> +function autoIncrementUpdateCursor()

If you were writing this again, it probably would have been worth trying to factor this out more properly.  No worries tho.

> LayoutTests/storage/indexeddb/cursor-update.html:180
> +        shouldBe("event.code", "webkitIDBDatabaseException.DATA_ERR");

According to the (imaginary version of the) spec, this is wrong, right?  We should probably make this work as it's supposed to.  See my comment in the cpp.

> LayoutTests/storage/indexeddb/cursor-update.html:182
> +        result = evalAndLog("event.source.update({id: counter, number: 100 + counter++})");

This likely doesn't work how you expect it to.  counter++ gets run when the onerror happens, which means they were all updated to just 100 I'm pretty sure.

>>> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:213
>>> +    if (objectStore->autoIncrement() && key && putMode != CursorUpdate) {
>> 
>> Hmm...if we implement autoIncrement properly, will we even need to distinguish between CursorUpdate and not?
> 
> Yeah, if we change the auto increment semantics, those checks could go away.
> 
> We still need the CursorUpdate mode for handling object stores with key paths, though.

Let's try to fix the autoIncrement implementation soon then.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:234
> +        if (putMode == CursorUpdate && !keyPathKey->isEqual(key.get())) {

Hmm....does this make sense?  The key is the old key, right?  That can change....right?

> Source/WebKit/chromium/public/WebIDBObjectStore.h:79
> +        put(value, key, putMode == AddOnly ? true : false, callbacks, transaction, ec);

don't need the ? true : false part

[Hans Wennborg]

Created attachment 80738 [details]
Patch

[Hans Wennborg]

Uploading new patch and waiting for review as there are still some question marks:


(In reply to comment #2)
> > Source/WebCore/storage/IDBCursorBackendImpl.cpp:81
> >  void IDBCursorBackendImpl::update(PassRefPtr<SerializedScriptValue> prpValue, PassRefPtr<IDBCallbacks> prpCallbacks, ExceptionCode& ec)
> 
> remove the prp...only use that when we're about to assign to a RefPtr

Just to make sure I understand correctly: do you want prpValue and prpCallbacks to be raw pointers instead?

I'm not sure I really understand this.. shouldn't that apply to the deleteFunction() too?

And IDBCursorBackendProxy, which implements the same interface, doesn't store its PassRefPtr parameters in RefPtrs either?

> > Source/WebCore/storage/IDBCursorBackendImpl.cpp:88
> > +    RefPtr<IDBKey> key = m_currentIDBKeyValue ? m_currentIDBKeyValue : m_currentKey;
> 
> Can you please fix up the naming a bit.  It's kind of confusing.  And/or add a comment in the header.
> 

Trying to clarify with comments.. I don't really have ideas for better names :S


(In reply to comment #4)
> > LayoutTests/storage/indexeddb/cursor-update.html:79
> > +    keyRange = webkitIDBKeyRange.lowerBound("myKey1");
> 
> maybe blank newline before?
Done.

> > LayoutTests/storage/indexeddb/cursor-update.html:80
> > +    result = evalAndLog("trans.objectStore('basicStore').openCursor({range: keyRange})");
> 
> Wait...did we not revert the options object for cursors...?
Oops. Fixed.

> 
> > LayoutTests/storage/indexeddb/cursor-update.html:125
> > +function autoIncrementUpdateCursor()
> 
> If you were writing this again, it probably would have been worth trying to factor this out more properly.  No worries tho.
Ok.

> 
> > LayoutTests/storage/indexeddb/cursor-update.html:180
> > +        shouldBe("event.code", "webkitIDBDatabaseException.DATA_ERR");
> 
> According to the (imaginary version of the) spec, this is wrong, right?  We should probably make this work as it's supposed to.  See my comment in the cpp.

No, I think the spec is right here: "If the effective object store of this cursor uses in-line keys and evaluating the key path of the value parameter results in a different value than the cursor's effective key, this method throws DATA_ERR."

I think this makes sense.

> > LayoutTests/storage/indexeddb/cursor-update.html:182
> > +        result = evalAndLog("event.source.update({id: counter, number: 100 + counter++})");
> 
> This likely doesn't work how you expect it to.  counter++ gets run when the onerror happens, which means they were all updated to just 100 I'm pretty sure.

But counter is global to the file, so it's not a problem that it is run when the onerror happens. I've updated keyPathCheckCursor() below to make it clearer what the actual values end up being.

> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:234
> > +        if (putMode == CursorUpdate && !keyPathKey->isEqual(key.get())) {
> 
> Hmm....does this make sense?  The key is the old key, right?  That can change....right?

No, one is not allowed to change the object so that the key from the keypath changes: "If the effective object store of this cursor uses in-line keys and evaluating the key path of the value parameter results in a different value than the cursor's effective key, this method throws DATA_ERR."

> > Source/WebKit/chromium/public/WebIDBObjectStore.h:79
> > +        put(value, key, putMode == AddOnly ? true : false, callbacks, transaction, ec);
> 
> don't need the ? true : false part

Silly me. Fixed.

[Jeremy Orlow]

(In reply to comment #6)
> Uploading new patch and waiting for review as there are still some question marks:
> 
> 
> (In reply to comment #2)
> > > Source/WebCore/storage/IDBCursorBackendImpl.cpp:81
> > >  void IDBCursorBackendImpl::update(PassRefPtr<SerializedScriptValue> prpValue, PassRefPtr<IDBCallbacks> prpCallbacks, ExceptionCode& ec)
> > 
> > remove the prp...only use that when we're about to assign to a RefPtr
> 
> Just to make sure I understand correctly: do you want prpValue and prpCallbacks to be raw pointers instead?
> 
> I'm not sure I really understand this.. shouldn't that apply to the deleteFunction() too?

No, just change the name to "value" rather than "prpValue" for example.

> And IDBCursorBackendProxy, which implements the same interface, doesn't store its PassRefPtr parameters in RefPtrs either?
> 
> > > Source/WebCore/storage/IDBCursorBackendImpl.cpp:88
> > > +    RefPtr<IDBKey> key = m_currentIDBKeyValue ? m_currentIDBKeyValue : m_currentKey;
> > 
> > Can you please fix up the naming a bit.  It's kind of confusing.  And/or add a comment in the header.
> > 
> 
> Trying to clarify with comments.. I don't really have ideas for better names :S

I guess just go with the names that'll show up in the IDL.  So m_currentValue and m_currentIndexKey IIRC?

> (In reply to comment #4)
> > > LayoutTests/storage/indexeddb/cursor-update.html:79
> > > +    keyRange = webkitIDBKeyRange.lowerBound("myKey1");
> > 
> > maybe blank newline before?
> Done.
> 
> > > LayoutTests/storage/indexeddb/cursor-update.html:80
> > > +    result = evalAndLog("trans.objectStore('basicStore').openCursor({range: keyRange})");
> > 
> > Wait...did we not revert the options object for cursors...?
> Oops. Fixed.
> 
> > 
> > > LayoutTests/storage/indexeddb/cursor-update.html:125
> > > +function autoIncrementUpdateCursor()
> > 
> > If you were writing this again, it probably would have been worth trying to factor this out more properly.  No worries tho.
> Ok.
> 
> > 
> > > LayoutTests/storage/indexeddb/cursor-update.html:180
> > > +        shouldBe("event.code", "webkitIDBDatabaseException.DATA_ERR");
> > 
> > According to the (imaginary version of the) spec, this is wrong, right?  We should probably make this work as it's supposed to.  See my comment in the cpp.
> 
> No, I think the spec is right here: "If the effective object store of this cursor uses in-line keys and evaluating the key path of the value parameter results in a different value than the cursor's effective key, this method throws DATA_ERR."
> 
> I think this makes sense.

Why do we need to limit it this way, though?
 
> > > LayoutTests/storage/indexeddb/cursor-update.html:182
> > > +        result = evalAndLog("event.source.update({id: counter, number: 100 + counter++})");
> > 
> > This likely doesn't work how you expect it to.  counter++ gets run when the onerror happens, which means they were all updated to just 100 I'm pretty sure.
> 
> But counter is global to the file, so it's not a problem that it is run when the onerror happens. I've updated keyPathCheckCursor() below to make it clearer what the actual values end up being.

I see..yeah, guess it's fine.

[Hans Wennborg]

(In reply to comment #7)
> > > > Source/WebCore/storage/IDBCursorBackendImpl.cpp:81
> > > >  void IDBCursorBackendImpl::update(PassRefPtr<SerializedScriptValue> prpValue, PassRefPtr<IDBCallbacks> prpCallbacks, ExceptionCode& ec)
> > > 
> > > remove the prp...only use that when we're about to assign to a RefPtr
> > 
> > Just to make sure I understand correctly: do you want prpValue and prpCallbacks to be raw pointers instead?
> > 
> > I'm not sure I really understand this.. shouldn't that apply to the deleteFunction() too?
> 
> No, just change the name to "value" rather than "prpValue" for example.

Ah, I misunderstood. Fixing.

> > > > Source/WebCore/storage/IDBCursorBackendImpl.cpp:88
> > > > +    RefPtr<IDBKey> key = m_currentIDBKeyValue ? m_currentIDBKeyValue : m_currentKey;
> > > 
> > > Can you please fix up the naming a bit.  It's kind of confusing.  And/or add a comment in the header.
> > > 
> > 
> > Trying to clarify with comments.. I don't really have ideas for better names :S
> 
> I guess just go with the names that'll show up in the IDL.  So m_currentValue and m_currentIndexKey IIRC?

Which IDL are you referring to? The cursor only has a "key" and a "value" attribute?

> > > > LayoutTests/storage/indexeddb/cursor-update.html:180
> > > > +        shouldBe("event.code", "webkitIDBDatabaseException.DATA_ERR");
> > > 
> > > According to the (imaginary version of the) spec, this is wrong, right?  We should probably make this work as it's supposed to.  See my comment in the cpp.
> > 
> > No, I think the spec is right here: "If the effective object store of this cursor uses in-line keys and evaluating the key path of the value parameter results in a different value than the cursor's effective key, this method throws DATA_ERR."
> > 
> > I think this makes sense.
> 
> Why do we need to limit it this way, though?

Because otherwise the keys that the cursor is iterating over can "change from underneath our feet", and that seems like a bad idea.

[Jeremy Orlow]

(In reply to comment #8)
> (In reply to comment #7)
> > > > > Source/WebCore/storage/IDBCursorBackendImpl.cpp:81
> > > > >  void IDBCursorBackendImpl::update(PassRefPtr<SerializedScriptValue> prpValue, PassRefPtr<IDBCallbacks> prpCallbacks, ExceptionCode& ec)
> > > > 
> > > > remove the prp...only use that when we're about to assign to a RefPtr
> > > 
> > > Just to make sure I understand correctly: do you want prpValue and prpCallbacks to be raw pointers instead?
> > > 
> > > I'm not sure I really understand this.. shouldn't that apply to the deleteFunction() too?
> > 
> > No, just change the name to "value" rather than "prpValue" for example.
> 
> Ah, I misunderstood. Fixing.
> 
> > > > > Source/WebCore/storage/IDBCursorBackendImpl.cpp:88
> > > > > +    RefPtr<IDBKey> key = m_currentIDBKeyValue ? m_currentIDBKeyValue : m_currentKey;
> > > > 
> > > > Can you please fix up the naming a bit.  It's kind of confusing.  And/or add a comment in the header.
> > > > 
> > > 
> > > Trying to clarify with comments.. I don't really have ideas for better names :S
> > 
> > I guess just go with the names that'll show up in the IDL.  So m_currentValue and m_currentIndexKey IIRC?
> 
> Which IDL are you referring to? The cursor only has a "key" and a "value" attribute?

Oops...wasn't really properly bugged: http://www.w3.org/Bugs/Public/show_bug.cgi?id=11948
 
> > > > > LayoutTests/storage/indexeddb/cursor-update.html:180
> > > > > +        shouldBe("event.code", "webkitIDBDatabaseException.DATA_ERR");
> > > > 
> > > > According to the (imaginary version of the) spec, this is wrong, right?  We should probably make this work as it's supposed to.  See my comment in the cpp.
> > > 
> > > No, I think the spec is right here: "If the effective object store of this cursor uses in-line keys and evaluating the key path of the value parameter results in a different value than the cursor's effective key, this method throws DATA_ERR."
> > > 
> > > I think this makes sense.
> > 
> > Why do we need to limit it this way, though?
> 
> Because otherwise the keys that the cursor is iterating over can "change from underneath our feet", and that seems like a bad idea.

They already can.  Just do a put.

On the other hand, the put would add a new item whereas this is essentially a remove + a put...but I guess I'm not sure why we disallow this.  Will bring it up on list.

[Jeremy Orlow]

Comment on attachment 80738 [details]
Patch

change the prp bit and you should be good to go

[Hans Wennborg]

Committed r77371: <http://trac.webkit.org/changeset/77371>