Bug 33755 - Make update-iexploder-cssproperties update htmltags.in and htmlattrs.in as well
Summary: Make update-iexploder-cssproperties update htmltags.in and htmlattrs.in as well
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Holger Freyther
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-15 21:25 PST by Holger Freyther
Modified: 2010-03-08 00:17 PST (History)
2 users (show)

See Also:


Attachments
Patch (5.81 KB, patch)
2010-01-15 21:25 PST, Holger Freyther
no flags Details | Formatted Diff | Diff
[iexploder] Automatically update htmltags.in and htmlattrs.in too (5.54 KB, patch)
2010-01-20 21:32 PST, Holger Freyther
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Freyther 2010-01-15 21:25:52 PST
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.
Comment 1 David Kilzer (:ddkilzer) 2010-01-16 07:29:31 PST
Comment on attachment 46732 [details]
Patch

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> +2010-01-15  Holger Hans Peter Freyther  <zecke@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. :)
Comment 2 Holger Freyther 2010-01-20 21:32:11 PST
Created attachment 47098 [details]
[iexploder] Automatically update htmltags.in and htmlattrs.in too
Comment 3 Holger Freyther 2010-01-20 21:37:50 PST
(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.
Comment 4 Eric Seidel (no email) 2010-02-01 16:12:11 PST
Attachment 47098 [details] was posted by a committer and has review+, assigning to Holger Freyther for commit.
Comment 5 Holger Freyther 2010-03-08 00:17:23 PST
Landed in r55658. The htmlattrs.in needs another now.