Bug 22441 - Bridge the gap between the generated ElementFactory and HTMLElementFactory
Summary: Bridge the gap between the generated ElementFactory and HTMLElementFactory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-23 16:11 PST by Julien Chaffraix
Modified: 2008-12-01 15:07 PST (History)
3 users (show)

See Also:


Attachments
First part: make HTMLElementFactory use QualifiedName (26.59 KB, patch)
2008-11-23 16:17 PST, Julien Chaffraix
darin: review+
Details | Formatted Diff | Diff
Second part: make the ElementFactory use PassRefPtr (3.56 KB, patch)
2008-11-23 16:27 PST, Julien Chaffraix
darin: review-
Details | Formatted Diff | Diff
Second Part - V2: Updated with Darin's comments (4.35 KB, patch)
2008-11-24 14:52 PST, Julien Chaffraix
darin: review+
Details | Formatted Diff | Diff
Third part: make some HTML elements take a QualifiedName (14.38 KB, patch)
2008-11-25 14:42 PST, Julien Chaffraix
eric: review+
Details | Formatted Diff | Diff
Fourth part: move more html elements' constructors to use a QualifiedName (51.43 KB, patch)
2008-11-26 14:40 PST, Julien Chaffraix
eric: review+
Details | Formatted Diff | Diff
Add the ASSERT to the constructors changed by this bug (19.74 KB, patch)
2008-11-30 04:46 PST, Julien Chaffraix
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2008-11-23 16:11:53 PST
There is a few differences between them that needs to be removed to autogenerate HTMLElementFactory.
Comment 1 Julien Chaffraix 2008-11-23 16:17:03 PST
Created attachment 25403 [details]
First part: make HTMLElementFactory use QualifiedName
Comment 2 Darin Adler 2008-11-23 16:20:02 PST
Comment on attachment 25403 [details]
First part: make HTMLElementFactory use QualifiedName

r=me
Comment 3 Julien Chaffraix 2008-11-23 16:27:02 PST
Created attachment 25404 [details]
Second part: make the ElementFactory use PassRefPtr
Comment 4 Darin Adler 2008-11-23 19:12:09 PST
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 5 Julien Chaffraix 2008-11-24 11:30:33 PST
Comment on attachment 25403 [details]
First part: make HTMLElementFactory use QualifiedName

First part landed in r38714.
Comment 6 Julien Chaffraix 2008-11-24 11:44:11 PST
(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!
Comment 7 Darin Adler 2008-11-24 11:56:12 PST
(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.
Comment 8 Julien Chaffraix 2008-11-24 14:52:22 PST
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.
Comment 9 Darin Adler 2008-11-24 14:58:49 PST
(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 10 Darin Adler 2008-11-24 15:00:32 PST
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
Comment 11 Julien Chaffraix 2008-11-24 15:34:02 PST
(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 12 Julien Chaffraix 2008-11-25 11:22:21 PST
Comment on attachment 25453 [details]
Second Part - V2: Updated with Darin's comments

Landed in r38754.
Comment 13 Julien Chaffraix 2008-11-25 14:42:25 PST
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 14 Eric Seidel (no email) 2008-11-25 15:53:23 PST
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 15 Eric Seidel (no email) 2008-11-25 15:54:11 PST
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.
Comment 16 Julien Chaffraix 2008-11-25 16:07:01 PST
(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 17 Julien Chaffraix 2008-11-25 16:34:53 PST
Comment on attachment 25506 [details]
Third part: make some HTML elements take a QualifiedName

Committed the 3rd part in r38769.
Comment 18 Julien Chaffraix 2008-11-26 14:40:25 PST
Created attachment 25539 [details]
Fourth part: move more html elements' constructors to use a QualifiedName
Comment 19 Eric Seidel (no email) 2008-11-26 14:55:37 PST
Comment on attachment 25539 [details]
Fourth part: move more html elements' constructors to use a QualifiedName

Fantastic!
Comment 20 Julien Chaffraix 2008-11-26 15:08:27 PST
Comment on attachment 25539 [details]
Fourth part: move more html elements' constructors to use a QualifiedName

Landed forth part in r38791.
Comment 21 Antti Koivisto 2008-11-26 16:06:49 PST
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?
Comment 22 Eric Seidel (no email) 2008-11-26 16:19:18 PST
(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.
Comment 23 Antti Koivisto 2008-11-26 17:05:29 PST
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.
Comment 24 Antti Koivisto 2008-11-27 16:55:16 PST
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.
Comment 25 Antti Koivisto 2008-11-27 16:57:05 PST
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.
Comment 26 Julien Chaffraix 2008-11-30 04:35:34 PST
(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.
Comment 27 Julien Chaffraix 2008-11-30 04:46:44 PST
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).
Comment 28 Julien Chaffraix 2008-12-01 15:07:52 PST
Landed the last patch in r38878.