[webkit-reviews] review denied: [Bug 41423] spelling underline gets lost on backspace : [Attachment 66691] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied MORITA Hajime <morrita at google.com>'s
request for review:
Bug 41423: spelling underline gets lost on backspace
https://bugs.webkit.org/show_bug.cgi?id=41423

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

------- Additional Comments from Tony Chang <tony at chromium.org>
> +document.execCommand("Delete", false);
> +sel.modify("move", "right", "line");
> +shouldBe("textInputController.hasSpellingMarker(0, 6)", "1");

Can you add some more test cases?  I think the current test works without this
patch because the cursor is on "Secand" so the word gets checked when you move
the cursor off of it.  For example, you could also test ForwardDelete or test
having multiple misspelled words on the second line.

> diff --git a/WebCore/editing/CompositeEditCommand.cpp
b/WebCore/editing/CompositeEditCommand.cpp
>      setEndingSelection(VisibleSelection(start, end, DOWNSTREAM));
> +   
document()->frame()->editor()->removeMarkersBeforeMovingParagrah(endingSelectio
n());
>      deleteSelection(false, false, false, false);

Out of curiosity, what happens if you don't remove the markers?  What is the
benefit of removing the markers?

> diff --git a/WebCore/editing/Editor.h b/WebCore/editing/Editor.h
> +    // Invoked from CompositeEditCommand::moveParagraphs()
> +    void removeMarkersBeforeMovingParagrah(const VisibleSelection&);
> +    void markMisspellingsAfterMovingParagrah(const VisibleSelection&);

Did you mean to rename these to what Ojan suggested?  r- because of this, but
will be happy to r+ with the methods renamed.