Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(601)

Issue 2746713003: bindings: Add support for nested union types (Closed)

Created:
3 years, 9 months ago by Raphael Kubo da Costa (rakuco)
Modified:
3 years, 9 months ago
Reviewers:
haraken, bashi, Yuki
CC:
chromium-reviews, blink-reviews, blink-reviews-bindings_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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} Committed: https://chromium.googlesource.com/chromium/src/+/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9

Patch Set 1 #

Total comments: 4

Patch Set 2 : Improve documentation #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1372 lines, -2 lines) Patch
M third_party/WebKit/Source/bindings/scripts/idl_types.py View 1 1 chunk +27 lines, -1 line 0 comments Download
M third_party/WebKit/Source/bindings/scripts/idl_types_test.py View 2 chunks +60 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/scripts/v8_union.py View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/tests/idls/core/TestTypedefs.idl View 2 chunks +3 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/bindings/tests/results/core/ByteStringOrNodeList.h View 1 chunk +96 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/bindings/tests/results/core/ByteStringOrNodeList.cpp View 1 chunk +111 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/bindings/tests/results/core/DoubleOrStringOrDoubleOrStringSequence.h View 1 chunk +101 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/bindings/tests/results/core/DoubleOrStringOrDoubleOrStringSequence.cpp View 1 chunk +135 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/bindings/tests/results/core/LongSequenceOrEvent.h View 1 chunk +96 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/bindings/tests/results/core/LongSequenceOrEvent.cpp View 1 chunk +109 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/bindings/tests/results/core/NodeOrLongSequenceOrEventOrXMLHttpRequestOrStringOrStringByteStringOrNodeListRecord.h View 1 chunk +133 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/bindings/tests/results/core/NodeOrLongSequenceOrEventOrXMLHttpRequestOrStringOrStringByteStringOrNodeListRecord.cpp View 1 chunk +217 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.h View 2 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp View 4 chunks +38 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/tests/results/core/V8TestTypedefs.h View 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/tests/results/core/V8TestTypedefs.cpp View 4 chunks +35 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/bindings/tests/results/core/XMLHttpRequestOrString.h View 1 chunk +96 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/bindings/tests/results/core/XMLHttpRequestOrString.cpp View 1 chunk +108 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (8 generated)
Raphael Kubo da Costa (rakuco)
PTAL.
3 years, 9 months ago (2017-03-12 14:56:23 UTC) #1
haraken
LGTM but I want to have bashi@ take a look. https://codereview.chromium.org/2746713003/diff/1/third_party/WebKit/Source/bindings/scripts/idl_types.py File third_party/WebKit/Source/bindings/scripts/idl_types.py (right): https://codereview.chromium.org/2746713003/diff/1/third_party/WebKit/Source/bindings/scripts/idl_types.py#newcode307 ...
3 years, 9 months ago (2017-03-12 22:14:17 UTC) #6
bashi
Thanks for fixing the issue. I was thinking the same solution and this seems a ...
3 years, 9 months ago (2017-03-13 00:17:14 UTC) #7
Yuki
LGTM in general, and defer a detailed review to bashi@.
3 years, 9 months ago (2017-03-13 08:40:34 UTC) #8
Raphael Kubo da Costa (rakuco)
Patch v2 is up. I've fixed the link to the member flattening algorithm in the ...
3 years, 9 months ago (2017-03-13 11:38:24 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2746713003/20001
3 years, 9 months ago (2017-03-13 11:38:36 UTC) #12
Yuki
On 2017/03/13 11:38:24, Raphael Kubo da Costa (rakuco) wrote: > Patch v2 is up. I've ...
3 years, 9 months ago (2017-03-13 12:23:31 UTC) #13
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/a15fea4b2a0062f98fa2c7b446dfa4035e7231c9
3 years, 9 months ago (2017-03-13 13:31:59 UTC) #16
bashi
3 years, 9 months ago (2017-03-13 23:19:05 UTC) #17
Message was sent while issue was closed.
On 2017/03/13 11:38:24, Raphael Kubo da Costa (rakuco) wrote:
> Patch v2 is up. I've fixed the link to the member flattening algorithm in the
> spec, and rephrased the IdlType('ByteString') bit.
> 
> As for the rest:
> - I noticed v8_types had some union-specific code, I'll see if I can move it
to
> idl_types in another CL.
> - I don't have access to crbug.com/611437 :( I remember having problems with
> paths that were too long on Windows in the past myself, so I'm guessing the
bug
> is about that. It's not clear to me if shorten_union_name() itself should
> validate the union name or if it should be done elsewhere. I'd be happy to do
> that in another CL too.

I think we need to make sure that we have aliases for long union names as soon
as possible. Otherwise developers may submit CLs which break win builds by
accident.

Powered by Google App Engine
This is Rietveld 408576698