Bug 56421 – Devirtualize isContentEditable and isRichlyContentEditable

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

This is a cleanup mentioned in the bug 56372.

[Darin Adler]

It’s also a little strange that an element like "<custom>" always returns false from isContentEditable. Devirtualizing in the most obvious way might change that.

[Ryosuke Niwa]

I'd say the strangest thing is that Node::isContentEditable/isContentRichlyEditable call parentOrHostNode() instead of parentNode().

[Ryosuke Niwa]

Created attachment 85870 [details]
work in progress

[Ryosuke Niwa]

Comment on attachment 85870 [details]
work in progress

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

> Source/WebCore/dom/Node.cpp:720
> +    if (document()->inDesignMode())
> +        return true;

This is a very expensive call.  It'll traverse all parent frames.  Given that this function walks the nodes up, isContentEditable is O(nk) where n is the number of nodes in the current document and k is the number of parent documents.  I don't think this is acceptable.  We should make it O(n+k).

[Ryosuke Niwa]

Created attachment 85888 [details]
cleanup

[Ryosuke Niwa]

Comment on attachment 85888 [details]
cleanup

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

> Source/WebCore/dom/Node.h:331
> +    bool isContentEditable() const { return isContentEditable(false); }
> +    bool isContentRichlyEditable() const { return isContentEditable(true); }

I'm not happy about the fact these two functions call isContentEditable with true/false but I wasn't sure about it's worth adding an enum just so that these two functions can use it.  Given that isContentEditable is Node's private function, it seems okay to use raw boolean values here.

[Ryosuke Niwa]

Oops, I had forgotten that C++ allows enum declaration to appear later than it's first referenced within a class declaration.

[Ryosuke Niwa]

Created attachment 85891 [details]
Use enum instead of boolean

[Darin Adler]

Comment on attachment 85891 [details]
Use enum instead of boolean

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

> Source/WebCore/dom/Node.cpp:719
> +    // Ideally we'd call ASSERT!needsStyleRecalc()) here, but

Missing parenthesis. I know you just moved this, but still.

> Source/WebCore/dom/Node.cpp:724
> +    const Node* node = this;
> +    while (node) {

I’d write this as a for loop. That will work really well if both sides end up doing node->parentNode, but it’s OK even without that.

    for (const Node* node = this; node; )

[Ryosuke Niwa]

Thanks for the review as always.

(In reply to comment #9)
> (From update of attachment 85891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85891&action=review
> 
> > Source/WebCore/dom/Node.cpp:719
> > +    // Ideally we'd call ASSERT!needsStyleRecalc()) here, but
> 
> Missing parenthesis. I know you just moved this, but still.

Oh yeah, I didn't realize that.  Will fix.

> > Source/WebCore/dom/Node.cpp:724
> > +    const Node* node = this;
> > +    while (node) {
> 
> I’d write this as a for loop. That will work really well if both sides end up doing node->parentNode, but it’s OK even without that.
> 
>     for (const Node* node = this; node; )

Okay, will do.  We should really be calling parentNode for both cases.  I bet the reason we haven't realized this bug is because shadow DOM's root element is almost always editable.

[Ryosuke Niwa]

I measured the time to run LayoutTests/editing with and without the patch:

With patch:
30.36
30.52
30.60
30.86
30.87
30.21
30.72
30.50
30.90
30.74

Without patch:
30.86
30.60
31.62
31.10
30.47
30.34
30.61
30.60
30.91
30.77

So it's about 0.52% improvement overall.

[Ryosuke Niwa]

(In reply to comment #10)
> >     for (const Node* node = this; node; )
> 
> Okay, will do.  We should really be calling parentNode for both cases.  I bet the reason we haven't realized this bug is because shadow DOM's root element is almost always editable.

After some research, this behavior seems to be intentional.  IE lets user edit readonly input element if the element is inside an editable region.  So I guess we need to keep this behavior.

[Ryosuke Niwa]

Committed r81220: <http://trac.webkit.org/changeset/81220>