Experiment with typemaps in Blink Modules

Experiment with typemaps in Blink Modules

BUG=

[Peter Beverloo, 2017-03-23 19:47:45]

peter@chromium.org changed reviewers:
+ dcheng@chromium.org

[Peter Beverloo, 2017-03-23 19:47:46]

https://codereview.chromium.org/2772893002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/background_fetch/BackgroundFetch.typemap
(right):

https://codereview.chromium.org/2772893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetch.typemap:6:
public_headers = [ "modules/background_fetch/IconDefinition.h" ]
This is a generated file, but it seems to be OK with this. I can't use a fully
qualified path, though.

https://codereview.chromium.org/2772893002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchTypeConverters.cpp
(right):

https://codereview.chromium.org/2772893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchTypeConverters.cpp:39:
WTF::Vector<blink::IconDefinition> mojoIcons;
This is where it fails, and it happens in the generated
background_fetch.mojom-blink.cc too. Traced objects, i.e. both dictionaries and
interfaces, can't be stored in regular WTF::Vectors<> due to the following
static_assert:

Vector.h:1273:3: error: static_assert failed "Cannot put
DISALLOW_NEW_EXCEPT_PLACEMENT_NEW objects that have trace methods into an
off-heap Vector"
  static_assert(Allocator::isGarbageCollected ||
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Peter Beverloo, 2017-03-23 19:50:19]

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

[commit-bot: I haz the power, 2017-03-23 19:50:51]

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, 2017-03-23 20:05:22]

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

[commit-bot: I haz the power, 2017-03-23 20:05:22]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[haraken, 2017-03-24 02:09:18]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-03-24 02:09:18]

https://codereview.chromium.org/2772893002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchTypeConverters.cpp
(right):

https://codereview.chromium.org/2772893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchTypeConverters.cpp:39:
WTF::Vector<blink::IconDefinition> mojoIcons;
On 2017/03/23 19:47:46, Peter Beverloo wrote:
> This is where it fails, and it happens in the generated
> background_fetch.mojom-blink.cc too. Traced objects, i.e. both dictionaries
and
> interfaces, can't be stored in regular WTF::Vectors<> due to the following
> static_assert:
> 
> Vector.h:1273:3: error: static_assert failed "Cannot put
> DISALLOW_NEW_EXCEPT_PLACEMENT_NEW objects that have trace methods into an
> off-heap Vector"
>   static_assert(Allocator::isGarbageCollected ||
>   ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Can we just use a HeapVector?

Also I think we should just fix the mojo-binding generator to generate a
HeapVector when the vector contents are garbage collected objects.

[dcheng, 2017-03-24 23:11:31]

On 2017/03/24 02:09:18, haraken wrote:
>
https://codereview.chromium.org/2772893002/diff/20001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchTypeConverters.cpp
> (right):
> 
>
https://codereview.chromium.org/2772893002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchTypeConverters.cpp:39:
> WTF::Vector<blink::IconDefinition> mojoIcons;
> On 2017/03/23 19:47:46, Peter Beverloo wrote:
> > This is where it fails, and it happens in the generated
> > background_fetch.mojom-blink.cc too. Traced objects, i.e. both dictionaries
> and
> > interfaces, can't be stored in regular WTF::Vectors<> due to the following
> > static_assert:
> > 
> > Vector.h:1273:3: error: static_assert failed "Cannot put
> > DISALLOW_NEW_EXCEPT_PLACEMENT_NEW objects that have trace methods into an
> > off-heap Vector"
> >   static_assert(Allocator::isGarbageCollected ||
> >   ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Can we just use a HeapVector?
> 
> Also I think we should just fix the mojo-binding generator to generate a
> HeapVector when the vector contents are garbage collected objects.

I think it's a bit tricky for the binding generator to know when the objects are
GCed or not.

I'm curious--can we make Vector's implementation conditional? Why do we need a
separate HeapVector?

[Peter Beverloo, 2017-03-24 23:32:03]

On 2017/03/24 23:11:31, dcheng wrote:
> On 2017/03/24 02:09:18, haraken wrote:
> >
>
https://codereview.chromium.org/2772893002/diff/20001/third_party/WebKit/Sour...
> > File
> >
>
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchTypeConverters.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2772893002/diff/20001/third_party/WebKit/Sour...
> >
>
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchTypeConverters.cpp:39:
> > WTF::Vector<blink::IconDefinition> mojoIcons;
> > On 2017/03/23 19:47:46, Peter Beverloo wrote:
> > > This is where it fails, and it happens in the generated
> > > background_fetch.mojom-blink.cc too. Traced objects, i.e. both
dictionaries
> > and
> > > interfaces, can't be stored in regular WTF::Vectors<> due to the following
> > > static_assert:
> > > 
> > > Vector.h:1273:3: error: static_assert failed "Cannot put
> > > DISALLOW_NEW_EXCEPT_PLACEMENT_NEW objects that have trace methods into an
> > > off-heap Vector"
> > >   static_assert(Allocator::isGarbageCollected ||
> > >   ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Can we just use a HeapVector?
> > 
> > Also I think we should just fix the mojo-binding generator to generate a
> > HeapVector when the vector contents are garbage collected objects.
> 
> I think it's a bit tricky for the binding generator to know when the objects
are
> GCed or not.
> 
> I'm curious--can we make Vector's implementation conditional? Why do we need a
> separate HeapVector?

I've been toying today with using an annotation in the typemap file for this --

type_mappings =
    [
"blink.mojom.IconDefinition=::blink::IconDefinition[garbage_collected_type]" ]

Haven't gotten it to compile yet, but it seemed worth investigating. The idea is
that it triggers the code generator to use WTF::HeapVector<Member<T>> as opposed
to WTF::Vector<T>.

[haraken, 2017-03-27 04:55:19]

haraken@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[haraken, 2017-03-27 04:55:20]

+oilpan-reviews

>
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchTypeConverters.cpp:39:
> > WTF::Vector<blink::IconDefinition> mojoIcons;
> > On 2017/03/23 19:47:46, Peter Beverloo wrote:
> > > This is where it fails, and it happens in the generated
> > > background_fetch.mojom-blink.cc too. Traced objects, i.e. both
dictionaries
> > and
> > > interfaces, can't be stored in regular WTF::Vectors<> due to the following
> > > static_assert:
> > > 
> > > Vector.h:1273:3: error: static_assert failed "Cannot put
> > > DISALLOW_NEW_EXCEPT_PLACEMENT_NEW objects that have trace methods into an
> > > off-heap Vector"
> > >   static_assert(Allocator::isGarbageCollected ||
> > >   ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Can we just use a HeapVector?
> > 
> > Also I think we should just fix the mojo-binding generator to generate a
> > HeapVector when the vector contents are garbage collected objects.
> 
> I think it's a bit tricky for the binding generator to know when the objects
are
> GCed or not.
> 
> I'm curious--can we make Vector's implementation conditional? Why do we need a
> separate HeapVector?

This is an interesting idea. If it's doable, I'm fine with it.

  Vector<Member<OnHeapObject>>
  Vector<CompoundObjectThatHasTraceMethod>

should be automatically treated as an on-heap vector.