Bug 5578 - WebKit does not support DOM Level 3 setIsId and isId
Summary: WebKit does not support DOM Level 3 setIsId and isId
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P4 Normal
Assignee: Nobody
URL: http://www.w3.org/TR/DOM-Level-3-Core...
Keywords:
: 30715 (view as bug list)
Depends on: 31428 31560
Blocks:
  Show dependency treegraph
 
Reported: 2005-10-31 16:12 PST by Eric Seidel (no email)
Modified: 2019-02-06 09:03 PST (History)
10 users (show)

See Also:


Attachments
patch 1 (10.35 KB, patch)
2009-10-29 14:44 PDT, Chang Shu
darin: review-
Details | Formatted Diff | Diff
patch based on Darin's review (13.65 KB, patch)
2009-10-30 11:22 PDT, Chang Shu
darin: review-
Details | Formatted Diff | Diff
fix performance issue; pass related test cases (32.58 KB, patch)
2009-11-09 12:26 PST, Chang Shu
darin: review-
Details | Formatted Diff | Diff
new patch to handle isId only (52.00 KB, patch)
2009-12-14 12:27 PST, Chang Shu
no flags Details | Formatted Diff | Diff
minor changes, isId only (53.61 KB, patch)
2009-12-15 11:26 PST, Chang Shu
no flags Details | Formatted Diff | Diff
minor change (53.61 KB, patch)
2009-12-15 11:42 PST, Chang Shu
darin: review-
Details | Formatted Diff | Diff
fix broken build and some minor changes (54.16 KB, patch)
2009-12-15 19:09 PST, Chang Shu
commit-queue: commit-queue-
Details | Formatted Diff | Diff
remove new test case fast/dom/Element/id-in-svgfont.svg (52.45 KB, patch)
2009-12-17 11:44 PST, Chang Shu
no flags Details | Formatted Diff | Diff
implement setIdAttribute (28.15 KB, patch)
2009-12-29 08:47 PST, Chang Shu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2005-10-31 16:12:32 PST
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.
Comment 1 Eric Seidel (no email) 2005-10-31 16:13:43 PST
http:://bugzilla.opendarwin.org/show_bug.cgi?id=3249 covers the KSVG2 kdom -> webcore dom merger.
Comment 2 Sam Weinig 2005-12-12 07:46:07 PST
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.
Comment 3 Alexey Proskuryakov 2009-10-23 13:17:52 PDT
*** Bug 30715 has been marked as a duplicate of this bug. ***
Comment 4 Chang Shu 2009-10-29 09:05:27 PDT
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.
Comment 5 Alexey Proskuryakov 2009-10-29 10:09:44 PDT
> 3. introduce an AtomicString type "m_IdAttributeName" member variable in
> Element.

We shouldn't be increasing the size of all Element objects.
Comment 6 Chang Shu 2009-10-29 14:44:13 PDT
Created attachment 42148 [details]
patch 1
Comment 7 Darin Adler 2009-10-29 14:55:51 PDT
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.
Comment 8 Chang Shu 2009-10-29 15:20:07 PDT
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?
Comment 9 Chang Shu 2009-10-30 11:22:26 PDT
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.
Comment 10 Chang Shu 2009-11-02 11:40:39 PST
(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.
Comment 11 Darin Adler 2009-11-05 16:40:56 PST
(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.
Comment 12 Darin Adler 2009-11-05 16:42:28 PST
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.
Comment 13 Darin Adler 2009-11-05 16:43:48 PST
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
Comment 14 Chang Shu 2009-11-06 06:38:08 PST
(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.
Comment 15 Chang Shu 2009-11-06 06:44:47 PST
(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.
Comment 16 Chang Shu 2009-11-06 08:20:05 PST
> 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!
Comment 17 Chang Shu 2009-11-09 09:04:25 PST
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
Comment 18 Alexey Proskuryakov 2009-11-09 09:59:34 PST
> 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.
Comment 19 Chang Shu 2009-11-09 12:26:11 PST
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!
Comment 20 Darin Adler 2009-11-09 16:14:36 PST
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.
Comment 21 Chang Shu 2009-11-09 17:49:52 PST
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.
Comment 22 Chang Shu 2009-11-12 07:53:18 PST
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
Comment 23 Alexey Proskuryakov 2009-11-12 11:35:04 PST
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.
Comment 24 Chang Shu 2009-11-12 12:15:27 PST
(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!
Comment 25 Chang Shu 2009-12-14 12:27:48 PST
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.
Comment 26 Darin Adler 2009-12-14 17:41:28 PST
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
Comment 27 Chang Shu 2009-12-14 18:13:06 PST
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.
Comment 28 Eric Seidel (no email) 2009-12-15 01:12:46 PST
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.
Comment 29 Chang Shu 2009-12-15 11:26:53 PST
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.
Comment 30 WebKit Review Bot 2009-12-15 11:31:50 PST
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
Comment 31 Chang Shu 2009-12-15 11:42:12 PST
Created attachment 44892 [details]
minor change
Comment 32 WebKit Review Bot 2009-12-15 11:42:46 PST
style-queue ran check-webkit-style on attachment 44892 [details] without any errors.
Comment 33 Darin Adler 2009-12-15 14:18:46 PST
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
Comment 34 Chang Shu 2009-12-15 19:09:10 PST
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.
Comment 35 WebKit Review Bot 2009-12-15 19:12:11 PST
style-queue ran check-webkit-style on attachment 44934 [details] without any errors.
Comment 36 Darin Adler 2009-12-16 09:11:25 PST
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.
Comment 37 Chang Shu 2009-12-16 09:13:51 PST
(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!
Comment 38 WebKit Commit Bot 2009-12-16 09:33:47 PST
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
Comment 39 Eric Seidel (no email) 2009-12-16 17:44:54 PST
This looks like a related failure.
Comment 40 Chang Shu 2009-12-16 18:31:19 PST
(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.
Comment 41 Chang Shu 2009-12-17 11:44:03 PST
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.
Comment 42 WebKit Review Bot 2009-12-17 11:49:25 PST
style-queue ran check-webkit-style on attachment 45098 [details] without any errors.
Comment 43 WebKit Commit Bot 2009-12-18 08:41:07 PST
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>
Comment 44 WebKit Commit Bot 2009-12-18 08:41:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 45 Chang Shu 2009-12-18 13:01:08 PST
The latest patch finished half of the implementation: isId.
setIsId is still undergoing. So reopen the bug.
Comment 46 Chang Shu 2009-12-29 08:47:58 PST
Created attachment 45604 [details]
implement setIdAttribute
Comment 47 WebKit Review Bot 2009-12-29 08:51:31 PST
style-queue ran check-webkit-style on attachment 45604 [details] without any errors.
Comment 48 Maciej Stachowiak 2009-12-30 16:33:02 PST
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.
Comment 49 Darin Adler 2010-01-04 09:50:27 PST
(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.
Comment 50 Chang Shu 2010-01-04 10:06:16 PST
(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.
Comment 51 Chang Shu 2010-01-04 10:08:02 PST
(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".
Comment 52 Darin Adler 2010-01-04 13:26:54 PST
Maciej said that if you disagree we should discuss on webkit-dev, so if you do, please move the discussion there.
Comment 53 Chang Shu 2010-01-04 13:31:13 PST
(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!
Comment 54 Darin Adler 2010-01-04 13:34:39 PST
(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!
Comment 55 Darin Adler 2010-01-05 13:10:30 PST
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.
Comment 56 Eric Seidel (no email) 2010-01-06 23:33:39 PST
Comment on attachment 44934 [details]
fix broken build and some minor changes

Clearing Darin's r+ from this obsolete patch.
Comment 57 Lucas Forschler 2019-02-06 09:03:19 PST
Mass moving XML DOM bugs to the "DOM" Component.