Bug 73686 - IndexedDB: Implement IDBObjectStore.count() and IDBIndex.count()
Summary: IndexedDB: Implement IDBObjectStore.count() and IDBIndex.count()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-02 13:11 PST by Joshua Bell
Modified: 2011-12-16 13:27 PST (History)
10 users (show)

See Also:


Attachments
Patch (54.41 KB, patch)
2011-12-02 13:15 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (53.46 KB, patch)
2011-12-05 09:40 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (53.25 KB, patch)
2011-12-07 12:39 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (53.25 KB, patch)
2011-12-07 13:57 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (51.25 KB, patch)
2011-12-09 11:43 PST, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2011-12-02 13:11:54 PST
IndexedDB: Implement IDBObjectStore.count() and IDBIndex.count()
Comment 1 Joshua Bell 2011-12-02 13:15:19 PST
Created attachment 117675 [details]
Patch
Comment 2 Joshua Bell 2011-12-02 13:16:26 PST
Ignore the Chromium public API changes here - they'll land via https://bugs.webkit.org/show_bug.cgi?id=73685 first and then be removed from this patch.
Comment 3 WebKit Review Bot 2011-12-02 13:18:43 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 Joshua Bell 2011-12-02 13:23:03 PST
This is Part 2 of a two-sided change. In between, I'll land http://codereview.chromium.org/8779003 in Chromium.
Comment 5 David Levin 2011-12-02 13:26:51 PST
Comment on attachment 117675 [details]
Patch

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

Just a drive by comment.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:554
> +    RefPtr<IDBTransactionBackendInterface> transactionPtr = transaction;

fwiw, you should be able to pass these directly into createCallbackTask.
Comment 6 Hans Wennborg 2011-12-05 03:57:31 PST
Comment on attachment 117675 [details]
Patch

Looks good to me!

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

> Source/WebCore/storage/IDBIndexBackendImpl.cpp:119
> +    RefPtr<IDBBackingStore::Cursor> backingStoreCursor = index->m_backingStore->openIndexCursor(index->m_databaseId, index->m_objectStoreBackend->id(), index->id(), range.get(), IDBCursor::NEXT);

maybe use openIndexKeyCursor instead to avoid shipping the corresponding object store values across IPC for each index entry.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:555
> +    if (!transaction->scheduleTask(

no need to linebreak
Comment 7 Joshua Bell 2011-12-05 09:11:51 PST
Comment on attachment 117675 [details]
Patch

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

>> Source/WebCore/storage/IDBIndexBackendImpl.cpp:119
>> +    RefPtr<IDBBackingStore::Cursor> backingStoreCursor = index->m_backingStore->openIndexCursor(index->m_databaseId, index->m_objectStoreBackend->id(), index->id(), range.get(), IDBCursor::NEXT);
> 
> maybe use openIndexKeyCursor instead to avoid shipping the corresponding object store values across IPC for each index entry.

Great catch, thanks!
Comment 8 Joshua Bell 2011-12-05 09:40:55 PST
Created attachment 117889 [details]
Patch
Comment 9 Joshua Bell 2011-12-07 11:49:39 PST
abarth@ - can you look at the V8 "binding" change? (Note that IDB is still not supported with JSC)
Comment 10 David Levin 2011-12-07 11:56:22 PST
(In reply to comment #5)
> (From update of attachment 117675 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117675&action=review
> 
> Just a drive by comment.
> 
> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:554
> > +    RefPtr<IDBTransactionBackendInterface> transactionPtr = transaction;
> 
> fwiw, you should be able to pass these directly into createCallbackTask.

This issue still exists.
Comment 11 Joshua Bell 2011-12-07 12:00:20 PST
(In reply to comment #10)
> (In reply to comment #5)
> > (From update of attachment 117675 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=117675&action=review
> > 
> > Just a drive by comment.
> > 
> > > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:554
> > > +    RefPtr<IDBTransactionBackendInterface> transactionPtr = transaction;
> > 
> > fwiw, you should be able to pass these directly into createCallbackTask.
> 
> This issue still exists.

Ooops, thanks. I must have cleaned up some instances of that pattern and missed the specific one you pointed out. :P
Comment 12 Joshua Bell 2011-12-07 12:39:35 PST
Created attachment 118253 [details]
Patch
Comment 13 David Grogan 2011-12-07 13:12:43 PST
Comment on attachment 118253 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        a single IDBKey. That will require some IDL/binding monkeying and it

Do we have a bug open about this?  I forgot the details.

> Source/WebCore/storage/IDBIndex.h:61
> +    PassRefPtr<IDBRequest> count(ScriptExecutionContext* context, ExceptionCode& ec) { return count(context, 0, ec); }

This first method will never be called because there's no binding to it, correct?

> LayoutTests/storage/indexeddb/index-count.html:63
> +    store = evalAndLog("index = store.index('indexName')");

Should this be "index = "?  It looks like the test still works because window.index was stored above.
Comment 14 Joshua Bell 2011-12-07 13:56:34 PST
Comment on attachment 118253 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        a single IDBKey. That will require some IDL/binding monkeying and it
> 
> Do we have a bug open about this?  I forgot the details.

crbug.com/92046 crbug.com/92047 crbug.com/101384 discuss other methods; I'll file a bug for this method

>> Source/WebCore/storage/IDBIndex.h:61
>> +    PassRefPtr<IDBRequest> count(ScriptExecutionContext* context, ExceptionCode& ec) { return count(context, 0, ec); }
> 
> This first method will never be called because there's no binding to it, correct?

The KeyRange argument is defined as Optional in the IDL; the generated binding code calls this first method if no arguments are passed on the JavaScript side.

>> LayoutTests/storage/indexeddb/index-count.html:63
>> +    store = evalAndLog("index = store.index('indexName')");
> 
> Should this be "index = "?  It looks like the test still works because window.index was stored above.

Good catch. But actually, in the pattern used in most of our tests:

    FOO = evalAndLog("BAR = EXPR"); 

...both window.FOO and window.BAR end up being set to the result of EXPR. We don't need both, but the inner one is logged and the outer one makes subsequent local uses more readable. So this worked because nothing later was dependent on window.store. Ah, JavaScript global variables...
Comment 15 Joshua Bell 2011-12-07 13:57:56 PST
Created attachment 118273 [details]
Patch
Comment 16 Joshua Bell 2011-12-09 11:43:29 PST
Created attachment 118600 [details]
Patch
Comment 17 Joshua Bell 2011-12-09 11:44:33 PST
This should be the final patch (same as previous, just minus the WebKit/chromium/public changes). Needs to wait for the Chromium side to land.
Comment 18 Joshua Bell 2011-12-16 12:13:58 PST
Comment on attachment 118600 [details]
Patch

Chromium side has landed, should be good to go.
Comment 19 Joshua Bell 2011-12-16 12:15:04 PST
tony@, can you r? cq?
Comment 20 David Grogan 2011-12-16 12:20:54 PST
Comment on attachment 118600 [details]
Patch

IDB functionality LGTM
Comment 21 Tony Chang 2011-12-16 12:42:13 PST
Comment on attachment 118600 [details]
Patch

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

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:566
> +    do {
> +        ++count;
> +    } while (backingStoreCursor->continueFunction(0));

This seems really inefficient.  Is this the only way to get the count?
Comment 22 Joshua Bell 2011-12-16 13:15:22 PST
(In reply to comment #21)
> (From update of attachment 118600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118600&action=review
> 
> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:566
> > +    do {
> > +        ++count;
> > +    } while (backingStoreCursor->continueFunction(0));
> 
> This seems really inefficient.  Is this the only way to get the count?

LevelDB doesn't have a count primitive, at the moment. We'll need to follow up on that as we do some IDB refactoring.
Comment 23 WebKit Review Bot 2011-12-16 13:27:22 PST
Comment on attachment 118600 [details]
Patch

Clearing flags on attachment: 118600

Committed r103100: <http://trac.webkit.org/changeset/103100>
Comment 24 WebKit Review Bot 2011-12-16 13:27:29 PST
All reviewed patches have been landed.  Closing bug.