Bug 56421

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.

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

Created attachment 85870 [details] work in progress

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).

Created attachment 85888 [details] cleanup

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.

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

Created attachment 85891 [details] Use enum instead of boolean

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; )

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.

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.

(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.

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