[webkit-reviews] review granted: [Bug 216235] [WebIDL] Support extended attributes on includes statements to allow for conditionalized inclusion : [Attachment 408163] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 216235: [WebIDL] Support extended attributes on includes statements to
allow for conditionalized inclusion
https://bugs.webkit.org/show_bug.cgi?id=216235

Attachment 408163: Patch

https://bugs.webkit.org/attachment.cgi?id=408163&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 408163
  --> https://bugs.webkit.org/attachment.cgi?id=408163
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408163&action=review

All about those sets.

> Source/WebCore/animation/AnimationFrameProvider.idl:29
> +    long requestAnimationFrame(RequestAnimationFrameCallback callback); //
FIXME: Should return an unsigned long.
> +    undefined cancelAnimationFrame(long handle); // FIXME: handle should be
an unsigned long.

I’m going to boldly suggest we move from long to unsigned long since the risk
of fixing this is infinitesimal. Doesn’t even matter if the C++ code uses int
or unsigned since it’s just an opaque handle.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:196
> +    die "Processing document " . $useDocument->fileName . " did not generate
anything.";

Since we are adding a period here, do we want the version of die with a newline
that doesn’t cite where the die statement is?

Are we going to rewrite in Python once we can require Python 3? It’s tempting!

> Source/WebCore/bindings/scripts/CodeGenerator.pm:226
> +    print "Generating $useGenerator bindings code for IDL interface \"" .
$interface->type->name . "\"...\n" if $verbose;

Not the biggest fan of the ellipses.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:344
>      if (!$supplementalDependencies) {
>	   return;
>      }

Missed opportunity for the idiomatic perl one-line if statement.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:384
> +		       # Record that this attribute is implemented by
$interfaceName.

Seems like a redundant comment, since code says exactly that.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:404
> +		       # Record that this method is implemented by
$interfaceName.

Seems like a redundant comment, since code says exactly that.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:424
> +		       # Record that this constant is implemented by
$interfaceName.

Seems like a redundant comment, since code says exactly that.

> Source/WebCore/bindings/scripts/preprocess-idls.pl:498
> -    while ($fileContents =~ /^\s*(\w+)\s+includes\s+(\w+)\s*;/mg) {
> +    while ($fileContents =~ /\s*(\w+)\s+includes\s+(\w+)\s*;/mg) {

This seems not so great. Now that the beginning of the regular expression is
not anchored at beginnings of lines, it seems like this expression could match
after any character. Maybe we need a \b in there?