[webkit-reviews] review denied: [Bug 88838] Delete Grammar markers on editing : [Attachment 147272] Patch v1

Wed Jun 13 15:14:25 PDT 2012

Ryosuke Niwa <rniwa at webkit.org> has denied 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 147272: Patch v1
https://bugs.webkit.org/attachment.cgi?id=147272&action=review

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