Get rid of [Callback] extended attribute for parameters in IDL files

Get rid of [Callback] extended attribute for parameters in IDL files

Remove the Blink-specific [Callback] extended attribute and make the bindings
generator smart enough to know when a parameter is of callback type on its
own.

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

[do-not-use, 2013-04-26 16:39:50]

Impact on IDL processing time is low:

Without patch (8 jobs / 1 job):
 0m32.841s / 1m37.190s

With patch (8 jobs / 1 job):
 0m33.462s / 1m38.942s

[do-not-use, 2013-04-26 16:48:16]

https://codereview.chromium.org/14384004/diff/1/Source/bindings/derived_sourc...
File Source/bindings/derived_sources.gyp (right):

https://codereview.chromium.org/14384004/diff/1/Source/bindings/derived_sourc...
Source/bindings/derived_sources.gyp:177: '--include', '../core',
The generator will look for IDL files recursively in these include dirs (using
File::Find in perl). The issue was that the include dirs were very specific and
incomplete. Therefore, some IDL interfaces could not be resolved.

We could just make add the missing dirs but I decided to include top-level dirs
instead so that we don't need to update this list all the time.

[haraken, 2013-04-26 18:03:35]

https://codereview.chromium.org/14384004/diff/1/Source/bindings/scripts/CodeG...
File Source/bindings/scripts/CodeGenerator.pm (right):

https://codereview.chromium.org/14384004/diff/1/Source/bindings/scripts/CodeG...
Source/bindings/scripts/CodeGenerator.pm:661: }
This is a bit weak for whitespaces. Let's use the same programming pattern as
getPartialInterfaceNameFromIDLFile() in preprocess-idls.pl.

https://codereview.chromium.org/14384004/diff/1/Source/bindings/scripts/CodeG...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/14384004/diff/1/Source/bindings/scripts/CodeG...
Source/bindings/scripts/CodeGeneratorV8.pm:4655: return 0 if
$codeGenerator->GetSequenceType($type);

I'm OK to land the patch with this, but I think IsWrapperType() should return 0
for Array and Sequence (and thus you can remove these lines). See FIXME in
IsWrapperType(). I'd be happy if you could investigate it in a follow-up patch.

[do-not-use, 2013-04-26 18:15:57]

https://codereview.chromium.org/14384004/diff/1/Source/bindings/scripts/CodeG...
File Source/bindings/scripts/CodeGenerator.pm (right):

https://codereview.chromium.org/14384004/diff/1/Source/bindings/scripts/CodeG...
Source/bindings/scripts/CodeGenerator.pm:661: }
On 2013/04/26 18:03:36, haraken wrote:
> This is a bit weak for whitespaces. Let's use the same programming pattern as
> getPartialInterfaceNameFromIDLFile() in preprocess-idls.pl.

Meaning add the 'g' modifier, right?

https://codereview.chromium.org/14384004/diff/1/Source/bindings/scripts/CodeG...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/14384004/diff/1/Source/bindings/scripts/CodeG...
Source/bindings/scripts/CodeGeneratorV8.pm:4655: return 0 if
$codeGenerator->GetSequenceType($type);
On 2013/04/26 18:03:36, haraken wrote:
> 
> I'm OK to land the patch with this, but I think IsWrapperType() should return
0
> for Array and Sequence (and thus you can remove these lines). See FIXME in
> IsWrapperType(). I'd be happy if you could investigate it in a follow-up
patch.

Yes, I saw but I did not want to be too greedy and I was worried about causing
behaviour changes. I'm happy to investigate and put up a follow-up patch though.
In the mean time, should I add a FIXME here to indicate that it should probably
be moved to IsWrapperType()?

[haraken, 2013-04-26 19:17:37]

> > This is a bit weak for whitespaces. Let's use the same programming pattern
as
> > getPartialInterfaceNameFromIDLFile() in preprocess-idls.pl.
> 
> Meaning add the 'g' modifier, right?

No, you need to read all lines first, concatenate them, and use the regular
expression. 'callback' and 'interface' might be written in different lines.

> Yes, I saw but I did not want to be too greedy and I was worried about causing
> behaviour changes. I'm happy to investigate and put up a follow-up patch
though.
> In the mean time, should I add a FIXME here to indicate that it should
probably
> be moved to IsWrapperType()?

Sounds nice.

[do-not-use, 2013-04-26 19:24:06]

On 2013/04/26 19:17:37, haraken wrote:
> > > This is a bit weak for whitespaces. Let's use the same programming pattern
> as
> > > getPartialInterfaceNameFromIDLFile() in preprocess-idls.pl.
> > 
> > Meaning add the 'g' modifier, right?
> 
> No, you need to read all lines first, concatenate them, and use the regular
> expression. 'callback' and 'interface' might be written in different lines.
> 
> > Yes, I saw but I did not want to be too greedy and I was worried about
causing
> > behaviour changes. I'm happy to investigate and put up a follow-up patch
> though.
> > In the mean time, should I add a FIXME here to indicate that it should
> probably
> > be moved to IsWrapperType()?
> 
> Sounds nice.

You're right.
I have fixed the patch accordingly.

[haraken, 2013-04-26 19:27:31]

LGTM

[commit-bot: I haz the power, 2013-04-26 19:27:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14384004/8002

[commit-bot: I haz the power, 2013-04-26 20:02:19]

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

[commit-bot: I haz the power, 2013-04-27 05:12:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14384004/8002

[commit-bot: I haz the power, 2013-04-27 05:13:01]

Failed to apply patch for Source/bindings/scripts/CodeGeneratorV8.pm:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/scripts/CodeGeneratorV8.pm
  Hunk #5 FAILED at 2055.
  1 out of 9 hunks FAILED -- saving rejects to file
Source/bindings/scripts/CodeGeneratorV8.pm.rej

Patch:       Source/bindings/scripts/CodeGeneratorV8.pm
Index: Source/bindings/scripts/CodeGeneratorV8.pm
diff --git a/Source/bindings/scripts/CodeGeneratorV8.pm
b/Source/bindings/scripts/CodeGeneratorV8.pm
index
46b25b4a7ee2e74f79e0002611fff96d39661dec..ada14205f8763226f37aeaa53d9b017147bc2f42
100644
--- a/Source/bindings/scripts/CodeGeneratorV8.pm
+++ b/Source/bindings/scripts/CodeGeneratorV8.pm
@@ -8,6 +8,7 @@
 # Copyright (C) Research In Motion Limited 2010. All rights reserved.
 # Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
 # Copyright (C) 2012 Ericsson AB. All rights reserved.
+# Copyright (C) 2013 Samsung Electronics. All rights reserved.
 #
 # This library is free software; you can redistribute it and/or
 # modify it under the terms of the GNU Library General Public
@@ -1665,7 +1666,7 @@ sub GenerateParametersCheckExpression
             if ($parameter->extendedAttributes->{"StrictTypeChecking"}) {
                 push(@andExpression, "(${value}->IsNull() ||
${value}->IsUndefined() || ${value}->IsString() || ${value}->IsObject())");
             }
-        } elsif ($parameter->extendedAttributes->{"Callback"}) {
+        } elsif (IsCallbackInterface($parameter->type)) {
             # For Callbacks only checks if the value is null or object.
             push(@andExpression, "(${value}->IsNull() ||
${value}->IsFunction())");
         } elsif ($codeGenerator->GetArrayType($type) ||
$codeGenerator->GetSequenceType($type)) {
@@ -1910,7 +1911,7 @@ END
     my $raisesExceptions =
$function->signature->extendedAttributes->{"RaisesException"};
     if (!$raisesExceptions) {
         foreach my $parameter (@{$function->parameters}) {
-            if ((!$parameter->extendedAttributes->{"Callback"} and
TypeCanFailConversion($parameter)) or
$parameter->extendedAttributes->{"IsIndex"}) {
+            if ((!IsCallbackInterface($parameter->type) and
TypeCanFailConversion($parameter)) or
$parameter->extendedAttributes->{"IsIndex"}) {
                 $raisesExceptions = 1;
             }
         }
@@ -2033,7 +2034,7 @@ sub GenerateParametersCheck
         # Optional arguments without [Default=...] should generate an early
call with fewer arguments.
         # Optional arguments with [Optional=...] should not generate the early
call.
         # Optional Dictionary arguments always considered to have default of
empty dictionary.
-        if ($parameter->isOptional &&
!$parameter->extendedAttributes->{"Default"} && $nativeType ne "Dictionary" &&
!$parameter->extendedAttributes->{"Callback"}) {
+        if ($parameter->isOptional &&
!$parameter->extendedAttributes->{"Default"} && $nativeType ne "Dictionary" &&
!IsCallbackInterface($parameter->type)) {
             $parameterCheckString .= "    if (args.Length() <= $paramIndex)
{\n";
             my $functionCall = GenerateFunctionCallString($function,
$paramIndex, "    " x 2, $interfaceName, $forMainWorldSuffix, %replacements);
             $parameterCheckString .= $functionCall;
@@ -2054,7 +2055,7 @@ sub GenerateParametersCheck
         }
 
         AddToImplIncludes("ExceptionCode.h");
-        if ($parameter->extendedAttributes->{"Callback"}) {
+        if (IsCallbackInterface($parameter->type)) {
             my $v8InterfaceName = "V8" . $parameter->type;
             AddToImplIncludes("$v8InterfaceName.h");
             if ($parameter->isOptional) {
@@ -2238,7 +2239,7 @@ sub GenerateSingleConstructorCallback
     }
     if (!$raisesExceptions) {
         foreach my $parameter (@{$function->parameters}) {
-            if ((!$parameter->extendedAttributes->{"Callback"} and
TypeCanFailConversion($parameter)) or
$parameter->extendedAttributes->{"IsIndex"}) {
+            if ((!IsCallbackInterface($parameter->type) and
TypeCanFailConversion($parameter)) or
$parameter->extendedAttributes->{"IsIndex"}) {
                 $raisesExceptions = 1;
             }
         }
@@ -2463,7 +2464,7 @@ sub GenerateNamedConstructor
     }
     if (!$raisesExceptions) {
         foreach my $parameter (@{$function->parameters}) {
-            if ((!$parameter->extendedAttributes->{"Callback"} and
TypeCanFailConversion($parameter)) or
$parameter->extendedAttributes->{"IsIndex"}) {
+            if ((!IsCallbackInterface($parameter->type) and
TypeCanFailConversion($parameter)) or
$parameter->extendedAttributes->{"IsIndex"}) {
                 $raisesExceptions = 1;
             }
         }
@@ -4604,7 +4605,7 @@ sub RequiresCustomSignature
       return 0;
     }
     foreach my $parameter (@{$function->parameters}) {
-        if (($parameter->isOptional &&
!$parameter->extendedAttributes->{"Default"}) ||
$parameter->extendedAttributes->{"Callback"}) {
+        if (($parameter->isOptional &&
!$parameter->extendedAttributes->{"Default"}) ||
IsCallbackInterface($parameter->type)) {
             return 0;
         }
     }
@@ -4656,6 +4657,18 @@ sub IsWrapperType
     return !($non_wrapper_types{$type});
 }
 
+sub IsCallbackInterface
+{
+    my $type = shift;
+    return 0 unless IsWrapperType($type);
+    # FIXME: Those checks for Sequence and Array types should probably
+    # be moved to IsWrapperType().
+    return 0 if $codeGenerator->GetArrayType($type);
+    return 0 if $codeGenerator->GetSequenceType($type);
+
+    return $codeGenerator->IsCallbackInterfaceFromFile($type);
+}
+
 sub GetTypeNameOfExternalTypedArray
 {
     my $interface = shift;

[do-not-use, 2013-04-30 08:11:50]

I rebased on master, could someone please cq+ again?

[haraken, 2013-04-30 08:12:35]

Done.

[commit-bot: I haz the power, 2013-04-30 08:12:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14384004/21001

[commit-bot: I haz the power, 2013-04-30 08:19:00]

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

[commit-bot: I haz the power, 2013-04-30 08:19:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14384004/21001

[commit-bot: I haz the power, 2013-04-30 08:30:02]

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

[do-not-use, 2013-04-30 12:00:42]

On 2013/04/30 08:30:02, I haz the power (commit-bot) wrote:
> Retried try job too often on blink_bare_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_bare...

Missing LGTM from an OWNER for these files:
    Source/modules/filesystem/DOMWindowFileSystem.idl
    Source/modules/filesystem/DirectoryEntry.idl
    Source/modules/filesystem/DirectoryReader.idl
    ....

[haraken, 2013-04-30 12:01:26]

jochen: rubber-stamp please?

[jochen (gone - plz use gerrit), 2013-04-30 12:11:22]

rubber stamp lgtm

[commit-bot: I haz the power, 2013-04-30 12:11:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14384004/21001

[commit-bot: I haz the power, 2013-04-30 12:21:37]

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

[do-not-use, 2013-04-30 12:37:27]

On 2013/04/30 12:21:37, I haz the power (commit-bot) wrote:
> Retried try job too often on blink_bare_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_bare...

The only thing I see in the presubmit log is:
** Presubmit Warnings **
Found line ending with white spaces in:

***************
Source/bindings/tests/results/V8TestNamedConstructor.cpp, line 98
Source/bindings/tests/results/V8TestNamedConstructor.cpp, line 99
***************

I'm surprised this would prevent committing.
Also, I cannot do much about this as those are generated files.

[haraken, 2013-04-30 12:39:44]

Would it be possible to fix CodeGeneratorV8.pm so that it doesn't output
redundant trailing spaces? Otherwise, we will face the same problem in the
future again and again...

[do-not-use, 2013-04-30 13:39:45]

On 2013/04/30 12:39:44, haraken wrote:
> Would it be possible to fix CodeGeneratorV8.pm so that it doesn't output
> redundant trailing spaces? Otherwise, we will face the same problem in the
> future again and again...

Sure, I'll fix this in the same patch then.

[do-not-use, 2013-04-30 13:51:58]

On 2013/04/30 13:39:45, Christophe Dumez wrote:
> On 2013/04/30 12:39:44, haraken wrote:
> > Would it be possible to fix CodeGeneratorV8.pm so that it doesn't output
> > redundant trailing spaces? Otherwise, we will face the same problem in the
> > future again and again...
> 
> Sure, I'll fix this in the same patch then.

Ok, this is fixed now in patch set 5.

[commit-bot: I haz the power, 2013-04-30 14:03:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14384004/37002

[commit-bot: I haz the power, 2013-04-30 14:44:56]

Change committed as 149436