[webkit-reviews] review denied: [Bug 5578] WebKit does not support DOM Level 3 setIsId and isId : [Attachment 42148] patch 1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 42148: patch 1
https://bugs.webkit.org/attachment.cgi?id=42148&action=review

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