Bug 22441 – Bridge the gap between the generated ElementFactory and HTMLElementFactory

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Julien Chaffraix]

There is a few differences between them that needs to be removed to autogenerate HTMLElementFactory.

[Julien Chaffraix]

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

[Darin Adler]

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

r=me

[Julien Chaffraix]

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

[Darin Adler]

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

[Julien Chaffraix]

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

First part landed in r38714.

[Julien Chaffraix]

(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!

[Darin Adler]

(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.

[Julien Chaffraix]

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.

[Darin Adler]

(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!

[Darin Adler]

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

[Julien Chaffraix]

(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.

[Julien Chaffraix]

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

Landed in r38754.

[Julien Chaffraix]

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).

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Julien Chaffraix]

(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.

[Julien Chaffraix]

Comment on attachment 25506 [details]
Third part: make some HTML elements take a QualifiedName

Committed the 3rd part in r38769.

[Julien Chaffraix]

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

[Eric Seidel (no email)]

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

Fantastic!

[Julien Chaffraix]

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

Landed forth part in r38791.

[Antti Koivisto]

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?

[Eric Seidel (no email)]

(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.

[Antti Koivisto]

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.

[Antti Koivisto]

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.

[Antti Koivisto]

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.

[Julien Chaffraix]

(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.

[Julien Chaffraix]

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).

[Julien Chaffraix]

Landed the last patch in r38878.