Bug 56421 - Devirtualize isContentEditable and isRichlyContentEditable
Summary: Devirtualize isContentEditable and isRichlyContentEditable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on: 56372
Blocks: 54290
  Show dependency treegraph
 
Reported: 2011-03-15 15:30 PDT by Ryosuke Niwa
Modified: 2011-03-17 11:51 PDT (History)
2 users (show)

See Also:


Attachments
work in progress (6.18 KB, patch)
2011-03-15 16:19 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
cleanup (6.85 KB, patch)
2011-03-15 18:03 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Use enum instead of boolean (6.94 KB, patch)
2011-03-15 18:22 PDT, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-03-15 15:30:47 PDT
This is a cleanup mentioned in the bug 56372.
Comment 1 Darin Adler 2011-03-15 16:07:10 PDT
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.
Comment 2 Ryosuke Niwa 2011-03-15 16:15:33 PDT
I'd say the strangest thing is that Node::isContentEditable/isContentRichlyEditable call parentOrHostNode() instead of parentNode().
Comment 3 Ryosuke Niwa 2011-03-15 16:19:31 PDT
Created attachment 85870 [details]
work in progress
Comment 4 Ryosuke Niwa 2011-03-15 16:21:19 PDT
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).
Comment 5 Ryosuke Niwa 2011-03-15 18:03:41 PDT
Created attachment 85888 [details]
cleanup
Comment 6 Ryosuke Niwa 2011-03-15 18:05:37 PDT
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.
Comment 7 Ryosuke Niwa 2011-03-15 18:21:58 PDT
Oops, I had forgotten that C++ allows enum declaration to appear later than it's first referenced within a class declaration.
Comment 8 Ryosuke Niwa 2011-03-15 18:22:51 PDT
Created attachment 85891 [details]
Use enum instead of boolean
Comment 9 Darin Adler 2011-03-15 19:10:01 PDT
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; )
Comment 10 Ryosuke Niwa 2011-03-15 19:14:51 PDT
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.
Comment 11 Ryosuke Niwa 2011-03-15 23:02:38 PDT
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.
Comment 12 Ryosuke Niwa 2011-03-15 23:11:34 PDT
(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.
Comment 13 Ryosuke Niwa 2011-03-15 23:21:55 PDT
Committed r81220: <http://trac.webkit.org/changeset/81220>