Bug 54290 – rename Node::isContentEditable and all call sites to rendererIsEditable

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chang Shu]

This effort is to differentiate Node::isContentEditable, which is for internal use, from HTMLElement::isContentEditable, which is a DOM interface.

[Darin Adler]

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]

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]

do we need this any more after 56421 is done?

[Ryosuke Niwa]

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

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

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

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

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

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

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]

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

Created attachment 86094 [details]
fix patch

[WebKit Review Bot]

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

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

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

[Chang Shu]

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

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

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

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

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

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]

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]

Created attachment 86501 [details]
fix patch 2

[Chang Shu]

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

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]

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

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

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

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

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

Created attachment 86768 [details]
fix patch 3

[Ryosuke Niwa]

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]

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

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

Created attachment 86931 [details]
fix wording

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 86931 [details]:

http/tests/websocket/tests/simple.html bug 55325 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 86931 [details]
fix wording

Clearing flags on attachment: 86931

Committed r81965: <http://trac.webkit.org/changeset/81965>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chang Shu]

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]

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