Bug 161018 – Location.ancestorOrigins should return a FrozenArray<USVString>

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Location.ancestorOrigins should return a FrozenArray<USVString>

[Sam Weinig]

Created attachment 286495 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Ryosuke Niwa]

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.

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

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.

[Sam Weinig]

Created attachment 286512 [details]
Patch

[Chris Dumez]

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() :)

[Ryosuke Niwa]

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.

[Chris Dumez]

Undefined subroutine &CodeGeneratorJS::isSequenceOrFrozenArrayParameter called at WebCore/bindings/scripts/CodeGeneratorJS.pm line 1798.

[Sam Weinig]

(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 :).

[Sam Weinig]

Created attachment 286522 [details]
Patch

[Sam Weinig]

Created attachment 286524 [details]
Patch

[Ryosuke Niwa]

I think you need to remove JSDOMStringListCustom.cpp from JSBindingsAllInOne.cpp to fix Windows build.

[Sam Weinig]

Landed in https://trac.webkit.org/changeset/204679.

[Chris Dumez]

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.

[Sam Weinig]

(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.