[webkit-reviews] review denied: [Bug 54193] Implement IDBObjectStore::clear : [Attachment 81955] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied jochen at chromium.org's request for
review:
Bug 54193: Implement IDBObjectStore::clear
https://bugs.webkit.org/show_bug.cgi?id=54193

Attachment 81955: Patch
https://bugs.webkit.org/attachment.cgi?id=81955&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81955&action=review

> LayoutTests/storage/indexeddb/objectstore-basics.html:281
> +    shouldBe("store.indexNames.length", "0");

clear should only clear the _data_ in the object store and indexes....not the
indexes themselves

the proper test is to open a cursor with no key range (it'll read everything)
and verify it immediately returns null (which tells you it's at the end)

And then, for good measure, open a keyCursor on some index and verify the same
thing.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:386
> +    RefPtr<IDBCallbacks> callbacks = prpCallbacks;

Are you sure this stuff is needed?  (I think some parts of this file went
pretty overboard with this...we only need to do prp and then copy it into
RefPtr when a param is a PassRefPtr and we pass it to another method that takes
a PassRefPtr.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:393
> +    m_indexes.clear();

Whoa...you don't want to do this

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:411
> +    callbacks->onSuccess(SerializedScriptValue::nullValue());

The spec says this should be undefined.