[webkit-reviews] review denied: [Bug 33755] Make update-iexploder-cssproperties update htmltags.in and htmlattrs.in as well : [Attachment 46732] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Holger Freyther
<zecke at selfish.org>'s request for review:
Bug 33755: Make update-iexploder-cssproperties update htmltags.in and
htmlattrs.in as well
https://bugs.webkit.org/show_bug.cgi?id=33755

Attachment 46732: Patch
https://bugs.webkit.org/attachment.cgi?id=46732&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> +2010-01-15  Holger Hans Peter Freyther  <zecke at selfish.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   [iexploder] Automatically update htmltags.in and htmlattrs.in too
> +	   Need a short description and bug URL (OOPS!)

Need the bug URL here instead of the placeholder text.

> diff --git a/WebKitTools/Scripts/update-iexploder-cssproperties
b/WebKitTools/Scripts/update-iexploder-cssproperties
> @@ -35,38 +37,54 @@ use strict;
>  use FindBin;
>  use lib $FindBin::Bin;
>  use webkitdirs;
> +use VCSUtils;

Alphabetically, VCSUtils should go before webkitdirs.

> +sub generateAttributesFromFile($);
> +sub readiExploderFile($);
> +sub writeiExploderFile($@);
> +sub update($$);
> +sub updateCSSProperties();
> +sub updateHTMLAttributes();
> +sub updateHTMLTags();

Alphabetically, writeiExploderFile() should go last in the list.

> +sub generateAttributesFromFile($)

"Attributes" sounds too specific.  How about "Entities"?

Also, we're returning a list of entities, so I think this should be called
generateEntityListFromFile().

>  {
> +    my ($filename) = @_;
> +
> +    my $revision = svnRevisionForDirectory(dirname($filename));
> +    my $path = File::Spec->abs2rel($filename, sourceDir());
>      my $result = "# From WebKit svn r" . $revision . " (" . $path . ")\n";
>  
>      my @properties = ();

This should change to @entities to match the subroutine name.  

>	   push(@properties, $l);

Ditto.

> +sub update($$)
> +{
> +    my ($iexploder, $webcore) = @_;

I think these should be called $iexploderPath and $webcorePath.

> +    $iexploder = File::Spec->catfile(sourceDir, split("/", $iexploder));
> +    $webcore = File::Spec->catfile(sourceDir, split("/", $webcore));

Please add the parenthesis back to "sourceDir" here.

> +sub updateCSSProperties()
>  {
> +    update("WebKitTools/iExploder/htdocs/cssproperties.in",
"WebCore/css/CSSPropertyNames.in");
> +}
> +
> +sub updateHTMLAttributes()
> +{
> +    update("WebKitTools/iExploder/htdocs/htmlattrs.in",
"WebCore/html/HTMLAttributeNames.in");
> +}
> +
> +sub updateHTMLTags()
> +{
> +    update("WebKitTools/iExploder/htdocs/htmltags.in",
"WebCore/html/HTMLTagNames.in");
>  }

I just realized you could make this more generic by using a hash to hold these
values, then just iterate over the hash in a single update() method.

If you'd rather keep separate methods, please add them before
writeiExploderFile() to keep the subroutines alphabetized.

r- to consider all the minor issues here.  (I'd like to be able to cq+ the
patch when finished. :)