Bug 216235 – [WebIDL] Support extended attributes on includes statements to allow for conditionalized inclusion

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[WebIDL] Support extended attributes on includes statements to allow for conditionalized inclusion

[Sam Weinig]

Created attachment 408163 [details]
Patch

[Darin Adler]

Comment on attachment 408163 [details]
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?

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 408163 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Sam Weinig]

(In reply to Darin Adler from comment #2)
> Comment on attachment 408163 [details]
> 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.

Bold! Made the change.

> 
> > 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?

I'm not familiar with that. How do you use this other version of die?

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

Yeah, I think I am going to. 

> 
> > 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?

[Sam Weinig]

(In reply to Darin Adler from comment #2)
> Comment on attachment 408163 [details]
> 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.

Bold! Made the change.

> 
> > 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?
> 

I'm not familiar with that. How do you use this other version of die?

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

Yeah, I think I will need to. 

> 
> > 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.

Gone.

> 
> > Source/WebCore/bindings/scripts/CodeGenerator.pm:344
> >      if (!$supplementalDependencies) {
> >          return;
> >      }
> 
> Missed opportunity for the idiomatic perl one-line if statement.

Fixed.

> 
> > 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.

Removed.

> 
> > 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.

Removed.

> 
> > 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.

Removed.

> 
> > 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?

Yeah, we need the \b in there. Will fix.

[Sam Weinig]

Created attachment 408187 [details]
Patch

[EWS]

Committed r266706: <https://trac.webkit.org/changeset/266706>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408187 [details].

[Darin Adler]

(In reply to Sam Weinig from comment #5)
> I'm not familiar with that. How do you use this other version of die?

<https://perldoc.perl.org/functions/die.html>: "If the string exception does not end in a newline, the current script line number and input line number (if any) and a newline are appended to it."

[Radar WebKit Bug Importer]

<rdar://problem/68470878>