[webkit-reviews] review denied: [Bug 54290] rename Node::isContentEditable and all call sites to fastIsContentEditable : [Attachment 86094] fix patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Chang Shu <cshu at webkit.org>'s
request for review:
Bug 54290: rename Node::isContentEditable and all call sites to
fastIsContentEditable
https://bugs.webkit.org/show_bug.cgi?id=54290

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

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:664
> -	   if (body && body->isContentEditable())
> +	   if (body && body->rendererIsContentEditable())

rendererIsContentEditable might be a verbose.  How about
rendererIsEditable/rendererIsRichlyEditable?

> Source/WebCore/dom/Document.cpp:322
> -    ASSERT(node->isContentEditable());
> +    ASSERT(node->rendererIsContentEditable());

I don't think this is right.  acceptsEditingFocus is called in
Document::setFocusedNode immediately after dispatching focusoutEvent and
DOMFocusOutEvent, both of which could modify DOM.  We should be calling
isContentEditable() here so that style recalc. is trigged if needed.

> Source/WebCore/dom/Node.cpp:743
> -    return isContentEditable();
> +    return rendererIsContentEditable();

This function is called in FocusController::setFocusedNode. For the same reason
above, we need to be calling isContentEditable.

> Source/WebCore/dom/Node.h:333
> +#if PLATFORM(MAC)
> +    // Objective-C extensions
> +    bool isContentEditable() const { return
rendererIsContentEditable(Editable); }
> +#endif

I don't think this is right.  It needs to be a virtual function because we're
overriding it in HTMLElement.  But I think a cleaner implementation is to add
non-virtual Node::isContentEditable in all platforms.

> Source/WebCore/html/HTMLElement.cpp:663
> +bool HTMLElement::isContentEditable() const
> +{
> +    return rendererIsContentEditable();
> +}

As I commented above, the correctness of this patch depends on the final
version of this function that triggers style recalc. For that reason, I think
we should do the rename and fix isContentEditable in one patch.