Add a MD5 checksum to the BDIC header....

Add a MD5 checksum to the BDIC header.
This change adds a MD5 checksum to the BDIC header and fills the checksum when creating a BDIC. (Even though older Chrome can use new BDICs, I have updated the BDIC version to 2.0.) This change also adds hunspell::BDict::Verify() and moves the sanity check of a BDIC file there.

BUG=61206
TEST=http://codereview.chromium.org/4477002

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

[Evan Stade, 2010-11-05 01:19:40]

nice. mostly LGTM.

when do you plan to call Verify()? Presumably:

a) after download
b) after an error is detected

http://codereview.chromium.org/4409002/diff/1/2
File third_party/hunspell/google/bdict.cc (right):

http://codereview.chromium.org/4409002/diff/1/2#newcode15
third_party/hunspell/google/bdict.cc:15: header->major_version < 1 ||
why?

http://codereview.chromium.org/4409002/diff/1/2#newcode17
third_party/hunspell/google/bdict.cc:17: header->aff_offset > bdict_length ||
this check is redundant with the check on L22, no?

http://codereview.chromium.org/4409002/diff/1/2#newcode28
third_party/hunspell/google/bdict.cc:28: &bdict_data[header->aff_offset]);
indent 4 more spaces please

http://codereview.chromium.org/4409002/diff/1/2#newcode34
third_party/hunspell/google/bdict.cc:34: MD5Sum(&bdict_data[header->aff_offset],
bdict_length - header->aff_offset,
can you use aff_header as the first parameter

http://codereview.chromium.org/4409002/diff/1/4
File third_party/hunspell/google/bdict_reader.cc (right):

http://codereview.chromium.org/4409002/diff/1/4#newcode654
third_party/hunspell/google/bdict_reader.cc:654: header_->aff_offset >
bdict_length ||
again, redundant?

http://codereview.chromium.org/4409002/diff/1/4#newcode669
third_party/hunspell/google/bdict_reader.cc:669: // WebKit. To avoid blocking
WebKit long time, we do not check the MD5 digest
nit: ...blocking Webkit for a long time...

[Hironori Bono, 2010-11-05 09:30:04]

Greetings,

Thank you for your review and comments. I have applied your comments and updated
this change.
As for the usage of BDict::Verify(), I'm planning to call it after download as
listed in the chrome-side change: <http://codereview.chromium.org/4477002>.
(This change calls this function in SpellCheckHost::SaveDictionaryData() because
I assume it is better to call it on the file thread than on the UI thread.)
Is it possible to take another look?

Regards,

Hironori Bono

http://codereview.chromium.org/4409002/diff/1/2
File third_party/hunspell/google/bdict.cc (right):

http://codereview.chromium.org/4409002/diff/1/2#newcode15
third_party/hunspell/google/bdict.cc:15: header->major_version < 1 ||
On 2010/11/05 01:19:40, Evan Stade wrote:
> why?

Done. This is just because I thought it was an int variable somehow. I have
deleted this check.

http://codereview.chromium.org/4409002/diff/1/2#newcode17
third_party/hunspell/google/bdict.cc:17: header->aff_offset > bdict_length ||
On 2010/11/05 01:19:40, Evan Stade wrote:
> this check is redundant with the check on L22, no?

Yes, it is redundant. (I have copied this code without checks.) Thank you for
noticing it.

http://codereview.chromium.org/4409002/diff/1/2#newcode28
third_party/hunspell/google/bdict.cc:28: &bdict_data[header->aff_offset]);
On 2010/11/05 01:19:40, Evan Stade wrote:
> indent 4 more spaces please

Done. Thank you for noticing it.

http://codereview.chromium.org/4409002/diff/1/2#newcode34
third_party/hunspell/google/bdict.cc:34: MD5Sum(&bdict_data[header->aff_offset],
bdict_length - header->aff_offset,
On 2010/11/05 01:19:40, Evan Stade wrote:
> can you use aff_header as the first parameter

Yes, I have changed the first parameter.

http://codereview.chromium.org/4409002/diff/1/4
File third_party/hunspell/google/bdict_reader.cc (right):

http://codereview.chromium.org/4409002/diff/1/4#newcode654
third_party/hunspell/google/bdict_reader.cc:654: header_->aff_offset >
bdict_length ||
On 2010/11/05 01:19:40, Evan Stade wrote:
> again, redundant?

Yes, it is. I have removed this check.

http://codereview.chromium.org/4409002/diff/1/4#newcode669
third_party/hunspell/google/bdict_reader.cc:669: // WebKit. To avoid blocking
WebKit long time, we do not check the MD5 digest
On 2010/11/05 01:19:40, Evan Stade wrote:
> nit: ...blocking Webkit for a long time...

Done.

[Evan Stade, 2010-11-05 18:57:26]

lgtm

[jungshik at Google, 2010-11-05 19:33:41]

lgtm