Bug 5578 – WebKit does not support DOM Level 3 setIsId and isId

• Component: DOM
• Status: RESOLVED WONTFIX

[Eric Seidel (no email)]

WebKit does not support DOM Level 3 setIsId and isId

kdom supports this, and we would like to eventually pick it up (ksvg2 already has the proper hooks).  The 
problem would be performance.  I'm not sure that adding a bool to AttrImpl is the best way, it seems a 
rather expensive route.

[Eric Seidel (no email)]

http:://bugzilla.opendarwin.org/show_bug.cgi?id=3249 covers the KSVG2 kdom -> webcore dom merger.

[Sam Weinig]

One possibility might be to use the m_hasId flag from the node base as the flag, since it is worthless for 
attributes as is.  This, however, would require the AttrImpl to be allocated for each Id attribute.

Another worthwhile question relating to this is whether an Element can have more than one Id attribute, 
and if so, what to do about NamedAttrMapImpl's id() and setID() methods.

[Alexey Proskuryakov]

*** Bug 30715 has been marked as a duplicate of this bug. ***

[Chang Shu]

I come up with half-baked code and hope the reviewers help me get it matured. Thanks very much in advance.
Here is the proposal of the code change:
1. introduce isId property in Attr, as specified in spec;
2. introduce setIdAttribute, setIdAttributeNS, setIdAttributeNode in Element, as specified in spec.
3. introduce an AtomicString type "m_IdAttributeName" member variable in Element. It remembers the name of the id attrribute. Its default value is "id" and can be set differently by the setIdAttribute functions.
4. Attr::isId check if its localname matches the m_IdAttributeName in owner element.

The thought of above proposal is to avoid using a bool flag in every Attr to save memory space.

Some excerpt of the code is pasted below:
bool Attr::isId() const
{
    if (!m_element)
        return false;

    return m_attribute->localName() == m_element->getIDAttributeName();
}
const AtomicString& Element::getIDAttributeName() const
{
    return m_IdAttributeName;
}
void setIdAttribute(const QualifiedName& qname, bool isId, ExceptionCode&)
{
    if (isId)
        m_IdAttributeName = qname.localName();
    else
        m_IdAttributeName = nullAtom;
}

BTW, the spec also mentioned schema-determined ID and DTD-determined ID. I am not sure how they may have an impact of the implementation.

[Alexey Proskuryakov]

> 3. introduce an AtomicString type "m_IdAttributeName" member variable in
> Element.

We shouldn't be increasing the size of all Element objects.

[Chang Shu]

Created attachment 42148 [details]
patch 1

[Darin Adler]

Comment on attachment 42148 [details]
patch 1

> Index: WebCore/dom/Attr.idl
> ===================================================================
> --- WebCore/dom/Attr.idl	(revision 49937)
> +++ WebCore/dom/Attr.idl	(working copy)
> @@ -38,7 +38,10 @@ module core {
>          // DOM Level 2
>  
>          readonly attribute Element ownerElement;
> -        
> +
> +        // DOM Level 3
> +        readonly attribute boolean isId;
> +

Why not keep the formatting consistent? The "DOM Level 2" comment has a blank line after it, so you should do the same.


> +    ElementRareData * data = ensureRareData();

The * here should be next to ElementRareData -- no space before it.

> +void Element::setIdAttributeNS(const AtomicString& namespaceURI, const AtomicString& localName, bool isId, ExceptionCode& ec)
> +{
> +    String sPrefix, sLocalName;
> +    if (!Document::parseQualifiedName(localName, sPrefix, sLocalName, ec))
> +        return;

Why is the argument named "localName" if it's actually a qualified name?

If it is just a local name, and not a qualified name, then why is it OK to use parseQualifiedName to parse it?

The local variable names should not have these "s" prefixes. We don't use the Hungarian notation where variable names encode their types with prefixes.

> +    QualifiedName qName(sPrefix, sLocalName, namespaceURI);
> +    setIdAttribute(qName, isId, ec);

I think you should avoid this local variable entirely by putting the expression in the setIdAttribute statement. But if not, I suggest using a name made of words rather than "qName". "qualifiedName" would do.

> +PassRefPtr<Attr> Element::setIdAttributeNode(Attr* attr, bool isId, ExceptionCode& ec)
> +{
> +    if (!attr) {
> +        ec = TYPE_MISMATCH_ERR;
> +        return 0;
> +    }
> +    setIdAttribute(attr->qualifiedName(), isId, ec);
> +    return attr;
> +}

This isn’t right. It will add a new attribute with a new Attr, but it will return the Attr you passed in, instead.

I think you need a test case that checks for this.

What's the point of implementing these functions if they don't actually affect code that works with the ID attribute? I think the real challenge is having things like this work with getElementById and such. Having these functions, but without them really working, seems like a mistake.

> +    AtomicString m_IdAttributeName;

This should not have a capital "I" in its name.

> Index: LayoutTests/dom/xhtml/level3/core/attrisid04.js
> ===================================================================
> --- LayoutTests/dom/xhtml/level3/core/attrisid04.js	(revision 49937)
> +++ LayoutTests/dom/xhtml/level3/core/attrisid04.js	(working copy)
> @@ -39,7 +39,6 @@ function setUpPage() {
>       //   creates test document builder, may throw exception
>       //
>       builder = createConfiguredBuilder();
> -       setImplementationAttribute("validating", true);
>  
>        docsLoaded = 0;
>        
> Index: LayoutTests/dom/xhtml/level3/core/attrisid05.js
> ===================================================================
> --- LayoutTests/dom/xhtml/level3/core/attrisid05.js	(revision 49937)
> +++ LayoutTests/dom/xhtml/level3/core/attrisid05.js	(working copy)
> @@ -39,7 +39,6 @@ function setUpPage() {
>       //   creates test document builder, may throw exception
>       //
>       builder = createConfiguredBuilder();
> -       setImplementationAttribute("validating", true);
>  
>        docsLoaded = 0;
>        

Why are you changing the test cases here? Nothing in the change log explains why, and in the past we have tried to avoid that whenever possible.

I think you also need to add some new test cases. The ones from the DOM test suite don't go far enough. I suspect they don't cover all the code you added.

[Chang Shu]

Darin, thank you for the quick review.

I will make the coding style change as you indicated. And I have some questions regarding your comments.

> > +void Element::setIdAttributeNS(const AtomicString& namespaceURI, const AtomicString& localName, bool isId, ExceptionCode& ec)
> > +{
> > +    String sPrefix, sLocalName;
> > +    if (!Document::parseQualifiedName(localName, sPrefix, sLocalName, ec))
> > +        return;
> 
> Why is the argument named "localName" if it's actually a qualified name?
> 
> If it is just a local name, and not a qualified name, then why is it OK to use
> parseQualifiedName to parse it?

I was a bit confused too. I basically refer to the implementation in function setAttributeNS where the localName in the arglist is called qualifiedName. However, its type is not QualifiedName but AtomicString. I will make sure I understand this and will make the naming change accordingly.

> > +PassRefPtr<Attr> Element::setIdAttributeNode(Attr* attr, bool isId, ExceptionCode& ec)
> > +{
> > +    if (!attr) {
> > +        ec = TYPE_MISMATCH_ERR;
> > +        return 0;
> > +    }
> > +    setIdAttribute(attr->qualifiedName(), isId, ec);
> > +    return attr;
> > +}
> 
> This isn’t right. It will add a new attribute with a new Attr, but it will
> return the Attr you passed in, instead.

I will revisit this coding.

> 
> I think you need a test case that checks for this.
> 
> What's the point of implementing these functions if they don't actually affect
> code that works with the ID attribute? I think the real challenge is having
> things like this work with getElementById and such. Having these functions, but
> without them really working, seems like a mistake.

These "setIdAttribte" functions indeed affect the ID attribute. They provide different ways of defining user-defined ID attribute and affect isId() calls. But it is true that the current attrisid test cases only cover setIdAttribteNS function. You are also true that I should update functions like getElementById to reflect this enhancement.

> > --- LayoutTests/dom/xhtml/level3/core/attrisid05.js	(revision 49937)
> > +++ LayoutTests/dom/xhtml/level3/core/attrisid05.js	(working copy)
> > @@ -39,7 +39,6 @@ function setUpPage() {
> >       //   creates test document builder, may throw exception
> >       //
> >       builder = createConfiguredBuilder();
> > -       setImplementationAttribute("validating", true);
> Why are you changing the test cases here? Nothing in the change log explains
> why, and in the past we have tried to avoid that whenever possible.

I think the ChangeLog said "Enable test cases...". The fact is if we call "setImplementationAttribute("validating", true)", the running of test case will be skipped. The expected result will expect "Skipped". I guess this should only be true before the feature is supported. Now, we should change these test cases.

 
> I think you also need to add some new test cases. The ones from the DOM test
> suite don't go far enough. I suspect they don't cover all the code you added.

Right. I should probably add test cases to cover areas that DOM test suite don't cover. Any suggestions on where the test cases should go? Or is it ok I remove the implementation that not necessary for passing the DOM tests?

[Chang Shu]

Created attachment 42220 [details]
patch based on Darin's review

I updated the patch with changes based on Darin's review. I haven't updated the test cases but will do after I understand the requirement.

[Chang Shu]

(In reply to comment #9)
> Created an attachment (id=42220) [details]
> patch based on Darin's review
> 
> I updated the patch with changes based on Darin's review. I haven't updated the
> test cases but will do after I understand the requirement.

The current patch may cause a slight performance degrade. I ran drumaeo.com DOM core tests using the builds with and without patch 2 and got results saved in http://dromaeo.com/?id=76616 and http://dromaeo.com/?id=76620, respectively. I am working on find the root cause of the problem.

[Darin Adler]

(In reply to comment #8)
> I think the ChangeLog said "Enable test cases...". The fact is if we call
> "setImplementationAttribute("validating", true)", the running of test case will
> be skipped. The expected result will expect "Skipped". I guess this should only
> be true before the feature is supported. Now, we should change these test
> cases.

For some reason, the person who wrote these test cases thought that they should not run unless the implementation is "validating" and somehow XHTML is not "validating". I don't understand why they thought this. I suggest you leave those lines of code in the test, but comment them out and add a comment explaining why we should remove them.

> Right. I should probably add test cases to cover areas that DOM test suite
> don't cover. Any suggestions on where the test cases should go? Or is it ok I
> remove the implementation that not necessary for passing the DOM tests?

I think adding test cases is the way to go. It's quite easy to write new test cases if you use the script-tests style, and I think you should add some new tests to fast/dom/Element for this.

What I don't understand is why there are only XHTML test cases for this and not HTML. If you add this to Element.idl it will add the feature for both XHTML and HTML documents. So if that's not correct, then we have to figure out why not.

If this does need to work in HTML then we need test cases that are in HTML documents. This can be addressed by doing the script-tests style of test.

[Darin Adler]

You mentioned in email that the performance problem is due to calling getIDAttributeName() too many times. And that making it an inline function resolved the issue, but you had to include ElementRareData.h in Element.h. There is a good way to resolve this. The trick is to make the inline function check hasRareData and then call a non-inline function for the case where the rare data exists. The non-inline function could be called rareIDAttributeName() or something like that.

We don’t use get in the names of so called “getter functions”, so the name of the function should be idAttributeName(), not getIDAttributeName().

I'm slightly concerned that you'll have to include HTMLNames.h in Element.h.

[Darin Adler]

Comment on attachment 42220 [details]
patch based on Darin's review

Thanks for tackling this!

review- because of the performance regression and insufficient test coverage

[Chang Shu]

(In reply to comment #11)
> (In reply to comment #8)
> > I think the ChangeLog said "Enable test cases...". The fact is if we call
> > "setImplementationAttribute("validating", true)", the running of test case will
> > be skipped. The expected result will expect "Skipped". I guess this should only
> > be true before the feature is supported. Now, we should change these test
> > cases.
> 
> For some reason, the person who wrote these test cases thought that they should
> not run unless the implementation is "validating" and somehow XHTML is not
> "validating". I don't understand why they thought this. I suggest you leave
> those lines of code in the test, but comment them out and add a comment
> explaining why we should remove them.
> 
> > Right. I should probably add test cases to cover areas that DOM test suite
> > don't cover. Any suggestions on where the test cases should go? Or is it ok I
> > remove the implementation that not necessary for passing the DOM tests?
> 
> I think adding test cases is the way to go. It's quite easy to write new test
> cases if you use the script-tests style, and I think you should add some new
> tests to fast/dom/Element for this.
> 
> What I don't understand is why there are only XHTML test cases for this and not
> HTML. If you add this to Element.idl it will add the feature for both XHTML and
> HTML documents. So if that's not correct, then we have to figure out why not.
> 
> If this does need to work in HTML then we need test cases that are in HTML
> documents. This can be addressed by doing the script-tests style of test.

Thanks for the comments on the test cases. I will work on that and let you see if they are satisfied.

[Chang Shu]

(In reply to comment #12)
> You mentioned in email that the performance problem is due to calling
> getIDAttributeName() too many times. And that making it an inline function
> resolved the issue, but you had to include ElementRareData.h in Element.h.
> There is a good way to resolve this. The trick is to make the inline function
> check hasRareData and then call a non-inline function for the case where the
> rare data exists. The non-inline function could be called rareIDAttributeName()
> or something like that.
> 
> We don’t use get in the names of so called “getter functions”, so the name of
> the function should be idAttributeName(), not getIDAttributeName().
> 
> I'm slightly concerned that you'll have to include HTMLNames.h in Element.h.

You know exactly what I have struggled these days. :)
The performance improvement I mentioned was based on making the function inline in cpp file as the major bottleneck is in Element.cpp itself. However, after I was trying to move it to Element.h, I had trouble to compile. Even if I managed to untangle the include files and made linux build work, it still does not compile on Mac (somehow Mac complains failing to find the ElementRareData.h). Anyway, I feel it's risky and probably against the original design, so I gave up this approach. I instead used two functions: one inline as private function and one for external calls, kind of ugly though. Your "trick" seems more attractive to me and I will try it right away.

[Chang Shu]

> Your "trick" seems more
> attractive to me and I will try it right away.

Yes, it works great. It compiles on Linux and the performance is no worse at all. I will see if it compiles on Mac (HTMLNames.h is included in Element.h indeed). Meanwhile, I am working on the test cases. Hopefully to submit the next patch soon. Thanks!

[Chang Shu]

I am almost done with 3rd patch. However, while I was trying to make all setidattributes test cases pass, I had problem with 2 setidattributeNS test cases. Basically, the problem is that the test case considers attribute "title" with namespace "*" the same as "title" w/o namespace while the current implementation treats them differently. I am not sure if there is a quick resolution for this. Is it Ok I skip these two cases for now and address them in separate bugs?
Btw, the above mentioned test cases are:
dom/xhtml/level3/core/elementsetidattributens05.xhtml
dom/xhtml/level3/core/elementsetidattributens13.xhtml

[Alexey Proskuryakov]

> Is it Ok I skip these two cases for now and address 
> them in separate bugs?

Yes. These tests don't pass in Firefox either, FWIW.

[Chang Shu]

Created attachment 42779 [details]
fix performance issue; pass related test cases

The performance issue was resolved using inline function (Using Darin's "trick").
Enabled all existing related test cases and made them pass except two cases due to separate namespace issues.

Darin,
So far I see the existing test cases are pretty comprehensive so I just simply updated the expected results and made them work. If you prefer creating new test cases, I can copy them over to a new place. I just feel the test cases may be duplicated.
Thanks!

[Darin Adler]

Comment on attachment 42779 [details]
fix performance issue; pass related test cases

(In reply to comment #19)
> So far I see the existing test cases are pretty comprehensive

That is not so.

The existing test cases don't even cover using getElementById after using setIdAttribute on an element. Nor do they cover any of the other indirect effects of having an id. The setIdAttribute function can change what the value of the id attribute is by changing its name from an attribute with one value to an attribute with another, so that function needs to trigger the work that StyledElement::parseMappedAttribute does as well as all the same work that Element::updateId does. You need test cases that cover the actual use of elements by id, including things like CSS styling, which works by id, getElementById, and you need to test cases where multiple elements have the same id as well as where exactly one element has the id or zero elements have it. And vary the order. The getElementById function must always return the first element in the document in document order. Even if the node is moved to a different place in the document.

Here are a few things that work by id that should be tested:

    - getElementById
    - CSS styling with a selector that specifies an id like this: #elementid
    - node lists, which can be indexed by an id like this list["elementid"], for example: document.getElementsByTagName("a")["elementid"]
    - SVG <use> elements

I found these by searching for clients of getIDAttribute and hasID and then figuring out how these clients in turn were used.

[Chang Shu]

Thanks for the detailed comments. I will look into them.

(In reply to comment #20)
> (From update of attachment 42779 [details])
> (In reply to comment #19)
> > So far I see the existing test cases are pretty comprehensive
> 
> That is not so.
> 
> The existing test cases don't even cover using getElementById after using
> setIdAttribute on an element. Nor do they cover any of the other indirect
> effects of having an id. The setIdAttribute function can change what the value
> of the id attribute is by changing its name from an attribute with one value to
> an attribute with another, so that function needs to trigger the work that
> StyledElement::parseMappedAttribute does as well as all the same work that
> Element::updateId does. You need test cases that cover the actual use of
> elements by id, including things like CSS styling, which works by id,
> getElementById, and you need to test cases where multiple elements have the
> same id as well as where exactly one element has the id or zero elements have
> it. And vary the order. The getElementById function must always return the
> first element in the document in document order. Even if the node is moved to a
> different place in the document.
> 
> Here are a few things that work by id that should be tested:
> 
>     - getElementById
>     - CSS styling with a selector that specifies an id like this: #elementid
>     - node lists, which can be indexed by an id like this list["elementid"],
> for example: document.getElementsByTagName("a")["elementid"]
>     - SVG <use> elements
> 
> I found these by searching for clients of getIDAttribute and hasID and then
> figuring out how these clients in turn were used.

[Chang Shu]

While I was creating a test case for using id as index of node list, I found webkit failed on the following test case:
<body>
<div name='elem1' id='attr_value11' attr_name12='attr_value12'/>
<p   name='elem4' id='attr_value21' attr_name42='attr_value42'/>
<div name='elem2' id='attr_value21' attr_name22='attr_value22'/>
<script>
elems = document.getElementsByTagName('div');
elem = elems['attr_value21'];
shouldBe('elem.attributes[0].value', '"elem2"');
</script>

Actual result: FAIL elem.attributes[0].value should be elem2. Threw exception TypeError: Result of expression 'elem' [undefined] is not an object.

The same test case passes on firefox.

If I remove the line with tag <p>, the test case will pass. Apparently, this <p> element with same id confuses the engine.

Then I found the following code in WebCore/dom/DynamicNodeList.cpp:
Node* DynamicNodeList::itemWithName(const AtomicString& elementId) const
{
    if (m_rootNode->isDocumentNode() || m_rootNode->inDocument()) {
        Element* node = m_rootNode->document()->getElementById(elementId);
        if (node && nodeMatches(node)) {
            for (Node* p = node->parentNode(); p; p = p->parentNode()) {
                if (p == m_rootNode)
                    return node;
            }
        }
        return 0;
    }

    unsigned length = this->length();
    for (unsigned i = 0; i < length; i++) {
        Node* node = item(i);
        if (node->isElementNode() && static_cast<Element*>(node)->getIDAttribute() == elementId)
            return node;
    }

    return 0;
}

I am quite confused by the first part of the function, which returns 0 in the above test case. Can anyone explain to me a little bit? Really appreciate it.

--Chang

[Alexey Proskuryakov]

IDs must be unique in valid documents. We can consider matching other browsers in this case though, please file a separate bug.

The confusing code in DynamicNodeList::itemWithName() looks like an optimization - it's faster to call getElementById() than to iterate the whole list.

[Chang Shu]

(In reply to comment #23)
> IDs must be unique in valid documents. We can consider matching other browsers
> in this case though, please file a separate bug.
> 
> The confusing code in DynamicNodeList::itemWithName() looks like an
> optimization - it's faster to call getElementById() than to iterate the whole
> list.

Understood. Will do the new bug. Thanks!

[Chang Shu]

Created attachment 44812 [details]
new patch to handle isId only

Finally after weeks of scattered work, I finished this patch to implement isId functionality. I basically added the isId interface and implementation in class Element. I also replaced all the instances of idAttr in the code with idAttributeName(), where customized id name may be returned after implementation is done in the next patch. So far, it always returns "id". For almost each of the above changes, I either created a new test case or used an existing one. These test cases will be enhanced to test customized id names in the next patch.

[Darin Adler]

Comment on attachment 44812 [details]
new patch to handle isId only

> +bool Attr::isId() const
> +{
> +    if (!m_element)
> +        return false;

Do we have a test case that covers this code path?

> +    const QualifiedName& qualifiedName = idAttributeName();
> +    Attribute* oldId = namedAttrMap ? namedAttrMap->getAttributeItem(qualifiedName) : 0;
> +    Attribute* newId = list ? list->getAttributeItem(qualifiedName) : 0;

I think this local variable name is strange. I would suggest:

    const QualifiedName& idAttributeName = this->idAttributeName();

Or something else. But qualifiedName is simply too vague. It’s the name of the type, not the value.

> +++ WebCore/dom/Element.h	(working copy)
> @@ -26,6 +26,7 @@
>  #define Element_h
>  
>  #include "ContainerNode.h"
> +#include "HTMLNames.h"
>  #include "QualifiedName.h"
>  #include "ScrollTypes.h"

Adding this new include to all includers of Element.h is unfortunate. The other approach would be to instead put the idAttributeName inline function into its own header, and have all clients include that header, perhaps called ElementIdAttributeName.h. See NodeRenderStyle.h for an example of this. It’s annoying to have to add a new header file, but perhaps less bad than having everything include HTMLNames.h now when only a few files need to call idAttributeName.

> -        if (e->isEnumeratable() && e->getAttribute(attrName) == name) {
> +        const QualifiedName& attributeName = (attrName == idAttr) ? e->idAttributeName() : attrName;
> +        if (e->isEnumeratable() && e->getAttribute(attributeName) == name) {

I'm a little concerned about this logic. It seems very strange that the attribute name "id" does not mean "id", it means whatever the ID attribute is. Are you sure that's right? What test case can we create for this (later, once we have a way to change the idAttributeName)?

If we really need this strange rule, then I think we might want to control it with a separate boolean, not attrName itself.

> -       setImplementationAttribute("validating", true);
> +     // remove the following line otherewise test case skipped.
> +     //setImplementationAttribute("validating", true);

Is there a better way to do this?

The new comment you added has a period, but should also start with a capital letter.

In the past we put "WebKit modification" or "WebKit fix" comments into files in any places we modified these tests. See examples in <http://trac.webkit.org/changeset/10362> and <http://trac.webkit.org/changeset/37549>.

Is the issue that there are no tests that should be skipped because we're not a validating parser, or is it really a mistake just in these tests? If it’s something about all tests, then it could be fixed in the shared JavaScript code instead.

I think this can go as-is, so I'll say r=me, but I think you could also consider improvements based on my comments above

[Chang Shu]

Thanks for the quick review, Darin. Please see my comments below.

(In reply to comment #26)
> (From update of attachment 44812 [details])
> > +bool Attr::isId() const
> > +{
> > +    if (!m_element)
> > +        return false;
> 
> Do we have a test case that covers this code path?

I don't explicitly have a test case for this. I think once I have something in mind about this rare case. I need a bit time thinking about it.

> 
> > +    const QualifiedName& qualifiedName = idAttributeName();
> > +    Attribute* oldId = namedAttrMap ? namedAttrMap->getAttributeItem(qualifiedName) : 0;
> > +    Attribute* newId = list ? list->getAttributeItem(qualifiedName) : 0;
> 
> I think this local variable name is strange. I would suggest:
> 
>     const QualifiedName& idAttributeName = this->idAttributeName();
> 
> Or something else. But qualifiedName is simply too vague. It’s the name of the
> type, not the value.

Yep, will do.

> 
> > +++ WebCore/dom/Element.h	(working copy)
> > @@ -26,6 +26,7 @@
> >  #define Element_h
> >  
> >  #include "ContainerNode.h"
> > +#include "HTMLNames.h"
> >  #include "QualifiedName.h"
> >  #include "ScrollTypes.h"
> 
> Adding this new include to all includers of Element.h is unfortunate. The other
> approach would be to instead put the idAttributeName inline function into its
> own header, and have all clients include that header, perhaps called
> ElementIdAttributeName.h. See NodeRenderStyle.h for an example of this. It’s
> annoying to have to add a new header file, but perhaps less bad than having
> everything include HTMLNames.h now when only a few files need to call
> idAttributeName.

Yeah. Once I thought HTMLNames.h was something good to include anyway. But I will do the new include file for I definitely trust your judgment.

> 
> > -        if (e->isEnumeratable() && e->getAttribute(attrName) == name) {
> > +        const QualifiedName& attributeName = (attrName == idAttr) ? e->idAttributeName() : attrName;
> > +        if (e->isEnumeratable() && e->getAttribute(attributeName) == name) {
> 
> I'm a little concerned about this logic. It seems very strange that the
> attribute name "id" does not mean "id", it means whatever the ID attribute is.
> Are you sure that's right? What test case can we create for this (later, once
> we have a way to change the idAttributeName)?

It's a bit tricky here. The reason I have to do this is because I cannot replace the "idAttr" to idAttributeName() in the caller function as there is no context of element. So I have to replace "id" to its customized name in this function. There should be a test case covering this. I will double-check.

> 
> If we really need this strange rule, then I think we might want to control it
> with a separate boolean, not attrName itself.
> 
> > -       setImplementationAttribute("validating", true);
> > +     // remove the following line otherewise test case skipped.
> > +     //setImplementationAttribute("validating", true);
> 
> Is there a better way to do this?
> 
> The new comment you added has a period, but should also start with a capital
> letter.

Will do.

> 
> In the past we put "WebKit modification" or "WebKit fix" comments into files in
> any places we modified these tests. See examples in
> <http://trac.webkit.org/changeset/10362> and
> <http://trac.webkit.org/changeset/37549>.
> 
> Is the issue that there are no tests that should be skipped because we're not a
> validating parser, or is it really a mistake just in these tests? If it’s
> something about all tests, then it could be fixed in the shared JavaScript code
> instead.

I don't fully understand the meaning of "validating" here. I literally/logically removed the line after I went through the script logic.

> 
> I think this can go as-is, so I'll say r=me, but I think you could also
> consider improvements based on my comments above

Thanks. I will update the patch based on your comments.

[Eric Seidel (no email)]

Comment on attachment 44812 [details]
new patch to handle isId only

Shu Chang has indicated he intends to update the patch, so removing Darin's r+ so that this isn't seen a patch needing landing for now.

[Chang Shu]

Created attachment 44890 [details]
minor changes, isId only

I have made changes based on Darin's last comments. I didn't create new include file yet and may do later in a separate patch.

[WebKit Review Bot]

Attachment 44890 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/dom/NamedAttrMap.cpp:245:  Declaration has space between type name and * in Attribute *oldId  [whitespace/declaration] [3]
WebCore/dom/NamedAttrMap.cpp:246:  Declaration has space between type name and * in Attribute *newId  [whitespace/declaration] [3]
Total errors found: 2

[Chang Shu]

Created attachment 44892 [details]
minor change

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44892 [details] without any errors.

[Darin Adler]

Comment on attachment 44892 [details]
minor change

> +    return qualifiedName().matches(m_element ? m_element->idAttributeName() : HTMLNames::idAttr);

Normally we'd put "using namespace HTMLNames" at the top of the file rather than qualifying idAttr with it here.

> +    , m_idAttributeName(HTMLNames::idAttr)

Same here.

> Index: WebCore/rendering/RenderLayerBacking.cpp
> ===================================================================
> --- WebCore/rendering/RenderLayerBacking.cpp	(revision 52092)
> +++ WebCore/rendering/RenderLayerBacking.cpp	(working copy)
> @@ -86,7 +86,7 @@ void RenderLayerBacking::createGraphicsL
>              m_graphicsLayer->setName("Document Node");
>          else {
>              if (renderer()->node()->isHTMLElement() && renderer()->node()->hasID())
> -                m_graphicsLayer->setName(renderer()->renderName() + String(" ") + static_cast<HTMLElement*>(renderer()->node())->getAttribute(idAttr));
> +                m_graphicsLayer->setName(renderer()->renderName() + String(" ") + static_cast<HTMLElement*>(renderer()->node())->getAttribute(idAttributeName()));

I don't think this will compile. RenderLayerBacking does not have an idAttributeName() member function.

> -       setImplementationAttribute("validating", true);
> +     // Remove the following line otherewise test case skipped.
> +     //setImplementationAttribute("validating", true);

Typo: "otherewise".

Should mention this is a WebKit change as I said last time.

> -       setImplementationAttribute("validating", true);
> +     // Remove the following line otherewise test case skipped.
> +     //setImplementationAttribute("validating", true);

Same typo.

review- because of the broken build of RenderLayerBacking.cpp

[Chang Shu]

Created attachment 44934 [details]
fix broken build and some minor changes

I am really sorry the previous patches broke the build. For some reason, RenderLayerBacking.cpp is not in the build on QtLinux so I overlooked.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44934 [details] without any errors.

[Darin Adler]

Comment on attachment 44934 [details]
fix broken build and some minor changes

Seems OK. r=me

The tests could go further than just checking that something is non-null. For example, in id-in-param.html you could check that the element returned is the single param element, which you could locate with getElementsByTagName, for example. And also the id-in-frameset tests seems to be a little cryptic. Just doing an alert with the content "1" seems a little weak.

[Chang Shu]

(In reply to comment #36)
> (From update of attachment 44934 [details])
> Seems OK. r=me
> 
> The tests could go further than just checking that something is non-null. For
> example, in id-in-param.html you could check that the element returned is the
> single param element, which you could locate with getElementsByTagName, for
> example. And also the id-in-frameset tests seems to be a little cryptic. Just
> doing an alert with the content "1" seems a little weak.

got it. I will take care of them in future patches. Thanks!

[WebKit Commit Bot]

Comment on attachment 44934 [details]
fix broken build and some minor changes

Rejecting patch 44934 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11788 test cases.
fast/dom/Element/id-in-svgfont.svg -> failed

Exiting early after 1 failures. 5690 tests run.
82.79s total testing time

5689 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/127602

[Eric Seidel (no email)]

This looks like a related failure.

[Chang Shu]

(In reply to comment #39)
> This looks like a related failure.

yes, the failed test case is not text based. Mac generates different pixels than qt-linux. I will try to see if I can use a text-based test case instead so I don't have to put different expected results for different platforms.

[Chang Shu]

Created attachment 45098 [details]
remove new test case fast/dom/Element/id-in-svgfont.svg

The two svg related code changes are covered by existing test case svg/custom/acid3-test-77.html. So no need to create a new test case.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45098 [details] without any errors.

[WebKit Commit Bot]

Comment on attachment 45098 [details]
remove new test case fast/dom/Element/id-in-svgfont.svg

Clearing flags on attachment: 45098

Committed r52312: <http://trac.webkit.org/changeset/52312>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chang Shu]

The latest patch finished half of the implementation: isId.
setIsId is still undergoing. So reopen the bug.

[Chang Shu]

Created attachment 45604 [details]
implement setIdAttribute

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45604 [details] without any errors.

[Maciej Stachowiak]

I don't think we should support setIsId or isId. Multiple ID attributes are a needless complication. I suggest we WONTFIX this bug. Does anyone disagree? If so, we should discuss it on webkit-dev.

[Darin Adler]

(In reply to comment #48)
> I don't think we should support setIsId or isId. Multiple ID attributes are a
> needless complication. I suggest we WONTFIX this bug. Does anyone disagree? If
> so, we should discuss it on webkit-dev.

I agree, and wish I had thought of this earlier. I've been helping Chang Shu get the mechanical details right on this. If we do WONTFIX it, I think we may want to undo some of the steps we have already done.

[Chang Shu]

(In reply to comment #49)
> (In reply to comment #48)
> > I don't think we should support setIsId or isId. Multiple ID attributes are a
> > needless complication. I suggest we WONTFIX this bug. Does anyone disagree? If
> > so, we should discuss it on webkit-dev.
> 
> I agree, and wish I had thought of this earlier. I've been helping Chang Shu
> get the mechanical details right on this. If we do WONTFIX it, I think we may
> want to undo some of the steps we have already done.

I agree that allowing multiple ID does not make too much sense and the current implementation does support this anyway. However, I think having the ability to customize id attribute is good to have since not all the identification of an element is called "id" by nature.

[Chang Shu]

(In reply to comment #50)
> (In reply to comment #49)
> > (In reply to comment #48)
> > > I don't think we should support setIsId or isId. Multiple ID attributes are a
> > > needless complication. I suggest we WONTFIX this bug. Does anyone disagree? If
> > > so, we should discuss it on webkit-dev.
> > 
> > I agree, and wish I had thought of this earlier. I've been helping Chang Shu
> > get the mechanical details right on this. If we do WONTFIX it, I think we may
> > want to undo some of the steps we have already done.
> 
> I agree that allowing multiple ID does not make too much sense and the current
> implementation does support this anyway. However, I think having the ability to
> customize id attribute is good to have since not all the identification of an
> element is called "id" by nature.

sorry, this line "the current implementation does support this anyway" should read "the current implementation does NOT support this anyway".

[Darin Adler]

Maciej said that if you disagree we should discuss on webkit-dev, so if you do, please move the discussion there.

[Chang Shu]

(In reply to comment #52)
> Maciej said that if you disagree we should discuss on webkit-dev, so if you do,
> please move the discussion there.

Which webkit-dev? Can you make sure I am in the loop? Thanks!

[Darin Adler]

(In reply to comment #53)
> (In reply to comment #52)
> > Maciej said that if you disagree we should discuss on webkit-dev, so if you do,
> > please move the discussion there.
> 
> Which webkit-dev? Can you make sure I am in the loop? Thanks!

The webkit-dev mailing list. See this page <http://webkit.org/contact.html> for details.

You could start the discussion there. Then you will definitely be in the loop!

[Darin Adler]

Comment on attachment 45604 [details]
implement setIdAttribute

For now I am clearing the review flag on this patch, pending discussion on webkit-dev. We can flag it for review again once we've got consensus on what we want to do.

[Eric Seidel (no email)]

Comment on attachment 44934 [details]
fix broken build and some minor changes

Clearing Darin's r+ from this obsolete patch.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.

[Sam Sneddon [:gsnedders]]

This has long-since been removed from the DOM spec, and now we're unambiguously following the WHATWG spec this is hence WONTFIX.