[webkit-reviews] review denied: [Bug 52056] contentEditable attribute should be "inherit" if missing : [Attachment 79306] fix patch

[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 52056: contentEditable attribute should be "inherit" if missing
https://bugs.webkit.org/show_bug.cgi?id=52056

Attachment 79306: fix patch
https://bugs.webkit.org/attachment.cgi?id=79306&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=79306&action=review

Please make a new patch that includes only the necessary changes to the tests.
I’m not sure the changes are all correct, in fact. Some of the tests have been
changed to have incorrect expectations.

> Source/WebCore/html/HTMLElement.cpp:691
> +    if (!hasAttribute(contenteditableAttr))
> +	   return "inherit";
> +
> +    const AtomicString& value = getAttribute(contenteditableAttr);

Better to not separately call hasAttribute and getAttribute. Both getAttribute
and fastGetAttribute return a null string if there is no attribute, and that’s
more efficient than a separate call. We can check that with isNull(). And since
"inherit" is the default, you’ll just have to check that this doesn’t
accidentally give us "true" instead of "inherit" since null returns true for
isEmpty.

Also, we can call fastGetAttribute here.

> Source/WebCore/html/HTMLElement.cpp:699
> +    if (value.isEmpty() || equalIgnoringCase(value, "true"))
> +	   return "true";
> +    else if (equalIgnoringCase(value, "false"))
> +	    return "false";
> +    else if (equalIgnoringCase(value, "plaintext-only"))
> +	   return "plaintext-only";
> +    else
> +	   return "inherit";

WebKit coding style prohibits else after return.

> LayoutTests/editing/editability/attr-empty-string.html:14
> -<div id="div" contentEditable=""></div>
> +<div id="div" contenteditable=""></div>
>  <script>
>  description('When contentEditable attribute is empty string,
element.contentEditable returns "true" and the element is editable.')
>  
>
-shouldBe('document.getElementById("div").getAttribute("contentEditable")','""'
);
>
+shouldBe('document.getElementById("div").getAttribute("contenteditable")','""'
);

Why are you making these changes? HTML attributes are not case sensitive, and
the test should be OK as is. There’s no reason to combine this change with this
patch. If you want to change the tests, that’s OK, but should be decided on its
own merits.

> LayoutTests/editing/editability/attr-false-string.html:14
> -<div id="div" contentEditable="false"></div>
> +<div id="div" contenteditable="false"></div>
>  <script>
>  description('When contentEditable attribute is "false" string,
element.contentEditable returns "false" and the element is NOT editable.')
>  
>
-shouldBe('document.getElementById("div").getAttribute("contentEditable")','"fa
lse"');
>
+shouldBe('document.getElementById("div").getAttribute("contenteditable")','"fa
lse"');

Same comment here.

> LayoutTests/editing/editability/attr-invalid-string.html:14
> -<div id="div" contentEditable="abc"></div>
> +<div id="div" contenteditable="abc"></div>
>  <script>
>  description('When contentEditable attribute is invalid string,
element.contentEditable returns "inherit".')
>  
>
-shouldBe('document.getElementById("div").getAttribute("contentEditable")','"ab
c"');
>
+shouldBe('document.getElementById("div").getAttribute("contenteditable")','"ab
c"');

And here.

> LayoutTests/editing/editability/attr-missing-ancestor-false.html:10
> -<div id="div" contentEditable="false">
> +<div id="div" contenteditable="false">

Same here.

> LayoutTests/editing/editability/attr-missing-ancestor-false.html:20
> -shouldBe('document.getElementById("p").hasAttribute("contentEditable")',
'false');
> +shouldBe('document.getElementById("p").hasAttribute("contenteditable")',
'false');

Same here and in lots more places below. Lets not make this change; it’s not
tied to this bug.