[webkit-reviews] review denied: [Bug 53031] isContentEditable is not working properly with document.designMode : [Attachment 79950] fix patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Chang Shu <Chang.Shu at nokia.com>'s
request for review:
Bug 53031: isContentEditable is not working properly with document.designMode
https://bugs.webkit.org/show_bug.cgi?id=53031

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=79950&action=review

> Source/WebCore/html/HTMLElement.cpp:660
> +	   for (const ContainerNode* p = this; p; p = p->parentNode()) {

Please don't one-letter variable name.

> Source/WebCore/html/HTMLElement.cpp:667
> +	       if (p->isHTMLElement()) {
> +		   const HTMLElement* htmlParent = static_cast<const
HTMLElement*>(p);
> +		   if (htmlParent->contentEditable() != "inherit") {
> +		       isContentEditableSet = true;
> +		       break;
> +		   }
> +	       }

htmlParent is used only once.  We don't need to declare this local variable
here.

Please do:
if (p->isHTMLElement() && tatic_cast<const HTMLElement*>(p)->contentEditable()
!= "inherit") {
    isCOntentEditableSet = true;
    break;
}

Also, it's still very expensive to compare strings here. The last time I
checked, isContentEditable was in our hot path, and it's crucial that we keep
this function fast.  I can't r+ this patch unless we come up with more
efficient way of figuring this out.