New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 700225 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
OoO until Feb 4th
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 700370



Sign in to add a comment

IDL compiler should support nested union types

Project Member Reported by bashi@chromium.org, Mar 10 2017

Issue description

For example, following definition causes an error:
  attribute ((double or DOMString) or sequence<(double or DOMString)>) fooAttribute;

$ third_party/WebKit/Tools/Scripts/run-bindings-test
...
Exception: DoubleOrString is not supported as an union member.

We should support it. Actual use case is `BackgroundFetchManager::fetch()`.
 

Comment 1 by bashi@chromium.org, Mar 10 2017

I'm not sure I have bandwidth for this right now though...

Comment 2 by peter@chromium.org, Mar 10 2017

Blocking: 700370

Comment 3 by peter@chromium.org, Mar 10 2017

While it will block shipping the API, we're quite far away from doing that and I think we can skip on this in the origin trial. That gives us some time to prioritize accordingly :)

+jakearchibald FYI

Owner: raphael....@intel.com
Status: Started (was: Available)
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a328b46a04ef32a9afdbed84930042c6d5b0d87f

commit a328b46a04ef32a9afdbed84930042c6d5b0d87f
Author: raphael.kubo.da.costa <raphael.kubo.da.costa@intel.com>
Date: Mon Mar 13 08:41:35 2017

bindings: Make v8_union iterate through its members by type name

This is being done in preparation for supporting flattening union member
types and deal with nested union types.

A union's flattened member types are represented as a set, so once we start
using it to generate the union V8->C++ conversion code we will need to do so
in a deterministic way.

This specific change just makes the current code iterate through its member
types sorted by name to update existing test expectations and reduce the
size of the upcoming nested union support patch.

BUG= 700225 
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2742213002
Cr-Commit-Position: refs/heads/master@{#456338}

[modify] https://crrev.com/a328b46a04ef32a9afdbed84930042c6d5b0d87f/third_party/WebKit/Source/bindings/scripts/v8_union.py
[modify] https://crrev.com/a328b46a04ef32a9afdbed84930042c6d5b0d87f/third_party/WebKit/Source/bindings/tests/results/core/ByteStringSequenceSequenceOrByteStringByteStringRecord.cpp
[modify] https://crrev.com/a328b46a04ef32a9afdbed84930042c6d5b0d87f/third_party/WebKit/Source/bindings/tests/results/core/ByteStringSequenceSequenceOrByteStringByteStringRecord.h
[modify] https://crrev.com/a328b46a04ef32a9afdbed84930042c6d5b0d87f/third_party/WebKit/Source/bindings/tests/results/core/StringOrArrayBufferOrArrayBufferView.cpp
[modify] https://crrev.com/a328b46a04ef32a9afdbed84930042c6d5b0d87f/third_party/WebKit/Source/bindings/tests/results/core/StringOrArrayBufferOrArrayBufferView.h
[modify] https://crrev.com/a328b46a04ef32a9afdbed84930042c6d5b0d87f/third_party/WebKit/Source/bindings/tests/results/core/StringOrDouble.cpp
[modify] https://crrev.com/a328b46a04ef32a9afdbed84930042c6d5b0d87f/third_party/WebKit/Source/bindings/tests/results/core/StringOrDouble.h
[modify] https://crrev.com/a328b46a04ef32a9afdbed84930042c6d5b0d87f/third_party/WebKit/Source/bindings/tests/results/core/TestEnumOrDouble.cpp
[modify] https://crrev.com/a328b46a04ef32a9afdbed84930042c6d5b0d87f/third_party/WebKit/Source/bindings/tests/results/core/TestEnumOrDouble.h
[modify] https://crrev.com/a328b46a04ef32a9afdbed84930042c6d5b0d87f/third_party/WebKit/Source/bindings/tests/results/core/TestInterfaceGarbageCollectedOrString.cpp
[modify] https://crrev.com/a328b46a04ef32a9afdbed84930042c6d5b0d87f/third_party/WebKit/Source/bindings/tests/results/core/TestInterfaceGarbageCollectedOrString.h
[modify] https://crrev.com/a328b46a04ef32a9afdbed84930042c6d5b0d87f/third_party/WebKit/Source/bindings/tests/results/core/TestInterfaceOrLong.cpp
[modify] https://crrev.com/a328b46a04ef32a9afdbed84930042c6d5b0d87f/third_party/WebKit/Source/bindings/tests/results/core/TestInterfaceOrLong.h
[modify] https://crrev.com/a328b46a04ef32a9afdbed84930042c6d5b0d87f/third_party/WebKit/Source/bindings/tests/results/core/UnrestrictedDoubleOrString.cpp
[modify] https://crrev.com/a328b46a04ef32a9afdbed84930042c6d5b0d87f/third_party/WebKit/Source/bindings/tests/results/core/UnrestrictedDoubleOrString.h

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9

commit a15fea4b2a0062f98fa2c7b446dfa4035e7231c9
Author: raphael.kubo.da.costa <raphael.kubo.da.costa@intel.com>
Date: Mon Mar 13 13:31:13 2017

bindings: Add support for nested union types

From a code perspective, this is more like "add support for retrieving a
union's flattened member types set", which is exposed via the
IdlUnionType.flattened_member_types property.

The algorithm is pretty simple and described in the spec:
https://heycam.github.io/webidl/#idl-union

v8_union can then iterate through its flattened member types when emitting
the C++ code for converting unions to and from JavaScript objects.

BUG= 700225 
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2746713003
Cr-Commit-Position: refs/heads/master@{#456363}

[modify] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/scripts/idl_types.py
[modify] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/scripts/idl_types_test.py
[modify] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/scripts/v8_union.py
[modify] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl
[modify] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/idls/core/TestTypedefs.idl
[add] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/results/core/ByteStringOrNodeList.cpp
[add] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/results/core/ByteStringOrNodeList.h
[add] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/results/core/DoubleOrStringOrDoubleOrStringSequence.cpp
[add] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/results/core/DoubleOrStringOrDoubleOrStringSequence.h
[add] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/results/core/LongSequenceOrEvent.cpp
[add] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/results/core/LongSequenceOrEvent.h
[add] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/results/core/NodeOrLongSequenceOrEventOrXMLHttpRequestOrStringOrStringByteStringOrNodeListRecord.cpp
[add] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/results/core/NodeOrLongSequenceOrEventOrXMLHttpRequestOrStringOrStringByteStringOrNodeListRecord.h
[modify] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
[modify] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.h
[modify] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/results/core/V8TestTypedefs.cpp
[modify] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/results/core/V8TestTypedefs.h
[add] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/results/core/XMLHttpRequestOrString.cpp
[add] https://crrev.com/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9/third_party/WebKit/Source/bindings/tests/results/core/XMLHttpRequestOrString.h

Status: Fixed (was: Started)
This should be implemented now.

One thing to note is that nested unions can lead to insanely large names, which currently also mean insanely large file names, such as "NodeOrLongSequenceOrEventOrXMLHttpRequestOrStringOrStringByteStringOrNodeListRecord.cpp".

If you end up having a union name that's too long and causes issues on Windows due to file path limitations, consider adding it to //third_party/WebKit/Source/bindings/scripts/utilities.py's shorten_union_name().  Issue 700907  and  issue 611545  are related to this.

Comment 8 by peter@chromium.org, Mar 14 2017

Status: Assigned (was: Fixed)
I don't think this works as expected yet. Specifically, take a look at the checked in test:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/tests/results/core/NodeOrLongSequenceOrEventOrXMLHttpRequestOrStringOrStringByteStringOrNodeListRecord.h

It uses one of the composite types (ByteStringOrNodeList) without either including or declaring it, so the file can't compile.

I've uploaded two CLs to further illustrate the problem. The first might just be an IWYU issue, but in case of the second we're using the wrong type name, as it would be RequestOrUSVString.

  (1) Using nested, local types.

    ((Request or USVString) or sequence<(Request or USVString)>) requests
    ---> https://codereview.chromium.org/2743203005

RequestOrUSVStringOrRequestOrUSVStringSequence.h:38: error: use of undeclared identifier 'RequestOrUSVString'
  const HeapVector<RequestOrUSVString>& getAsRequestOrUSVStringSequence() const;


  (2) Using type definitions defined elsewhere. (As specified.)

    (RequestInfo or sequence<RequestInfo>) requests
    ---> https://codereview.chromium.org/2752543004/

RequestInfoOrRequestInfoSequence.h:31: error: unknown type name 'RequestInfo'
  RequestInfo* getAsRequestInfo() const;

Status: Started (was: Assigned)
Sorry about that; I think I remember seeing something similar in the past. I'm on it.
I don't think  issue 2 , "(RequestInfo or sequence<RequestInfo>) requests" is caused by my CL.

The problem's that RequestInfo is currently manually written and there's no IDL for it, so v8_union.py doesn't know it's supposed to be an interface and doesn't forward-declare the class. If you try any other type from modules/fetch, e.g. Headers, it works as expected. Actually porting RequestInfo to IDL has been in my TODO list for a while, but it's not something I can work on immediately.

I'm still checking issue 1.
Er, sorry, I mixed up RequestInfo and RequestInit, it's the latter that's not defined as an IDL.
Status: Fixed (was: Started)
None of the issues above is caused by the nested union type support code, it only made them more apparent.

I've filed  issue 701410  for (1) and  issue 701411  for (2).

Comment 13 by peter@chromium.org, Mar 14 2017

Thank you!!

Sign in to add a comment