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

Wed Jun 13 15:14:26 PDT 2012

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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #147272|review?                     |review-
               Flag|                            |

--- Comment #9 from Ryosuke Niwa <rniwa at webkit.org>  2012-06-13 15:14:25 PST ---
(From update of attachment 147272)
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.

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