Delete Grammar markers on editing

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Hironori Bono]

Greetings,

Sorry for my stupid mistakes.
Even though I my r113405 <http://trac.webkit.org/changeset/113405> prevent a selection change from removing markers as well as OS X Lion, I forgot setting WTF_USE_MARKER_REMOVAL_UPON_EDITING to 1 so WebKit can remove them when we edit misspelled words as Safari does. Also, Editor::updateMarkersForWordsAffectedByEditing() somehow does not remove grammar markers. It would be definitely helpful for Chromium to set the WTF_USE_MARKER_REMOVAL_UPON_EDITING flag and change the function to remove grammar markers as well?

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 147046 [details]
Patch v0 (only for running on EWS bots)

Greetings,

Even though I have written a fix, its layout tests needs to implement an unimplemented function and I wonder if it works as I expected. (I have tested this change only on Windows.) I would like to set r? just to run this change on EWS bots.

Regards,

Hironori Bono

[Ryosuke Niwa]

(In reply to comment #1)
> Even though I have written a fix, its layout tests needs to implement an unimplemented function and I wonder if it works as I expected. (I have tested this change only on Windows.) I would like to set r? just to run this change on EWS bots.

You don't need to set r? in order to submit your patch to EWS. Just click on "Submit to EWS".

[WebKit Review Bot]

Comment on attachment 147046 [details]
Patch v0 (only for running on EWS bots)

Attachment 147046 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12937926

New failing tests:
editing/deleting/delete-to-select-table.html
platform/chromium/editing/spelling/move-cursor-to-misspelled-word.html
editing/style/create-block-for-style-011.html
editing/style/create-block-for-style-012.html

[WebKit Review Bot]

Created attachment 147105 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 147046 [details]
Patch v0 (only for running on EWS bots)

Attachment 147046 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12944605

New failing tests:
editing/deleting/delete-to-select-table.html
platform/chromium/editing/spelling/move-cursor-to-misspelled-word.html
editing/style/create-block-for-style-011.html
editing/style/create-block-for-style-012.html

[WebKit Review Bot]

Created attachment 147116 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hironori Bono]

Greetings Niwa-san,

Many thanks for your advice. I totally did not notice it.

Regards,

Hironori Bono

(In reply to comment #2)
> You don't need to set r? in order to submit your patch to EWS. Just click on "Submit to EWS".

[Hironori Bono]

Created attachment 147272 [details]
Patch v1

Greetings,

While investigating layout-test errors, I notice it may be better to call TextCheckClient::shouldEraseMarkersAfterChangeSelection() in Editor::updateMarkersForWordsAffectedByEditing() instead of setting WTF_USE_MARKER_REMOVAL_UPON_EDITING to 1. (We need to delete markers on editing only when this function returns false. If this function returns true, WebKit removes markers when we start editing grammatically-incorrect words.)

Regards,

Hironori Bono

[Ryosuke Niwa]

Comment on attachment 147272 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=147272&action=review

r- because I think we want one more iteration.

> Source/WebCore/editing/Editor.cpp:2207
> +    if (!m_alternativeTextController->shouldRemoveMarkersUponEditing()) {
> +        if (!textChecker() || textChecker()->shouldEraseMarkersAfterChangeSelection(TextCheckingTypeGrammar))

It seems like we can combine these two if-statements.
But why do we need to bail out when shouldEraseMarkersAfterChangeSelection is true?
Please explain that in the change log.

> Source/WebKit/chromium/src/EditorClientImpl.cpp:772
> +void EditorClientImpl::checkGrammarOfString(const UChar* text, int length,
> +                                            WTF::Vector<GrammarDetail>& details,
>                                              int* badGrammarLocation,
>                                              int* badGrammarLength)

I'd prefer putting all arguments in single line given that we no longer have 80-character limit in WebKit/chromium code.

> Source/WebKit/chromium/src/EditorClientImpl.cpp:788
> +    WebVector<WebTextCheckingResult> webResults;
> +    m_webView->spellCheckClient()->checkTextOfParagraph(WebString(text, length), WebTextCheckingTypeGrammar, &webResults);
> +    if (!webResults.size())
> +        return;

We can make use of some blank lines here.

> Source/WebKit/chromium/src/EditorClientImpl.cpp:790
> +    int start = length;
> +    int end = 0;

Why are we using these initial values? They look completely arbitrary to me.

> Source/WebKit/chromium/src/EditorClientImpl.cpp:798
> +    for (size_t i = 0; i < webResults.size(); ++i) {
> +        if (webResults[i].type == WebTextCheckingTypeGrammar) {
> +            int errorStart = webResults[i].location;
> +            int errorEnd = webResults[i].location + webResults[i].length;
> +            start = (start > errorStart) ? errorStart : start;
> +            end = (end < errorEnd) ? errorEnd : end;
> +        }
> +    }

I would extract a helper function that describes what we're doing with start & end.

> Source/WebKit/chromium/src/EditorClientImpl.cpp:800
> +    if (end - start <= 0)
> +        return;

Why is this ever true?

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:476
> +        const WebUChar* data = text.data();

I'm not having this local variable is useful. It's used exactly once when we obtain the pointer at offset.
I'd prefer just doing test.data() + offset instead.

> LayoutTests/editing/spelling/grammar-edit-word.html:1
> +<html>

No DOCTYPE?

> LayoutTests/editing/spelling/grammar-edit-word.html:12
> +description('Test if WebKit removes grammar markers when we edit a grammatically-incorrect word. ' +
> +            'To test manually, type a grammatically-incorrect sentence "You has the right." and ' +
> +            'type a backspace key to delete the last character of "has". ' +
> +            'This test succeeds when "ha" does not have grammar markers.');

We don't normally align indentation like this.
Also, + should appear at the beginning of a line instead.

> LayoutTests/editing/spelling/grammar-edit-word.html:22
> +internals.settings.setUnifiedTextCheckingEnabled(true);
> +var target = document.getElementById('src');
> +target.focus();
> +document.execCommand('InsertText', false, 'You has the right.');
> +shouldBeTrue('internals.hasGrammarMarker(document, 4, 3)');
> +for (var i = 0; i < 12; ++i)
> +    layoutTestController.execCommand('DeleteBackward');
> +shouldBeFalse('internals.hasGrammarMarker(document, 4, 2)');
> +internals.settings.setUnifiedTextCheckingEnabled(false);

Again, we can make use of some blank lines here.

[Hironori Bono]

Created attachment 147523 [details]
Patch v2 (applied comments and updated description)

Greetings Niwa-san,

Thanks for your review and comments. While reading your comments, I notice I just like to allow the WebKit layer to decide whether to delete markers in Editor::updateMarkersForWordsAffectedByEditing(). (I got confused while integrating grammar checkers to EditorClientImpl::checkGrammarOfString().) Even though I have applied all of your comments, it would be definitely helpful to shoot me if this change still has problems.

Regards,

Hironori Bono

[Ryosuke Niwa]

Comment on attachment 147523 [details]
Patch v2 (applied comments and updated description)

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.

[Hironori Bono]

Created attachment 147724 [details]
Patch v3 (for landing)

Greetings Niwa-san,

Thanks for your review and comments.
I have revised some ChangeLog descriptions and updated layout tests. I would like to land this change if it is OK for you.

Regards,

Hironori Bono

[WebKit Review Bot]

Comment on attachment 147724 [details]
Patch v3 (for landing)

Clearing flags on attachment: 147724

Committed r120423: <http://trac.webkit.org/changeset/120423>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #13)
> (From update of attachment 147724 [details])
> Clearing flags on attachment: 147724
> 
> Committed r120423: <http://trac.webkit.org/changeset/120423>

The new test introduced in it fails on GTK and Qt - https://bugs.webkit.org/show_bug.cgi?id=89199

Could you check it, please?

[Beth Dakin]

(In reply to comment #15)
> (In reply to comment #13)
> > (From update of attachment 147724 [details] [details])
> > Clearing flags on attachment: 147724
> > 
> > Committed r120423: <http://trac.webkit.org/changeset/120423>
> 
> The new test introduced in it fails on GTK and Qt - https://bugs.webkit.org/show_bug.cgi?id=89199
> 
> Could you check it, please?

This also fails on the Mac bots. I assume that this is also because of TextChecker::checkGrammarOfString().

[Hironori Bono]

Greetings,

Thanks for noticing it. Even though this test succeeded when I ran it on Lion, I'm going to re-check if it succeeds on my Mac.

Regards,

Hironori Bono

(In reply to comment #16)
> This also fails on the Mac bots. I assume that this is also because of TextChecker::checkGrammarOfString().