Update union conversion algorithm

Update union conversion algorithm

The algorithm[1] is updated to fix a bug on distinguishing
dictionary and sequence<T>[2]. Update our implementation.

[1] https://heycam.github.io/webidl/#es-union
[2] https://github.com/heycam/webidl/issues/123

Committed: https://crrev.com/b81bc587695645f362434a4599241db2efe8ad32
Cr-Commit-Position: refs/heads/master@{#395297}

[bashi, 2016-05-20 06:24:35]

Description was changed from

==========
Update union conversion algorithm

The algorithm[1] has updated to fix a bug on distinguishing
dictionary and sequence<T>[2]. Update our implementation.

[1] https://heycam.github.io/webidl/#es-union
[2] https://github.com/heycam/webidl/issues/123
==========

to

==========
Update union conversion algorithm

The algorithm[1] is updated to fix a bug on distinguishing
dictionary and sequence<T>[2]. Update our implementation.

[1] https://heycam.github.io/webidl/#es-union
[2] https://github.com/heycam/webidl/issues/123
==========

[bashi, 2016-05-20 06:44:15]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, yukishiino@chromium.org

[bashi, 2016-05-20 06:44:16]

PTAL?

https://codereview.chromium.org/2001493002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/dom/idl-union-type-unittest.html
(right):

https://codereview.chromium.org/2001493002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/idl-union-type-unittest.html:159: 
I'll remove blank lines.

[Yuki, 2016-05-20 06:53:48]

LGTM.

[haraken, 2016-05-20 07:25:06]

LGTM

[bashi, 2016-05-23 05:36:03]

Chatted with tasak@ on how to fix link errors on Win. tasak@ found that we
shouldn't set CORE_EXPORT for testing-only union containers. This is very
difficult with the current design because at the point we generate union
containers there is no information about where union types come from ('core' or
'testing'). I couldn't find workaround so I removed tests.

[Yuki, 2016-05-23 05:43:27]

On 2016/05/23 05:36:03, bashi1 wrote:
> Chatted with tasak@ on how to fix link errors on Win. tasak@ found that we
> shouldn't set CORE_EXPORT for testing-only union containers. This is very
> difficult with the current design because at the point we generate union
> containers there is no information about where union types come from ('core'
or
> 'testing'). I couldn't find workaround so I removed tests.

I'm okay to commit this CL.  However, we should be able to determine if it's
core or testing as same as if it's core or modules, shouldn't we?  For example,
we can tell it from the path to the IDL file.

[bashi, 2016-05-23 06:14:11]

On 2016/05/23 05:43:27, Yuki wrote:
> On 2016/05/23 05:36:03, bashi1 wrote:
> > Chatted with tasak@ on how to fix link errors on Win. tasak@ found that we
> > shouldn't set CORE_EXPORT for testing-only union containers. This is very
> > difficult with the current design because at the point we generate union
> > containers there is no information about where union types come from ('core'
> or
> > 'testing'). I couldn't find workaround so I removed tests.
> 
> I'm okay to commit this CL.  However, we should be able to determine if it's
> core or testing as same as if it's core or modules, shouldn't we?  For
example,
> we can tell it from the path to the IDL file.

Unfortunately it's difficult with the current implementation. Please refer
compute_interface_info_{individual,overall}.py to see how we collect union
types. we collect them as IdlUnionType then store them in a pickle. When
invoking idl_compiler.py we only use the pickle. There is no information where
union types come from. What we know at that point is component ('core' or
'modules'), which isn't enough to distinguish 'core' and 'testing'. One
workaround would be adding a property to IDlUnionType which contains paths of
the IDL files like:

idl_union.files_which_use_this_union_type = ['core/dom/A.idl',
'core/testing/B.idl', 'modules/foo/C.idl', ...]

I don't think we want to have this kind of hack even temporary. Probably more
appropriate fix would be:
1. add a map (IDLUnionType -> paths) to |interfaces_info| in
compute_interface_info_individual.py, then
2. aggregate them and detect which is the best component to place generate union
containers in compute_interface_info_overall.py, then
3. propagate them to code_generator_v8.py.

This is a substantial work so I didn't do that (and I don't think it's worth
doing. I'd prefer to do more fundamental refactoring).

[Yuki, 2016-05-23 06:33:20]

On 2016/05/23 06:14:11, bashi1 wrote:
> On 2016/05/23 05:43:27, Yuki wrote:
> > On 2016/05/23 05:36:03, bashi1 wrote:
> > > Chatted with tasak@ on how to fix link errors on Win. tasak@ found that we
> > > shouldn't set CORE_EXPORT for testing-only union containers. This is very
> > > difficult with the current design because at the point we generate union
> > > containers there is no information about where union types come from
('core'
> > or
> > > 'testing'). I couldn't find workaround so I removed tests.
> > 
> > I'm okay to commit this CL.  However, we should be able to determine if it's
> > core or testing as same as if it's core or modules, shouldn't we?  For
> example,
> > we can tell it from the path to the IDL file.
> 
> Unfortunately it's difficult with the current implementation. Please refer
> compute_interface_info_{individual,overall}.py to see how we collect union
> types. we collect them as IdlUnionType then store them in a pickle. When
> invoking idl_compiler.py we only use the pickle. There is no information where
> union types come from. What we know at that point is component ('core' or
> 'modules'), which isn't enough to distinguish 'core' and 'testing'. One
> workaround would be adding a property to IDlUnionType which contains paths of
> the IDL files like:
> 
> idl_union.files_which_use_this_union_type = ['core/dom/A.idl',
> 'core/testing/B.idl', 'modules/foo/C.idl', ...]
> 
> I don't think we want to have this kind of hack even temporary. Probably more
> appropriate fix would be:
> 1. add a map (IDLUnionType -> paths) to |interfaces_info| in
> compute_interface_info_individual.py, then
> 2. aggregate them and detect which is the best component to place generate
union
> containers in compute_interface_info_overall.py, then
> 3. propagate them to code_generator_v8.py.
> 
> This is a substantial work so I didn't do that (and I don't think it's worth
> doing. I'd prefer to do more fundamental refactoring).

Another good reason to refactor the IDL compiler. ;)

[bashi, 2016-05-23 08:57:30]

The CQ bit was checked by bashi@chromium.org

[bashi, 2016-05-23 08:57:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2001493002/#ps60001
(title: "Remove tests")

[commit-bot: I haz the power, 2016-05-23 08:57:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001493002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001493002/60001

[commit-bot: I haz the power, 2016-05-23 09:52:05]

Description was changed from

==========
Update union conversion algorithm

The algorithm[1] is updated to fix a bug on distinguishing
dictionary and sequence<T>[2]. Update our implementation.

[1] https://heycam.github.io/webidl/#es-union
[2] https://github.com/heycam/webidl/issues/123
==========

to

==========
Update union conversion algorithm

The algorithm[1] is updated to fix a bug on distinguishing
dictionary and sequence<T>[2]. Update our implementation.

[1] https://heycam.github.io/webidl/#es-union
[2] https://github.com/heycam/webidl/issues/123
==========

[commit-bot: I haz the power, 2016-05-23 09:52:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-23 09:53:18]

Description was changed from

==========
Update union conversion algorithm

The algorithm[1] is updated to fix a bug on distinguishing
dictionary and sequence<T>[2]. Update our implementation.

[1] https://heycam.github.io/webidl/#es-union
[2] https://github.com/heycam/webidl/issues/123
==========

to

==========
Update union conversion algorithm

The algorithm[1] is updated to fix a bug on distinguishing
dictionary and sequence<T>[2]. Update our implementation.

[1] https://heycam.github.io/webidl/#es-union
[2] https://github.com/heycam/webidl/issues/123

Committed: https://crrev.com/b81bc587695645f362434a4599241db2efe8ad32
Cr-Commit-Position: refs/heads/master@{#395297}
==========

[commit-bot: I haz the power, 2016-05-23 09:53:21]

Patchset 4 (id:??) landed as
https://crrev.com/b81bc587695645f362434a4599241db2efe8ad32
Cr-Commit-Position: refs/heads/master@{#395297}