Bug 54290

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.

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.

do we need this any more after 56421 is done?

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

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

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

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

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

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

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) */

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

Created attachment 86094 [details] fix patch

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.

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.

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.

> 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?

Attachment 86094 [details] did not build on win: Build output: http://queues.webkit.org/results/8187942

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

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

> 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?

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

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

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.

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

Created attachment 86501 [details] fix patch 2

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.

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.

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?

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

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

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

> > Source/WebCore/html/HTMLElement.h:61 > > + virtual bool isContentEditable() const; > > I don't think this function needs to virtual. any other comments, niwa?

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

Created attachment 86768 [details] fix patch 3

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.

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

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

Created attachment 86931 [details] fix wording

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.

Comment on attachment 86931 [details] fix wording Clearing flags on attachment: 86931 Committed r81965: <http://trac.webkit.org/changeset/81965>

All reviewed patches have been landed. Closing bug.

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

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