[webkit-reviews] review granted: [Bug 53421] IndexedDB: Implement support for cursor updates : [Attachment 80638] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 31 20:46:24 PST 2011
Jeremy Orlow <jorlow at chromium.org> has granted Hans Wennborg
<hans at chromium.org>'s request for review:
Bug 53421: IndexedDB: Implement support for cursor updates
https://bugs.webkit.org/show_bug.cgi?id=53421
Attachment 80638: Patch
https://bugs.webkit.org/attachment.cgi?id=80638&action=review
------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
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
More information about the webkit-reviews
mailing list