Bug 150785 – Modern IDB: IBDObjectStore.count() support

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Modern IDB: IBDObjectStore.count() support

[Brady Eidson]

Created attachment 264537 [details]
Patch v1

[Darin Adler]

Comment on attachment 264537 [details]
Patch v1

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

> LayoutTests/storage/indexeddb/modern/idbobjectstore-count-1.html:29
> +        alert("Count is: " + request.result);

Using alert for all the logging is lame: impractical to run in browser (long series of alerts, unclear which ones are success and which are failure), and ugly when running with the test runner.

But also, the output gives no indication of what is being tested. We’d need to fix that.

We have functions to help write tests that write out pass and fail, and testing practices where we write out the expression we are testing and we try to indicate what we are doing and what’s being tested.

> Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.cpp:335
> +    if (!context) {
> +        ec = INVALID_STATE_ERR;
> +        return nullptr;
> +    }

Is a null value for the context actually a possibility? I think this is dead, untested code. Right way to deal with this slightly longer term is to make sure we pass ScriptExecutionContext& instead. Same comment for all our null checking.

> Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.cpp:391
> +    Ref<IDBRequest> request = m_transaction->requestCount(context, *this, range);
> +    return WTF::move(request);

Should just do this in one line without a local variable. Also no need for WTF::move when you do that.

> Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.h:88
> +    RefPtr<WebCore::IDBRequest> doCount(ScriptExecutionContext&, const IDBKeyRangeData&, ExceptionCode&);

Can this return null or not? If not, please make it return Ref instead of RefPtr.

> Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:444
> +    auto operation = createTransactionOperation(*this, request.get(), &IDBTransaction::didGetCountOnServer, &IDBTransaction::getCountOnServer, range);
> +    scheduleOperation(WTF::move(operation));

Could consider doing this all on one line; would eliminate the need for WTF::move.

> Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:446
> +    return WTF::move(request);

I think a return at the end of a function is one case where we never need to do a WTF::move.

> Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:148
> +        key = lowestKeyWithRecordInRange(range);

A real shame that the loop is structured like this. Can we find a way to not have to repeat this twice?

> Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:151
> +    WTFLogAlways("Returning count %llu", count);

I don’t think we want to use WTFLogAlways for anything like this. Would we really want this to always be logged, even with all logging channels turned off?

Also, %llu for a uint64_t is not portable. You can use unsigned long long with %llu, or you can use the PRIu64 macro with uint64_t.

> Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:170
> +        return IDBKeyData();

Could do:

    return { };

I think it’s a bit nicer than repeating the type all the time.

> Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:180
> +        return IDBKeyData();

Ditto, etc.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:160
> +    m_countCallbacks.set(identifier, callback);

Should say:

    ASSERT(!m_countCallbacks.contains(identifier));
    m_countCallbacks.add(identifier, callback);

Using add rather than set is ever so slightly more efficient.

> Source/WebCore/Modules/indexeddb/shared/IDBResultData.h:99
> +    uint64_t m_resultInteger;

Maybe initialize this to 0 rather than leaving it uninitialized?

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:164
> +    RefPtr<InProcessIDBServer> self(this);

I think this should be named protectedThis. Calling it “self” is a bit too mysterious.

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:232
> +    RefPtr<InProcessIDBServer> self(this);

I think this should be named protectedThis. Calling it “self” is a bit too mysterious.

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:233
> +

Lets not leave this blank line.

[Brady Eidson]

(In reply to comment #2)
> Comment on attachment 264537 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264537&action=review
> 
> > LayoutTests/storage/indexeddb/modern/idbobjectstore-count-1.html:29
> > +        alert("Count is: " + request.result);
> 
> Using alert for all the logging is lame: impractical to run in browser (long
> series of alerts, unclear which ones are success and which are failure), and
> ugly when running with the test runner.
> 
> But also, the output gives no indication of what is being tested. We’d need
> to fix that.
> 
> We have functions to help write tests that write out pass and fail, and
> testing practices where we write out the expression we are testing and we
> try to indicate what we are doing and what’s being tested.

and

> > Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.cpp:335
> > +    if (!context) {
> > +        ec = INVALID_STATE_ERR;
> > +        return nullptr;
> > +    }
> 
> Is a null value for the context actually a possibility? I think this is
> dead, untested code. Right way to deal with this slightly longer term is to
> make sure we pass ScriptExecutionContext& instead. Same comment for all our
> null checking.

Points taken.

> > Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.cpp:391
> > +    Ref<IDBRequest> request = m_transaction->requestCount(context, *this, range);
> > +    return WTF::move(request);
> 
> Should just do this in one line without a local variable. Also no need for
> WTF::move when you do that.
> 
> > Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.h:88
> > +    RefPtr<WebCore::IDBRequest> doCount(ScriptExecutionContext&, const IDBKeyRangeData&, ExceptionCode&);
> 
> Can this return null or not? If not, please make it return Ref instead of
> RefPtr.

Yes it can (correctly) return nullptr.

> > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:444
> > +    auto operation = createTransactionOperation(*this, request.get(), &IDBTransaction::didGetCountOnServer, &IDBTransaction::getCountOnServer, range);
> > +    scheduleOperation(WTF::move(operation));
> 
> Could consider doing this all on one line; would eliminate the need for
> WTF::move.

Seems reasonable.

> 
> > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:446
> > +    return WTF::move(request);
> 
> I think a return at the end of a function is one case where we never need to
> do a WTF::move.

I "knew" this, but still have a hard time internalizing this. Will change.

> > Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:148
> > +        key = lowestKeyWithRecordInRange(range);
> 
> A real shame that the loop is structured like this. Can we find a way to not
> have to repeat this twice?

With a while(true) and a break, yes. I don't like the loop as much, but will stick with it because it removes that line being written twice.

> > Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:151
> > +    WTFLogAlways("Returning count %llu", count);
> 
> I don’t think we want to use WTFLogAlways for anything like this. Would we
> really want this to always be logged, even with all logging channels turned
> off?
> 
> Also, %llu for a uint64_t is not portable. You can use unsigned long long
> with %llu, or you can use the PRIu64 macro with uint64_t.

This was simply errantly left in the patch and is now gone.

> > Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:170
> > +        return IDBKeyData();
> 
> Could do:
> 
>     return { };
> 
> I think it’s a bit nicer than repeating the type all the time.
> 
> > Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:180
> > +        return IDBKeyData();
> 
> Ditto, etc.

Done, ditto, etc.

> > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:160
> > +    m_countCallbacks.set(identifier, callback);
> 
> Should say:
> 
>     ASSERT(!m_countCallbacks.contains(identifier));
>     m_countCallbacks.add(identifier, callback);
> 
> Using add rather than set is ever so slightly more efficient.
> 
> > Source/WebCore/Modules/indexeddb/shared/IDBResultData.h:99
> > +    uint64_t m_resultInteger;
> 
> Maybe initialize this to 0 rather than leaving it uninitialized?

Undoubtedly. 

> > Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:164
> > +    RefPtr<InProcessIDBServer> self(this);
> 
> I think this should be named protectedThis. Calling it “self” is a bit too
> mysterious.
> 
> > Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:232
> > +    RefPtr<InProcessIDBServer> self(this);
> 
> I think this should be named protectedThis. Calling it “self” is a bit too
> mysterious.

I will consider a followup that does a sweep of the whole file, as "self" is rather established practice at this point.

> > Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:233
> > +
> 
> Lets not leave this blank line.

[Brady Eidson]

Created attachment 264546 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 264546 [details]
Patch for landing.

Rejecting attachment 264546 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
bKit/Source/WebCore/Modules/indexeddb/legacy/IDBDatabaseBackend.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/IDBDatabaseBackend.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/IDBConnectionToServer.o Modules/indexeddb/client/IDBConnectionToServer.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/369559

[Brady Eidson]

Created attachment 264550 [details]
Patch for landing v2

[WebKit Commit Bot]

Comment on attachment 264550 [details]
Patch for landing v2

Rejecting attachment 264550 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
es/Data/EWS/WebKit/Source/WebCore/svg/graphics/SVGImageForContainer.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/SVGImageForContainer.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/IDBObjectStoreImpl.o Modules/indexeddb/client/IDBObjectStoreImpl.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/369943

[Brady Eidson]

Created attachment 264554 [details]
Patch for landing 3

[WebKit Commit Bot]

Comment on attachment 264554 [details]
Patch for landing 3

Clearing flags on attachment: 264554

Committed r191877: <http://trac.webkit.org/changeset/191877>