Oilpan: move Touch related objects to the oilpan heap.

Oilpan: move Touch related objects to the oilpan heap.

In order to do so, make the IDL compiler able to emit code that uses
heap vectors if the types involved are of the garbage collected kind.
(For the most part done by adopting the changes from
 https://codereview.chromium.org/173363002 )

R=
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168607

[sof, 2014-03-05 07:33:33]

Please take a look.

[Nils Barth (inactive), 2014-03-05 07:49:12]

A few style and test quibbles, but otherwise the CG changes
are very nice - thanks!

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_callback_interface.py (right):

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_callback_interface.py:76: if
usual_cpp_type.startswith(('Vector', 'WillBeHeapVector')):
Could you add a test case to TestCallbackInterface.idl?
Thanks!

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:158: 'vector_type': 'WillBeHeapVector' if
use_heap_vector_type else 'Vector',
+haraken@: thoughts on naming?
Technically Vector/WillBeHeapVector are templates, not types themselves.
Tersest would simply be 'vector' (as for 'ref_ptr', 'pass_ref_ptr'),
as this is basically VectorExceptWeNeedTransitionTypes,
and this avoids the transition types making the CG ugly.
Most verbose would be 'vector_template'.

Thoughts?

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:481: ref_ptr_type = None
Could you move this assignment into the else: block?

As a rule I've been using single assignment if possible,
rather than define-then-override.
(Look at how expression_format is defined here.)

define-then-override (for an if block) is required with block scope
(C, Perl, Go), but Python has function scope,
so we don't need to declare the variable outside the block,
and single assignment helps with reading code, since less is happening.

https://codereview.chromium.org/185413023/diff/20001/Source/core/dom/Touch.idl
File Source/core/dom/Touch.idl (right):

https://codereview.chromium.org/185413023/diff/20001/Source/core/dom/Touch.id...
Source/core/dom/Touch.idl:27: WillBeGarbageCollected
Could you add a trailing comma?
(We've decided to have them, as it makes editing easier.)

https://codereview.chromium.org/185413023/diff/20001/Source/core/dom/TouchLis...
File Source/core/dom/TouchList.idl (right):

https://codereview.chromium.org/185413023/diff/20001/Source/core/dom/TouchLis...
Source/core/dom/TouchList.idl:27: WillBeGarbageCollected
Ditto.

[sof, 2014-03-05 08:15:17]

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_callback_interface.py (right):

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_callback_interface.py:76: if
usual_cpp_type.startswith(('Vector', 'WillBeHeapVector')):
On 2014/03/05 07:49:13, Nils Barth wrote:
> Could you add a test case to TestCallbackInterface.idl?
> Thanks!

Done; I actually think 'HeapVector' should also be in this tuple, but I didn't
see that raised as an issue in the other CL.

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:158: 'vector_type': 'WillBeHeapVector' if
use_heap_vector_type else 'Vector',
On 2014/03/05 07:49:13, Nils Barth wrote:
> +haraken@: thoughts on naming?
> Technically Vector/WillBeHeapVector are templates, not types themselves.
> Tersest would simply be 'vector' (as for 'ref_ptr', 'pass_ref_ptr'),
> as this is basically VectorExceptWeNeedTransitionTypes,
> and this avoids the transition types making the CG ugly.
> Most verbose would be 'vector_template'.
> 
> Thoughts?

I think vector_type is fine (possibly with a cpp_ prefix), but
(cpp_)vector_type_name is an alternative.

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:481: ref_ptr_type = None
On 2014/03/05 07:49:13, Nils Barth wrote:
> Could you move this assignment into the else: block?
> 
> As a rule I've been using single assignment if possible,
> rather than define-then-override.
> (Look at how expression_format is defined here.)
> 
> define-then-override (for an if block) is required with block scope
> (C, Perl, Go), but Python has function scope,
> so we don't need to declare the variable outside the block,
> and single assignment helps with reading code, since less is happening.

Done; it's just a style i prefer when used in moderation.

[zerny-chromium, 2014-03-05 09:12:56]

Oilpan changes lgtm

[Nils Barth (inactive), 2014-03-05 09:20:10]

Thanks for revisions; CG & IDL changes LGTM.

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_callback_interface.py (right):

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_callback_interface.py:76: if
usual_cpp_type.startswith(('Vector', 'WillBeHeapVector')):
On 2014/03/05 08:15:17, sof wrote:
> On 2014/03/05 07:49:13, Nils Barth wrote:
> > Could you add a test case to TestCallbackInterface.idl?
> > Thanks!
> 
> Done; I actually think 'HeapVector' should also be in this tuple, but I didn't
> see that raised as an issue in the other CL.

We can cross that bridge when we come to it, I assume ;)

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:158: 'vector_type': 'WillBeHeapVector' if
use_heap_vector_type else 'Vector',
On 2014/03/05 08:15:17, sof wrote:
> I think vector_type is fine (possibly with a cpp_ prefix), but
> (cpp_)vector_type_name is an alternative.

Barring objections from haraken, this seems fine.

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/185413023/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:481: ref_ptr_type = None
On 2014/03/05 08:15:17, sof wrote:
> Done; it's just a style i prefer when used in moderation.

Understood; hopefully logic in CG isn't too complex (^.^),
and consistency helps...

[haraken, 2014-03-05 14:52:46]

LGTM with comments.

https://codereview.chromium.org/185413023/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/185413023/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:258: (which maps to some vector
representation in C++.)

Instead of |used_as_member|, shall we use |will_be_garbage_collected| ?

https://codereview.chromium.org/185413023/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:485: expression_format =
'(to{ref_ptr_type}PtrNativeArray<{array_or_sequence_type},
V8{array_or_sequence_type}>({v8_value}, {index}, info.GetIsolate()))'

"MemberPtr" sounds strange. Shall we call it toMemberNativeArray ?

https://codereview.chromium.org/185413023/diff/40001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/185413023/diff/40001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.h:502: HeapVector<Member<T> > result;

Would you add:

  if (success)
    *success = true;

?

https://codereview.chromium.org/185413023/diff/40001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/185413023/diff/40001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3590: typedef WillBeHeapHashMap<EventTarget*,
RefPtrWillBeMember<TouchList> > TargetTouchesMap;

TargetTouchesMap => TargetTouchesHeapMap (to make the underlying data structure
clearer)

https://codereview.chromium.org/185413023/diff/40001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3595: struct {

Shall we add STACK_ALLOCATED ?

[sof, 2014-03-05 16:01:11]

https://codereview.chromium.org/185413023/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/185413023/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:258: (which maps to some vector
representation in C++.)
On 2014/03/05 14:52:46, haraken wrote:
> 
> Instead of |used_as_member|, shall we use |will_be_garbage_collected| ?

I renamed it as will_be_in_heap_object; its the presence within some
heap-allocated object that is controlled/sought here.

https://codereview.chromium.org/185413023/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:485: expression_format =
'(to{ref_ptr_type}PtrNativeArray<{array_or_sequence_type},
V8{array_or_sequence_type}>({v8_value}, {index}, info.GetIsolate()))'
On 2014/03/05 14:52:46, haraken wrote:
> 
> "MemberPtr" sounds strange. Shall we call it toMemberNativeArray ?

Great catch; my typo (the underlying method is called toMemberNativeArray()
already.) Updated the binding results & double checked output.

https://codereview.chromium.org/185413023/diff/40001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/185413023/diff/40001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.h:502: HeapVector<Member<T> > result;
On 2014/03/05 14:52:46, haraken wrote:
> 
> Would you add:
> 
>   if (success)
>     *success = true;
> 
> ?

Indirectly done by inlining the Unchecked() version into toMemberNativeArray()
(its only caller.)

https://codereview.chromium.org/185413023/diff/40001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/185413023/diff/40001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3590: typedef WillBeHeapHashMap<EventTarget*,
RefPtrWillBeMember<TouchList> > TargetTouchesMap;
On 2014/03/05 14:52:46, haraken wrote:
> 
> TargetTouchesMap => TargetTouchesHeapMap (to make the underlying data
structure
> clearer)

Done.

https://codereview.chromium.org/185413023/diff/40001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:3595: struct {
On 2014/03/05 14:52:46, haraken wrote:
> 
> Shall we add STACK_ALLOCATED ?

Done.

[sof, 2014-03-05 16:01:19]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-03-05 16:02:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/185413023/60001

[commit-bot: I haz the power, 2014-03-05 16:35:21]

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

[commit-bot: I haz the power, 2014-03-05 16:35:21]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[sof, 2014-03-05 16:54:57]

Source/web/tests/ adjustment needs Source/web/ owner approval.

tkent, abarth: please take a look?

[tkent, 2014-03-06 00:30:51]

lgtm

[commit-bot: I haz the power, 2014-03-06 00:31:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/185413023/60001

[commit-bot: I haz the power, 2014-03-06 00:31:51]

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

[commit-bot: I haz the power, 2014-03-06 00:31:51]

Try jobs failed on following builders: blink_presubmit

[tkent, 2014-03-06 00:52:19]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-06 00:52:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/185413023/60001

[commit-bot: I haz the power, 2014-03-06 05:54:55]

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

[commit-bot: I haz the power, 2014-03-06 05:54:56]

Try jobs failed on following builders: win_blink_compile, win_blink_rel

[sof, 2014-03-06 07:32:05]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-03-06 07:32:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/185413023/80001

[commit-bot: I haz the power, 2014-03-06 09:29:13]

Change committed as 168607