Bug 22441

Created attachment 25403 [details] First part: make HTMLElementFactory use QualifiedName

Comment on attachment 25403 [details] First part: make HTMLElementFactory use QualifiedName r=me

Created attachment 25404 [details] Second part: make the ElementFactory use PassRefPtr

Comment on attachment 25404 [details] Second part: make the ElementFactory use PassRefPtr > +typedef PassRefPtr<$parameters{'namespace'}Element> (*ConstructorFunc)(Document* doc, bool createdByParser); There should not be a space between the ">" and the "(" symbols. (These issues already existed.) The Document* argument should not have the name "doc" -- the name should just be omitted. And the type should be named ConstructorFunction, not ConstructorFunc. There's no value in abbreviating the type name. > +#include "config.h" Headers should not include the "config.h" header; that's to be included by cpp files only. > +#ifndef $parameters{'namespace'}ElementFactory_h > +#define $parameters{'namespace'}ElementFactory_h The #ifndef part should go before the includes, not after them. > class $parameters{'namespace'}ElementFactory > { (This issue already existed.) The brace should be on the same line as the class name. > public: > - WebCore::Element* createElement(const WebCore::QualifiedName& qName, WebCore::Document* doc, bool createdByParser = true); > - static $parameters{'namespace'}Element* create$parameters{'namespace'}Element(const WebCore::QualifiedName& qName, WebCore::Document* doc, bool createdByParser = true); > + PassRefPtr<Element> createElement(const WebCore::QualifiedName& qName, WebCore::Document* doc, bool createdByParser = true); > + static PassRefPtr<$parameters{'namespace'}Element> create$parameters{'namespace'}Element(const WebCore::QualifiedName& qName, WebCore::Document* doc, bool createdByParser = true); (This issue already existed.) The argument names qName and doc should be omitted. I don't see the change to the createElement definition. review- because of the "cofig.h" issue

Comment on attachment 25403 [details] First part: make HTMLElementFactory use QualifiedName First part landed in r38714.

(In reply to comment #4) > (From update of attachment 25404 [details] [review]) > > +typedef PassRefPtr<$parameters{'namespace'}Element> (*ConstructorFunc)(Document* doc, bool createdByParser); > > There should not be a space between the ">" and the "(" symbols. I am not sure about this one: we are putting a space between the type and the name in other typedef's like in the FrameLoader.h (http://trac.webkit.org/browser/trunk/WebCore/loader/FrameLoader.h#L96). So I would prefer being conservative and matching the existing implicit coding style. > > Headers should not include the "config.h" header; that's to be included by cpp > files only. I have to disagree here. Our coding style states that all files should include "config.h" first. > I don't see the change to the createElement definition. I have removed it at the last minute as it was not really needed and forgot to update the ChangeLog. The other issues you mentioned will be addressed in a later version and should have been part of my clean-up from the beginning. Thanks for pointing them out!

(In reply to comment #6) > (In reply to comment #4) > > (From update of attachment 25404 [details] [review] [review]) > > > +typedef PassRefPtr<$parameters{'namespace'}Element> (*ConstructorFunc)(Document* doc, bool createdByParser); > > > > There should not be a space between the ">" and the "(" symbols. > > I am not sure about this one: we are putting a space between the type and the > name in other typedef's like in the FrameLoader.h > (http://trac.webkit.org/browser/trunk/WebCore/loader/FrameLoader.h#L96). So I > would prefer being conservative and matching the existing implicit coding > style. Oops, my mistake. There *should* be a space here. Sorry, I was wrong. > > Headers should not include the "config.h" header; that's to be included by cpp > > files only. > > I have to disagree here. Our coding style states that all files should include > "config.h" first. Yes, I see that's what the coding style document says, but it's wrong. All implementation files should include "config.h" first; header files should never include "config.h". If you look at the actual code checked in to the tree see that despite what the coding style document says, there is no ".h" file that includes "config.h" except for these 5 files that have it wrong: JavaScriptCore/runtime/CollectorHeapIterator.h JavaScriptCore/wtf/unicode/qt4/UnicodeQt4.h WebCore/svg/Filter.h WebCore/svg/FilterBuilder.h WebKit/wx/WebViewPrivate.h These are all wrong and the patch should not be landed this way. We've discussed this many times in the past. But until now I didn't realize that <http://webkit.org/coding/coding-style.html> had this wrong.

Created attachment 25453 [details] Second Part - V2: Updated with Darin's comments > These are all wrong and the patch should not be landed this way. We've > discussed this many times in the past. But until now I didn't realize that > <http://webkit.org/coding/coding-style.html> had this wrong. That's quite bad as new people follow those instructions when they contribute new code and they do not know the previous discussions. I have filed bug 22468 about this flaw in the documentation.

(In reply to comment #8) > That's quite bad as new people follow those instructions when they contribute > new code and they do not know the previous discussions. I have filed bug 22468 > about this flaw in the documentation. Absolutely. Lets fix it!

Comment on attachment 25453 [details] Second Part - V2: Updated with Darin's comments > - WebCore::Element* createElement(const WebCore::QualifiedName& qName, WebCore::Document* doc, bool createdByParser = true); > - static $parameters{'namespace'}Element* create$parameters{'namespace'}Element(const WebCore::QualifiedName& qName, WebCore::Document* doc, bool createdByParser = true); > + PassRefPtr<Element> createElement(const WebCore::QualifiedName&, WebCore::Document*, bool createdByParser = true); > + static PassRefPtr<$parameters{'namespace'}Element> create$parameters{'namespace'}Element(const WebCore::QualifiedName&, WebCore::Document*, bool createdByParser = true); I don't understand how you can change the declaration of the createElement function here without also changing the return type in the definition of createElement. I guess that's just a placeholder for the future. r=me

(In reply to comment #10) > (From update of attachment 25453 [details] [review]) > > - WebCore::Element* createElement(const WebCore::QualifiedName& qName, WebCore::Document* doc, bool createdByParser = true); > > - static $parameters{'namespace'}Element* create$parameters{'namespace'}Element(const WebCore::QualifiedName& qName, WebCore::Document* doc, bool createdByParser = true); > > + PassRefPtr<Element> createElement(const WebCore::QualifiedName&, WebCore::Document*, bool createdByParser = true); > > + static PassRefPtr<$parameters{'namespace'}Element> create$parameters{'namespace'}Element(const WebCore::QualifiedName&, WebCore::Document*, bool createdByParser = true); > > I don't understand how you can change the declaration of the createElement > function here without also changing the return type in the definition of > createElement. I guess that's just a placeholder for the future. FYI, createElement is never implemented and is indeed a placeholder related to a comment in make_names.pl (http://trac.webkit.org/browser/trunk/WebCore/dom/make_names.pl#L633). I have weighted about removing it from both make_names.pl and HTMLElementFactory but I liked the idea of leaving this FIXME.

Comment on attachment 25453 [details] Second Part - V2: Updated with Darin's comments Landed in r38754.

Created attachment 25506 [details] Third part: make some HTML elements take a QualifiedName This patch changes only the first 5 on purpose. They will go into the next one or into this one based on reviewer's comment (I am fine with both).

Comment on attachment 25506 [details] Third part: make some HTML elements take a QualifiedName It's possible with this change to actually fix bugs in our XHTML handling, but you seem to have chosen not to. :) I think it's find to land as-is, but you should plan to fix (and test) the real bug with a later change. The bug: <foo:html xmlns:foo="http://www.w3c.org/1999/xhtml"> <script> alert(document.getElementsByTagName("html")[0].prefix) you get "null", when it should be "foo" It's probably also possible to repro the bug with: document.createElementNS("foo:html", "http://www.w3c.org/1999/xhtml") That would need to be done on an XHTML doc, as I expect that HTML docs ignore prefixes. There may already even be a test case testing this. I look forward to seeing the rest of the patches.

Comment on attachment 25506 [details] Third part: make some HTML elements take a QualifiedName To clarify, if you were to fix the bug: static PassRefPtr<HTMLElement> htmlConstructor(const QualifiedName&, Document* doc, HTMLFormElement*, bool) 9797 { 98 return new HTMLHtmlElement(doc); 98 return new HTMLHtmlElement(htmlTag, doc); you would give the qName argument a name, and pass it through, instead of using htmlTag there.

(In reply to comment #15) > (From update of attachment 25506 [details] [review]) > To clarify, if you were to fix the bug: > > static PassRefPtr<HTMLElement> htmlConstructor(const QualifiedName&, Document* > doc, HTMLFormElement*, bool) > 9797 { > 98 return new HTMLHtmlElement(doc); > 98 return new HTMLHtmlElement(htmlTag, doc); > > you would give the qName argument a name, and pass it through, instead of using > htmlTag there. > Yes, there is a comment about that in Document::createElement. It would be good to solve it but it is not the purpose of this bug so I will stick to been simple in the fixes until the ElementFactory have been all been merged. As a side note, I am not sure the ElementFactory would survive this change for the moment. Thanks for the review and the reminder.

Comment on attachment 25506 [details] Third part: make some HTML elements take a QualifiedName Committed the 3rd part in r38769.

Created attachment 25539 [details] Fourth part: move more html elements' constructors to use a QualifiedName

Comment on attachment 25539 [details] Fourth part: move more html elements' constructors to use a QualifiedName Fantastic!

Comment on attachment 25539 [details] Fourth part: move more html elements' constructors to use a QualifiedName Landed forth part in r38791.

I don't agree with adding qname parameter to every element. There is no reason to ever construct HTMLHtmlElement if other tag name than <html>. We lose self-documenting nature of code like this: RefPtr<HTMLTableCaptionElement> caption = new HTMLTableCaptionElement(document()); vs. RefPtr<HTMLTableSectionElement> newBody = new HTMLTableSectionElement(tbodyTag, document()); where we can immeditealy see that HTMLTableSectionElement can represent multiple tags while HTMLTableCaptionElement can not. Surely you it would be better to make your autogeneration script slightly smarter rather than losing this architectural feature?

(In reply to comment #21) > I don't agree with adding qname parameter to every element. There is no reason > to ever construct HTMLHtmlElement if other tag name than <html>. We lose > self-documenting nature of code like this: > > RefPtr<HTMLTableCaptionElement> caption = new > HTMLTableCaptionElement(document()); > > vs. > > RefPtr<HTMLTableSectionElement> newBody = new > HTMLTableSectionElement(tbodyTag, document()); > > where we can immeditealy see that HTMLTableSectionElement can represent > multiple tags while HTMLTableCaptionElement can not. > > Surely you it would be better to make your autogeneration script slightly > smarter rather than losing this architectural feature? > Moving this discussion to bug 22518. May this bug RIP. :) If you feel strongly, we certainly can roll out these changes and put them back up for review. Of coruse, just because they've landed doesn't mean they can't be rolled out if that's necessary.

Even more important self-documenting feature you lose is that currently you can look at any HTML*Element constructor and see which tag it corresponds to, or in (rare) cases where there is a qname parameter you know that it can represent multiple tags. I think this is valuable. Yes, I would prefer to have the last two patches rolled back and the adjustments for different constructors done in the autogeneration level. That will make autogeneration code more self-documenting too. Reopening for that. Possible need for prefix parameter (which I don't fully understand) is a separate issue from this bug.

After futher thinking and reading Eric's point about prefixes I think this approach is acceptable. However I don't want to lose information of make the code more bug prone so I'd like to see a patch that adds ASSERT(hasTagName(fooTag)) too all constructors where the qname parameter was added.

The awkward HTMLHtmlElement(htmlTag, doc) syntax is easier to clean up when DOM is switched to new style refcounting using static create() like RefCounted subclasses use.

(In reply to comment #25) > The awkward HTMLHtmlElement(htmlTag, doc) syntax is easier to clean up when DOM > is switched to new style refcounting using static create() like RefCounted > subclasses use. > Filed bug22563 about the awkward syntax.

Created attachment 25608 [details] Add the ASSERT to the constructors changed by this bug Antti, the attached patch should tackle your comment. This bug is getting pretty cluttered so it will be closed after this patch (unless other issues remains about the HTML elements' changes). I have tried to file the other issues as separate bugs to keep track of them (feel free to open others that I may have missed).

Landed the last patch in r38878.