Bug 23536 - Auto-generate HTMLElementFactory
Summary: Auto-generate HTMLElementFactory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-25 14:31 PST by Julien Chaffraix
Modified: 2009-02-11 13:18 PST (History)
1 user (show)

See Also:


Attachments
Last bits of auto-generation get a working generated version (4.67 KB, patch)
2009-01-25 14:35 PST, Julien Chaffraix
eric: review-
Details | Formatted Diff | Diff
Make the platform auto-generate the file (16.94 KB, patch)
2009-01-25 14:39 PST, Julien Chaffraix
eric: review+
Details | Formatted Diff | Diff
Last part: remove the current files (20.08 KB, patch)
2009-01-25 14:43 PST, Julien Chaffraix
eric: review+
Details | Formatted Diff | Diff
Last bits of autogeneration hackery - Updated with Eric's comment (5.90 KB, patch)
2009-01-26 14:05 PST, Julien Chaffraix
eric: review+
Details | Formatted Diff | Diff
Diff between the generated and the new factory (44.87 KB, text/plain)
2009-01-26 14:22 PST, Julien Chaffraix
no flags Details
Last bits (again) - tweaked to remove some differences between generated and non-generated (7.80 KB, patch)
2009-01-26 17:37 PST, Julien Chaffraix
eric: review-
Details | Formatted Diff | Diff
Latest autogenerated version (21.55 KB, text/plain)
2009-01-26 17:40 PST, Julien Chaffraix
no flags Details
Sorted non-generated HTMLElementFactory (16.91 KB, text/plain)
2009-01-26 17:41 PST, Julien Chaffraix
no flags Details
Reworked version that should address the review's comments (11.69 KB, patch)
2009-02-07 15:57 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
HTMLElementFactory's Diff of my previous patch (29.29 KB, text/plain)
2009-02-07 16:09 PST, Julien Chaffraix
no flags Details
The same as previously without the functional change (12.52 KB, patch)
2009-02-10 21:53 PST, Julien Chaffraix
eric: review+
Details | Formatted Diff | Diff
The diff associated with the latest patch (26.95 KB, patch)
2009-02-10 21:56 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2009-01-25 14:31:15 PST
Patch forthcoming.
Comment 1 Julien Chaffraix 2009-01-25 14:35:09 PST
Created attachment 27027 [details]
Last bits of auto-generation get a working generated version
Comment 2 Julien Chaffraix 2009-01-25 14:39:21 PST
Created attachment 27028 [details]
Make the platform auto-generate the file
Comment 3 Julien Chaffraix 2009-01-25 14:43:02 PST
Created attachment 27029 [details]
Last part: remove the current files
Comment 4 Eric Seidel (no email) 2009-01-26 12:33:17 PST
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!
Comment 5 Julien Chaffraix 2009-01-26 14:05:41 PST
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.
Comment 6 Eric Seidel (no email) 2009-01-26 14:09:43 PST
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!
Comment 7 Eric Seidel (no email) 2009-01-26 14:10:40 PST
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.
Comment 8 Julien Chaffraix 2009-01-26 14:22:36 PST
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.
Comment 9 Eric Seidel (no email) 2009-01-26 15:14:44 PST
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?
Comment 10 Eric Seidel (no email) 2009-01-26 15:20:22 PST
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. :)
Comment 11 Julien Chaffraix 2009-01-26 15:58:22 PST
(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.
Comment 12 Julien Chaffraix 2009-01-26 16:58:30 PST
(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.
Comment 13 Julien Chaffraix 2009-01-26 17:37:59 PST
Created attachment 27057 [details]
Last bits (again) - tweaked to remove some differences between generated and non-generated
Comment 14 Julien Chaffraix 2009-01-26 17:40:56 PST
Created attachment 27058 [details]
Latest autogenerated version
Comment 15 Julien Chaffraix 2009-01-26 17:41:55 PST
Created attachment 27059 [details]
Sorted non-generated HTMLElementFactory
Comment 16 Eric Seidel (no email) 2009-01-29 11:18:50 PST
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!
Comment 17 Julien Chaffraix 2009-02-07 15:54:07 PST
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).
Comment 18 Julien Chaffraix 2009-02-07 15:57:11 PST
Created attachment 27453 [details]
Reworked version that should address the review's comments
Comment 19 Julien Chaffraix 2009-02-07 16:05:16 PST
(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
Comment 20 Julien Chaffraix 2009-02-07 16:09:32 PST
Created attachment 27455 [details]
HTMLElementFactory's Diff of my previous patch
Comment 21 Julien Chaffraix 2009-02-10 21:48:24 PST
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.
Comment 22 Julien Chaffraix 2009-02-10 21:53:25 PST
Created attachment 27551 [details]
The same as previously without the functional change
Comment 23 Julien Chaffraix 2009-02-10 21:56:11 PST
Created attachment 27552 [details]
The diff associated with the latest patch
Comment 24 Eric Seidel (no email) 2009-02-10 22:02:04 PST
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!
Comment 25 Julien Chaffraix 2009-02-11 13:16:15 PST
Comment on attachment 27029 [details]
Last part: remove the current files

Reset the obsolete flag that I had set by mistake.
Comment 26 Julien Chaffraix 2009-02-11 13:18:35 PST
Landed the autogeneration patch in r40853, the platform part in 40865 and removed the HTMLElementFactory files in r40866.

Closing the bug.