Bug 33755 – Make update-iexploder-cssproperties update htmltags.in and htmlattrs.in as well

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

[Holger Freyther]

Created attachment 46732 [details]
Patch

In recent years we have added a couple of new attributes and tags and with HTML5
being there we will add some more tags as well.
The following patch is fixing it and as a follow up patch we should rename
the script to update-iexploder or update-iexploder-properties or some     
better name.

[David Kilzer (:ddkilzer)]

Comment on attachment 46732 [details]
Patch

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> [phone]  Holger Hans Peter Freyther  <[e-mail]>
> +
> +        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. :)

[Holger Freyther]

Created attachment 47098 [details]
[iexploder] Automatically update htmltags.in and htmlattrs.in too

[Holger Freyther]

(In reply to comment #1)

I should have addressed all comments form above (sorting, naming, bug id).


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

I'm not sure if I follow here? We are writing three different files so the hash would be the input to the method?



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

I nuked them. And I also append the path for htmldocs and WebCore inside the update method.


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

I'm fine to land the patch myself. :)



As follow up commits we should rename the file, and I'm working on a commit to automatically append deleted entities to the end of the file.

[Eric Seidel (no email)]

Attachment 47098 [details] was posted by a committer and has review+, assigning to Holger Freyther for commit.

[Holger Freyther]

Landed in r55658. The htmlattrs.in needs another now.