Bug 150743

In addition to a test that deals solely with dates-as-keys, the test landed in https://trac.webkit.org/changeset/191832 can be augmented with dates.

Created attachment 264465 [details] Patch v1

(In reply to comment #1) > In addition to a test that deals solely with dates-as-keys, the test landed > in https://trac.webkit.org/changeset/191832 can be augmented with dates. This patch includes a new targeted test as well as adding Dates to the r191832 test

Comment on attachment 264465 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=264465&action=review All the function arguments should be just JSValue instead of const JSValue&, because a JSValue is a small scalar and better to pass by value. > Source/WebCore/Modules/indexeddb/IDBObjectStore.idl:37 > + [CallWith=ScriptExecutionContext, Custom, ImplementedAs=putRecord, RaisesException] IDBRequest put(any value, optional any key); The ImplementedAs is awkward. I assume there's a name conflict, but I don’t understand why put has a conflict and add does not. > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:310 > + Vector<RefPtr<IDBKey>> result; result.reserveInitialCapacity(array.size()); > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:312 > + for (size_t i = 0; i < array.size(); i++) { Use a modern for loop here: for (auto& string : array) { > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:316 > + result.append(key); WTF::move(key) result.uncheckedAppend(WTF::move(key)); > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:318 > + return IDBKey::createArray(result); WTF::move(result) maybe? Not sure it’s helpful. > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:59 > + size_t argsCount = state.argumentCount(); > + if (UNLIKELY(argsCount < 1)) > + return JSValue::decode(throwVMError(&state, createNotEnoughArgumentsError(&state))); As I understand it, we should almost never be checking argument count. Modern DOM bindings check for undefined to see if an argument is missing, and never have to check the argument count itself. > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:61 > + auto value = state.argument(0); If we retain the argumentCount check, then this should be uncheckedArgument(0). If we get rid of it, then we’ll likely be checking for undefined at some point. > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:63 > + if (UNLIKELY(state.hadException())) > + return jsUndefined(); This hadException check is not needed; extracting an argument from ExecState cannot cause an exception. > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:72 > + if (overwrite) > + result = toJS(&state, castedThis->globalObject(), WTF::getPtr(impl.put(state, value, ec))); > + else > + result = toJS(&state, castedThis->globalObject(), WTF::getPtr(impl.add(state, value, ec))); Hand written code should not need to use WTF::getPtr. Instead we can either put in a .get() or not depending on the specific type. The automatically generated code uses getPtr to be generic, but that’s not important for hand-written code. > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:77 > + if (UNLIKELY(state.hadException())) > + return jsUndefined(); > + return result; The hadException check is not needed; the result will be ignored if an exception is raised; it’s OK to return result. > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:80 > + auto key = state.argument(1); If we retain the argumentCount check, then this should be uncheckedArgument(1), because we’ve already determined that the argument count is not < 1 and it’s not 1, so it’s at least 2. If we get rid of the argumentCount check, then we’ll likely be checking for undefined at some point. > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:82 > + if (UNLIKELY(state.hadException())) > + return jsUndefined(); This hadException check is not needed; extracting an argument from ExecState cannot cause an exception. > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:94 > + if (UNLIKELY(state.hadException())) > + return jsUndefined(); > + > + return result; The hadException check is not needed; the result will be ignored if an exception is raised; it’s OK to return result.

(In reply to comment #4) > Comment on attachment 264465 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264465&action=review > > All the function arguments should be just JSValue instead of const JSValue&, > because a JSValue is a small scalar and better to pass by value. I did not know that about JSValue! Great! > > Source/WebCore/Modules/indexeddb/IDBObjectStore.idl:37 > > + [CallWith=ScriptExecutionContext, Custom, ImplementedAs=putRecord, RaisesException] IDBRequest put(any value, optional any key); > > The ImplementedAs is awkward. I assume there's a name conflict, but I don’t > understand why put has a conflict and add does not. There's a name conflict with the JS object structures but only on put. > > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:310 > > + Vector<RefPtr<IDBKey>> result; > > result.reserveInitialCapacity(array.size()); Yup. > > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:312 > > + for (size_t i = 0; i < array.size(); i++) { > > Use a modern for loop here: > > for (auto& string : array) { Yup. > > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:316 > > + result.append(key); > > WTF::move(key) > > result.uncheckedAppend(WTF::move(key)); Cool. > > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:318 > > + return IDBKey::createArray(result); > > WTF::move(result) maybe? Not sure it’s helpful. Will look into it. > > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:59 > > + size_t argsCount = state.argumentCount(); > > + if (UNLIKELY(argsCount < 1)) > > + return JSValue::decode(throwVMError(&state, createNotEnoughArgumentsError(&state))); > > As I understand it, we should almost never be checking argument count. > Modern DOM bindings check for undefined to see if an argument is missing, > and never have to check the argument count itself. While I'll trust you on the argsCount < 1 part of this, on the argsCount < 2 part it still seems necessary since it's an optional argument. > > > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:61 > > + auto value = state.argument(0); > > If we retain the argumentCount check, then this should be > uncheckedArgument(0). If we get rid of it, then we’ll likely be checking for > undefined at some point. > > > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:63 > > + if (UNLIKELY(state.hadException())) > > + return jsUndefined(); > > This hadException check is not needed; extracting an argument from ExecState > cannot cause an exception. I pulled that from auto-gen code, but it appears it is only relevant WRT making Deprecated::ScriptValues in the VM. Will remove! > > > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:72 > > + if (overwrite) > > + result = toJS(&state, castedThis->globalObject(), WTF::getPtr(impl.put(state, value, ec))); > > + else > > + result = toJS(&state, castedThis->globalObject(), WTF::getPtr(impl.add(state, value, ec))); > > Hand written code should not need to use WTF::getPtr. Instead we can either > put in a .get() or not depending on the specific type. The automatically > generated code uses getPtr to be generic, but that’s not important for > hand-written code. Totally makes sense (You can probably tell by now I started with the auto-gen'ed impl) > > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:77 > > + if (UNLIKELY(state.hadException())) > > + return jsUndefined(); > > + return result; > > The hadException check is not needed; the result will be ignored if an > exception is raised; it’s OK to return result. Yup. > > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:80 > > + auto key = state.argument(1); > > If we retain the argumentCount check, then this should be > uncheckedArgument(1), because we’ve already determined that the argument > count is not < 1 and it’s not 1, so it’s at least 2. If we get rid of the > argumentCount check, then we’ll likely be checking for undefined at some > point. Makes sense. > > > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:82 > > + if (UNLIKELY(state.hadException())) > > + return jsUndefined(); > > This hadException check is not needed; extracting an argument from ExecState > cannot cause an exception. > > > Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:94 > > + if (UNLIKELY(state.hadException())) > > + return jsUndefined(); > > + > > + return result; > > The hadException check is not needed; the result will be ignored if an > exception is raised; it’s OK to return result. Got it and got it!

Created attachment 264501 [details] Patch for landing.

Comment on attachment 264501 [details] Patch for landing. Clearing flags on attachment: 264501 Committed r191850: <http://trac.webkit.org/changeset/191850>