Bug 161018 - Location.ancestorOrigins should return a FrozenArray<USVString>
Summary: Location.ancestorOrigins should return a FrozenArray<USVString>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-19 15:34 PDT by Sam Weinig
Modified: 2016-10-11 15:20 PDT (History)
15 users (show)

See Also:


Attachments
Patch (34.22 KB, patch)
2016-08-19 16:19 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (948.40 KB, application/zip)
2016-08-19 17:14 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.00 MB, application/zip)
2016-08-19 17:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.56 MB, application/zip)
2016-08-19 17:30 PDT, Build Bot
no flags Details
Patch (37.48 KB, patch)
2016-08-19 18:42 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (37.89 KB, patch)
2016-08-19 20:51 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (43.24 KB, patch)
2016-08-19 21:21 PDT, Sam Weinig
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2016-08-19 15:34:07 PDT
Location.ancestorOrigins should return a FrozenArray<USVString>
Comment 1 Sam Weinig 2016-08-19 16:19:02 PDT
Created attachment 286495 [details]
Patch
Comment 2 Chris Dumez 2016-08-19 16:56:59 PDT
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.
Comment 3 Chris Dumez 2016-08-19 16:58:16 PDT
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 4 Build Bot 2016-08-19 17:14:19 PDT
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
Comment 5 Build Bot 2016-08-19 17:14:24 PDT
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 6 Build Bot 2016-08-19 17:17:54 PDT
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
Comment 7 Build Bot 2016-08-19 17:17:59 PDT
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 8 Ryosuke Niwa 2016-08-19 17:25:33 PDT
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 9 Build Bot 2016-08-19 17:30:40 PDT
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
Comment 10 Build Bot 2016-08-19 17:30:45 PDT
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
Comment 11 Chris Dumez 2016-08-19 18:32:06 PDT
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.
Comment 12 Sam Weinig 2016-08-19 18:42:18 PDT
Created attachment 286512 [details]
Patch
Comment 13 Chris Dumez 2016-08-19 18:49:50 PDT
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 14 Ryosuke Niwa 2016-08-19 18:50:23 PDT
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.
Comment 15 Chris Dumez 2016-08-19 18:50:38 PDT
Undefined subroutine &CodeGeneratorJS::isSequenceOrFrozenArrayParameter called at WebCore/bindings/scripts/CodeGeneratorJS.pm line 1798.
Comment 16 Sam Weinig 2016-08-19 19:55:37 PDT
(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 :).
Comment 17 Sam Weinig 2016-08-19 20:51:27 PDT
Created attachment 286522 [details]
Patch
Comment 18 Sam Weinig 2016-08-19 21:21:34 PDT
Created attachment 286524 [details]
Patch
Comment 19 Ryosuke Niwa 2016-08-19 23:58:54 PDT
I think you need to remove JSDOMStringListCustom.cpp from JSBindingsAllInOne.cpp to fix Windows build.
Comment 20 Sam Weinig 2016-08-20 07:47:14 PDT
Landed in https://trac.webkit.org/changeset/204679.
Comment 21 Chris Dumez 2016-10-10 15:23:45 PDT
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.
Comment 22 Sam Weinig 2016-10-11 15:20:50 PDT
(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.