|
Description
Julien Chaffraix
2009-01-25 14:31:15 PST
Created attachment 27027 [details]
Last bits of auto-generation get a working generated version
Created attachment 27028 [details]
Make the platform auto-generate the file
Created attachment 27029 [details]
Last part: remove the current files
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!
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 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!
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. 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 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 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. :)
(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. (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. Created attachment 27057 [details]
Last bits (again) - tweaked to remove some differences between generated and non-generated
Created attachment 27058 [details]
Latest autogenerated version
Created attachment 27059 [details]
Sorted non-generated HTMLElementFactory
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!
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).
Created attachment 27453 [details]
Reworked version that should address the review's comments
(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 Created attachment 27455 [details]
HTMLElementFactory's Diff of my previous patch
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.
Created attachment 27551 [details]
The same as previously without the functional change
Created attachment 27552 [details]
The diff associated with the latest patch
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 on attachment 27029 [details]
Last part: remove the current files
Reset the obsolete flag that I had set by mistake.
|