Bug 39471 – Reduce copy/paste code in HTMLParser using some template functions

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

[Eric Seidel (no email)]

Reduce copy/paste code in HTMLParser using some template functions

[Eric Seidel (no email)]

Created attachment 56675 [details]
Patch

[Eric Seidel (no email)]

I used awk, sort, and some search-replace to write this patch, so I'm confident that it's functionally correct.  (I also ran all the layout tests.)

The real question in review is if this code is now clearer to folks.  If it is, great.  If not, I'm happy to chuck the patch.

The copy/paste code bothered me when looking at how much of HTMLParser could be re-used by HTML5Parser, so I went through and did this cleanup.

[Adam Barth]

Comment on attachment 56675 [details]
Patch

Ok.  I'm not 100% convinced this is better, but it looks ok.

[Eric Seidel (no email)]

Comment on attachment 56675 [details]
Patch

I'm certainly interested in any feedback from others.  This will sit in the cq for a while I'm sure... :)

[Eric Seidel (no email)]

Btw, I got hyatt's feedback in the channel.  He said something to the effect of it "looked slightly better"  (that is not a direct quote).

[James Robinson]

Comment on attachment 56675 [details]
Patch

I think it's clearer.  A few nits, though (I'm not a reviewer so I won't touch the review flag).

> +template< size_t ArraySize >
> +static void addTags(TagNameSet& set, QualifiedName (&names)[ArraySize])
Can we get a comment with an explanation of this syntax or a link or something?  I just looked at this so I know how to read the second declaration but by next week I will probably forget and have to spend a while pondering what the parens signify here.
> +{
> +    for (size_t x = 0; x < ArraySize; x++) {
x is an odd choice for a loop index. Why not i?

> +        const QualifiedName& name = names[x];
I don't think the temp var here adds very much
> +        set.add(name.localName().impl());
> +    }
>  
> +static void mapTagToFunc(FunctionMap& map, const QualifiedName& tag, CreateErrorCheckFunc func)
> +{
> +    map.set(tag.localName().impl(), func);
> +}
> +
> +template< size_t ArraySize >
> +static void mapTagsToFunc(FunctionMap& map, QualifiedName (&names)[ArraySize], CreateErrorCheckFunc func)
> +{
> +    for (size_t x = 0; x < ArraySize; x++) {
> +        const QualifiedName& name = names[x];
Same comments as above (x is unusual for a loop index, temp is of iffy value).
> +        mapTagToFunc(map, name, func);
> +    }
> +}
> +
>  PassRefPtr<Node> HTMLParser::getNode(Token* t)
>  {
>      // Init our error handling table.

[WebKit Commit Bot]

Comment on attachment 56675 [details]
Patch

Clearing flags on attachment: 56675

Committed r60031: <http://trac.webkit.org/changeset/60031>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.