Bug 23536 – Auto-generate HTMLElementFactory

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Julien Chaffraix]

Patch forthcoming.

[Julien Chaffraix]

Created attachment 27027 [details]
Last bits of auto-generation get a working generated version

[Julien Chaffraix]

Created attachment 27028 [details]
Make the platform auto-generate the file

[Julien Chaffraix]

Created attachment 27029 [details]
Last part: remove the current files

[Eric Seidel (no email)]

Comment on attachment 27027 [details]
Last bits of auto-generation get a working generated version

Seems image should just pass another parameter.  MapToTagName="img" or something.

+    doc->setUsesBeforeAfterRules(true);

looks no worse than it was before, but is still not as elegant as it could be. You should add a FIXME that HTMLQuoteElements should set that flag on document insertion instead of construction.  I bet we could make a test where you create a Quote in one document, adopt it in another, and then before/after rules are not turned on for the document which adopted the quote!

[Julien Chaffraix]

Created attachment 27045 [details]
Last bits of autogeneration hackery - Updated with Eric's comment

> Seems image should just pass another parameter.  MapToTagName="img" or
> something.

I used a parameter switch here (I could not find a better name that mapToTagName so I used it).

> +    doc->setUsesBeforeAfterRules(true);
> 
> looks no worse than it was before, but is still not as elegant as it could be.
> You should add a FIXME that HTMLQuoteElements should set that flag on document
> insertion instead of construction.  I bet we could make a test where you create
> a Quote in one document, adopt it in another, and then before/after rules are
> not turned on for the document which adopted the quote!
> 

I agree with you: my goal was to get it out of HTMLElementFactory.cpp as we could miss some quote elements but the logic is wrong. I have added the requested FIXME.

[Eric Seidel (no email)]

Comment on attachment 27045 [details]
Last bits of autogeneration hackery - Updated with Eric's comment

Looks great!  Could you file a bug about <q> likely behaving wrong when moving between documents?  Thank you!

[Eric Seidel (no email)]

Could you post a difference between the old files and the new autogen files?  That would make this a no-brainer to review.  W/o it I have less confidence in my review.

[Julien Chaffraix]

Created attachment 27047 [details]
Diff between the generated and the new factory

(In reply to comment #7)
> Could you post a difference between the old files and the new autogen files? 
> That would make this a no-brainer to review.  W/o it I have less confidence in
> my review.
> 

Attached the difference.

There is one difference between the 2 that you will see: we generate a constructor even if we are using the default element (that is HTMLElement). I did not want to correct that as part of this patch (it does not change the behaviour): I will include it in another round of make_names.pl clean-up.

[Eric Seidel (no email)]

Comment on attachment 27028 [details]
Make the platform auto-generate the file

This looks fine to land as soon as we can better validate your differences from the existing HTMLElementFactory.cpp

It's unclear to me what magic is used to make the HTMLElementFactory generator not pass the tagName off to the element constructors and instead pass pre-determined (prefix-less) HTMLName values.  Is that a hack in the generator itself? or a special value set in the .in file?

[Eric Seidel (no email)]

Comment on attachment 27029 [details]
Last part: remove the current files

This is good to land as is.  We just still need to have some easy way of  verifying that the autogen files are similar enough to the old ones before landing. :)

[Julien Chaffraix]

(In reply to comment #9)
> (From update of attachment 27028 [details] [review])
> This looks fine to land as soon as we can better validate your differences from
> the existing HTMLElementFactory.cpp
> 
> It's unclear to me what magic is used to make the HTMLElementFactory generator
> not pass the tagName off to the element constructors and instead pass
> pre-determined (prefix-less) HTMLName values.  Is that a hack in the generator
> itself? or a special value set in the .in file?

Neither : all calls to the ElementFactory goes throught Document::createElement wrapper which re-apply the prefix after the element generation. It is currently required because some elements in HTMLElementFactory.cpp (or the generated ones) do not pass throught the QualifiedName.

[Julien Chaffraix]

(In reply to comment #10)
> (From update of attachment 27029 [details] [review])
> This is good to land as is.  We just still need to have some easy way of 
> verifying that the autogen files are similar enough to the old ones before
> landing. :)
> 

Actually I have tried to produce a nice diff but it seems that diff is confused by all the tiny changes and will report the 2 files intermixed even if they do not differ by much.
As you r+ under the assumption I would provide a way to check the differences, I will tweaked a bit the autogeneration part (and obsolete one of the patch you r+). I will also attach the 2 HTMLElementFactory ordered to ease the comparison. Opening them side-by-side in your favorite editor is the best way to see the differences.

[Julien Chaffraix]

Created attachment 27057 [details]
Last bits (again) - tweaked to remove some differences between generated and non-generated

[Julien Chaffraix]

Created attachment 27058 [details]
Latest autogenerated version

[Julien Chaffraix]

Created attachment 27059 [details]
Sorted non-generated HTMLElementFactory

[Eric Seidel (no email)]

Comment on attachment 27057 [details]
Last bits (again) - tweaked to remove some differences between generated and non-generated

This isn't ready yet.

Notice that image no longer uses the form element in your generated copy.  Your generated copy also doesn't support disabling video? (maybe it doesn't need to cause it generates w/o the video tags when video is off?)

I think the generated version should also use something like addTag for better readability of the generated output.

There are a couple returns missing from the typedef line for the constructor function at teh top of the line.

I would encourge you to fully sort the original file, and then generate yours side by side and compare again.

This is really close, but we need it to be perfect before we can replace the hand-rolled version or we'll have bugs!

[Julien Chaffraix]

First sorry for the long delay before replying.

> This isn't ready yet.

Oups, I have uploaded the wrong patch / generated version that had some issues you pointed out that I had already solved.

I have completely reworked the generation to match as closely as possible the current file (as you were suggesting). It should correct your review's comments.

The new patch may be a bit big but I will provide this time a diff between the HTMLElementFactory's (the variation in SVGElementFactory's are limited to a few constructor name changes).

[Julien Chaffraix]

Created attachment 27453 [details]
Reworked version that should address the review's comments

[Julien Chaffraix]

(In reply to comment #4)
> You should add a FIXME that HTMLQuoteElements should set that flag on document
> insertion instead of construction.  I bet we could make a test where you create
> a Quote in one document, adopt it in another, and then before/after rules are
> not turned on for the document which adopted the quote!

I have filed a follow-up bug about this issue: https://bugs.webkit.org/show_bug.cgi?id=23826

[Julien Chaffraix]

Created attachment 27455 [details]
HTMLElementFactory's Diff of my previous patch

[Julien Chaffraix]

Comment on attachment 27453 [details]
Reworked version that should address the review's comments

As discussed on IRC, this patch has a functional change (we now pass the tagName to the constructor which means that prefix is always set) that will need a test case. So to avoid this change, I will attach an updated version without it.

[Julien Chaffraix]

Created attachment 27551 [details]
The same as previously without the functional change

[Julien Chaffraix]

Created attachment 27552 [details]
The diff associated with the latest patch

[Eric Seidel (no email)]

Comment on attachment 27551 [details]
The same as previously without the functional change

Looks great!

I think you meant imageToImgConstructor to be imgToImageConstructor instead?

Land away!

[Julien Chaffraix]

Comment on attachment 27029 [details]
Last part: remove the current files

Reset the obsolete flag that I had set by mistake.

[Julien Chaffraix]

Landed the autogeneration patch in r40853, the platform part in 40865 and removed the HTMLElementFactory files in r40866.

Closing the bug.