[webkit-reviews] review granted: [Bug 224259] Nullptr dereference in ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline : [Attachment 425464] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Julian Gonzalez
<julian_a_gonzalez at apple.com>'s request for review:
Bug 224259: Nullptr dereference in
ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline
https://bugs.webkit.org/show_bug.cgi?id=224259

Attachment 425464: Patch

https://bugs.webkit.org/attachment.cgi?id=425464&action=review




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 425464
  --> https://bugs.webkit.org/attachment.cgi?id=425464
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425464&action=review

Our new code has many branches. Our new test does not cover them. I think we
need more tests to cover all the cases that require different logic.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:428
> +    ASSERT(!m_firstNodeInserted || m_firstNodeInserted == node ||
!node->contains(m_firstNodeInserted.get()));
> +    ASSERT(!m_lastNodeInserted || m_lastNodeInserted == node ||
!node->contains(m_lastNodeInserted.get()));

Seems like we should consider "!isDescendantOf" here to shorten these
assertions:

    ASSERT(!m_firstNodeInserted || !m_firstNodeInserted->isDescendantOf(node));
    ASSERT(!m_lastNodeInserted || !m_lastNodeInserted->isDescendantOf(node));

Would be even better if we had a non-member version of isDescendantOf that
understands that null isn’t a descendant of anything.