Summary: | REGRESSION (r79411): "Check grammar with spelling" context menu doesn't check as you type | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adele Peterson <adele> | ||||||
Component: | HTML Editing | Assignee: | Adele Peterson <adele> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, buildbot, eric, webkit.review.bot | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Adele Peterson
2011-03-26 23:12:38 PDT
Created attachment 87057 [details]
patch
Comment on attachment 87057 [details]
patch
Need to add the new test to some Skipped lists. I'll post a new patch.
Created attachment 87058 [details]
patch
Now with some Skipped entries for the new test.
Attachment 87057 [details] did not build on win: Build output: http://queues.webkit.org/results/8257922 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 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 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. Committed revision 82159. http://trac.webkit.org/changeset/82159 might have broken Windows Release (Build), Windows Debug (Build), and Chromium Linux Release Follow up build fix in revision 82164. And... committed revision 82166. 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. |