Bug 86217 – Add helper function for node()->rootEditableElement() == node()

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Shezan Baig]

Per bug 85385, comment 5, this statement can be optimized as:

node()->rendererIsEditable() && (!node()->parentNode() || !node()->parentNode()->rendererIsEditable() || node()->hasTagName(bodyTag))

[Shezan Baig]

Created attachment 141429 [details]
Patch

[Shezan Baig]

I also see a bunch of places where rootEditableElement() is used to determine whether the element is editable, for example editing/FrameSelection.cpp@1788:

    if (!rootEditableElement())
        return;

Couldn't these statements also be replaced with a simple rendererIsEditable()?

[Ryosuke Niwa]

Comment on attachment 141429 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141429&action=review

The change looks reasonable.

> Source/WebCore/dom/Node.cpp:1572
> +    return rendererIsEditable() && (!parentNode() || !parentNode()->rendererIsEditable() || hasTagName(bodyTag));

Actually, you can remove hasTagName(bodyTag) here. I wasn't thinking straight about this.

[Shezan Baig]

Hmm, so presumably the body tag check can also be removed from Node::rootEditableElement?

[Ryosuke Niwa]

(In reply to comment #4)
> Hmm, so presumably the body tag check can also be removed from Node::rootEditableElement?

Oh oops, I didn't see that. Okay, I was right. We do need this bodyTag check.

[Shezan Baig]

Created attachment 141459 [details]
Patch (added isElementNode to the condition)

[Ryosuke Niwa]

Comment on attachment 141459 [details]
Patch (added isElementNode to the condition)

Thank you for the refactoring! I've always wanted to do this :)

[Ryosuke Niwa]

(In reply to comment #2)
> I also see a bunch of places where rootEditableElement() is used to determine whether the element is editable, for example editing/FrameSelection.cpp@1788:
> 
>     if (!rootEditableElement())
>         return;
> 
> Couldn't these statements also be replaced with a simple rendererIsEditable()?

There is a possibility that we're also checking that there is an element somewhere up in the ancestor chain (as supposed to a document) but I bet most of those places can just be calling rendererIsEditable.

[WebKit Review Bot]

Comment on attachment 141459 [details]
Patch (added isElementNode to the condition)

Clearing flags on attachment: 141459

Committed r116798: <http://trac.webkit.org/changeset/116798>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Fixed a regression in http://trac.webkit.org/changeset/116814.