Update Hunspell to the latest stable version to use the latest dictionary for...

Update Hunspell to the latest stable version to use the latest bdict format.

Updated Hunspell to version 1.2.8 which properly deals with UTF8 and fixes many bugs. This CL will use the BDict format and remove the usage of FileMgr. Removed the unwanted "key" parameter constructors from hunspell since we are managing them through bdict. Removed all line numbers from the errors  since we don't support that.

BUG=14756
(http://crbug.com/14756)
TEST= Compiled Hunspell, Compiled Chromium, Ran Chromium, Fixed some of my spelling mistakes. Ran unit tests for SpellCheckTest.*

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

[Mohamed Mansour, 2009-07-21 06:03:32]

Since this is a big change, I decided to first upload the original files from
the hunspell source (which is Patch Set 1). Then I applied my changes which is
Patch Set 2.

I don't know if this is the correct way, as always, comments are welcomed.
Already spent too much time on this, and I don't want to spend more if I am
going in the wrong direction. Any advice is appreciated.

[Mark Larson, 2009-07-21 06:23:53]

Wow. I don't know whether this is the right/wrong direction, but thanks for
taking a stab at this.

I'm adding Jungshik to the review since he did some investigation around support
for the current French dictionaries and the ~AffixMgr() crashes.

Thanks again, Mohamed!

[brettw, 2009-07-21 17:47:55]

Did you compile & run Chrome with this change?

On Windows it didn't compile or link. I added HUNSPELL_STATIC in two places in
the .gyp file where HUNSPELL_CHROME_CLIENT was also defined.

When I fixed that I get a crash the first time we spellcheck in
HashMgr::parse_aliasf.

[Mohamed Mansour, 2009-07-21 19:16:48]

brettw: I compiled the hunspell project only. It was late at night so I wanted
feedback before I continue. I wanted to see what I am doing is the correct way.

I will look into that crash and see whats up, might be a dictionary problem :/
or might be something else. I will work on it when I come home from work today. 

Thanks for the feedback!

[jungshik at Google, 2009-07-21 19:25:53]

Thank you for working on this. Can you also udpate google.patch and
README.chromium files?

[Mohamed Mansour, 2009-07-21 19:52:21]

No problem updating the README.chromium. 

But do we really need to update the google.patch? What would be the correct way
creating that patch if the original 1.2.8 hunspell source isn't committed yet. I
guess I could manually diff each file in the code review tool wrt to the first
patch set and join them together in the google.patch. 

Is it really needed?

[brettw, 2009-07-21 20:13:47]

I think updating the patch is probably not useful at this point, since I think
it's really out-of-date and nontrivial to generate. Since all changes are
ifdefed, it shouldn't be too bad to merge in the future.

But you should delete the old one so people aren't confused.

[jungshik at Google, 2009-07-21 20:21:39]

Thank you for the clarification. I thought it's strange that
google.patch is very short although our changes are much more
extensive than that.

2009/7/21  <brettw@chromium.org>:
> I think updating the patch is probably not useful at this point, since I
> think it's really out-of-date and nontrivial to generate. Since all
> changes are ifdefed, it shouldn't be too bad to merge in the future.
>
> But you should delete the old one so people aren't confused.
>
> http://codereview.chromium.org/155841
>

[Mohamed Mansour, 2009-07-22 04:04:12]

Alright team, the above compiles and chromium runs. It spellcheck's too.

I don't know how you are going to review this since its massive but keep in mind
that Patch Set 1 is the Hunspell 1.2.8 copied over to Chromium. So, you can diff
the files wrt to patchset 1 to know what I have changed.

A lot of stuff is repetitive which will make it simpler to review.

Any questions let me know. (How do I update the dictionary files)

[Mark Larson, 2009-07-23 06:29:50]

I'm about to go on holiday, so this is kind of my last chance to comment on this
patch for a while.

If this is on the right track, let's please give it lots of attention and get it
landed. Otherwise, let's cut it off early.

The worst possible outcome for a large patch like this is to let it languish and
waste everyone's time.

BTW, nit for Mohamed: add a period at the end of the first sentence in the log
message.

[jungshik at Google, 2009-07-23 16:53:44]

It might help "reviewing" if mhm can make and attach two diff files
(not for check-in but for 'review' purpose). One diff between the
pristine Hunspell 1.2.8 and your revised Hunspell 1.2.8.  The other
diff between the original Hunspell 1.1.x(?) that is the basis of our
current Hunspell and our revised one currently in the tree. It's still
not easy, but it may help a little bit more (at least 'sanity'
check-wise). Perhaps, a third diff between Hunspell 1.1.x(?) and
Hunspell 1.2.8 also helps, too.

Jungshik

2009/7/22  <mal@chromium.org>:
> I'm about to go on holiday, so this is kind of my last chance to comment
> on this patch for a while.
>
> If this is on the right track, let's please give it lots of attention
> and get it landed. Otherwise, let's cut it off early.
>
> The worst possible outcome for a large patch like this is to let it
> languish and waste everyone's time.
>
> BTW, nit for Mohamed: add a period at the end of the first sentence in
> the log message.
>
> http://codereview.chromium.org/155841
>

[Mohamed Mansour, 2009-07-23 19:26:16]

Jungshik: I "kinda" did two different files here, the first "Patch Set 1" is
copy/paste of what Hunspell 1.2.8 in Chromium.

"Patch Set 3" is what I have modified to make it work in chromium.

So when you review, you can first check "Patch Set 1" and see if the new
Hunspell interferes with the old our current one. If it is okay, you can check
the changes I have done in "Patch Set 3" to see if they are correct.

Sorry for the confusion, next time I can split them up into different reviews
where I don't commit the first one.

[brettw, 2009-07-23 20:21:06]

I checked all the stuff pretty well labeled HUNSPELL_CHROME_CLIENT.

http://codereview.chromium.org/155841/diff/1016/123
File chrome/third_party/hunspell/src/hunspell/affixmgr.cxx (right):

http://codereview.chromium.org/155841/diff/1016/123#newcode269
Line 269: #define fclose(a);
I believe you can delete this line since the code doesn't use fclose anymore.

http://codereview.chromium.org/155841/diff/1016/123#newcode296
Line 296: // a LineIterator* instead of a FILE* afflst.
This changed. The old code did fclose which we neutralized. Now it does delete,
and it looks like you'll be deleting this stack variable now, which is bad. I
think you should just move the deletes to be inside the defines and makes sure
there are no extra deletes anywhere of this variable.

http://codereview.chromium.org/155841/diff/1016/123#newcode659
Line 659: if (parse_array(line, &ignorechars, &ignorechars_utf16,
&ignorechars_utf16_len, utf8, , afflst->getlinenum())) {
Looks like accidentally added an extra comma here.

http://codereview.chromium.org/155841/diff/1016/132
File chrome/third_party/hunspell/src/hunspell/csutil.cxx (right):

http://codereview.chromium.org/155841/diff/1016/132#newcode5515
Line 5515: // free(piece);
It's kind of astonishing that the Hunspell code is full of stuff like this
random commented-out free. I was going to complain about you doing it, but they
wrote it!

http://codereview.chromium.org/155841/diff/1016/129
File chrome/third_party/hunspell/src/hunspell/hunspell.cxx (left):

http://codereview.chromium.org/155841/diff/1016/129#oldcode372
Line 372: if (pHMgr) pHMgr->EmptyHentryCache();
Why did you delete this? I think we need it.

http://codereview.chromium.org/155841/diff/1016/117
File chrome/third_party/hunspell/src/hunspell/replist.hxx (right):

http://codereview.chromium.org/155841/diff/1016/117#newcode5
Line 5: #undef near
Can you add a comment about why you had to add this line?

[Mohamed Mansour, 2009-07-24 04:07:49]

Updated! Compiled and tested the spellchecker.

http://codereview.chromium.org/155841/diff/1016/123
File chrome/third_party/hunspell/src/hunspell/affixmgr.cxx (right):

http://codereview.chromium.org/155841/diff/1016/123#newcode269
Line 269: #define fclose(a);
On 2009/07/23 20:21:06, brettw wrote:
> I believe you can delete this line since the code doesn't use fclose anymore.

Done.

http://codereview.chromium.org/155841/diff/1016/123#newcode296
Line 296: // a LineIterator* instead of a FILE* afflst.
On 2009/07/23 20:21:06, brettw wrote:
> This changed. The old code did fclose which we neutralized. Now it does
delete,
> and it looks like you'll be deleting this stack variable now, which is bad. I
> think you should just move the deletes to be inside the defines and makes sure
> there are no extra deletes anywhere of this variable.
Done. I was worried about that, will do.

http://codereview.chromium.org/155841/diff/1016/123#newcode659
Line 659: if (parse_array(line, &ignorechars, &ignorechars_utf16,
&ignorechars_utf16_len, utf8, , afflst->getlinenum())) {
On 2009/07/23 20:21:06, brettw wrote:
> Looks like accidentally added an extra comma here.

Done.

http://codereview.chromium.org/155841/diff/1016/132
File chrome/third_party/hunspell/src/hunspell/csutil.cxx (right):

http://codereview.chromium.org/155841/diff/1016/132#newcode5515
Line 5515: // free(piece);
On 2009/07/23 20:21:06, brettw wrote:
> It's kind of astonishing that the Hunspell code is full of stuff like this
> random commented-out free. I was going to complain about you doing it, but
they
> wrote it!

What do you recommend to do in this case, leave it as is?

http://codereview.chromium.org/155841/diff/1016/129
File chrome/third_party/hunspell/src/hunspell/hunspell.cxx (left):

http://codereview.chromium.org/155841/diff/1016/129#oldcode372
Line 372: if (pHMgr) pHMgr->EmptyHentryCache();
On 2009/07/23 20:21:06, brettw wrote:
> Why did you delete this? I think we need it.

Done. Forgot it :<

http://codereview.chromium.org/155841/diff/1016/117
File chrome/third_party/hunspell/src/hunspell/replist.hxx (right):

http://codereview.chromium.org/155841/diff/1016/117#newcode5
Line 5: #undef near
On 2009/07/23 20:21:06, brettw wrote:
> Can you add a comment about why you had to add this line?

Done. This is the only way I could make it compile.

[Hironori Bono, 2009-07-27 06:54:44]

Thank you so much for your great work!
As you can see in <http://codereview.chromium.org/160096>, changing the
line-endings of a file to CRLF causes a problem when creating another change. To
prevent future changes from changing the line-endings to CRLF, would it be
possible to set the svn:eol-style property of the files added (and modified) by
this change as shown in the following example?

> svn propset svn:eol-style LF (filename)

Regards,

Hironori Bono

[Mohamed Mansour, 2009-07-27 11:56:17]

I have set the svn:eol-style LF on all the files but I am still getting patching
issues from trybot which states the following:

Hunk #1 FAILED at 1.
File src/chrome/third_party/hunspell/google.patch is not empty after patch, as
expected
1 out of 1 hunk FAILED -- saving rejects to file
src/chrome/third_party/hunspell/google.patch.rej

Is it because It is trying to delete a patch which itself is a patch? You helped
me on IRC fixing the other Hunk's but this one is not being fixed, I might as
well just leave it in the repository and fix it afterwards.

[Mohamed Mansour, 2009-07-28 01:25:26]

Thanks guys for your help! It patches correctly now, and I altered unit test
cases to incorporate the new changes that hunspell brings to us.

Everything look good? I rebased the tree to make sure its ToT.

[Mohamed Mansour, 2009-07-28 02:00:13]

1 problem ... :( 

Linux slave is crashing on the same test over and over.
[----------] 3 tests from SpellCheckTest
[ RUN      ] SpellCheckTest.SpellCheckStrings_EN_US
[       OK ] SpellCheckTest.SpellCheckStrings_EN_US (15 ms)
[ RUN      ] SpellCheckTest.SpellCheckSuggestions_EN_US
program finished with exit code 245

I have no idea what is happening. Did you guys do any Linux specific changes to
hunspell?

[Hironori Bono, 2009-07-28 05:26:15]

It seems your unit-test crash on Linux is caused by UMR (uninitialized memory
read), i.e. the var (and clen) members of a hentry struct are not initialized in
some cases. (Maybe Windows and Mac automatically initializes them with 0 so this
crash doesn't happen on Windows and Mac.) Even though I added comments to code
that needs to initialize the above members, is it possible to double-check it?

By the way, it seems your change contains TAB characters. Is it intentional?

Regards,

Hironori Bono

http://codereview.chromium.org/155841/diff/1169/1185
File chrome/third_party/hunspell/src/hunspell/hashmgr.cxx (right):

http://codereview.chromium.org/155841/diff/1169/1185#newcode454
Line 454: hp->next_homonym = NULL;
To prevent uninitialized memory read, is it possible to add the following lines
here and initialize all members of this |hp|?
+ hp->var = 0;
+ hp->clen = 0;

http://codereview.chromium.org/155841/diff/1169/1198
File chrome/third_party/hunspell/src/hunspell/suggestmgr.cxx (right):

http://codereview.chromium.org/155841/diff/1169/1198#newcode1146
Line 1146: roots[lp]->next_homonym = NULL;
To prevent uninitialized memory read (which causes a unit-test crash on Linux),
is it possible to add the following lines and initialize the 'var' and 'clen'
members of this |root[lp]|?
+ root[lp]->var = 0;
+ root[lp]->clen = 0;

[brettw, 2009-07-28 14:46:55]

On 2009/07/28 05:26:15, hbono wrote:
> It seems your unit-test crash on Linux is caused by UMR (uninitialized memory
> read), i.e. the var (and clen) members of a hentry struct are not initialized
in
> some cases. (Maybe Windows and Mac automatically initializes them with 0 so
this
> crash doesn't happen on Windows and Mac.) Even though I added comments to code
> that needs to initialize the above members, is it possible to double-check it?
> 
> By the way, it seems your change contains TAB characters. Is it intentional?

If there are ones we added, they should probably be changed, but we should keep
any old Hunspell code as-is.

[Mohamed Mansour, 2009-07-29 06:26:13]

All three try bots are now green! I think hunspell has been updated
successfully, what do you guys think?

[Mohamed Mansour, 2009-07-31 06:09:28]

ping.

Safe to commit yet? Everything works fine at my end and passes unit tests fine.

[Hironori Bono, 2009-07-31 08:17:09]

Sorry for my slow response.
It takes long time to test the spellchecker since we have lots of
dictionaries/platforms to be tested.
As far as I tested with the following dictionaries/platform, this change works
good.
 * Catalan - Windows
 * Czech - Windows, Linux
 * Danish - Windows, Linux
 * Dutch - Windows
 * English (Austrailia) - Windows
 * English (UK) - Windows, Linux
 * Estonian - Windows
 * French - Windows, Linux
 * Spanish - Windows, Linux
 * German - Windows, Linux
 * Greek - Windows, Linux
 * Hebrew - Windows
 * Hindi - Windows
 * Indonesian - Windows
 * Italian - Windows
 * Latvian - Windows
 * Lithuanian - Windows
 * Norwegian - Windows
 * Polish - Windows, Linux
 * Portuguese (Brasil) - Windows
 * Portuguese (Portugal) - Windows, Linux
 * Romanian - Windows
 * Russian - Windows, Linux
 * Slovak - Windows
 * Slovenian - Windows
 * Spanish - Windows, Linux
 * Swedish - Windows
 * Turkish - Windows, Linux
 * Vietnamese - Windows

Even though this change looks good for me, it is better for you to get approvals
of other reviewers as well.

Regards,

Hironori Bono

On 2009/07/31 06:09:28, Mohamed Mansour wrote:
> ping.
> 
> Safe to commit yet? Everything works fine at my end and passes unit tests
fine.

[Mohamed Mansour, 2009-07-31 14:59:53]

For curiosity reasons, how did you test those combinations?

[brettw, 2009-07-31 16:43:21]

LGTM

http://codereview.chromium.org/155841/diff/1016/132
File chrome/third_party/hunspell/src/hunspell/csutil.cxx (right):

http://codereview.chromium.org/155841/diff/1016/132#newcode5515
Line 5515: // free(piece);
On 2009/07/24 04:07:49, Mohamed Mansour wrote:
> On 2009/07/23 20:21:06, brettw wrote:
> > It's kind of astonishing that the Hunspell code is full of stuff like this
> > random commented-out free. I was going to complain about you doing it, but
> they
> > wrote it!
> 
> What do you recommend to do in this case, leave it as is?

Yes, I was just venting. I assume the Hunspell people did it for a good reason
(I hope).

[jungshik at Google, 2009-07-31 18:06:57]

Thank you for making this happen :-)

On 2009/07/31 14:59:53, Mohamed Mansour wrote:
> For curiosity reasons, how did you test those combinations?

I'm not sure how Hironori did it. Perhaps, he applied the patch, build Chrome
and change the spellcheck language in the UI and see if it works. 

Anyway, I've just filed http://crbug.com/18184 for adding more tests.

[sidchat_google.com, 2009-07-31 18:08:34]

Good job !

On Fri, Jul 31, 2009 at 11:06 AM, <jshin@chromium.org> wrote:
> Thank you for making this happen :-)
>
> On 2009/07/31 14:59:53, Mohamed Mansour wrote:
>>
>> For curiosity reasons, how did you test those combinations?
>
> I'm not sure how Hironori did it. Perhaps, he applied the patch, build
> Chrome and change the spellcheck language in the UI and see if it works.
>
>
> Anyway, I've just filed http://crbug.com/18184 for adding more tests.
>
>
>
> http://codereview.chromium.org/155841
>

[Hironori Bono, 2009-08-03 01:24:07]

On 2009/07/31 14:59:53, Mohamed Mansour wrote:
> For curiosity reasons, how did you test those combinations?

I just wrote a UI test that sends key events into a <textarea> form and compare
its results before and after applying your change. (The texts used in this UI
test is retrieved from Wikipedia.) Unfortunately, this UI test is so far too
flaky and it doesn't work with our dictionary downloader, i.e. we need to
download all dictionaries before running this UI test.

Regards,

Hironori Bono

[laforge, 2009-08-05 21:55:37]

Mohamed, I'm very excited to see this change make it to 3.0, have you been able
to get the help you need to get past the Valgrind issues?

On 2009/08/03 01:24:07, hbono wrote:
> On 2009/07/31 14:59:53, Mohamed Mansour wrote:
> > For curiosity reasons, how did you test those combinations?
> 
> I just wrote a UI test that sends key events into a <textarea> form and
compare
> its results before and after applying your change. (The texts used in this UI
> test is retrieved from Wikipedia.) Unfortunately, this UI test is so far too
> flaky and it doesn't work with our dictionary downloader, i.e. we need to
> download all dictionaries before running this UI test.
> 
> Regards,
> 
> Hironori Bono

[not_the_right_dank, 2009-08-05 22:00:03]

I don't think he's been able to run the tests under valgrind successfully yet.

Somebody mentioned there were lots of new coverity warnings on his change, too,
so I sent him a couple of those and gave him a little advice on
how to approach them.

mhm, if you still need help with valgrind, feel free to chat me up...

On Wed, Aug 5, 2009 at 2:55 PM, <laforge@chromium.org> wrote:
>
> Mohamed, I'm very excited to see this change make it to 3.0, have you
> been able to get the help you need to get past the Valgrind issues?
>
> On 2009/08/03 01:24:07, hbono wrote:
>>
>> On 2009/07/31 14:59:53, Mohamed Mansour wrote:
>> > For curiosity reasons, how did you test those combinations?
>
>> I just wrote a UI test that sends key events into a <textarea> form
>
> and compare
>>
>> its results before and after applying your change. (The texts used in
>
> this UI
>>
>> test is retrieved from Wikipedia.) Unfortunately, this UI test is so
>
> far too
>>
>> flaky and it doesn't work with our dictionary downloader, i.e. we need
>
> to
>>
>> download all dictionaries before running this UI test.
>
>> Regards,
>
>> Hironori Bono
>
>
>
> http://codereview.chromium.org/155841
>

[Mohamed Mansour, 2009-08-06 02:10:09]

I feel kinda "shy" asking for help, because the questions I usually have are
syntax questions, Google search has been a good friend "somewhat":/ I am not
in the same level of knowledge of you guys, and I graduated University last
year with no c++ knowledge, and that language (the way chromium uses it) is
pretty steep learning curve (which I love).  Especially when I am trying to
solve leaks, uninitialized memory, etc while my only c++ programming
experience is with chromium... Dan has been super helpful giving me some
coverity warnings (learned something new that day), I solved 4/5th of them
on the night he gave me them. I didn't take a look at hunspell ever since.
Would be nice to have more guidance, but I could manage without. My day job
is pretty heavy (9-11 hours a day, I just came home now, 10PM). I will try
to spend a couple of hours tonight. I will try to ping anyone if I have any
questions on IRC (but everyone is usually away night times).

It would be nice to have access to coverity, so I can learn more about that
neat tool, but it seems like a commercial product.

Regarding valgrind, I have no idea how to fix them, they don't make sense to
me, Dan is giving me coverity warnings so I hope that will fix them. I am
just following orders. =)

-- Mohamed Mansour


On Wed, Aug 5, 2009 at 5:55 PM, <laforge@chromium.org> wrote:

> Mohamed, I'm very excited to see this change make it to 3.0, have you
> been able to get the help you need to get past the Valgrind issues?
>
>
> On 2009/08/03 01:24:07, hbono wrote:
>
>> On 2009/07/31 14:59:53, Mohamed Mansour wrote:
>> > For curiosity reasons, how did you test those combinations?
>>
>
>  I just wrote a UI test that sends key events into a <textarea> form
>>
> and compare
>
>> its results before and after applying your change. (The texts used in
>>
> this UI
>
>> test is retrieved from Wikipedia.) Unfortunately, this UI test is so
>>
> far too
>
>> flaky and it doesn't work with our dictionary downloader, i.e. we need
>>
> to
>
>> download all dictionaries before running this UI test.
>>
>
>  Regards,
>>
>
>  Hironori Bono
>>
>
>
>
> http://codereview.chromium.org/155841
>