Bug 57173 – REGRESSION (r79411): "Check grammar with spelling" context menu doesn't check as you type

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Adele Peterson]

When I fixed some of the spelling context menu items to toggle correctly, I noticed that when the "check grammar with spelling" item is checked, it doesn't seem to do anything as you type.  You can still do grammar checking from when you bring up the panel though.

Tracked this down to:
http://trac.webkit.org/changeset/79411

The issue is that "paragraph" is used in places that should be using "spellingParagraph".  I have a fix.

<rdar://problem/9112694>

[Adele Peterson]

Created attachment 87057 [details]
patch

[Adele Peterson]

Comment on attachment 87057 [details]
patch

Need to add the new test to some Skipped lists.  I'll post a new patch.

[Adele Peterson]

Created attachment 87058 [details]
patch

Now with some Skipped entries for the new test.

[Build Bot]

Attachment 87057 [details] did not build on win:
Build output: http://queues.webkit.org/results/8257922

[Eric Seidel (no email)]

Comment on attachment 87058 [details]
patch

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

This seems fine.  I really like that you added DRT capability for this.  But this also could use one more round of cleanup.  I don't really need to see the new patch though, but I'd be happy to review it if you're willing to post.

> Source/WebCore/editing/Editor.cpp:2287
> +        textChecker()->checkTextOfParagraph(grammarParagraph.textCharacters(), grammarParagraph.textLength(), checkingTypes, results);
> +    else
> +        textChecker()->checkTextOfParagraph(spellingParagraph.textCharacters(), spellingParagraph.textLength(), checkingTypes, results);

This seems like a helper functoin.  Why does this take Uchar* instead of a String& variant?

> Source/WebCore/editing/Editor.cpp:3578
> +bool Editor::selectionStartHasMarkerFor(DocumentMarker::MarkerType type, int from, int length) const

I would have called this markerType instead of "type" to be less confusing.

> Source/WebKit/mac/WebView/WebFrame.mm:1323
> +    Frame* coreFrame = _private->coreFrame;
> +    if (!coreFrame)
> +        return NO;

Isn't this just a core(self) call?  Do we have any way to test this branch?

> Source/WebKit/mac/WebView/WebFramePrivate.h:146
> +- (BOOL)hasGrammarMarker:(int)from length:(int)length;

I'm surprised these are ints (and not size_t, etc).  But you know the Mac API conventions better than I do these days!

> Tools/DumpRenderTree/LayoutTestController.cpp:1966
> +static JSValueRef hasGrammarMarkerCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)

It's a shame we don't autogen this stuff yet. :)

> LayoutTests/editing/spelling/grammar.html:38
> +    typeCharacterCommand('I');
> +    typeCharacterCommand(' ');
> +    typeCharacterCommand('h');
> +    typeCharacterCommand('a');
> +    typeCharacterCommand('v');
> +    typeCharacterCommand('e');
> +    typeCharacterCommand(' ');
> +    typeCharacterCommand('a');
> +    typeCharacterCommand(' ');
> +    typeCharacterCommand('i');
> +    typeCharacterCommand('s');
> +    typeCharacterCommand('s');
> +    typeCharacterCommand('u');
> +    typeCharacterCommand('e');
> +    typeCharacterCommand('.');

I'm surprised we don't have a nicer way to do this?  You might consider making a local JS function "typeString()" which takes a string and does  for loop. :)

[Adele Peterson]

Comment on attachment 87058 [details]
patch

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

Thanks for the review!

>> Source/WebCore/editing/Editor.cpp:2287
>> +        textChecker()->checkTextOfParagraph(spellingParagraph.textCharacters(), spellingParagraph.textLength(), checkingTypes, results);
> 
> This seems like a helper functoin.  Why does this take Uchar* instead of a String& variant?

Good question.  I'm not sure.

>> Source/WebCore/editing/Editor.cpp:3578
>> +bool Editor::selectionStartHasMarkerFor(DocumentMarker::MarkerType type, int from, int length) const
> 
> I would have called this markerType instead of "type" to be less confusing.

ok - will change.

>> Source/WebKit/mac/WebView/WebFrame.mm:1323
>> +        return NO;
> 
> Isn't this just a core(self) call?  Do we have any way to test this branch?

I just made this match hasSpellingMarker, but I can change this to core(self).  I don't think we can test this branch, but I wouldn't want to remove it without knowing more about it.

>> Source/WebKit/mac/WebView/WebFramePrivate.h:146
>> +- (BOOL)hasGrammarMarker:(int)from length:(int)length;
> 
> I'm surprised these are ints (and not size_t, etc).  But you know the Mac API conventions better than I do these days!

Again, I just made it match hasSpellingMarker.  You're probably right though.

>> LayoutTests/editing/spelling/grammar.html:38
>> +    typeCharacterCommand('.');
> 
> I'm surprised we don't have a nicer way to do this?  You might consider making a local JS function "typeString()" which takes a string and does  for loop. :)

Yeah, I just realized this test works just as well calling the InsertText command directly.  I'll make that change.

[Jia Pu]

Comment on attachment 87058 [details]
patch

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

>>> Source/WebCore/editing/Editor.cpp:2287
>>> +        textChecker()->checkTextOfParagraph(spellingParagraph.textCharacters(), spellingParagraph.textLength(), checkingTypes, results);
>> 
>> This seems like a helper functoin.  Why does this take Uchar* instead of a String& variant?
> 
> Good question.  I'm not sure.

I guess this is to avoid constructing temporary string object for substring that we want to check.

>>> LayoutTests/editing/spelling/grammar.html:38
>>> +    typeCharacterCommand('.');
>> 
>> I'm surprised we don't have a nicer way to do this?  You might consider making a local JS function "typeString()" which takes a string and does  for loop. :)
> 
> Yeah, I just realized this test works just as well calling the InsertText command directly.  I'll make that change.

If I remember correctly, using InsertText() will not trigger markMisspellingsAfterTypingToWord() after each word. But it's probably irrelevant here.

[Adele Peterson]

Committed revision 82159.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82159 might have broken Windows Release (Build), Windows Debug (Build), and Chromium Linux Release

[Adele Peterson]

Follow up build fix in revision 82164.

[Adele Peterson]

And... committed revision 82166.

[Adam Barth]

http://trac.webkit.org/changeset/82159

Appears to have cause this test failure on Leopard:

http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r82398%20(30362)/editing/spelling/grammar-pretty-diff.html

The newly added test appears to fail on Leopard.