Bug 39692 – Partly done support for alternate ID attributes should be removed

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Darin Adler]

A while back we got partway into adding support for alternate ID attributes.

Maciej pointed out we should not add the feature, so we stopped.

The partly-done support currently has some costs:

    1) There is a small runtime cost.
    2) There is a lot of code more complex than it needs to be.
    3) Element.h includes HTMLNames.h which means rebuilding most of the world any time HTMLTagNames.in changes.

Issue (3) is bothering me a lot, so I’d like to see this fixed.

[Alexey Proskuryakov]

There were two sides to this work:
1) Make ID attributes with names other than "id" work, as specified by DTD.
2) Make it possible to have per-element alternate IDs.

Part 1 is something that I think we still want, it's tracked as bug 12971.

[Darin Adler]

(In reply to comment #1)
> There were two sides to this work:
> 1) Make ID attributes with names other than "id" work, as specified by DTD.
> 2) Make it possible to have per-element alternate IDs.
> 
> Part 1 is something that I think we still want, it's tracked as bug 12971.

Thanks. I would probably have gotten that wrong.

[Darin Adler]

Created attachment 57481 [details]
Patch

[WebKit Review Bot]

Attachment 57481 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2808013

[Alexey Proskuryakov]

Comment on attachment 57481 [details]
Patch

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

I don't see this change explained in ChangeLog, and I'm not sure if I understand it.

>      RenderSVGResourceContainer(SVGStyledElement* node)
>          : RenderSVGHiddenContainer(node)
>          , RenderSVGResource()
> -        , m_id(node->getIDAttribute())
> +        // FIXME: Should probably be using getIdAttribute rather than idForStyleResolution.
> +        , m_id(node->hasID() ? node->idForStyleResolution() : nullAtom)

The FIXME doesn't make it immediately clear why (it's also not always clear elsewhere, but this instance particularly caught my eye, because this is rendering code, so it's kind of closer to "for style resolution").

r=me

[Darin Adler]

(In reply to comment #5)
> (From update of attachment 57481 [details])
> > -        const QualifiedName& attributeName = (attrName == idAttr) ? e->idAttributeName() : attrName;
> > -        if (e->isEnumeratable() && e->getAttribute(attributeName) == name) {
> > +        if (e->isEnumeratable() && e->getAttribute(attrName) == name) {
> 
> I don't see this change explained in ChangeLog, and I'm not sure if I understand it.

This code was added as part of attempting to make custom attribute names work. For someone reason the idea is that if you explicitly pass "id" as the attribute here, that should map to the abstract "id" attribute. The code has no effect since we don't support custom id attributes. And it slows things down unnecessarily. At some point, if we actually implement alternate ID attributes we'll have to figure out what code we do and don't need. This code was just too strange to live, though.

> >      RenderSVGResourceContainer(SVGStyledElement* node)
> >          : RenderSVGHiddenContainer(node)
> >          , RenderSVGResource()
> > -        , m_id(node->getIDAttribute())
> > +        // FIXME: Should probably be using getIdAttribute rather than idForStyleResolution.
> > +        , m_id(node->hasID() ? node->idForStyleResolution() : nullAtom)
> 
> The FIXME doesn't make it immediately clear why (it's also not always clear elsewhere, but this instance particularly caught my eye, because this is rendering code, so it's kind of closer to "for style resolution").

I really don’t know which of these cases should and should not be changed. The issue is simply whether a lowercased version of the ID is right for HTML documents. If it is right, then this is fine, and perhaps it's the name idForStyleResolution that is wrong. More likely, the lowercasing of the ID actually breaks something, in which case the code does need to be changed.

But I wanted to avoid fixing a bug in this patch. The fix can go in a separate patch with a regression test.

[Darin Adler]

Based on the EWS bot it seems an include of HTMLNames.h has to be added to BindingSecurity.h.

[Darin Adler]

Committed r61094: <http://trac.webkit.org/changeset/61094>

[WebKit Review Bot]

http://trac.webkit.org/changeset/61094 might have broken Chromium Linux Release

[Eric Seidel (no email)]

Chromium builders seem unhappy after this change:

3>V8NamedNodesCollection.cpp
3>..\bindings\v8\custom\V8NamedNodesCollection.cpp(49) : error C2039: 'id' : is not a member of 'WebCore::NamedNodeMap'
3>        c:\WebKitBuildSlave\chromium-win-release\build\WebCore\dom\NamedNodeMap.h(37) : see declaration of 'WebCore::NamedNodeMap'
3>Build log was saved at "file://C:\WebKitBuildSlave\chromium-win-release\build\WebKit\chromium\Release\obj\webcore\BuildLog.htm"
3>webcore - 1 error(s), 0 warning(s)
4>------ Build started: Project: webkit, Configuration: Release Win32 ------
4>Compiling...
4>WebFormControlElement.cpp
4>c:\webkitbuildslave\chromium-win-release\build\javascriptcore\wtf\text\StringImpl.h(96) : warning C4355: 'this' : used in base member initializer list
4>.\src\WebFormControlElement.cpp(62) : error C2653: 'HTMLNames' : is not a class or namespace name
4>.\src\WebFormControlElement.cpp(62) : error C2065: 'idAttr' : undeclared identifier
4>ChromeClientImpl.cpp
4>c:\webkitbuildslave\chromium-win-release\build\javascriptcore\wtf\text\StringImpl.h(96) : warning C4355: 'this' : used in base member initializer list
4>c:\webkitbuildslave\chromium-win-release\build\webcore\page\PositionCallback.h(38) : warning C4355: 'this' : used in base member initializer list
4>c:\webkitbuildslave\chromium-win-release\build\webcore\page\PositionErrorCallback.h(38) : warning C4355: 'this' : used in base member initializer list
4>.\src\ChromeClientImpl.cpp(734) : error C3083: 'HTMLNames': the symbol to the left of a '::' must be a type
4>.\src\ChromeClientImpl.cpp(734) : error C2039: 'videoTag' : is not a member of 'WebCore'
4>.\src\ChromeClientImpl.cpp(734) : error C2065: 'videoTag' : undeclared identifier

[Tony Chang]

(In reply to comment #10)
> Chromium builders seem unhappy after this change:

I submitted a build fix in http://trac.webkit.org/changeset/61097
https://bugs.webkit.org/show_bug.cgi?id=40553

[Kentaro Hara]

Comment on attachment 57481 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=57481&action=review

> WebCore/ChangeLog:24
> +        Added an idAttributeName function to Document. This is for a future
> +        where a document can have a custom id attribute name specified in its
> +        doctype. It's possible this will be insufficient because the same

Although it's been a long time since this patch was landed, a Document has not yet supported a custom id attribute name. Document::m_idAttributeName is always idAttr. Can we remove Document::m_idAttributeName for performance? (I'll write a patch.) Or do you have a plan to support a custom id name in a future?

[Darin Adler]

(In reply to comment #12)
> Although it's been a long time since this patch was landed, a Document has not yet supported a custom id attribute name. Document::m_idAttributeName is always idAttr. Can we remove Document::m_idAttributeName for performance? (I'll write a patch.) Or do you have a plan to support a custom id name in a future?

Alexey is the one you should ask, not me. I have no opinion on this. See comment #1 and bug 12971 for details.

[Kentaro Hara]

(In reply to comment #13)
> (In reply to comment #12)
> Alexey is the one you should ask, not me. I have no opinion on this. See comment #1 and bug 12971 for details.

darin: Thanks for giving me the context!

ap: Do you have any idea on supporting a custom id attribute name?

[Alexey Proskuryakov]

I still think that we want to fix bug 12971 eventually. It's a glaring correctness issue when working with arbitrary XML documents retrieved with XHR. I don't have a very strong opinion about this though.

Keeping the unfinished implementation in tree does not necessarily help achieve the goal. As Darin mentioned in ChangeLog, "it's possible this will be insufficient <...>"

[Kentaro Hara]

(In reply to comment #15)
> Keeping the unfinished implementation in tree does not necessarily help achieve the goal. As Darin mentioned in ChangeLog, "it's possible this will be insufficient <...>"

OK, thanks. Then if I could observe any performance win by removing Document::m_idAttributeName, I will consider removing it. Let me investigate.

[Lucas Forschler]

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