[webkit-reviews] review denied: [Bug 40092] spellcheck does not check pasted text : [Attachment 61488] Draft implementation #3

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Hironori Bono <hbono at chromium.org>'s
request for review:
Bug 40092: spellcheck does not check pasted text
https://bugs.webkit.org/show_bug.cgi?id=40092

Attachment 61488: Draft implementation #3
https://bugs.webkit.org/attachment.cgi?id=61488&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
WebCore/editing/Editor.cpp:1172
 +	String text;
I would have put this "get me the text I need to process" block in its own
function to make this one simpler.

WebCore/editing/Editor.cpp:1187
 +  Node* Editor::getSpellingNode(Node* node)
We don't use get in names in WebKit.

WebCore/editing/Editor.cpp:1236
 +	m_spellingNode = 0;
This could be a set or reset function given how you always set these both at
once.  Maybe these need to be a struct held by the object?

WebCore/editing/Editor.cpp:1207
 +	if (node) {
Early return is almost always cleaner.

WebCore/editing/Editor.cpp:1220
 +		Position end(start);
We have a PositionIterator for this sort of thing.

WebCore/editing/Editor.cpp:1230
 +		String source(m_spellingText.substring(results[i].location,
results[i].length));
Personally I tend to use = over assignment constructors myself.

WebCore/editing/Editor.h:70
 +	    : type(DocumentMarker::Spelling), location(0), length(0) { }
Each assignment gets its own line.

WebCore/editing/Editor.h:350
 +	Node* getSpellingNode(Node*);
Again, no "get" functions in WebKit.

WebKit/mac/ChangeLog:31
 +  2010-07-14	Hironori Bono  <hbono at chromium.org>
Double ChangeLog entry!

WebKit/mac/WebCoreSupport/WebEditorClient.mm:827
 +  #if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_6 &&
NS_BLOCKS_AVAILABLE
NS_BLOCKS_AVAILABLE?

WebKit/mac/WebCoreSupport/WebEditorClient.mm:839
 +	[[NSSpellChecker sharedSpellChecker] requestCheckingOfString:text
range:NSMakeRange(0, text.length()) types:NSTextCheckingAllSystemTypes
options:0 inSpellDocumentWithTag:0 completionHandler:^(NSInteger
sequenceNumber, NSArray *results, NSOrthography *orthography, NSInteger
wordCount) {
Woh.  Please split this into multiple lines using local variables.

WebKit/mac/WebView/WebFrame.mm:1596
 +	    switch ([[results objectAtIndex:i] resultType]) {
I would have put this switch into an inline function instead of inlining it
here to make this loop easier to read.

WebKit/mac/WebView/WebFrame.mm:1614
 +  static Node* getSpellingNode(Frame* coreFrame)
No "get" in function names.