Bug 88838

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

(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".

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

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

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

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

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".

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

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.

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

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.

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

Comment on attachment 147724 [details] Patch v3 (for landing) Clearing flags on attachment: 147724 Committed r120423: <http://trac.webkit.org/changeset/120423>

All reviewed patches have been landed. Closing bug.

(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?

(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().

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().