Add Perl IDL support for method parameter type sequence<Dictionary>

Add Perl IDL support for method parameter type sequence<Dictionary>

This patch adds support to the Perl IDL generator for sequence<Dictionary>
parameters on IDL methods.
This introduces a DictionarySequence class to wrap the JS array-like
object instead of using a Vector<Dictionary> because Dictionaries cannot
be heap allocated.

BUG=257235
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162335

[haraken, 2013-11-19 06:22:24]

> This introduces a DictionarySequence class to wrap the JS array-like
> object instead of using a Vector<Dictionary> because Dictionaries cannot
> be heap allocated.

This sounds ugly. What makes it hard to make Dictionaries heap-allocated?

[Nils Barth (inactive), 2013-11-19 06:45:42]

2 notes on Perl code;
I'll defer to haraken re: overall approach.

https://codereview.chromium.org/74533003/diff/1/Source/bindings/scripts/code_...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/74533003/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.pm:443: if
(IsDictionaryType($arrayType)) {
I think you can just add:
} elsif ($type eq "Dictionary") {
    AddToImplIncludes("bindings/v8/Dictionary.h");
}
in the main elsif sequence.

https://codereview.chromium.org/74533003/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.pm:5954: sub IsDictionaryType
This subroutine doesn't add anything, and is less clear than just inline
$type eq "Dictionary"

Are you planning to add more logic here?
If not, could you just inline $type eq "Dictionary" instead?

[alancutter (OOO until 2018), 2013-11-19 11:00:43]

On 2013/11/19 06:22:24, haraken wrote:
> > This introduces a DictionarySequence class to wrap the JS array-like
> > object instead of using a Vector<Dictionary> because Dictionaries cannot
> > be heap allocated.
> 
> This sounds ugly. What makes it hard to make Dictionaries heap-allocated?

I'm not confident about putting V8 handles in a WTF Vector. I'm afraid that the
memory management done in Vector might manifest as security bugs on the V8 side.
I have uploaded an alternate patch that uses Vector<Dictionary> instead of
DictionarySequence for you to critique.

https://codereview.chromium.org/74533003/diff/1/Source/bindings/scripts/code_...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/74533003/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.pm:443: if
(IsDictionaryType($arrayType)) {
On 2013/11/19 06:45:43, Nils Barth wrote:
> I think you can just add:
> } elsif ($type eq "Dictionary") {
>     AddToImplIncludes("bindings/v8/Dictionary.h");
> }
> in the main elsif sequence.

Done.

https://codereview.chromium.org/74533003/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.pm:5954: sub IsDictionaryType
On 2013/11/19 06:45:43, Nils Barth wrote:
> This subroutine doesn't add anything, and is less clear than just inline
> $type eq "Dictionary"
> 
> Are you planning to add more logic here?
> If not, could you just inline $type eq "Dictionary" instead?

Done.

[haraken, 2013-11-19 11:12:04]

> I'm not confident about putting V8 handles in a WTF Vector. > I'm afraid that
the
> memory management done in Vector might manifest as security bugs on the V8
side.

I don't see problems in putting V8 handles in a WTF vector. (We're already doing
it in several places.) Do you have any specific example that causes problems?

https://codereview.chromium.org/74533003/diff/70001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/74533003/diff/70001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:5999: return $parameter->isNullable
&& !IsRefPtrType($parameter->type) && $parameter->type ne "Dictionary";

You won't need to check '$parameter->type ne "Dictionary"' because you're
already checking it in IsRefPtrType.

https://codereview.chromium.org/74533003/diff/70001/Source/bindings/v8/Dictio...
File Source/bindings/v8/Dictionary.h (left):

https://codereview.chromium.org/74533003/diff/70001/Source/bindings/v8/Dictio...
Source/bindings/v8/Dictionary.h:109: static void operator delete(void *);

Instead of removing these, won't the following code work?

  public:
    // Only allow inline allocation.
    void* operator new(size_t, NotNullTag, void* location) { return location; }
  private:
    // Disallow new allocation.
    void* operator new(size_t);

https://codereview.chromium.org/74533003/diff/70001/Source/bindings/v8/V8Bind...
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/74533003/diff/70001/Source/bindings/v8/V8Bind...
Source/bindings/v8/V8Binding.h:573:
result.uncheckedAppend(TraitsType::nativeValue(info[i], 0));

0 => info.GetIsolate()

[alancutter (OOO until 2018), 2013-11-19 23:43:47]

On 2013/11/19 11:12:04, haraken wrote:
> I don't see problems in putting V8 handles in a WTF vector. (We're already
doing
> it in several places.) Do you have any specific example that causes problems?

Mostly just a general unease about mixing memory management systems together,
especially when the Dictionary class was designed to avoid it.
Restricting Vector<Dictionary> to the stack should be fine but I think there can
be problems if it gets stored as a member of an object on the heap.

https://codereview.chromium.org/74533003/diff/70001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/74533003/diff/70001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:5999: return $parameter->isNullable
&& !IsRefPtrType($parameter->type) && $parameter->type ne "Dictionary";
On 2013/11/19 11:12:05, haraken wrote:
> 
> You won't need to check '$parameter->type ne "Dictionary"' because you're
> already checking it in IsRefPtrType.

Without '$parameter->type ne "Dictionary"' it will return true (due to the
change in IsRefPtrType). I needed to add this check to preserve existing
behaviour.

https://codereview.chromium.org/74533003/diff/70001/Source/bindings/v8/V8Bind...
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/74533003/diff/70001/Source/bindings/v8/V8Bind...
Source/bindings/v8/V8Binding.h:573:
result.uncheckedAppend(TraitsType::nativeValue(info[i], 0));
On 2013/11/19 11:12:05, haraken wrote:
> 
> 0 => info.GetIsolate()

Done.

[alancutter (OOO until 2018), 2013-11-20 00:40:14]

https://codereview.chromium.org/74533003/diff/70001/Source/bindings/v8/Dictio...
File Source/bindings/v8/Dictionary.h (left):

https://codereview.chromium.org/74533003/diff/70001/Source/bindings/v8/Dictio...
Source/bindings/v8/Dictionary.h:109: static void operator delete(void *);
On 2013/11/19 11:12:05, haraken wrote:
> 
> Instead of removing these, won't the following code work?
> 
>   public:
>     // Only allow inline allocation.
>     void* operator new(size_t, NotNullTag, void* location) { return location;
}
>   private:
>     // Disallow new allocation.
>     void* operator new(size_t);

Done.

[Nils Barth (inactive), 2013-11-20 00:45:56]

Thanks for Perl changes; the Perl CG changes look fine!

[haraken, 2013-11-20 04:29:30]

LGTM

> Mostly just a general unease about mixing memory management systems together,
> especially when the Dictionary class was designed to avoid it.
> Restricting Vector<Dictionary> to the stack should be fine but I think there
can
> be problems if it gets stored as a member of an object on the heap.

The operator new hack will be helpful to avoid it, but I agree that we should
invent a better way to avoid allocating V8 handles on heap. If there are more
similar cases, we can make a macro for the operator new hack (As far as I know,
Dictionary is the only class that allocates V8 handles inside it at the moment).

[commit-bot: I haz the power, 2013-11-20 04:53:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/74533003/150001

[commit-bot: I haz the power, 2013-11-20 06:03:47]

Change committed as 162335