Bug 156681 – Get rid of IDBAny

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Darin Adler]

Get rid of IDBAny

[Darin Adler]

Created attachment 276606 [details]
Patch

[Darin Adler]

Brady, as I promised, here’s a patch that deletes the IDBAny class.

[Darin Adler]

Comment on attachment 276606 [details]
Patch

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

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:311
> +    // FIXME: I think itâs peculiar to use undefined to mean "null data" and null to mean "empty data".

Fixed this locally to use a straight apostrophe.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:332
> +    // FIXME: I think itâs peculiar to return an empty JSValue, undefined, and null for three different error cases.

Fixed this locally to use a straight apostrophe.

[Darin Adler]

Comment on attachment 276606 [details]
Patch

Oops, this compiles but it‘s failing all the tests! Gotta fix it.

[Darin Adler]

Comment on attachment 276606 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:171
> +    return m_isDone ? pendingString : doneString;

Tests were failing because I had this backwards.

[Build Bot]

Comment on attachment 276606 [details]
Patch

Attachment 276606 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1177551

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 276607 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 276606 [details]
Patch

Attachment 276606 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1177555

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 276608 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 276606 [details]
Patch

Attachment 276606 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1177565

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 276609 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 276606 [details]
Patch

Attachment 276606 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1177569

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 276610 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Darin Adler]

Created attachment 276614 [details]
Patch

[Darin Adler]

OK, got the tests all passing now.

[Brady Eidson]

Comment on attachment 276614 [details]
Patch

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

So much of this cleanup is reading my mind.

I hadn't taken it as far as "get rid of IDBAny" but I'm glad you did!

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:162
> +RefPtr<WebCore::IDBRequest> IDBCursor::update(JSC::ExecState& exec, JSC::JSValue value, ExceptionCodeWithMessage& ec)

JSC:: not needed in this file.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:263
> +void IDBCursor::continueFunction(ScriptExecutionContext& context, JSC::JSValue keyValue, ExceptionCodeWithMessage& ec)

Ditto.

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:150
> +short IDBFactory::cmp(ScriptExecutionContext& context, JSC::JSValue firstValue, JSC::JSValue secondValue, ExceptionCodeWithMessage& ec)

Would rather see a `using namespace JSC;` in this file.

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:126
> +RefPtr<IDBRequest> IDBIndex::openCursor(ScriptExecutionContext& context, JSC::JSValue key, const String& direction, ExceptionCodeWithMessage& ec)

Would rather see a `using namespace JSC;` in this file.

> Source/WebCore/Modules/indexeddb/IDBIndex.idl:35
> +    [ImplementationReturnType=IDBKeyPath] readonly attribute any keyPath;

I love that ImplementationReturnType is already being expanded upon!

> Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:84
> +RefPtr<IDBKeyRange> IDBKeyRange::only(ScriptExecutionContext& context, JSC::JSValue keyValue, ExceptionCode& ec)

No need for JSC:: in this file.

> Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:95
> +RefPtr<IDBKeyRange> IDBKeyRange::lowerBound(ScriptExecutionContext& context, JSC::JSValue boundValue, bool open, ExceptionCode& ec)

Ditto.

> Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:106
> +RefPtr<IDBKeyRange> IDBKeyRange::upperBound(ScriptExecutionContext& context, JSC::JSValue boundValue, bool open, ExceptionCode& ec)

Ditto.

> Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:117
> +RefPtr<IDBKeyRange> IDBKeyRange::bound(ScriptExecutionContext& context, JSC::JSValue lowerValue, JSC::JSValue upperValue, bool lowerOpen, bool upperOpen, ExceptionCode& ec)

Ditto.

> Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:160
> +RefPtr<IDBKeyRange> IDBKeyRange::lowerBound(ScriptExecutionContext& context, JSC::JSValue bound, ExceptionCode& ec)
> +{
> +    return lowerBound(context, bound, false, ec);
> +}
> +
> +RefPtr<IDBKeyRange> IDBKeyRange::upperBound(ScriptExecutionContext& context, JSC::JSValue bound, ExceptionCode& ec)
> +{
> +    return upperBound(context, bound, false, ec);
> +}
> +
> +RefPtr<IDBKeyRange> IDBKeyRange::bound(ScriptExecutionContext& context, JSC::JSValue lower, JSC::JSValue upper, ExceptionCode& ec)
> +{
> +    return bound(context, lower, upper, false, false, ec);
> +}
> +
> +RefPtr<IDBKeyRange> IDBKeyRange::bound(ScriptExecutionContext& context, JSC::JSValue lower, JSC::JSValue upper, bool lowerOpen, ExceptionCode& ec)
> +{
> +    return bound(context, lower, upper, lowerOpen, false, ec);

Ditto.

> Source/WebCore/Modules/indexeddb/IDBKeyRange.h:26
> +#pragma once

Woohoo!

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:123
> +RefPtr<IDBRequest> IDBObjectStore::openCursor(ScriptExecutionContext& context, JSC::JSValue key, ExceptionCodeWithMessage& ec)

Would rather see a `using namespace JSC;` in this file.

> Source/WebCore/Modules/indexeddb/server/MemoryIndex.h:36
> -#include <set>
>  #include <wtf/HashMap.h>
> +#include <wtf/HashSet.h>

<set> was, indeed, erroneous.

But since MemoryIndex already had a HashSet member, HashSet must've been included via a different .h, right?

Is the <wtf/HashSet.h> needed?

> Source/WebCore/bindings/js/JSFontFaceCustom.cpp:40
> -    auto& promise = wrapped().promise();
> -    return promise.deferred().promise();
> +    return wrapped().promise().deferred().promise();

Even though this patch has a lot of random cleanup, it's exclusively in IDB code... except for this. Which seems peculiar.

[Darin Adler]

Comment on attachment 276614 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/server/MemoryIndex.h:36
>> +#include <wtf/HashSet.h>
> 
> <set> was, indeed, erroneous.
> 
> But since MemoryIndex already had a HashSet member, HashSet must've been included via a different .h, right?
> 
> Is the <wtf/HashSet.h> needed?

I am pretty sure it’s needed, because of some include I removed from some other file. But I’ll double check before landing.

>> Source/WebCore/bindings/js/JSFontFaceCustom.cpp:40
>> +    return wrapped().promise().deferred().promise();
> 
> Even though this patch has a lot of random cleanup, it's exclusively in IDB code... except for this. Which seems peculiar.

I’ll omit this change. I can’t remember how it got into the patch. Maybe I was looking at files that needlessly included the header Dictionary.h?

[Darin Adler]

Committed r199668: <http://trac.webkit.org/changeset/199668>