[webkit-reviews] review requested: [Bug 56372] Crash in ReplaceSelectionCommand::doApply when inserting a node under a document node : [Attachment 85844] fixes the crash

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has asked  for review:
Bug 56372: Crash in ReplaceSelectionCommand::doApply when inserting a node
under a document node
https://bugs.webkit.org/show_bug.cgi?id=56372

Attachment 85844: fixes the crash
https://bugs.webkit.org/attachment.cgi?id=85844&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=85844&action=review

> Source/WebCore/dom/Document.cpp:4134
> +bool Document::isContentEditable() const
> +{
> +    if (document()->inDesignMode())
> +	   return true;
> +
> +    return renderer() && (renderer()->style()->userModify() == READ_WRITE ||
renderer()->style()->userModify() == READ_WRITE_PLAINTEXT_ONLY);
> +}
> +
> +bool Document::isContentRichlyEditable() const
> +{
> +    if (document()->inDesignMode())
> +	   return true;
> +
> +    return renderer() && renderer()->style()->userModify() == READ_WRITE;
> +}

Is there a way we can share code with the HTMLElement functions? It’s not good
to have a near exact copy of those functions in the document class.

Also, it’s strange to call document() in a function on Document.

> Source/WebCore/dom/Document.h:898
> +    virtual bool isContentEditable() const;
> +    virtual bool isContentRichlyEditable() const;

We often make this sort of function private, because if it’s going to be called
in performance-critical code and you have a Document*, then it would be better
to have a non-virtual version of the function. I think that makes sense in this
case. Nobody needs to call isContentEditable on a Document* yet. If they did we
could provide something slightly faster.