Bug 53672

Summary: Modify make_names.pl to not include conditional includes unconditionally
Product: WebKit Reporter: Adam Bergkvist <adam.bergkvist>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aroben, commit-queue, darin, dbates, eric, jchaffraix
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 47264    
Attachments:
Description Flags
Proposed patch
dbates: review+, dbates: commit-queue-
diff between JSHTMLElementWrapperFactory.cpp before and after modification of make_names.pl
none
Updated patch none

Description Adam Bergkvist 2011-02-03 04:46:24 PST
Generated files should have feature guards around includes of conditional features. This makes it possible to add new elements without adding the files to every build system.
Comment 1 Adam Bergkvist 2011-02-03 04:55:49 PST
Created attachment 81051 [details]
Proposed patch
Comment 2 Adam Roben (:aroben) 2011-02-09 08:05:37 PST
CCing folks who've worked on this script in the past.
Comment 3 Adam Barth 2011-02-09 14:28:38 PST
Comment on attachment 81051 [details]
Proposed patch

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

This looks plausible, but I don't understand Perl very well.

> Source/WebCore/dom/make_names.pl:600
> +        next if $enabledTags{$tagName}{conditional} || defined($tagsSeen{$JSInterfaceName})
> +                || usesDefaultJSWrapper($tagName);

This should probably be on one line.
Comment 4 Adam Bergkvist 2011-02-10 07:11:14 PST
Created attachment 81977 [details]
diff between JSHTMLElementWrapperFactory.cpp before and after modification of make_names.pl

If nothing else, this patch makes the generated files more readable.
Comment 5 Eric Seidel (no email) 2011-04-06 10:28:43 PDT
dbates and darin adler do most of the perl reviewing.  Also looks plausibly sane to me, but better to ask them.
Comment 6 Daniel Bates 2011-04-11 20:29:06 PDT
Comment on attachment 81051 [details]
Proposed patch

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

> Source/WebCore/ChangeLog:10
> +        No new tests (no change in functionality)

I wish we could test this :-(. I don't think we have unit test coverage for make_names.pl (since it's a driver program as opposed to a Perl module).

> Source/WebCore/ChangeLog:12
> +        * dom/make_names.pl:

Because prepareChangeLog doesn't support Perl, it is convention that we add analogous notes that document the added functions. This makes it convenient for a person to determine which changeset added/changed a particular function by searching the change log entries for the function name. For an example of this, see <http://trac.webkit.org/changeset/52692>.

> Source/WebCore/dom/make_names.pl:614
> +        next if $enabledTags{$tagName}{conditional} || defined($tagsSeen{$interfaceName});

I suggest adding a comment above this line that explains that we are skipping feature-define-specific #includes because we will handle such #includes separately. OR, even better, break the disjuncts into separate if statements, like:

next if defined($tagsSeen{$interfaceName});

if ($enabledTags{$tagName}{conditional}) {
    # We skip feature-define-specific #includes here since will we handle such #includes separately.
    next;
}

> Source/WebCore/dom/make_names.pl:621
> +sub printConditionalElementIncludes

This function only handles feature-define #includes as opposed to all such conditional #include. Maybe a better name would be printFeatureDefineIncludes()?

> Source/WebCore/dom/make_names.pl:624
> +    my $F = shift;
> +    my $wrapperFactoryType = shift;

I would write this as:

my ($F, $wrapperFactoryType) = @_;

> Source/WebCore/dom/make_names.pl:625
> +    my $printJSElementIncludes = !!$wrapperFactoryType;

It is necessary to explicitly convert $wrapperFactoryType to a boolean value (see my comment for line 656). Moreover, this variable is unnecessary since it's being referenced only once in this function; => the value should just be inlined into the if-statement.

> Source/WebCore/dom/make_names.pl:629
> +    my %conditionals = ();
> +    my %unconditionalElementIncludes = ();
> +    my %unconditionalJSElementIncludes = ();

It is unnecessary to explicitly initialize these. You can just write these as:

my %conditionals;
my %unconditionalElementIncludes;
my %unconditionalJSElementIncludes;

> Source/WebCore/dom/make_names.pl:641
> +            if (!$conditionals{$conditional}) {
> +                $conditionals{$conditional} = ();
> +                $conditionals{$conditional}{interfaceNames} = ();
> +                $conditionals{$conditional}{JSInterfaceNames} = ();
> +            }

This code is unnecessary. No need to explicitly initialize the values of these hash keys. Perl is reasonable when it comes it using undefined values.

> Source/WebCore/dom/make_names.pl:651
> +        print F "\n#if ENABLE(${conditional})\n";

The curly bracket '{' and '}' are unnecessary. It seems that make_names.pl doesn't have a consistent notation. Some code uses the curly-bracket notation and some doesn't. Regardless, I suggest removing the curly brackets since there is only one variable to be interpolated and I don't feel there presence improves the readability of this string, which already feels crowded by the presence of all the surrounding punctuation characters (e.g. '(', ')') and new-line escape sequences .

> Source/WebCore/dom/make_names.pl:652
> +        for my $interfaceName (sort keys %{$conditionals{$conditional}{interfaceNames}}) {

I would remove the curly brackets around the expression $conditionals{...}{...} as they are unnecessary.

> Source/WebCore/dom/make_names.pl:654
> +            print F "#include \"${interfaceName}.h\"\n";

Curly brackets are also used here and are unnecessary.

> Source/WebCore/dom/make_names.pl:656
> +        if ($printJSElementIncludes) {

This can be written as:

if ($wrapperFactoryType) {

> Source/WebCore/dom/make_names.pl:657
> +            for my $JSInterfaceName (sort keys %{$conditionals{$conditional}{JSInterfaceNames}}) {

I would remove the curly brackets around the expression $conditionals{...}{...} as they are unnecessary.

> Source/WebCore/dom/make_names.pl:713
> +printConditionalElementIncludes($F, "");

Leave off the second argument; it's syntactically unnecessary and doesn't add any value.

Notice, when you leave off the second argument then $wrapperFactoryType is undefined inside the function body printConditionalElementIncludes(); => $wrapperFactoryType evaluates to false in the if-statment on line 656.
Comment 7 Daniel Bates 2011-04-11 20:31:18 PDT
Comment on attachment 81051 [details]
Proposed patch

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

>> Source/WebCore/dom/make_names.pl:625
>> +    my $printJSElementIncludes = !!$wrapperFactoryType;
> 
> It is necessary to explicitly convert $wrapperFactoryType to a boolean value (see my comment for line 656). Moreover, this variable is unnecessary since it's being referenced only once in this function; => the value should just be inlined into the if-statement.

*It is unnecessary to explicitly ...
Comment 8 Adam Bergkvist 2011-04-15 06:20:09 PDT
Created attachment 89773 [details]
Updated patch

(In reply to comment #6)
> (From update of attachment 81051 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81051&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        No new tests (no change in functionality)
> 
> I wish we could test this :-(. I don't think we have unit test coverage for make_names.pl (since it's a driver program as opposed to a Perl module).
> 
> > Source/WebCore/ChangeLog:12
> > +        * dom/make_names.pl:
> 
> Because prepareChangeLog doesn't support Perl, it is convention that we add analogous notes that document the added functions. This makes it convenient for a person to determine which changeset added/changed a particular function by searching the change log entries for the function name. For an example of this, see <http://trac.webkit.org/changeset/52692>.
>

Fixed.

> > Source/WebCore/dom/make_names.pl:614
> > +        next if $enabledTags{$tagName}{conditional} || defined($tagsSeen{$interfaceName});
> 
> I suggest adding a comment above this line that explains that we are skipping feature-define-specific #includes because we will handle such #includes separately. OR, even better, break the disjuncts into separate if statements, like:
> 
> next if defined($tagsSeen{$interfaceName});
> 
> if ($enabledTags{$tagName}{conditional}) {
>     # We skip feature-define-specific #includes here since will we handle such #includes separately.
>     next;
> }

Fixed. (Also fixed the similar if statement in printJSElementIncludes.)

> > Source/WebCore/dom/make_names.pl:621
> > +sub printConditionalElementIncludes
> 
> This function only handles feature-define #includes as opposed to all such conditional #include. Maybe a better name would be printFeatureDefineIncludes()?
>

I think printConditionalElementIncludes is OK since the name hints that it only deals with conditional elements (and not feature defined includes in general).

> > Source/WebCore/dom/make_names.pl:624
> > +    my $F = shift;
> > +    my $wrapperFactoryType = shift;
> 
> I would write this as:
> 
> my ($F, $wrapperFactoryType) = @_;
>

Fixed.

> > Source/WebCore/dom/make_names.pl:625
> > +    my $printJSElementIncludes = !!$wrapperFactoryType;
> 
> It is necessary to explicitly convert $wrapperFactoryType to a boolean value (see my comment for line 656). Moreover, this variable is unnecessary since it's being referenced only once in this function; => the value should just be inlined into the if-statement.
>

Fixed. (The intention was to make the if statement at line 656 easier to understand.)

> > Source/WebCore/dom/make_names.pl:629
> > +    my %conditionals = ();
> > +    my %unconditionalElementIncludes = ();
> > +    my %unconditionalJSElementIncludes = ();
> 
> It is unnecessary to explicitly initialize these. You can just write these as:
> 
> my %conditionals;
> my %unconditionalElementIncludes;
> my %unconditionalJSElementIncludes;
>

Fixed.

> > Source/WebCore/dom/make_names.pl:641
> > +            if (!$conditionals{$conditional}) {
> > +                $conditionals{$conditional} = ();
> > +                $conditionals{$conditional}{interfaceNames} = ();
> > +                $conditionals{$conditional}{JSInterfaceNames} = ();
> > +            }
> 
> This code is unnecessary. No need to explicitly initialize the values of these hash keys. Perl is reasonable when it comes it using undefined values.
> 

Fixed.

> > Source/WebCore/dom/make_names.pl:651
> > +        print F "\n#if ENABLE(${conditional})\n";
> 
> The curly bracket '{' and '}' are unnecessary. It seems that make_names.pl doesn't have a consistent notation. Some code uses the curly-bracket notation and some doesn't. Regardless, I suggest removing the curly brackets since there is only one variable to be interpolated and I don't feel there presence improves the readability of this string, which already feels crowded by the presence of all the surrounding punctuation characters (e.g. '(', ')') and new-line escape sequences .
>

Fixed.

> > Source/WebCore/dom/make_names.pl:652
> > +        for my $interfaceName (sort keys %{$conditionals{$conditional}{interfaceNames}}) {
> 
> I would remove the curly brackets around the expression $conditionals{...}{...} as they are unnecessary.
>

I dont't think the curly brackets are optional in this case since I get an error when I remove them.

> > Source/WebCore/dom/make_names.pl:654
> > +            print F "#include \"${interfaceName}.h\"\n";
> 
> Curly brackets are also used here and are unnecessary.
>

Fixed.

> > Source/WebCore/dom/make_names.pl:656
> > +        if ($printJSElementIncludes) {
> 
> This can be written as:
> 
> if ($wrapperFactoryType) {
>

Fixed.

> > Source/WebCore/dom/make_names.pl:657
> > +            for my $JSInterfaceName (sort keys %{$conditionals{$conditional}{JSInterfaceNames}}) {
> 
> I would remove the curly brackets around the expression $conditionals{...}{...} as they are unnecessary.
>

See comment above.

> > Source/WebCore/dom/make_names.pl:713
> > +printConditionalElementIncludes($F, "");
> 
> Leave off the second argument; it's syntactically unnecessary and doesn't add any value.
> 
> Notice, when you leave off the second argument then $wrapperFactoryType is undefined inside the function body printConditionalElementIncludes(); => $wrapperFactoryType evaluates to false in the if-statment on line 656.

Fixed.
Comment 9 Daniel Bates 2011-04-15 08:29:42 PDT
Comment on attachment 89773 [details]
Updated patch

Thanks Adam for making the changes.

r=me
Comment 10 WebKit Review Bot 2011-04-18 02:31:28 PDT
Comment on attachment 89773 [details]
Updated patch

Rejecting attachment 89773 [details] from commit-queue.

adam.bergkvist@ericsson.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 11 WebKit Commit Bot 2011-04-18 08:13:54 PDT
Comment on attachment 89773 [details]
Updated patch

Clearing flags on attachment: 89773

Committed r84145: <http://trac.webkit.org/changeset/84145>
Comment 12 WebKit Commit Bot 2011-04-18 08:14:03 PDT
All reviewed patches have been landed.  Closing bug.