Bug 54193 - Implement IDBObjectStore::clear
Summary: Implement IDBObjectStore::clear
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-10 05:21 PST by jochen
Modified: 2011-02-12 03:56 PST (History)
6 users (show)

See Also:


Attachments
Patch (18.06 KB, patch)
2011-02-10 05:21 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (19.26 KB, patch)
2011-02-11 07:35 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (19.31 KB, patch)
2011-02-11 07:39 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (18.76 KB, patch)
2011-02-11 07:40 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (24.71 KB, patch)
2011-02-11 15:36 PST, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2011-02-10 05:21:05 PST
Implement IDBObjectStore::clear
Comment 1 jochen 2011-02-10 05:21:51 PST
Created attachment 81955 [details]
Patch
Comment 2 Jeremy Orlow 2011-02-10 11:34:58 PST
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.
Comment 3 Jeremy Orlow 2011-02-10 11:36:28 PST
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.  :-)
Comment 4 jochen 2011-02-11 07:35:44 PST
Created attachment 82133 [details]
Patch
Comment 5 jochen 2011-02-11 07:39:08 PST
Created attachment 82134 [details]
Patch
Comment 6 jochen 2011-02-11 07:40:51 PST
Created attachment 82135 [details]
Patch
Comment 7 jochen 2011-02-11 07:42:23 PST
(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.
Comment 8 Jeremy Orlow 2011-02-11 09:06:42 PST
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.
Comment 9 jochen 2011-02-11 15:36:56 PST
Created attachment 82198 [details]
Patch
Comment 10 Jeremy Orlow 2011-02-11 15:45:24 PST
Comment on attachment 82198 [details]
Patch

r=me
Comment 11 WebKit Commit Bot 2011-02-12 03:56:21 PST
Comment on attachment 82198 [details]
Patch

Clearing flags on attachment: 82198

Committed r78416: <http://trac.webkit.org/changeset/78416>
Comment 12 WebKit Commit Bot 2011-02-12 03:56:25 PST
All reviewed patches have been landed.  Closing bug.