Bug 166002 – Remove setDOMException and a few custom bindings

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

[Darin Adler]

Remove setDOMException and a few custom bindings

[Darin Adler]

Created attachment 297418 [details]
Patch

[Build Bot]

Comment on attachment 297418 [details]
Patch

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

New failing tests:
storage/indexeddb/modern/index-cursor-1.html
storage/indexeddb/modern/index-2-private.html
storage/indexeddb/index-basics-private.html
imported/w3c/web-platform-tests/IndexedDB/idbobjectstore-rename-store.html
storage/indexeddb/modern/index-2.html
storage/indexeddb/modern/index-1-private.html
storage/indexeddb/mozilla/autoincrement-indexes-private.html
storage/indexeddb/mozilla/cursor-update-updates-indexes-private.html
storage/indexeddb/index-duplicate-keypaths-private.html
storage/indexeddb/mozilla/autoincrement-indexes.html
storage/indexeddb/modern/index-3-private.html
storage/indexeddb/index-duplicate-keypaths.html
storage/indexeddb/index-basics-workers.html
storage/indexeddb/index-basics.html
storage/indexeddb/lazy-index-types.html
storage/indexeddb/modern/index-cursor-1-private.html
storage/indexeddb/mozilla/cursor-update-updates-indexes.html
storage/indexeddb/modern/index-1.html
storage/indexeddb/lazy-index-types-private.html
storage/indexeddb/modern/index-3.html

[Build Bot]

Created attachment 297419 [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 297418 [details]
Patch

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

New failing tests:
storage/indexeddb/modern/index-cursor-1.html
storage/indexeddb/modern/index-1.html
storage/indexeddb/modern/index-2-private.html
imported/w3c/web-platform-tests/IndexedDB/idbobjectstore-rename-store.html
storage/indexeddb/modern/index-2.html
storage/indexeddb/modern/index-1-private.html
storage/indexeddb/mozilla/autoincrement-indexes-private.html
storage/indexeddb/mozilla/cursor-update-updates-indexes-private.html
storage/indexeddb/index-duplicate-keypaths-private.html
storage/indexeddb/lazy-index-types-private.html
storage/indexeddb/modern/index-3.html
storage/indexeddb/index-duplicate-keypaths.html
storage/indexeddb/index-basics-workers.html
storage/indexeddb/index-basics.html
storage/indexeddb/lazy-index-types.html
storage/indexeddb/modern/index-cursor-1-private.html
storage/indexeddb/mozilla/cursor-update-updates-indexes.html
storage/indexeddb/modern/index-3-private.html
storage/indexeddb/mozilla/autoincrement-indexes.html
storage/indexeddb/index-basics-private.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 297418 [details]
Patch

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

New failing tests:
storage/indexeddb/modern/index-cursor-1.html
storage/indexeddb/modern/index-2-private.html
storage/indexeddb/index-basics-private.html
imported/w3c/web-platform-tests/IndexedDB/idbobjectstore-rename-store.html
storage/indexeddb/modern/index-2.html
storage/indexeddb/modern/index-1-private.html
storage/indexeddb/mozilla/autoincrement-indexes-private.html
storage/indexeddb/mozilla/cursor-update-updates-indexes-private.html
storage/indexeddb/index-duplicate-keypaths-private.html
storage/indexeddb/mozilla/autoincrement-indexes.html
storage/indexeddb/modern/index-3-private.html
storage/indexeddb/index-duplicate-keypaths.html
storage/indexeddb/index-basics-workers.html
storage/indexeddb/index-basics.html
storage/indexeddb/lazy-index-types.html
storage/indexeddb/modern/index-cursor-1-private.html
storage/indexeddb/mozilla/cursor-update-updates-indexes.html
storage/indexeddb/modern/index-1.html
storage/indexeddb/lazy-index-types-private.html
storage/indexeddb/modern/index-3.html

[Build Bot]

Created attachment 297421 [details]
Archive of layout-test-results from ews117 for mac-yosemite

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

[Build Bot]

Comment on attachment 297418 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/idbobjectstore-rename-store.html

[Build Bot]

Created attachment 297433 [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.11.6

[Darin Adler]

Created attachment 297762 [details]
Patch

[Darin Adler]

EWS passing now, so time for review.

[Sam Weinig]

Comment on attachment 297762 [details]
Patch

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

> Source/WebCore/bindings/js/CallbackFunction.cpp:43
>      JSC::CallData callData;
>      if (getCallData(value, callData) == JSC::CallType::None) {

Can this use JSValue::isFunction() which doesn't require the dummy callData? (Obviously not required for this change).

> Source/WebCore/bindings/js/CallbackFunction.cpp:45
> +        auto& vm = state->vm();
> +        auto scope = DECLARE_THROW_SCOPE(vm);

I haven't decided if it makes sense to put vm in it's own variable if it is used once.  We should define a pattern we like and stick with it, but so far, I think we are a bit inconsistent.

> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:137
> +    CryptoOperationData ivData = cryptoOperationDataFromJSValue(state, scope, iv);

auto?

> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:274
> +    CryptoOperationData labelData = cryptoOperationDataFromJSValue(state, scope, labelValue);

auto?

> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.h:45
> +    static CryptoAlgorithmIdentifier getAlgorithmIdentifier(JSC::ExecState&, JSC::ThrowScope&, JSC::JSValue);

Does this really need the get prefix? Can it be something other than 'get' if JSCryptoAlgorithmDictionary::algorithmIdentifier() is too generic. Maybe, algorithmIdentifierFromJSValue to match cryptoOperationDataFromJSValue?

> Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:92
> +static CryptoKeyFormat cryptoKeyFormatFromJSValue(ExecState& state, ThrowScope& scope, JSValue value)
>  {
> -    VM& vm = state.vm();
> -    auto scope = DECLARE_THROW_SCOPE(vm);
> -
>      String keyFormatString = value.toWTFString(&state);
> -    RETURN_IF_EXCEPTION(scope, false);
> +    RETURN_IF_EXCEPTION(scope, { });
> +
>      if (keyFormatString == "raw")
> -        result = CryptoKeyFormat::Raw;
> -    else if (keyFormatString == "pkcs8")
> -        result = CryptoKeyFormat::PKCS8;
> -    else if (keyFormatString == "spki")
> -        result = CryptoKeyFormat::SPKI;
> -    else if (keyFormatString == "jwk")
> -        result = CryptoKeyFormat::JWK;
> -    else {
> -        throwTypeError(&state, scope, ASCIILiteral("Unknown key format"));
> -        return false;
> -    }
> -    return true;
> +        return CryptoKeyFormat::Raw;
> +    if (keyFormatString == "pkcs8")
> +        return CryptoKeyFormat::PKCS8;
> +    if (keyFormatString == "spki")
> +        return CryptoKeyFormat::SPKI;
> +    if (keyFormatString == "jwk")
> +        return CryptoKeyFormat::JWK;
> +
> +    throwTypeError(&state, scope, ASCIILiteral("Unknown key format"));
> +    return { };

Different patch, but this should just be calling convert<IDLEnumeration<KeyFormat>>(...) and adding the KeyFormat idl file.

> Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:100
> +static CryptoKeyUsageBitmap cryptoKeyUsagesFromJSValue(ExecState& state, ThrowScope& scope, JSValue value)
>  {
> -    VM& vm = state.vm();
> -    auto scope = DECLARE_THROW_SCOPE(vm);
> -
>      if (!isJSArray(value)) {
>          throwTypeError(&state, scope);
> -        return false;
> +        return { };
>      }

Seems like this should be generated as well.

> Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:105
> +        String usageString = array->getIndex(&state, i).toWTFString(&state);

auto?

[Darin Adler]

Comment on attachment 297762 [details]
Patch

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

>> Source/WebCore/bindings/js/CallbackFunction.cpp:43
>>      if (getCallData(value, callData) == JSC::CallType::None) {
> 
> Can this use JSValue::isFunction() which doesn't require the dummy callData? (Obviously not required for this change).

Good question; I’m not sure whether the behavior is any different.

>> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:137
>> +    CryptoOperationData ivData = cryptoOperationDataFromJSValue(state, scope, iv);
> 
> auto?

Yes, will do.

>> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:274
>> +    CryptoOperationData labelData = cryptoOperationDataFromJSValue(state, scope, labelValue);
> 
> auto?

Will do.

>> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.h:45
>> +    static CryptoAlgorithmIdentifier getAlgorithmIdentifier(JSC::ExecState&, JSC::ThrowScope&, JSC::JSValue);
> 
> Does this really need the get prefix? Can it be something other than 'get' if JSCryptoAlgorithmDictionary::algorithmIdentifier() is too generic. Maybe, algorithmIdentifierFromJSValue to match cryptoOperationDataFromJSValue?

Sure, we could change the name. I didn't even think about it. I think parseAlgorithmIdentifier is what I will call it.

>> Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:92
>> +    return { };
> 
> Different patch, but this should just be calling convert<IDLEnumeration<KeyFormat>>(...) and adding the KeyFormat idl file.

I see your point.

>> Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:100
>>      }
> 
> Seems like this should be generated as well.

Agreed.

>> Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:105
>> +        String usageString = array->getIndex(&state, i).toWTFString(&state);
> 
> auto?

Yes, will do.

[Darin Adler]

Committed r210217: <http://trac.webkit.org/changeset/210217>

[Chris Dumez]

Comment on attachment 297762 [details]
Patch

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

> Source/WebCore/bindings/js/JSStorageCustom.cpp:48
> +        return false;

This fixed <rdar://problem/30260068>.