[Webkit-unassigned] [Bug 89526] Crash at WebCore::TextIterator::handleTextBox

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=89526


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #149048|review?                     |review+
               Flag|                            |




--- Comment #5 from Darin Adler <darin at apple.com>  2012-06-22 10:40:23 PST ---
(From update of attachment 149048)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.