[Webkit-unassigned] [Bug 90269] [GTK] Add a new and reusable enchant-based spellchecker in WebCore

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=90269





--- Comment #4 from Mario Sanchez Prada <msanchez at igalia.com>  2012-07-18 07:23:42 PST ---
(From update of attachment 150155)
View in context: https://bugs.webkit.org/attachment.cgi?id=150155&action=review

Now working on this. See some comments in the meanwhile...

>> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:57
>> +{
> 
> These should probably take Strings, which encapsulate encoding information better.

Makes sense. I'll change that.

>> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:62
>> +void TextCheckerEnchant::checkSpellingOfString(const char* string, int* misspellingLocation, int* misspellingLength)
> 
> It think it makes sense to rename this to be more precise. checkSpellingOfString could be getLocationAndLengthOfFirstMisspellilng. This aligns with the WebKit style of using the word "get" for functions and methods that have output arguments.

Even if I agree with you in that something like that could be a better name as a general thought, I don't think we should do it in this case since it would be confusing, as checkSpellingOfString() is the expected name according to what is defined in WebCore/platform/text/TextCheckerClient.h:

  class TextCheckerClient {
  public:
      virtual ~TextCheckerClient() {}

      [...]
      virtual void learnWord(const String&) = 0;
      virtual void checkSpellingOfString(const UChar*, int length, int* misspellingLocation, int* misspellingLength) = 0;
      virtual String getAutoCorrectSuggestionForMisspelledWord(const String& misspelledWord) = 0;
      [...]
};

So, I think we should leave this name as is, to reflect that its related to that.

>> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:86
>> +                end++;
> 
> You're missing the fix that I recently made to the WebKit1 enchant code to handle contractions.

I guess you mean SVN r121360. I'll fix that in an upcoming patch.

>> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:114
>> +char** TextCheckerEnchant::getGuessesForWord(const char* word)
> 
> Since this is a WebCore interface, a Vector<String> makes more sense here.

Makes sense. I'll change that.

>> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:125
>> +        if (numberOfSuggestions > 0) {
> 
> This looks like it should be an early continue.

Ok.

>> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:127
>> +                numberOfSuggestions = 10;
> 
> 10 should probably be a constant like:
> static const int maximumNumberOfSuggestions = 10;

Just moved code from WebKit1, but I agree with this change too. I'll fix it.

>> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:141
>> +void TextCheckerEnchant::updateSpellCheckingLanguages(const char* languages)
> 
> It makes sense for this to take a String instead of a const char*.

Ok.

>> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:146
>> +        char** langs = g_strsplit(languages, ",", -1);
> 
> You could use String.split here and avoid having to free the list of languages.

Ok.

>> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:179
>> +#endif /* ENABLE(SPELLCHECK) */
> 
> Nit: You should use a C++ comment here.

Ok.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.