[webkit-reviews] review granted: [Bug 88838] Delete Grammar markers on editing : [Attachment 147523] Patch v2 (applied comments and updated description)

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Hironori Bono
<hbono at chromium.org>'s request for review:
Bug 88838: Delete Grammar markers on editing
https://bugs.webkit.org/show_bug.cgi?id=88838

Attachment 147523: Patch v2 (applied comments and updated description)
https://bugs.webkit.org/attachment.cgi?id=147523&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.