RESOLVED FIXED 54290
rename Node::isContentEditable and all call sites to rendererIsEditable
https://bugs.webkit.org/show_bug.cgi?id=54290
Summary rename Node::isContentEditable and all call sites to rendererIsEditable
Chang Shu
Reported 2011-02-11 10:03:52 PST
This effort is to differentiate Node::isContentEditable, which is for internal use, from HTMLElement::isContentEditable, which is a DOM interface.
Attachments
fix patch (68.51 KB, patch)
2011-03-17 14:36 PDT, Chang Shu
rniwa: review-
fix patch 2 (69.27 KB, patch)
2011-03-22 14:08 PDT, Chang Shu
no flags
fix patch 3 (69.28 KB, patch)
2011-03-24 06:49 PDT, Chang Shu
rniwa: review+
rniwa: commit-queue-
fix wording (69.27 KB, patch)
2011-03-25 06:37 PDT, Chang Shu
no flags
Darin Adler
Comment 1 2011-02-11 10:53:18 PST
The prefix “fast” might be OK, but I think it has too positive a connotation. The fastGetAttribute function can almost always be used and is almost always faster. But fastIsContentEditable will often give you the wrong result so may need a less positive name. Maybe isContentEditableWithoutStyleCalc (ugly, abbreviates a word, but more precise). It would help if we had a clearer sense of when it’s safe to call Node::isContentEditable and when it will give an incorrect result.
Darin Adler
Comment 2 2011-02-11 10:55:16 PST
Another possible name that’s not perfect but might be better than fastIsContentEditable would be rendererIsContentEditable. It communicates that this function is just a shortcut to ask the renderer this question and so is only safe to call when the renderer is known to be up to date.
Chang Shu
Comment 3 2011-03-17 08:51:40 PDT
do we need this any more after 56421 is done?
Ryosuke Niwa
Comment 4 2011-03-17 09:43:06 PDT
(In reply to comment #3) > do we need this any more after 56421 is done? We do. All I did in the bug 56421 was to combine Node::isContentEditable, HTMLElement::isContentEditable, and Document::isContentEditable into one function and made it iterative. It doesn't solve the underlying problem we're trying to solve in this bug.
Chang Shu
Comment 5 2011-03-17 10:07:35 PDT
> We do. All I did in the bug 56421 was to combine Node::isContentEditable, HTMLElement::isContentEditable, and Document::isContentEditable into one function and made it iterative. It doesn't solve the underlying problem we're trying to solve in this bug. just want to confirm the following things to do: 1. rename node::isContentEditable to another name. this should be style-dependent. 2. add isContentEditable back to HTMLElement. This should be dom-dependent only.
Ryosuke Niwa
Comment 6 2011-03-17 11:57:39 PDT
(In reply to comment #5) > 1. rename node::isContentEditable to another name. this should be style-dependent. > 2. add isContentEditable back to HTMLElement. This should be dom-dependent only. I'm not sure what you mean by 2 but HTMLElement::isContentEditable should be calling the function renamed in 1. i.e. 2 should look like: bool HTMLElement::isContentEditable() { document()->updateStyleIfNeeded(); return rendererIsEditable(); }
Chang Shu
Comment 7 2011-03-17 12:16:17 PDT
> > 1. rename node::isContentEditable to another name. this should be style-dependent. > > 2. add isContentEditable back to HTMLElement. This should be dom-dependent only. > > I'm not sure what you mean by 2 but HTMLElement::isContentEditable should be calling the function renamed in 1. i.e. 2 should look like: > > bool HTMLElement::isContentEditable() > { > document()->updateStyleIfNeeded(); > return rendererIsEditable(); > } What I thought was that the above is part of step 1 (Not sure if we like to call updateStyle). So the JS interface won't break. As the next step, we should remove the call to Node::rendererIsEditable() in HTMLElement::isContentEditable (because it's not reliable) and check the DOM properties. The code may look like: value = contentEditable(); if (value == "inherit") return parent->contentEditable; else return value;
Ryosuke Niwa
Comment 8 2011-03-17 12:32:19 PDT
(In reply to comment #7) > What I thought was that the above is part of step 1 (Not sure if we like to call updateStyle). So the JS interface won't break. > As the next step, we should remove the call to Node::rendererIsEditable() in HTMLElement::isContentEditable (because it's not reliable) and check the DOM properties. The code may look like: > value = contentEditable(); > if (value == "inherit") > return parent->contentEditable; > else > return value; Why? That value must match that of isContentEditable, right? Otherwise, WebCore treats the node as editable while script treats it non-editable. How does that make sense?
Chang Shu
Comment 9 2011-03-17 13:04:03 PDT
(In reply to comment #8) > (In reply to comment #7) > > What I thought was that the above is part of step 1 (Not sure if we like to call updateStyle). So the JS interface won't break. > > As the next step, we should remove the call to Node::rendererIsEditable() in HTMLElement::isContentEditable (because it's not reliable) and check the DOM properties. The code may look like: > > value = contentEditable(); > > if (value == "inherit") > > return parent->contentEditable; > > else > > return value; > > Why? That value must match that of isContentEditable, right? Otherwise, WebCore treats the node as editable while script treats it non-editable. How does that make sense? I thought we had some issues (performance?) by calling updateStyle in isContentEditable. Maybe it's no longer a problem after bug 54292? :) Then, you are right. No need to call DOM properties.
Chang Shu
Comment 10 2011-03-17 13:10:38 PDT
I saw the following code in WebCore/dom/Node.idl. Why is this necessary? This code prevents us renaming Node::isContentEdiable on Mac without doing something extra. #if defined(LANGUAGE_OBJECTIVE_C) && LANGUAGE_OBJECTIVE_C // Objective-C extensions readonly attribute boolean isContentEditable; #endif /* defined(LANGUAGE_OBJECTIVE_C) */
Ryosuke Niwa
Comment 11 2011-03-17 13:39:09 PDT
(In reply to comment #9) > I thought we had some issues (performance?) by calling updateStyle in isContentEditable. Maybe it's no longer a problem after bug 54292? :) > Then, you are right. No need to call DOM properties. I don't think so. We have security and performance implications if we were to add updateLayout in modify isContentEditable but if we won't have those problems if we're doing this rename.
Chang Shu
Comment 12 2011-03-17 14:36:05 PDT
Created attachment 86094 [details] fix patch
WebKit Review Bot
Comment 13 2011-03-17 14:38:15 PDT
Attachment 86094 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/editing/IndentOutdentCommand.cpp:123: One space before end of line comments [whitespace/comments] [5] Source/WebCore/editing/htmlediting.cpp:760: One space before end of line comments [whitespace/comments] [5] Total errors found: 2 in 51 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Adler
Comment 14 2011-03-17 14:52:17 PDT
Comment on attachment 86094 [details] fix patch View in context: https://bugs.webkit.org/attachment.cgi?id=86094&action=review > Source/WebCore/dom/Node.h:333 > +#if PLATFORM(MAC) > + // Objective-C extensions > + bool isContentEditable() const { return rendererIsContentEditable(Editable); } > +#endif This is OK for the short term, but not great. Longer term the auto-generated isContentEditable method should be removed. If we implement custom methods for ObjC, then the method could go into a file such as DOMNodeCustom.mm. Or the method can go in the DOMNodeExtensions section of DOM.mm, where the boundingBox and lineBoxRects methods are. The only problem I can think of with that is that the if we move the method to DOMNodeExtensions, then the property declaration would need to move from the DOMNode.h and DOMHTMLElement.h headers into the DOMExtensions.h header, which might break compilation of some existing code. It would be fine at runtime, just a problem at compile time.
Ryosuke Niwa
Comment 15 2011-03-17 14:53:54 PDT
Comment on attachment 86094 [details] fix patch View in context: https://bugs.webkit.org/attachment.cgi?id=86094&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:664 > - if (body && body->isContentEditable()) > + if (body && body->rendererIsContentEditable()) rendererIsContentEditable might be a verbose. How about rendererIsEditable/rendererIsRichlyEditable? > Source/WebCore/dom/Document.cpp:322 > - ASSERT(node->isContentEditable()); > + ASSERT(node->rendererIsContentEditable()); I don't think this is right. acceptsEditingFocus is called in Document::setFocusedNode immediately after dispatching focusoutEvent and DOMFocusOutEvent, both of which could modify DOM. We should be calling isContentEditable() here so that style recalc. is trigged if needed. > Source/WebCore/dom/Node.cpp:743 > - return isContentEditable(); > + return rendererIsContentEditable(); This function is called in FocusController::setFocusedNode. For the same reason above, we need to be calling isContentEditable. > Source/WebCore/dom/Node.h:333 > +#if PLATFORM(MAC) > + // Objective-C extensions > + bool isContentEditable() const { return rendererIsContentEditable(Editable); } > +#endif I don't think this is right. It needs to be a virtual function because we're overriding it in HTMLElement. But I think a cleaner implementation is to add non-virtual Node::isContentEditable in all platforms. > Source/WebCore/html/HTMLElement.cpp:663 > +bool HTMLElement::isContentEditable() const > +{ > + return rendererIsContentEditable(); > +} As I commented above, the correctness of this patch depends on the final version of this function that triggers style recalc. For that reason, I think we should do the rename and fix isContentEditable in one patch.
Chang Shu
Comment 16 2011-03-17 15:01:43 PDT
> This is OK for the short term, but not great. > > Longer term the auto-generated isContentEditable method should be removed. If we implement custom methods for ObjC, then the method could go into a file such as DOMNodeCustom.mm. Or the method can go in the DOMNodeExtensions section of DOM.mm, where the boundingBox and lineBoxRects methods are. > > The only problem I can think of with that is that the if we move the method to DOMNodeExtensions, then the property declaration would need to move from the DOMNode.h and DOMHTMLElement.h headers into the DOMExtensions.h header, which might break compilation of some existing code. It would be fine at runtime, just a problem at compile time. Thanks, I was thinking doing this but I chose a lazy approach. :) I will do the custom method. Would it be possible to remove this support completely?
Build Bot
Comment 17 2011-03-17 15:05:27 PDT
Chang Shu
Comment 18 2011-03-17 15:10:20 PDT
> rendererIsContentEditable might be a verbose. How about rendererIsEditable/rendererIsRichlyEditable? any other votes? :) > > > Source/WebCore/dom/Document.cpp:322 > > - ASSERT(node->isContentEditable()); > > + ASSERT(node->rendererIsContentEditable()); > > I don't think this is right. acceptsEditingFocus is called in Document::setFocusedNode immediately after dispatching focusoutEvent and DOMFocusOutEvent, both of which could modify DOM. We should be calling isContentEditable() here so that style recalc. is trigged if needed. > > > Source/WebCore/dom/Node.cpp:743 > > - return isContentEditable(); > > + return rendererIsContentEditable(); > > This function is called in FocusController::setFocusedNode. For the same reason above, we need to be calling isContentEditable. > > > Source/WebCore/dom/Node.h:333 > > +#if PLATFORM(MAC) > > + // Objective-C extensions > > + bool isContentEditable() const { return rendererIsContentEditable(Editable); } > > +#endif > > I don't think this is right. It needs to be a virtual function because we're overriding it in HTMLElement. But I think a cleaner implementation is to add non-virtual Node::isContentEditable in all platforms. > > > Source/WebCore/html/HTMLElement.cpp:663 > > +bool HTMLElement::isContentEditable() const > > +{ > > + return rendererIsContentEditable(); > > +} > > As I commented above, the correctness of this patch depends on the final version of this function that triggers style recalc. For that reason, I think we should do the rename and fix isContentEditable in one patch. For the comments above, the goal of this patch is to do the change literally without fixing any existing issues. If this patch does not break anything, I think we can split the work of renaming and fixing bugs. The updated test results should be included if I fix any existing issues.
Ryosuke Niwa
Comment 19 2011-03-17 15:19:23 PDT
(In reply to comment #18) > > As I commented above, the correctness of this patch depends on the final version of this function that triggers style recalc. For that reason, I think we should do the rename and fix isContentEditable in one patch. > > For the comments above, the goal of this patch is to do the change literally without fixing any existing issues. If this patch does not break anything, I think we can split the work of renaming and fixing bugs. The updated test results should be included if I fix any existing issues. Okay. I guess that approach works as well.
Chang Shu
Comment 20 2011-03-21 14:52:41 PDT
> Longer term the auto-generated isContentEditable method should be removed. If we implement custom methods for ObjC, then the method could go into a file such as DOMNodeCustom.mm. Or the method can go in the DOMNodeExtensions section of DOM.mm, where the boundingBox and lineBoxRects methods are. > > The only problem I can think of with that is that the if we move the method to DOMNodeExtensions, then the property declaration would need to move from the DOMNode.h and DOMHTMLElement.h headers into the DOMExtensions.h header, which might break compilation of some existing code. It would be fine at runtime, just a problem at compile time. Would it be ok to put the method in DOM.mm but not inside the DOMExtensions section?
Darin Adler
Comment 21 2011-03-21 15:02:28 PDT
(In reply to comment #20) > Would it be ok to put the method in DOM.mm but not inside the DOMExtensions section? The method needs to be declared in a header as well as defined in an implementation file. The location of the definition is unimportant; the location of the declaration is important. Does that answer your question?
Chang Shu
Comment 22 2011-03-21 16:43:04 PDT
(In reply to comment #21) > (In reply to comment #20) > > Would it be ok to put the method in DOM.mm but not inside the DOMExtensions section? > > The method needs to be declared in a header as well as defined in an implementation file. The location of the definition is unimportant; the location of the declaration is important. > > Does that answer your question? Yeah, for one second, I thought the generated header file would have the declaration.
Chang Shu
Comment 23 2011-03-22 06:28:06 PDT
Hi, Darin, correct me if I am wrong. It looks to me the function is always declared in the generated header file (DOMNode.h) as long as isContentEditable is put in the idl file even it's custom type.
Chang Shu
Comment 24 2011-03-22 07:23:57 PDT
I guess I am a bit confused here. I added Custom to isContentEditable in Node.idl like this: readonly attribute [Custom] boolean isContentEditable; But I don't see any changes in the generated .h and .mm files comparing to the ones generated without [Custom].
Chang Shu
Comment 25 2011-03-22 14:08:07 PDT
Created attachment 86501 [details] fix patch 2
Chang Shu
Comment 26 2011-03-22 14:09:05 PDT
Comment on attachment 86501 [details] fix patch 2 [Custom] for ObjC is not working so I am back to the original hack for Node::isContentEditable.
Ryosuke Niwa
Comment 27 2011-03-22 14:32:31 PDT
Comment on attachment 86501 [details] fix patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=86501&action=review > Source/WebCore/html/HTMLElement.h:61 > + virtual bool isContentEditable() const; > + I don't think this function needs to virtual.
Ryosuke Niwa
Comment 28 2011-03-22 14:34:39 PDT
I guess we'll figure out what to do with objective C++ binding and replace some of call sites to call new version of isContentEditable in follow up bugs. Is that right?
Chang Shu
Comment 29 2011-03-22 16:02:37 PDT
(In reply to comment #28) > I guess we'll figure out what to do with objective C++ binding and replace some of call sites to call new version of isContentEditable in follow up bugs. Is that right? Yes, exactly.
Chang Shu
Comment 30 2011-03-22 16:03:17 PDT
(In reply to comment #27) > (From update of attachment 86501 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=86501&action=review > > > Source/WebCore/html/HTMLElement.h:61 > > + virtual bool isContentEditable() const; > > + > > I don't think this function needs to virtual. Right, will fix.
Chang Shu
Comment 31 2011-03-22 16:07:02 PDT
(In reply to comment #29) > (In reply to comment #28) > > I guess we'll figure out what to do with objective C++ binding and replace some of call sites to call new version of isContentEditable in follow up bugs. Is that right? > > Yes, exactly. Btw, to fix the call sites, shall we cast node to htmlelement and call isContentEditable?
Chang Shu
Comment 32 2011-03-23 12:10:30 PDT
> > Source/WebCore/html/HTMLElement.h:61 > > + virtual bool isContentEditable() const; > > I don't think this function needs to virtual. any other comments, niwa?
Ryosuke Niwa
Comment 33 2011-03-23 19:01:36 PDT
(In reply to comment #31) > Btw, to fix the call sites, shall we cast node to htmlelement and call isContentEditable? That doesn't sound right. We probably need to add back Node::isContentEditable later. (In reply to comment #32) > > > Source/WebCore/html/HTMLElement.h:61 > > > + virtual bool isContentEditable() const; > > > > I don't think this function needs to virtual. > > any other comments, niwa? Not at the moment.
Chang Shu
Comment 34 2011-03-24 06:49:32 PDT
Created attachment 86768 [details] fix patch 3
Ryosuke Niwa
Comment 35 2011-03-25 01:45:09 PDT
Comment on attachment 86768 [details] fix patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=86768&action=review > Source/WebCore/ChangeLog:11 > + Code optimization. No new tests. Nit: this isn't really an optimization, right? It's more of a refactoring. > Source/WebKit/chromium/src/WebNode.cpp:150 > - return m_private->isContentEditable(); > + return m_private->rendererIsEditable(); It's not great that WebKit code is referring to renderIsEditable. Ideally, rendererIsEditable will be WebCore's implementation detail and won't be exposed to WebKit layer at all. We can of course make such a change when you add updateStyleIfNeeded to isContentEditable.
Chang Shu
Comment 36 2011-03-25 04:57:24 PDT
> Nit: this isn't really an optimization, right? It's more of a refactoring. Right, wrong wording. :) > It's not great that WebKit code is referring to renderIsEditable. Ideally, rendererIsEditable will be WebCore's implementation detail and won't be exposed to WebKit layer at all. We can of course make such a change when you add updateStyleIfNeeded to isContentEditable. sure. The follow-up patch should come soon.
Ryosuke Niwa
Comment 37 2011-03-25 06:23:22 PDT
(In reply to comment #36) > > Nit: this isn't really an optimization, right? It's more of a refactoring. > > Right, wrong wording. :) Could you fix that for the landing?
Chang Shu
Comment 38 2011-03-25 06:37:25 PDT
Created attachment 86931 [details] fix wording
WebKit Commit Bot
Comment 39 2011-03-25 09:18:08 PDT
The commit-queue encountered the following flaky tests while processing attachment 86931 [details]: http/tests/websocket/tests/simple.html bug 55325 (author: abarth@webkit.org) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 40 2011-03-25 09:21:51 PDT
Comment on attachment 86931 [details] fix wording Clearing flags on attachment: 86931 Committed r81965: <http://trac.webkit.org/changeset/81965>
WebKit Commit Bot
Comment 41 2011-03-25 09:21:57 PDT
All reviewed patches have been landed. Closing bug.
Chang Shu
Comment 42 2011-03-25 11:15:11 PDT
Hi, Ryosuke, Darin, I still want to know you guys' opinion on whether we bring back the virtual isContentEditable in Node, or cast the Node to HTMLElement in so many places. Thanks
Ryosuke Niwa
Comment 43 2011-03-25 12:54:01 PDT
(In reply to comment #42) > Hi, Ryosuke, Darin, > I still want to know you guys' opinion on whether we bring back the virtual isContentEditable in Node, or cast the Node to HTMLElement in so many places. We should add back non-virtual isContentEditable back in Node.
Note You need to log in before you can comment on or make changes to this bug.