[webkit-reviews] review granted: [Bug 24953] Add automatic spell correction support in WebKit : [Attachment 29450] Support for automatic spelling corrections

[bugzilla-daemon at webkit.org]

Justin Garcia <justin.garcia at apple.com> has granted Siddhartha Chattopadhyay
<sidchat at google.com>'s request for review:
Bug 24953: Add automatic spell correction support in WebKit
https://bugs.webkit.org/show_bug.cgi?id=24953

Attachment 29450: Support for automatic spelling corrections
https://bugs.webkit.org/attachment.cgi?id=29450&action=review

------- Additional Comments from Justin Garcia <justin.garcia at apple.com>
 #endif // Editor_h
+
+
+
Index: WebCore/editing/TypingCommand.cpp
=======

Looks like you accidently added some unnecessary newlines to the end of this
file.

-    // If the selection starts inside a table, just insert the paragraph
separator normally
-    // Breaking the blockquote would also break apart the table, which is
unecessary when inserting a newline
-    if (enclosingNodeOfType(endingSelection().start(), &isTableStructureNode))
{
-	 insertParagraphSeparator();
-	 return;
-    }
-	

Please remove this before you check in.  If it's a workaround for a bug you're
seeing please file it.

-	 if (p1 != p2)
-	    
document()->frame()->editor()->markMisspellingsAfterTypingToPosition(p1);
+	 if (p1 != p2) {
+	     RefPtr<Range> misspellingRange;
+	     Frame* frame = document()->frame();

Would be nice if you turned this into an early return to avoid the extra level
of if-nesting.

+	     // Autocorrect the misspelled word.
+	     if (misspellingRange.get() != NULL) {
+		 // Get the misspelled word.

In WebCore the convention is to use != 0, and you don't need the .get() here. 
And again, you could turn this into an early return.

+		    
frame->editor()->replaceSelectionWithText(autocorrectedString, false, true);

You actually don't need smart replace here, but it won't matter because smart
replace will only do anything if the selection being replaced is between is
flanked by non-whitespace, which isn't the case when replacing a word.	Your
use of it makes me think that smart replace needs to be renamed or commented
better.

Other than that r=me