Bug 22518 - Element subclasses need only pass an optional prefix in their constructor
Summary: Element subclasses need only pass an optional prefix in their constructor
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-26 16:08 PST by Eric Seidel (no email)
Modified: 2008-11-26 20:32 PST (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-11-26 16:08:51 PST
HTMLElement subclasses need only pass an optional prefix in their constructor

The same applies to SVGElement subclasses too.  Currently the constructor signature for most classes is:

  SVGSVGElement(const QualifiedName&, Document*);
  HTMLHtmlElement(const QualifiedName&, Document*);

But they really should be more like:

SVGSVGElement(Document*, AtomicString prefix = nullAtom);
HTMLHtmlElement(Document*, , AtomicString prefix = nullAtom);

non-leafnode Element subclasses will still require a full Qualified Name (At least a localname and prefix pair).

I'm personally ambivalent as to if the tradeoff changing from QualifiedName to AtomicString is worth it (Since i will make the method signatures for all Element subclasses less uniform.  But others in the project feel stronger about it, and so I'm filing this bug for proper discussion and eventual code change.)  :)
Comment 1 Eric Seidel (no email) 2008-11-26 16:09:38 PST
This is related to bug 22441.
Comment 2 Eric Seidel (no email) 2008-11-26 16:17:49 PST
Moving discussion from bug 22441 comment 21

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

There is also a (rather un-important) bug in the current code that code like this:

<foo:html id="test" xmlns:foo="http://www.w3c.org/1999/xhtml">
document.getElementById("test").prefix; // is null when it should be "foo"

The autogen scripts certainly could be made "smarter" to not pass the prefix when not needed (or not to pass it for HTML where it is not universally supported).

The "common case" for creating elements is via the ElementFactory, direct "new" creation of Element subclasses is only done in a few places where we handle places where inserting one HTMl element actually causes several to end up in the tree.  We could of course use document->createElement(tBodyTag) in those cases, which would execute more code, but might be even cleaner still (certainly safer, if we ever decide to move HTMLElement/TreeShared subclasses away from allowing direct "new" like we have for RefCounted subclasses.)
Comment 3 Julien Chaffraix 2008-11-26 17:01:28 PST
> The same applies to SVGElement subclasses too.  Currently the constructor
> signature for most classes is:
> 
>   SVGSVGElement(const QualifiedName&, Document*);
>   HTMLHtmlElement(const QualifiedName&, Document*);
> 
> But they really should be more like:
> 
> SVGSVGElement(Document*, AtomicString prefix = nullAtom);
> HTMLHtmlElement(Document*, , AtomicString prefix = nullAtom);

I do agree with you that we could be giving only the prefix but I see some drawbacks. First the current signatures are close to the Element constructor; apart from the niceness of having a close interface for classes derived from Element, we would introduce unneeded object creations (take the Document::createElement(const QualifiedName&) that would need to extract the prefix only to create another QualifiedName matching the original one for Element constructor).


> But others in the project feel stronger about
> it, and so I'm filing this bug for proper discussion and eventual code change.)
>  :)
> 

Thanks for raising this point I had overlooked.
Comment 4 Antti Koivisto 2008-11-26 18:00:41 PST
Two QualifiedNames are the same if all components including prefix are equal. We have massive amounts of code that does things like e->hasTagName(htmlTag) which reduces to simple pointer equality comparison and would break if prefix differs from null. I think fixing this would be wider change than just adding a parameter. This needs to be thought through before checking in anything.
Comment 5 Eric Seidel (no email) 2008-11-26 20:32:44 PST
(In reply to comment #4)
> Two QualifiedNames are the same if all components including prefix are equal.
> We have massive amounts of code that does things like e->hasTagName(htmlTag)
> which reduces to simple pointer equality comparison and would break if prefix
> differs from null. I think fixing this would be wider change than just adding a
> parameter. This needs to be thought through before checking in anything.

Agreed.  We have both conflated two points here.

1.  The goal was to make HTMLElementFactory auto-generated to replace the use of manual (and possibly buggy) code, with the same auto-generated code as we use for all other Element subclasses in WebCore.  Replacing HTMLElementFactory with autogen code does not require fixing the second issue (below) but this is part of why Element* subclasses all take a QualifiedName in their constructor.

2.  There is an issue in xhtml where prefixes are not respected.  The autogenerated code would respect these prefixes, but due to various issues (one of them being that the HTMLElement subclasses do not take a QualifiedName), it is not possible to replace the manual code with the autogen code yet.  Replacing HTMLElementFactory with autogen code does not require fixing the xhtml prefix bug, it's just one of the fortunate side-effects of doing so.

Your statement 

> We have massive amounts of code that does things like e->hasTagName(htmlTag)
> which reduces to simple pointer equality comparison and would break if prefix
> differs from null.

Isn't actually true:

    bool hasTagName(const QualifiedName& tagName) const { return m_tagName.matches(tagName); }

    bool matches(const QualifiedName& other) const { return m_impl == other.m_impl || (localName() == other.localName() && namespaceURI() == other.namespaceURI()); }

Not that two qualified names are only equal if they are the same pointer (common case) or if their localNames and namespaceURIs are identical (not including prefix).

Thus, e->hasTagName(htmlTag) will function as it always has.  e->qName() == htmlTag will change behavior however (this is much less common in usage however).

There are some places in our code where we don't use .matches() and instead us == which *will* change behavior for XHTML elements if they start getting prefixes.  This will not however effect HTML code, as HTML elements will still not have prefixes because the HTMLTokenizer discards them. :)  Thus fixing the prefix discarding problem for xhtml (which would be an optional side-effect of autogeneration) may introduce other bugs for XHTML documents, but *would not* effect HTML documents.  I do not expect it would introduce static_cast related crashes, as generally you check *for* a tag match, not for a tag non-match before static casting, and the worst that could happen in an XHTML case with fixed prefixes is that tags will fail to match where they would have before.

Your primary complaint remains one of style/self-documenting-code/excessive-constructor-flexibility.  I share your complaint, but we differ on our views of the severity.

As I noted in the channel, all SVGElement subclasses have forever taken a QualifiedName as an argument to their constructor.  These constructors are basically only ever called from the autogenerated code, so it's not been a big code-cleanliness issue.  For HTML the story is slightly different since we manually construct HTML elements frequently throughout our code.