Location.ancestorOrigins should return a FrozenArray<USVString>
Created attachment 286495 [details] Patch
Comment on attachment 286495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286495&action=review r=me with fixes > Source/WebCore/Modules/indexeddb/IDBDatabase.idl:39 > + [RaisesExceptionWithMessage] IDBTransaction transaction(DOMString storeName, optional DOMString mode); mode = "readonly" > Source/WebCore/Modules/indexeddb/IDBDatabase.idl:40 > + [RaisesExceptionWithMessage] IDBTransaction transaction(sequence<DOMString> storeNames, optional DOMString mode); mode = "readonly" > Source/WebCore/bindings/js/JSDOMBinding.h:638 > + list.append(JSValueTraits<T>::arrayJSValue(exec, globalObject, element)); Cannot arrayJSValue() throw? Is it OK not to check if we had an exception while converting? > Source/WebCore/bindings/scripts/CodeGenerator.pm:521 > +sub IsSequenceType Should probably be moved to CodeGenerator.pm? > Source/WebCore/bindings/scripts/CodeGenerator.pm:538 > +sub IsFrozenArrayType Should probably be moved to CodeGenerator.pm? > Source/WebCore/bindings/scripts/CodeGenerator.pm:546 > +sub GetFrozenArrayInnerType Should probably be moved to CodeGenerator.pm? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1769 > + return 0 if $codeGenerator->GetFrozenArrayInnerType($typeA) && $codeGenerator->GetFrozenArrayInnerType($typeB); This is actually not entirely accurate because we cannot distinguish a sequence type and a Frozen array either: http://heycam.github.io/webidl/#dfn-distinguishable You'll need a isSequenceOrFrozenArray() subroutine then: return 0 if $codeGenerator->isSequenceOrFrozenArray($typeA) && $codeGenerator->isSequenceOrFrozenArray($typeB); Then we can drop: return 0 if $codeGenerator->GetSequenceType($typeA) && $codeGenerator->GetSequenceType($typeB); > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1864 > + return $codeGenerator->GetFrozenArrayInnerType($type); Don't you mean IsFrozenArrayType() ? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1942 > + $overload = GetOverloadThatMatches($S, $d, \&$isFrozenArrayParameter); Should be merged with the one for sequence above using a isSequenceOrFrozenArrayParameter subroutine. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4677 > + my $subtype = $codeGenerator->GetSequenceType($type); Why aren't you using innerType naming here also? it sounds better than subtype to me. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4685 > + if ($codeGenerator->IsFrozenArrayType($type)) { It's be nice to avoid the duplication with the if block above somehow. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4770 > + if ($codeGenerator->IsFrozenArrayType($type)) { ditto, it'd be nice to share more code with the if block above. > LayoutTests/fast/dom/Window/Location/ancestor-origins.html:9 > + would be nice to have a description(); for this test.
Looks also like you: - Broke the GTK / EFL build - Need to rebaseline the bindings test - Broke a few IDB tests: Regressions: Unexpected text-only failures (3) imported/w3c/web-platform-tests/html/dom/interfaces.html [ Failure ] storage/indexeddb/intversion-close-between-events-private.html [ Failure ] storage/indexeddb/intversion-close-between-events.html [ Failure ] Regressions: Unexpected timeouts (2) storage/indexeddb/transaction-basics-private.html [ Timeout ] storage/indexeddb/transaction-basics.html [ Timeout ] Could be related to the review comment I made.
Comment on attachment 286495 [details] Patch Attachment 286495 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1901382 New failing tests: storage/indexeddb/intversion-close-between-events-private.html storage/indexeddb/transaction-basics-private.html storage/indexeddb/intversion-close-between-events.html storage/indexeddb/transaction-basics.html
Created attachment 286500 [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
Comment on attachment 286495 [details] Patch Attachment 286495 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1901404 New failing tests: storage/indexeddb/intversion-close-between-events-private.html storage/indexeddb/transaction-basics-private.html storage/indexeddb/intversion-close-between-events.html storage/indexeddb/transaction-basics.html
Created attachment 286502 [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
Comment on attachment 286495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286495&action=review >> Source/WebCore/Modules/indexeddb/IDBDatabase.idl:40 >> + [RaisesExceptionWithMessage] IDBTransaction transaction(sequence<DOMString> storeNames, optional DOMString mode); > > mode = "readonly" I think you need to add one more overload which takes DOMStringList because objectStoreNames returns DOMStringList which can be used as arguments for this function. An alternative fix is to make DOMStringList iterable.
Comment on attachment 286495 [details] Patch Attachment 286495 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1901442 New failing tests: storage/indexeddb/intversion-close-between-events-private.html storage/indexeddb/transaction-error-private.html storage/indexeddb/transaction-basics-private.html storage/indexeddb/intversion-close-between-events.html storage/indexeddb/transaction-basics.html storage/indexeddb/transaction-error.html
Created attachment 286504 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Given the pain with IndexedDB and given that the changes to IndexedDB don't seem strictly related to Location.ancestorOrigins and FrozenArray, I would recommend splitting that part into a separate patch.
Created attachment 286512 [details] Patch
Comment on attachment 286512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286512&action=review Looks good if the bots are happy. > LayoutTests/fast/dom/Window/Location/ancestor-origins.html:7 > + still no description() :)
Comment on attachment 286512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286512&action=review > Source/WebCore/Modules/indexeddb/IDBDatabase.idl:42 > + // FIXME: This is not part of the spec, but is needed for compatibility. > + [RaisesExceptionWithMessage] IDBTransaction transaction(DOMStringList storeNames, optional DOMString mode = "readonly"); Filed https://github.com/w3c/IndexedDB/issues/85. You might wanna add it in the comment or mention it in the change log.
Undefined subroutine &CodeGeneratorJS::isSequenceOrFrozenArrayParameter called at WebCore/bindings/scripts/CodeGeneratorJS.pm line 1798.
(In reply to comment #13) > Comment on attachment 286512 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286512&action=review > > Looks good if the bots are happy. > > > LayoutTests/fast/dom/Window/Location/ancestor-origins.html:7 > > + > > still no description() :) Was just giving it to the bots while I drove home :).
Created attachment 286522 [details] Patch
Created attachment 286524 [details] Patch
I think you need to remove JSDOMStringListCustom.cpp from JSBindingsAllInOne.cpp to fix Windows build.
Landed in https://trac.webkit.org/changeset/204679.
Comment on attachment 286524 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286524&action=review > Source/WebCore/bindings/js/JSDOMBinding.h:634 > +template<typename T, size_t inlineCapacity> JSC::JSValue jsFrozenArray(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, const Vector<T, inlineCapacity>& vector) Unfortunately, this implementation of FrozenArray is incomplete. One of the characteristics of a FrozenArray is that it is passed by reference and not by values (unlike sequences). Therefore, converting back and forth to a Vector is not the right thing to do. For e.g. location.ancestorOrigins === location.ancestorOrigins should be true but is not currently.
(In reply to comment #21) > Comment on attachment 286524 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286524&action=review > > > Source/WebCore/bindings/js/JSDOMBinding.h:634 > > +template<typename T, size_t inlineCapacity> JSC::JSValue jsFrozenArray(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, const Vector<T, inlineCapacity>& vector) > > Unfortunately, this implementation of FrozenArray is incomplete. One of the > characteristics of a FrozenArray is that it is passed by reference and not > by values (unlike sequences). Therefore, converting back and forth to a > Vector is not the right thing to do. > > For e.g. location.ancestorOrigins === location.ancestorOrigins should be > true but is not currently. location.ancestorOrigins === location.ancestorOrigins should be true due to the CachedAttribute extended attribute, but if others adopt FrozenArray it won't work as a reference.