[webkit-reviews] review granted: [Bug 89526] Crash at WebCore::TextIterator::handleTextBox : [Attachment 149048] Patch with a test

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> 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 149048: Patch with a test
https://bugs.webkit.org/attachment.cgi?id=149048&action=review

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


Does the test cover both fixes? I’m usually concerned when there is a single
test case but two bug fixes.

I’m going to say r=me, but please do write some “why” comments as I request
below.

> Source/WebCore/editing/AlternativeTextController.cpp:279
> +    int paragraphStartIndex =
TextIterator::rangeLength(Range::create(m_frame->document(),
m_frame->document(), 0,
paragraphRangeContainingCorrection.get()->startContainer(),
paragraphRangeContainingCorrection.get()->startOffset()).get());
>      applyCommand(SpellingCorrectionCommand::create(rangeWithAlternative,
alternative));
> +    paragraphRangeContainingCorrection =
TextIterator::rangeFromLocationAndLength(m_frame->document(),
paragraphStartIndex, correctionStartOffsetInParagraph + alternative.length());

This needs a “why” comment. It’s important to recompute
paragraphRangeContainingCorrection after applying the command, but we need to
state why.

> Source/WebCore/editing/Editor.cpp:2125
> +		   int paragraphStartIndex =
TextIterator::rangeLength(Range::create(m_frame->document(),
m_frame->document(), 0, paragraph.paragraphRange()->startContainer(),
paragraph.paragraphRange()->startOffset()).get());
> +		   int paragraphLength =
TextIterator::rangeLength(paragraph.paragraphRange().get());
>		  
applyCommand(SpellingCorrectionCommand::create(rangeToReplace,
result->replacement));
> -
> +		   RefPtr<Range> newParagraphRange =
TextIterator::rangeFromLocationAndLength(m_frame->document(),
paragraphStartIndex, paragraphLength+replacementLength-resultLength);

This needs a “why” comment. It’s important to recompute paragraph after
applying the command, but we need to state why.

>
LayoutTests/platform/mac/editing/spelling/autocorrection-blockquote-crash.html:
13
> +    //do typo

WebKit coding style says we should use a sentence format here:

    // Insert some text with a typographical error in it, so autocorrection
occurs.

>
LayoutTests/platform/mac/editing/spelling/autocorrection-blockquote-crash.html:
19
> +    //test marking

This comment should be omitted. It doesn’t add anything. The code below clearly
checks if hasAutocorrectedMarker is true, so we should only include a comment
if we have something else to say other than what the code itself says.

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

WebKit coding style means there should be a space after the “if” and no braces
around a single line if statement body.