[webkit-reviews] review granted: [Bug 5578] WebKit does not support DOM Level 3 setIsId and isId : [Attachment 44812] new patch to handle isId only

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chang Shu <Chang.Shu at nokia.com>'s
request for review:
Bug 5578: WebKit does not support DOM Level 3 setIsId and isId
https://bugs.webkit.org/show_bug.cgi?id=5578

Attachment 44812: new patch to handle isId only
https://bugs.webkit.org/attachment.cgi?id=44812&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +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