[webkit-reviews] review granted: [Bug 57173] REGRESSION (r79411): "Check grammar with spelling" context menu doesn't check as you type : [Attachment 87058] patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Adele Peterson <adele at apple.com>'s
request for review:
Bug 57173: REGRESSION (r79411): "Check grammar with spelling" context menu
doesn't check as you type
https://bugs.webkit.org/show_bug.cgi?id=57173

Attachment 87058: patch
https://bugs.webkit.org/attachment.cgi?id=87058&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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. :)