Bug 54193 – Implement IDBObjectStore::clear

• Component: New Bugs
• Status: RESOLVED FIXED

Implement IDBObjectStore::clear

Created attachment 81955 [details]
Patch

[Jeremy Orlow]

Comment on attachment 81955 [details]
Patch

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.

[Jeremy Orlow]

Hans, can you please review Jochen's stuff if I'm asleep (to shorten the cycles)?

Jochen, let me know if you want any more IndexedDB patches.  :-)

Created attachment 82133 [details]
Patch

Created attachment 82134 [details]
Patch

Created attachment 82135 [details]
Patch

(In reply to comment #2)
> (From update of attachment 81955 [details])
> 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.

Well, the method takes a prp and invokes a method that takes a prp, so I think it's required.

> 
> > 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.

[Jeremy Orlow]

Comment on attachment 82135 [details]
Patch

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

close

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:390
> +        return;

not needed...so you can get rid of the {}'s too

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:405
> +    doClear(objectStore->sqliteDatabase(), "DELETE FROM ObjectStoreData");

Sorry for missing this earlier, but you need to only delete the data for your object store.  Please make sure your layout test double checks this as well.

Created attachment 82198 [details]
Patch

[Jeremy Orlow]

Comment on attachment 82198 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 82198 [details]
Patch

Clearing flags on attachment: 82198

Committed r78416: <http://trac.webkit.org/changeset/78416>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.