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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has asked  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 Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=79950&action=review

I am confused about this patch. Is editability actually wrong, or just the
value of the isContentEditable function? I suspect this change is incorrect,
but I don’t have enough information to be sure.

Also, this change has a high risk of causing a major performance regression.
Changing the isContentEditable function to an operation that is O(depth of
tree) and that calls the contentEditable function, which allocates a new string
object each time it is called, might cause a significant slowdown of many
editing operations.

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

Please use a word for the name of the local variable, such as parent or node,
rather than the letter p.

> Source/WebCore/html/HTMLElement.cpp:665
> +		       isContentEditableSet = true;
> +		       break;

If the semantics of this function is correct, then the implementation can be
simplified by just doing a return true here. There is no need to use for the
boolean local variable.

Or we could factor this loop out into a separate helper function.