Summary: | Add helper function for node()->rootEditableElement() == node() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shezan Baig <shezbaig.wk> | ||||||
Component: | HTML Editing | Assignee: | Shezan Baig <shezbaig.wk> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cshu, darin, enrica, eric, rniwa, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 85385 | ||||||||
Attachments: |
|
Description
Shezan Baig
2012-05-11 07:35:47 PDT
Created attachment 141429 [details]
Patch
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()? 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. Hmm, so presumably the body tag check can also be removed from Node::rootEditableElement? (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. Created attachment 141459 [details]
Patch (added isElementNode to the condition)
Comment on attachment 141459 [details]
Patch (added isElementNode to the condition)
Thank you for the refactoring! I've always wanted to do this :)
(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. Comment on attachment 141459 [details] Patch (added isElementNode to the condition) Clearing flags on attachment: 141459 Committed r116798: <http://trac.webkit.org/changeset/116798> All reviewed patches have been landed. Closing bug. Fixed a regression in http://trac.webkit.org/changeset/116814. |