[webkit-reviews] review denied: [Bug 131475] Autocorrection causes ASSERT when replacing alternative string : [Attachment 229063] Patch

Thu Apr 10 13:08:38 PDT 2014

Ryosuke Niwa <rniwa at webkit.org> has denied Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 131475: Autocorrection causes ASSERT when replacing alternative string
https://bugs.webkit.org/show_bug.cgi?id=131475

Attachment 229063: Patch
https://bugs.webkit.org/attachment.cgi?id=229063&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=229063&action=review

> Source/WebCore/editing/AlternativeTextController.cpp:278
> -    int paragraphStartIndex =
TextIterator::rangeLength(Range::create(*m_frame.document(),
m_frame.document(), 0,
paragraphRangeContainingCorrection.get()->startContainer(),
paragraphRangeContainingCorrection.get()->startOffset()).get());
> +    // Find the root container for the node, so we can create a range from
that point
> +    Node* rootNode =
paragraphRangeContainingCorrection.get()->startContainer();
> +    ContainerNode* rootContainer = isContainerNode(*rootNode) ?
toContainerNode(rootNode) : rootNode->parentNode();
> +    while (rootContainer->parentNode())
> +	   rootContainer = rootContainer->parentNode();

A better way to do this will be obtaining the tree scope's rootNode.

> ManualTests/autocorrection/autocorrection-accept-crash.html:4
> +<head>
> +</head>

We don't need head.

> ManualTests/autocorrection/autocorrection-accept-crash.html:12
> +<p>
> +<ol>
> +<li>Type "word loadp" in the first box.</li>
> +<li>When the suggestion popup appears, click in the second box.</li>
> +</ol>
> +The test is successful if there is no crash.
> +</p>

p can't contain ol.  Also, please describe what we're testing.

> ManualTests/autocorrection/autocorrection-accept-crash.html:18
> +<p>
> +<input id="t" type="text" spellCheck="true">
> +</p>
> +<p>
> +<textarea id="a"></textarea>
> +</p>

I don't think we need all these p's.