[Webkit-unassigned] [Bug 33755] Make update-iexploder-cssproperties update htmltags.in and htmlattrs.in as well

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33755


David Kilzer (ddkilzer) <ddkilzer at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #46732|review?                     |review-
               Flag|                            |




--- Comment #1 from David Kilzer (ddkilzer) <ddkilzer at webkit.org>  2010-01-16 07:29:31 PST ---
(From update of attachment 46732)
> 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. :)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.