[webkit-reviews] review granted: [Bug 89526] Crash at WebCore::TextIterator::handleTextBox : [Attachment 149546] Patch revised

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Alice Cheng
<alice_cheng at apple.com>'s request for review:
Bug 89526: Crash at WebCore::TextIterator::handleTextBox
https://bugs.webkit.org/show_bug.cgi?id=89526

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

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


> Source/WebCore/ChangeLog:11
> +	   Reviewed by NOBODY (OOPS!).

This line should appear before the long description.

> Source/WebCore/editing/AlternativeTextController.cpp:279
> +    // Recalculate pragraphRangeContainingCorrection, since
SpellingCorrectionCommand modified the DOM, such that the original
paragraphRangeContainingCorrection is no longer valid, especially in
blockquotes.

Also I'm not certain "especially in blockquote" adds any valuable information
here. If anything, it's probably better refer to the bug like "See bug 89526"
but I would just omit that if I were you.

>> Source/WebCore/editing/Editor.cpp:2125
>> +		    //Recalculate newParagraphRange, since
SpellingCorrectionCommand modifies the DOM, such that the original paragraph
range is no longer valid, especially in blockquotes.
> 
> Should have a space between // and comment  [whitespace/comments] [4]

Ditto, and please fix this style.

>
LayoutTests/platform/mac/editing/spelling/autocorrection-blockquote-crash.html:
20
> +    if(window.internals)
> +	   shouldBeTrue('internals.hasAutocorrectedMarker(document, 0, 1)');

Doesn't auto correction occurs asynchronously? Most importantly, did you verify
that the crash reproduces when this test is ran inside DRT?