IndexedDB: Implement IDBObjectStore.count() and IDBIndex.count()

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Implement IDBObjectStore.count() and IDBIndex.count()

[Joshua Bell]

Created attachment 117675 [details]
Patch

[Joshua Bell]

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.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Joshua Bell]

This is Part 2 of a two-sided change. In between, I'll land http://codereview.chromium.org/8779003 in Chromium.

[David Levin]

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.

[Hans Wennborg]

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

[Joshua Bell]

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!

[Joshua Bell]

Created attachment 117889 [details]
Patch

[Joshua Bell]

abarth@ - can you look at the V8 "binding" change? (Note that IDB is still not supported with JSC)

[David Levin]

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

[Joshua Bell]

(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

[Joshua Bell]

Created attachment 118253 [details]
Patch

[David Grogan]

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.

[Joshua Bell]

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

[Joshua Bell]

Created attachment 118273 [details]
Patch

[Joshua Bell]

Created attachment 118600 [details]
Patch

[Joshua Bell]

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.

[Joshua Bell]

Comment on attachment 118600 [details]
Patch

Chromium side has landed, should be good to go.

[Joshua Bell]

tony@, can you r? cq?

[David Grogan]

Comment on attachment 118600 [details]
Patch

IDB functionality LGTM

[Tony Chang]

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?

[Joshua Bell]

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

[WebKit Review Bot]

Comment on attachment 118600 [details]
Patch

Clearing flags on attachment: 118600

Committed r103100: <http://trac.webkit.org/changeset/103100>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.