bindings: Use forward declarations for wrapper types in dictionary_impl

bindings: Use forward declarations for wrapper types in dictionary_impl

We used to include wrapper type headers in generated dictionary headers
but it could cause circular dependency if there is an idl dictionary
which refers an interface and the interface also refers the dictionary.

Avoid such circular dependency by using forward declarations.

BUG=662403
Committed: https://crrev.com/b90d742c7e342d29dcc8b64b3a9ed5fdb398f1d0
Cr-Commit-Position: refs/heads/master@{#431850}

[bashi, 2016-11-04 02:36:47]

The CQ bit was checked by bashi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 02:37:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 06:07:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 06:07:53]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Navid Zolghadr, 2016-11-04 14:25:11]

Description was changed from

==========
WIP: bindings: Use forward declarations for interface types in dictionary_impl

BUG=TBD
==========

to

==========
WIP: bindings: Use forward declarations for interface types in dictionary_impl

BUG=662403
==========

[bashi, 2016-11-11 00:47:50]

The CQ bit was checked by bashi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 00:48:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bashi, 2016-11-11 01:51:46]

Description was changed from

==========
WIP: bindings: Use forward declarations for interface types in dictionary_impl

BUG=662403
==========

to

==========
bindings: Use forward declarations for wrapper types in dictionary_impl

We used to include wrapper type headers in generated dictionary headers
but it could cause circular dependency if there is an idl dictionary
which refers an interface and the interface also refers the dictionary.

Avoid such circular dependency by using forward declarations.

BUG=662403
==========

[bashi, 2016-11-11 01:54:01]

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

[bashi, 2016-11-11 01:54:02]

PTAL

https://codereview.chromium.org/2471393004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/dictionary_impl.h.tmpl (left):

https://codereview.chromium.org/2471393004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/dictionary_impl.h.tmpl:21: bool
{{member.has_method_name}}() const { return {{member.has_method_expression}}; }
I had to move definitions into .cpp.tmpl as some types can be incomplete now.

https://codereview.chromium.org/2471393004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/tests/idls/core/TestDictionary.idl
(right):

https://codereview.chromium.org/2471393004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/tests/idls/core/TestDictionary.idl:20:
sequence<TestObject> testObjectSequenceMember;
This tests the change. TestObject refers TestDictionary.

[haraken, 2016-11-11 01:57:36]

LGTM

[commit-bot: I haz the power, 2016-11-11 02:28:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 02:28:07]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[bashi, 2016-11-11 02:38:48]

Does the following win build failure means we can't define HeapVector<Member<T>>
where T is incomplete (forward declaration) ?

e:\b\c\b\win\src\out\debug\gen\blink\modules\peerconnection\rtcconfiguration.h(59):
note: see reference to class template instantiation
'blink::Nullable<blink::HeapVector<blink::Member<blink::RTCCertificate>,0>>'
being compiled
e:\b\c\b\win\src\third_party\webkit\source\platform\heap\visitor.h(147): error
C2338: T must be fully defined
e:\b\c\b\win\src\third_party\webkit\source\wtf\typetraits.h(340): error C2027:
use of undefined type 'blink::RTCCertificate'
e:\b\c\b\win\src\out\debug\gen\blink\modules\peerconnection\rtcconfiguration.h(22):
note: see declaration of
'blink::RTCCertificate'e:\b\c\b\win\src\third_party\webkit\source\platform\heap\visitor.h(148):
note: see reference to class template instantiation
'WTF::IsGarbageCollectedType<T>' being compiled
        with
        [
            T=blink::RTCCertificate
        ]
e:\b\c\b\win\src\third_party\webkit\source\wtf\typetraits.h(340): error C2338: T
must be fully defined

[peria, 2016-11-11 03:06:38]

https://codereview.chromium.org/2471393004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_dictionary.py (right):

https://codereview.chromium.org/2471393004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_dictionary.py:164:
header_forward_decls = set()
I think we rarely use forward declarations in .cpp file,
so I recommend to remove "header_" from this variable's name.
(optional)

https://codereview.chromium.org/2471393004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_dictionary.py:171:
'header_forward_decls': header_forward_decls,
ditto. for the key.

[haraken, 2016-11-11 03:35:01]

On 2016/11/11 02:38:48, bashi1 wrote:
> Does the following win build failure means we can't define
HeapVector<Member<T>>
> where T is incomplete (forward declaration) ?
> 
>
e:\b\c\b\win\src\out\debug\gen\blink\modules\peerconnection\rtcconfiguration.h(59):
> note: see reference to class template instantiation
> 'blink::Nullable<blink::HeapVector<blink::Member<blink::RTCCertificate>,0>>'
> being compiled
> e:\b\c\b\win\src\third_party\webkit\source\platform\heap\visitor.h(147): error
> C2338: T must be fully defined
> e:\b\c\b\win\src\third_party\webkit\source\wtf\typetraits.h(340): error C2027:
> use of undefined type 'blink::RTCCertificate'
>
e:\b\c\b\win\src\out\debug\gen\blink\modules\peerconnection\rtcconfiguration.h(22):
> note: see declaration of
>
'blink::RTCCertificate'e:\b\c\b\win\src\third_party\webkit\source\platform\heap\visitor.h(148):
> note: see reference to class template instantiation
> 'WTF::IsGarbageCollectedType<T>' being compiled
>         with
>         [
>             T=blink::RTCCertificate
>         ]
> e:\b\c\b\win\src\third_party\webkit\source\wtf\typetraits.h(340): error C2338:
T
> must be fully defined

You can define HeapVector<Member<T>> without fully defining T.

The problem is Nullable<HeapVector<Member<T>>> is using T in the header file...

[bashi, 2016-11-11 03:50:12]

On 2016/11/11 03:35:01, haraken wrote:
> On 2016/11/11 02:38:48, bashi1 wrote:
> > Does the following win build failure means we can't define
> HeapVector<Member<T>>
> > where T is incomplete (forward declaration) ?
> > 
> >
>
e:\b\c\b\win\src\out\debug\gen\blink\modules\peerconnection\rtcconfiguration.h(59):
> > note: see reference to class template instantiation
> > 'blink::Nullable<blink::HeapVector<blink::Member<blink::RTCCertificate>,0>>'
> > being compiled
> > e:\b\c\b\win\src\third_party\webkit\source\platform\heap\visitor.h(147):
error
> > C2338: T must be fully defined
> > e:\b\c\b\win\src\third_party\webkit\source\wtf\typetraits.h(340): error
C2027:
> > use of undefined type 'blink::RTCCertificate'
> >
>
e:\b\c\b\win\src\out\debug\gen\blink\modules\peerconnection\rtcconfiguration.h(22):
> > note: see declaration of
> >
>
'blink::RTCCertificate'e:\b\c\b\win\src\third_party\webkit\source\platform\heap\visitor.h(148):
> > note: see reference to class template instantiation
> > 'WTF::IsGarbageCollectedType<T>' being compiled
> >         with
> >         [
> >             T=blink::RTCCertificate
> >         ]
> > e:\b\c\b\win\src\third_party\webkit\source\wtf\typetraits.h(340): error
C2338:
> T
> > must be fully defined
> 
> You can define HeapVector<Member<T>> without fully defining T.
> 
> The problem is Nullable<HeapVector<Member<T>>> is using T in the header
file...

Ah, understood. This sounds we can't use forward declaration for nullable or we
should stop using Nullable for sequence. The latter will be a mount of work.

[bashi, 2016-11-14 00:09:24]

The CQ bit was checked by bashi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 00:09:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 01:26:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-14 01:26:55]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[bashi, 2016-11-14 02:48:28]

On 2016/11/11 03:50:12, bashi1 wrote:
> On 2016/11/11 03:35:01, haraken wrote:
> > On 2016/11/11 02:38:48, bashi1 wrote:
> > > Does the following win build failure means we can't define
> > HeapVector<Member<T>>
> > > where T is incomplete (forward declaration) ?
> > > 
> > >
> >
>
e:\b\c\b\win\src\out\debug\gen\blink\modules\peerconnection\rtcconfiguration.h(59):
> > > note: see reference to class template instantiation
> > >
'blink::Nullable<blink::HeapVector<blink::Member<blink::RTCCertificate>,0>>'
> > > being compiled
> > > e:\b\c\b\win\src\third_party\webkit\source\platform\heap\visitor.h(147):
> error
> > > C2338: T must be fully defined
> > > e:\b\c\b\win\src\third_party\webkit\source\wtf\typetraits.h(340): error
> C2027:
> > > use of undefined type 'blink::RTCCertificate'
> > >
> >
>
e:\b\c\b\win\src\out\debug\gen\blink\modules\peerconnection\rtcconfiguration.h(22):
> > > note: see declaration of
> > >
> >
>
'blink::RTCCertificate'e:\b\c\b\win\src\third_party\webkit\source\platform\heap\visitor.h(148):
> > > note: see reference to class template instantiation
> > > 'WTF::IsGarbageCollectedType<T>' being compiled
> > >         with
> > >         [
> > >             T=blink::RTCCertificate
> > >         ]
> > > e:\b\c\b\win\src\third_party\webkit\source\wtf\typetraits.h(340): error
> C2338:
> > T
> > > must be fully defined
> > 
> > You can define HeapVector<Member<T>> without fully defining T.
> > 
> > The problem is Nullable<HeapVector<Member<T>>> is using T in the header
> file...
> 
> Ah, understood. This sounds we can't use forward declaration for nullable or
we
> should stop using Nullable for sequence. The latter will be a mount of work.

Even we don't use HeapVector (only declare HeapVector<Member<T>>) in header
files, win compilers still require full definition. Looks like we can't use
forward declarations anyway?

e:\b\c\b\win\src\third_party\webkit\source\platform\heap\heapallocator.h(405):
note: see reference to class template instantiation
'WTF::Vector<T,0,blink::HeapAllocator>' being compiled
        with
        [
            T=blink::Member<blink::RTCCertificate>
        ]
e:\b\c\b\win\src\out\debug\gen\blink\modules\peerconnection\rtcconfiguration.h(59):
note: see reference to class template instantiation
'blink::HeapVector<blink::Member<blink::RTCCertificate>,0>' being compiled
e:\b\c\b\win\src\third_party\webkit\source\platform\heap\visitor.h(147): error
C2338: T must be fully defined
e:\b\c\b\win\src\third_party\webkit\source\wtf\typetraits.h(340): error C2027:
use of undefined type 'blink::RTCCertificate'
e:\b\c\b\win\src\out\debug\gen\blink\modules\peerconnection\rtcconfiguration.h(21):
note: see declaration of 'blink::RTCCertificate'
e:\b\c\b\win\src\third_party\webkit\source\platform\heap\visitor.h(148): note:
see reference to class template instantiation 'WTF::IsGarbageCollectedType<T>'
being compiled
        with
        [
            T=blink::RTCCertificate
        ]

[haraken, 2016-11-14 02:59:13]

On 2016/11/14 02:48:28, bashi1 wrote:
> On 2016/11/11 03:50:12, bashi1 wrote:
> > On 2016/11/11 03:35:01, haraken wrote:
> > > On 2016/11/11 02:38:48, bashi1 wrote:
> > > > Does the following win build failure means we can't define
> > > HeapVector<Member<T>>
> > > > where T is incomplete (forward declaration) ?
> > > > 
> > > >
> > >
> >
>
e:\b\c\b\win\src\out\debug\gen\blink\modules\peerconnection\rtcconfiguration.h(59):
> > > > note: see reference to class template instantiation
> > > >
> 'blink::Nullable<blink::HeapVector<blink::Member<blink::RTCCertificate>,0>>'
> > > > being compiled
> > > > e:\b\c\b\win\src\third_party\webkit\source\platform\heap\visitor.h(147):
> > error
> > > > C2338: T must be fully defined
> > > > e:\b\c\b\win\src\third_party\webkit\source\wtf\typetraits.h(340): error
> > C2027:
> > > > use of undefined type 'blink::RTCCertificate'
> > > >
> > >
> >
>
e:\b\c\b\win\src\out\debug\gen\blink\modules\peerconnection\rtcconfiguration.h(22):
> > > > note: see declaration of
> > > >
> > >
> >
>
'blink::RTCCertificate'e:\b\c\b\win\src\third_party\webkit\source\platform\heap\visitor.h(148):
> > > > note: see reference to class template instantiation
> > > > 'WTF::IsGarbageCollectedType<T>' being compiled
> > > >         with
> > > >         [
> > > >             T=blink::RTCCertificate
> > > >         ]
> > > > e:\b\c\b\win\src\third_party\webkit\source\wtf\typetraits.h(340): error
> > C2338:
> > > T
> > > > must be fully defined
> > > 
> > > You can define HeapVector<Member<T>> without fully defining T.
> > > 
> > > The problem is Nullable<HeapVector<Member<T>>> is using T in the header
> > file...
> > 
> > Ah, understood. This sounds we can't use forward declaration for nullable or
> we
> > should stop using Nullable for sequence. The latter will be a mount of work.
> 
> Even we don't use HeapVector (only declare HeapVector<Member<T>>) in header
> files, win compilers still require full definition. Looks like we can't use
> forward declarations anyway?

Hmm, I think it should work. Would you ask tasak@? tasak@ is familiar with link
issues on Win.

> 
> e:\b\c\b\win\src\third_party\webkit\source\platform\heap\heapallocator.h(405):
> note: see reference to class template instantiation
> 'WTF::Vector<T,0,blink::HeapAllocator>' being compiled
>         with
>         [
>             T=blink::Member<blink::RTCCertificate>
>         ]
>
e:\b\c\b\win\src\out\debug\gen\blink\modules\peerconnection\rtcconfiguration.h(59):
> note: see reference to class template instantiation
> 'blink::HeapVector<blink::Member<blink::RTCCertificate>,0>' being compiled
> e:\b\c\b\win\src\third_party\webkit\source\platform\heap\visitor.h(147): error
> C2338: T must be fully defined
> e:\b\c\b\win\src\third_party\webkit\source\wtf\typetraits.h(340): error C2027:
> use of undefined type 'blink::RTCCertificate'
>
e:\b\c\b\win\src\out\debug\gen\blink\modules\peerconnection\rtcconfiguration.h(21):
> note: see declaration of 'blink::RTCCertificate'
> e:\b\c\b\win\src\third_party\webkit\source\platform\heap\visitor.h(148): note:
> see reference to class template instantiation 'WTF::IsGarbageCollectedType<T>'
> being compiled
>         with
>         [
>             T=blink::RTCCertificate
>         ]

[bashi, 2016-11-14 06:59:59]

bashi@chromium.org changed reviewers:
+ tasak@google.com

[bashi, 2016-11-14 06:59:59]

+tasak@

Chatted offline and we thought adding an explicit copy constructor solves the
problem, but it seems not.

[bashi, 2016-11-14 08:33:32]

It seems adding operator=() as well solved the problem.

[bashi, 2016-11-14 08:33:38]

The CQ bit was checked by bashi@chromium.org

[bashi, 2016-11-14 08:33:38]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2471393004/#ps80001
(title: "Added copy operator")

[commit-bot: I haz the power, 2016-11-14 08:33:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2016-11-14 09:26:14]

lgtm

https://codereview.chromium.org/2471393004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/dictionary_impl.cpp.tmpl
(right):

https://codereview.chromium.org/2471393004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/dictionary_impl.cpp.tmpl:21:
{{cpp_class}}::{{cpp_class}}(const {{cpp_class}}&) = default;
I don't know well about the windows-specific issue, but if you can write "=
default" in the header, it's definitely better.

[commit-bot: I haz the power, 2016-11-14 10:31:35]

Description was changed from

==========
bindings: Use forward declarations for wrapper types in dictionary_impl

We used to include wrapper type headers in generated dictionary headers
but it could cause circular dependency if there is an idl dictionary
which refers an interface and the interface also refers the dictionary.

Avoid such circular dependency by using forward declarations.

BUG=662403
==========

to

==========
bindings: Use forward declarations for wrapper types in dictionary_impl

We used to include wrapper type headers in generated dictionary headers
but it could cause circular dependency if there is an idl dictionary
which refers an interface and the interface also refers the dictionary.

Avoid such circular dependency by using forward declarations.

BUG=662403
==========

[commit-bot: I haz the power, 2016-11-14 10:31:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-14 10:36:24]

Description was changed from

==========
bindings: Use forward declarations for wrapper types in dictionary_impl

We used to include wrapper type headers in generated dictionary headers
but it could cause circular dependency if there is an idl dictionary
which refers an interface and the interface also refers the dictionary.

Avoid such circular dependency by using forward declarations.

BUG=662403
==========

to

==========
bindings: Use forward declarations for wrapper types in dictionary_impl

We used to include wrapper type headers in generated dictionary headers
but it could cause circular dependency if there is an idl dictionary
which refers an interface and the interface also refers the dictionary.

Avoid such circular dependency by using forward declarations.

BUG=662403

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

[commit-bot: I haz the power, 2016-11-14 10:36:25]

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

[bashi, 2016-11-14 22:08:49]

https://codereview.chromium.org/2471393004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/dictionary_impl.cpp.tmpl
(right):

https://codereview.chromium.org/2471393004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/dictionary_impl.cpp.tmpl:21:
{{cpp_class}}::{{cpp_class}}(const {{cpp_class}}&) = default;
On 2016/11/14 09:26:14, Yuki wrote:
> I don't know well about the windows-specific issue, but if you can write "=
> default" in the header, it's definitely better.

I had to write them in .cpp to avoid instanciation of HeapVector<Member<T>>.

[tasak, 2016-11-16 17:10:59]

On 2016/11/14 22:08:49, bashi1 wrote:
>
https://codereview.chromium.org/2471393004/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/templates/dictionary_impl.cpp.tmpl
> (right):
> 
>
https://codereview.chromium.org/2471393004/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/dictionary_impl.cpp.tmpl:21:
> {{cpp_class}}::{{cpp_class}}(const {{cpp_class}}&) = default;
> On 2016/11/14 09:26:14, Yuki wrote:
> > I don't know well about the windows-specific issue, but if you can write "=
> > default" in the header, it's definitely better.
> 
> I had to write them in .cpp to avoid instanciation of HeapVector<Member<T>>.

As far as I know, if we write "= default" in the header, we still need to create
.cpp which just includes the header because of DLL.