[chromium-reviews] Part 1 of 'Add common words for each language, and remove forbidden words'....

Part 1 of 'Add common words for each language, and remove forbidden words'.

Updated the Dic Reader to read additional words from additional-words dictionary file, created as a text file with UTF-8 encoding, with extension dic_delta.

Test: Tested with all the 21 languages that are currently supported by the spell checker and for which common/additional words have been obtained. Resulting bdic files generated have those additional words.

Issue=2837.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=7492

[brettw, 2008-12-18 20:38:20]

http://codereview.chromium.org/14856/diff/6/203
File chrome/tools/convert_dict/dic_reader.cc (right):

http://codereview.chromium.org/14856/diff/6/203#newcode62
Line 62: if (!got_count) {
This is wrong for the additional files, which have no count line.

[sidchat (Google), 2008-12-18 20:54:38]

On 2008/12/18 20:38:20, brettw wrote:
> http://codereview.chromium.org/14856/diff/6/203
> File chrome/tools/convert_dict/dic_reader.cc (right):
> 
> http://codereview.chromium.org/14856/diff/6/203#newcode62
> Line 62: if (!got_count) {
> This is wrong for the additional files, which have no count line.
Right - I missed that - fixed it - please have a look again.

-Sid

[brettw, 2008-12-19 22:54:07]

http://codereview.chromium.org/14856/diff/208/8
File chrome/tools/convert_dict/dic_reader.cc (right):

http://codereview.chromium.org/14856/diff/208/8#newcode47
Line 47: const char* file_type, bool got_count) {
Looking at this function declaration, it's not clear at all why this is called
got_count. It doesn't imply at all that you previously got the count for the
file. Can you please find another way to do this?

[sidchat (Google), 2008-12-19 23:36:27]

On 2008/12/19 22:54:07, brettw wrote:
> http://codereview.chromium.org/14856/diff/208/8
> File chrome/tools/convert_dict/dic_reader.cc (right):
> 
> http://codereview.chromium.org/14856/diff/208/8#newcode47
> Line 47: const char* file_type, bool got_count) {
> Looking at this function declaration, it's not clear at all why this is called
> got_count. It doesn't imply at all that you previously got the count for the
> file. Can you please find another way to do this?
I think the ambiguity comes from the name of the parameter rather than the
implementation. The 'got_count' flag should probably be called
'file_has_word_count_in_the_first line'. If set true, it would mean that the
file pointer points to a file (a .dic file) whose first line contains the number
of words in the file, and should be skipped to move on to the second line to
read the first word. Setting it to false would mean that the first line does not
contain the number of words, and should be used to read the first word (used for
.dic_delta files).

[sidchat (Google), 2008-12-29 21:47:55]

Removed ambiguity by renaming a variable and adding a detailed explanation in
comments. Please have a look again.

-Sid

On 2008/12/19 23:36:27, sidchat (Google) wrote:
> On 2008/12/19 22:54:07, brettw wrote:
> > http://codereview.chromium.org/14856/diff/208/8
> > File chrome/tools/convert_dict/dic_reader.cc (right):
> > 
> > http://codereview.chromium.org/14856/diff/208/8#newcode47
> > Line 47: const char* file_type, bool got_count) {
> > Looking at this function declaration, it's not clear at all why this is
called
> > got_count. It doesn't imply at all that you previously got the count for the
> > file. Can you please find another way to do this?
> I think the ambiguity comes from the name of the parameter rather than the
> implementation. The 'got_count' flag should probably be called
> 'file_has_word_count_in_the_first line'. If set true, it would mean that the
> file pointer points to a file (a .dic file) whose first line contains the
number
> of words in the file, and should be skipped to move on to the second line to
> read the first word. Setting it to false would mean that the first line does
not
> contain the number of words, and should be used to read the first word (used
for
> .dic_delta files).

[brettw, 2008-12-29 22:25:22]

LGTM

http://codereview.chromium.org/14856/diff/401/402
File chrome/tools/convert_dict/dic_reader.cc (right):

http://codereview.chromium.org/14856/diff/401/402#newcode46
Line 46: // This function reads words from a .dic file, or a .dic_delta file.
Note that
Can you add "// This function reads words from a .dic file, or a .dic_delta file
and appends it to the given set."

http://codereview.chromium.org/14856/diff/401/402#newcode151
Line 151: PopulateWordSet(&word_set, additional_words_file_, aff_reader, "dic
delta",
It seems like it would be better to not call the function with a NULL file
rather than checking at the beginning of the populate function.