[webkit-reviews] review denied: [Bug 25330] Implement text checking/autocorrection with new SnowLeopard text checking API : [Attachment 29750] new patch taking into account suggestions from Justin

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 25330: Implement text checking/autocorrection with new SnowLeopard text
checking API
https://bugs.webkit.org/show_bug.cgi?id=25330

Attachment 29750: new patch taking into account suggestions from Justin
https://bugs.webkit.org/attachment.cgi?id=29750&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Mostly style complaints:

I would think the Editor stuff should be in its own EditorMac.cpp file, since
none of the rest of the ports care about it.

WebKit style normally uses:
if (!ec) instead:
+	     if (ec == 0) {
Personally I find (ec == 0) more readable in some cases, but we should follow
the style guidelines here.

     if (client()->substitutionsPanelIsShowing()) {
 1154	      client()->showSubstitutionsPanel(false);
 1155	  } else {

no { } according to the style guidelines.

Why is this one different from all the others:
2     if (!client())
 1163	      return false;
 1164	  return client()->substitutionsPanelIsShowing();

return client() && ... would fit the pattern.

Even though much of the editing code hasn't been updated to the modern style
guidelines, we still should try to follow them here:
 2227 void Editor::markAllMisspellingsAndBadGrammarInRanges(Range
*spellingRange, bool markGrammar, Range *grammarRange, bool
performTextChecking)

for C++ * goes next to the type in WebKit.  In Obj-C the * goes next to the
variable name (to match the rest of AppKit/Cocoa)

Looks like you have some tabs in the file:
 2273		restoreSelectionAfterChange = true;
 2274		adjustSelectionForParagraphBoundaries = (selectionOffset >=
paragraphLength) ? true : false;
 2275	    }
the commit will fail if there are tabs in the file.

I would think this whole block would make more sense in a small function:
 uint64_t checkingTypes = markGrammar ? (TextCheckingTypeSpelling |
TextCheckingTypeGrammar) : TextCheckingTypeSpelling;
 2281	  if (performTextChecking) {
 2282	      if (isAutomaticLinkDetectionEnabled())
 2283

something like uint64_t checkingTypes =  typesToCheck(markGrammar) or some
better name...

It looks like you changed the encoding of the strings file?

Seems TextCheckingResult result; could really use a constructor function :)

This should all be a static inline funtion:
    if (action == @selector(toggleAutomaticQuoteSubstitution:)) {
 2649	      NSMenuItem *menuItem = (NSMenuItem *)item;
 2650	      if ([menuItem isKindOfClass:[NSMenuItem class]])
 2651		  [menuItem setState:[self isAutomaticQuoteSubstitutionEnabled]
? NSOnState : NSOffState];
 2652	      return YES;
 2653	  }
cause you repeat the same copy/paste code a bunch there...

AGain here:
} else if (action == @selector(toggleAutomaticLinkDetection:)) {
 3767	      BOOL checkMark = [self isAutomaticLinkDetectionEnabled];
 3768	      if ([(NSObject *)item isKindOfClass:[NSMenuItem class]]) {
 3769		  NSMenuItem *menuItem = (NSMenuItem *)item;
 3770		  [menuItem setState:checkMark ? NSOnState : NSOffState];
 3771	      }
 3772	      return YES;
 3773	  } 

And here:
4655	 if (automaticDashSubstitutionEnabled == flag)
 4656	      return;
 4657	  automaticDashSubstitutionEnabled = flag;
 4658	  [[NSUserDefaults standardUserDefaults]
setBool:automaticDashSubstitutionEnabled
forKey:WebAutomaticDashSubstitutionEnabled];	
 4659	  [[NSSpellChecker sharedSpellChecker] updatePanels];

I think you're going to need to go more rounds with Darin/Justin (folks who
actually still work at Apple).	 But hopefully you'll find the style comments
useful.