[Webkit-unassigned] [Bug 22518] Element subclasses need only pass an optional prefix in their constructor

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=22518





------- Comment #5 from eric at webkit.org  2008-11-26 20:32 PDT -------
(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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.