[Webkit-unassigned] [Bug 88838] Delete Grammar markers on editing

[bugzilla-daemon at webkit.org]

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


Ryosuke Niwa <rniwa at webkit.org> changed:

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




--- Comment #11 from Ryosuke Niwa <rniwa at webkit.org>  2012-06-14 02:47:47 PST ---
(From update of attachment 147523)
View in context: https://bugs.webkit.org/attachment.cgi?id=147523&action=review

r=me provided the following comments are addressed.

> Source/WebCore/ChangeLog:12
> +        1 so Editor::pdateMarkersForWordsAffectedByEditing can remove markers. This

Nit: *u*pdate

> LayoutTests/ChangeLog:15
> +        This change allows platforms to choose whether to remove markers on a word being
> +        edited. WebKit does not remove markers when we move a selection to a markered
> +        word on platforms that shouldEraseMarkersAfterChangeSelection returns false.
> +        On such platforms, WebKit expects to set WTF_USE_MARKER_REMOVAL_UPON_EDITING to
> +        1 so Editor::pdateMarkersForWordsAffectedByEditing can remove markers. This
> +        change also checks the return value of shouldEraseMarkersAfterChangeSelection so
> +        platform can choose it. This change also adds grammar markers so it can also
> +        remove grammar markers.

We don't normally repeat the description for WebCore in LayoutTests. Here, you should describe what kind of a test you're adding.

> LayoutTests/editing/spelling/grammar-edit-word-expected.txt:8
> +PASS internals.hasGrammarMarker(document, 4, 3) is true
> +PASS internals.hasGrammarMarker(document, 4, 2) is false
> +PASS successfullyParsed is true

This output isn't so helpful. While I can see what has been tested, the correctness of the output isn't self-evident because I can't see the pre-condition of these statements.

> LayoutTests/editing/spelling/grammar-edit-word.html:17
> +// Insert a grammatically-incorrect sentense 'You has the right.' and verify a word 'has' has a grammar marker.
> +document.execCommand('InsertText', false, 'You has the right.');

We wouldn't need this comment if you used evalAndLog to run execCommand here.

> LayoutTests/editing/spelling/grammar-edit-word.html:22
> +for (var i = 0; i < 12; ++i)
> +    layoutTestController.execCommand('DeleteBackward');

Ditto. Also show the remaining text using debug() so that you can delete this comment.

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