[webkit-reviews] review denied: [Bug 61787] [GTK] Add WebKitSpellChecker interface and implementations : [Attachment 95437] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jun 2 15:37:53 PDT 2011
Martin Robinson <mrobinson at webkit.org> has denied Xan Lopez
<xan.lopez at gmail.com>'s request for review:
Bug 61787: [GTK] Add WebKitSpellChecker interface and implementations
https://bugs.webkit.org/show_bug.cgi?id=61787
Attachment 95437: Patch
https://bugs.webkit.org/attachment.cgi?id=95437&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=95437&action=review
I like this implementation a lot. r- mostly because I think
Source/WebKit/gtk/webkit/webkitspellchecker.c should be C++.
> Source/WebKit/gtk/WebCoreSupport/TextCheckerClientEnchant.cpp:3
> * Copyright (C) 2008 Nuanti Ltd.
I think you want to rename this file to TextCheckerClient since it no longer
depends on Enchant.
> Source/WebKit/gtk/WebCoreSupport/TextCheckerClientEnchant.cpp:31
> #include <enchant.h>
I think you can remove this include.
> Source/WebKit/gtk/WebCoreSupport/TextCheckerClientEnchant.cpp:67
> // Currently, it computes an empty string.
This comment can be removed now.
> Source/WebKit/gtk/WebCoreSupport/TextCheckerClientEnchant.cpp:88
> + char* suggestion;
> + int i = 0;
>
> - if (numberOfSuggestions > 0)
> - enchant_dict_free_suggestions(dict, suggestions);
> - }
> -}
> + guesses.clear();
>
> -static void getAvailableDictionariesCallback(const char* const languageTag,
const char* const, const char* const, const char* const, void* data)
> -{
> - Vector<CString>* dicts = static_cast<Vector<CString>*>(data);
> + for (suggestion = suggestions[i]; suggestion; i++)
> + guesses.append(String::fromUTF8(suggestion));
I think it makes more sense to write this as:
for (int i = 0; suggestion[i]; i++)
guesses.append(String::fromUTF8(suggestions[i]));
> Source/WebKit/gtk/webkit/webkitdefines.h:107
> +typedef struct _WebKitSpellChecker WebKitSpellChecker; /* dummy typedef */
I think you can remove the comment here.
> Source/WebKit/gtk/webkit/webkitglobals.cpp:250
> +static WebKitSpellChecker* spellChecker = 0;
Please make this a GRefPtr.
> Source/WebKit/gtk/webkit/webkitspellchecker.c:1
> +/*
I think we decided to change this file to C++ in the end? Let's call this
WebKitTextChecker because it may one day do grammar checking!
> Source/WebKit/gtk/webkit/webkitspellchecker.c:27
> + * #WebKitSpellChecker provides APIs for the spell checking
> + * functionality used internally by WebKit to perform spell checking
spell checking -> text checking
> Source/WebKit/gtk/webkit/webkitspellchecker.c:35
> +static void webkit_spell_checker_default_init(WebKitSpellCheckerInterface*
iface)
iface -> interface.
> Source/WebKit/gtk/webkit/webkitspellchecker.c:48
> + * Checks @string for misspellings using @checker, storing the
> + * location and length of the first misspelling in
> + * @misspelling_location and @misspelling_length respectively.
Might as well let this be two lines for consistency, since the lines above it
are so long.
>>> Source/WebKit/gtk/webkit/webkitspellchecker.c:54
>>> + WebKitSpellCheckerInterface* iface;
>>
>> Declaration has space between * and variable name in
WebKitSpellCheckerInterface* iface [whitespace/declaration] [3]
>
> The code style in C file is actually to have the asterisk with the variable
name. It would probably be better to make this file a C++ file now. My
reasoning is that one day we may find out we need to use some WTF, WebCore or
WebCoreSupport class here and then have to change the entire style of the file
when we convert it to C++.
iface -> interface. When this becomes C++, please define interface when you
first call WEBKIT_SPELL_CHECKER_GET_IFACE.
> Source/WebKit/gtk/webkit/webkitspellchecker.c:75
> + * Returns: a newly allocated %NULL-terminated array of guessed
> + * corrections for a misspelled word @word. Free it with %g_strfreev
> + * when done with it.
I cannot remember if a transfer annotation is necesary here or not.
>> Source/WebKit/gtk/webkit/webkitspellchecker.c:81
>> + WebKitSpellCheckerInterface* iface;
>
> Declaration has space between * and variable name in
WebKitSpellCheckerInterface* iface [whitespace/declaration] [3]
Same comment as above here.
> Source/WebKit/gtk/webkit/webkitspellchecker.c:100
> + * Sets @languages as the list of languages to use by @checker. The
> + * accepted format is a list of comma (',') separated language codes
> + * of the form 'en_US', ie, language_VARIANT.
Nice doc here.
> Source/WebKit/gtk/webkit/webkitspellchecker.c:122
> + * Returns: the suggestion for the autocorrection of @word
Should mention that the word is newly allocated.
>> Source/WebKit/gtk/webkit/webkitspellchecker.c:128
>> + WebKitSpellCheckerInterface* iface;
>
> Declaration has space between * and variable name in
WebKitSpellCheckerInterface* iface [whitespace/declaration] [3]
Same comments as above.
> Source/WebKit/gtk/webkit/webkitspellchecker.c:146
> + * Instructs the @checker to add @word to its dictionary as a properly
> + * spelled word.
Here it is worth clarifying if the word is learned for the session or
permanently. You might say something like, "The @checker learns the word
permanently, possibly adding it to the spelling dictionary."
>> Source/WebKit/gtk/webkit/webkitspellchecker.c:152
>> + WebKitSpellCheckerInterface* iface;
>
> Declaration has space between * and variable name in
WebKitSpellCheckerInterface* iface [whitespace/declaration] [3]
Same comments as above.
> Source/WebKit/gtk/webkit/webkitspellchecker.c:168
> + * Instructs the @checker to ignore @word as a misspelling from now
> + * on.
Should probably describe what the difference is between ignored words and
learned words and whether or not this change is permanent.
>> Source/WebKit/gtk/webkit/webkitspellchecker.c:174
>> + WebKitSpellCheckerInterface* iface;
>
> Declaration has space between * and variable name in
WebKitSpellCheckerInterface* iface [whitespace/declaration] [3]
Same comments as above.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:29
> +#include "GOwnPtr.h"
> +#include "webkitspellchecker.h"
> +
> +#include <enchant.h>
> +#include <gtk/gtk.h>
> +#include <wtf/text/CString.h>
This should be one big block.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:31
> +static EnchantBroker* broker = 0;
Instead of manually creating this wherever you need it, I think it would be
better to make this a helper function like:
static EnchantBroker* getEnchantBroker()
{
static EnchantBroker* broker;
if (!broker)
...etc...
}
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:34
> + GSList* m_enchantDicts;
I think we don't usually use m_foo for GObject private sections or plain
structs.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:40
> + G_IMPLEMENT_INTERFACE (WEBKIT_TYPE_SPELL_CHECKER,
Extra space here after G_IMPLEMENT_INTERFACE.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:49
> + EnchantDict* dict = static_cast<EnchantDict*>(data);
> + enchant_broker_free_dict(broker, dict);
Both these lines are very simple, so I think they can be combined into one.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:57
> + g_slist_foreach(priv->m_enchantDicts, freeSpellCheckingLanguage, 0);
> + g_slist_free(priv->m_enchantDicts);
If you follow the approach below, you'll need to call delete priv; here.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:66
> + g_type_class_add_private(klass,
sizeof(WebKitSpellCheckerEnchantPrivate));
I think it would be better to consistently use the "new" approach that I've
listed below for all new gobjects. Eventually I want to be able to safely use
C++ objects everywhere. :)
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:71
> + WebKitSpellCheckerEnchantPrivate* priv =
G_TYPE_INSTANCE_GET_PRIVATE(checker, WEBKIT_TYPE_SPELL_CHECKER_ENCHANT,
WebKitSpellCheckerEnchantPrivate);
Here you can just do:
checker->priv = new WebKitSpellCheckerEnchantPrivate();
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:75
> +static void check_spelling_of_string(WebKitSpellChecker* checker, const
char* string, int* misspellingLocation, int* misspellingLength)
This should be named checkSpellingOfString.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:105
> + // Set the iterator to be at the current word end, so we don't
> + // check characters twice.
This can be one line.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:130
> +static char** get_guesses_for_word(WebKitSpellChecker* checker, const char*
word, const char* context)
This should be named getGuessesForWord.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:165
> + Vector<CString>* dicts = static_cast<Vector<CString>*>(data);
> +
> + dicts->append(languageTag);
Extra newline here.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:168
> +static void update_spell_checking_languages(WebKitSpellChecker* checker,
const char* languages)
This should be named updateSpellCheckingLanguages.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:170
> + EnchantDict* dict;
dict is only used three times and right at the beginning of the block, so it
should be defined in those locations instead of function wide.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:193
> + // No dictionaries selected, we get one from the list
This comment is missing a period.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:209
> + return NULL;
NULL -> 0.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:212
> +static void learn_word(WebKitSpellChecker* checker, const char* word)
This should be named learnWord.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:215
> + WebKitSpellCheckerEnchantPrivate* priv =
WEBKIT_SPELL_CHECKER_ENCHANT(checker)->priv;
> + GSList* dicts = priv->m_enchantDicts;
this can just be GSList* dicts =
WEBKIT_SPELL_CHECKER_ENCHANT(checker)->priv->enchantDicts.
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:221
> + for (; dicts; dicts = dicts->next) {
> + EnchantDict* dict = static_cast<EnchantDict*>(dicts->data);
> +
> + enchant_dict_add_to_personal(dict, word, -1);
> + }
It seems weird to add this word to all dictionaries. Is that the right
approach?
> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:233
> + WebKitSpellCheckerEnchantPrivate* priv =
WEBKIT_SPELL_CHECKER_ENCHANT(checker)->priv;
> + GSList* dicts = priv->m_enchantDicts;
> +
> + for (; dicts; dicts = dicts->next) {
> + EnchantDict* dict = static_cast<EnchantDict*>(dicts->data);
> +
> + enchant_dict_add_to_session(dict, word, -1);
> + }
Same comments as above.
> Source/WebKit/gtk/webkit/webkitspellcheckernone.cpp:27
> + G_IMPLEMENT_INTERFACE (WEBKIT_TYPE_SPELL_CHECKER,
Extra space here after G_IMPLEMENT_INTERFACE.
More information about the webkit-reviews
mailing list