Bug 57173 - REGRESSION (r79411): "Check grammar with spelling" context menu doesn't check as you type
Summary: REGRESSION (r79411): "Check grammar with spelling" context menu doesn't check...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Adele Peterson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-03-26 23:12 PDT by Adele Peterson
Modified: 2011-03-30 00:13 PDT (History)
4 users (show)

See Also:


Attachments
patch (18.98 KB, patch)
2011-03-26 23:55 PDT, Adele Peterson
no flags Details | Formatted Diff | Diff
patch (21.39 KB, patch)
2011-03-27 00:20 PDT, Adele Peterson
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adele Peterson 2011-03-26 23:12:38 PDT
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>
Comment 1 Adele Peterson 2011-03-26 23:55:51 PDT
Created attachment 87057 [details]
patch
Comment 2 Adele Peterson 2011-03-27 00:02:51 PDT
Comment on attachment 87057 [details]
patch

Need to add the new test to some Skipped lists.  I'll post a new patch.
Comment 3 Adele Peterson 2011-03-27 00:20:24 PDT
Created attachment 87058 [details]
patch

Now with some Skipped entries for the new test.
Comment 4 Build Bot 2011-03-27 00:58:17 PDT
Attachment 87057 [details] did not build on win:
Build output: http://queues.webkit.org/results/8257922
Comment 5 Eric Seidel (no email) 2011-03-27 01:01:43 PDT
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. :)
Comment 6 Adele Peterson 2011-03-27 01:18:16 PDT
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.
Comment 7 Jia Pu 2011-03-28 09:16:37 PDT
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.
Comment 8 Adele Peterson 2011-03-28 14:48:45 PDT
Committed revision 82159.
Comment 9 WebKit Review Bot 2011-03-28 14:58:57 PDT
http://trac.webkit.org/changeset/82159 might have broken Windows Release (Build), Windows Debug (Build), and Chromium Linux Release
Comment 10 Adele Peterson 2011-03-28 15:07:13 PDT
Follow up build fix in revision 82164.
Comment 11 Adele Peterson 2011-03-28 15:22:55 PDT
And... committed revision 82166.
Comment 12 Adam Barth 2011-03-30 00:13:31 PDT
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.