[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