Closed Bug 1318955 Opened 4 years ago Closed 4 years ago

Upgrade to Hunspell 1.5.4

Categories

(Core :: Spelling checker, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: RyanVM, Assigned: RyanVM)

References

Details

Attachments

(1 file, 2 obsolete files)

Changes since v1.4.1:

2016-11-18 Dimitrij Mijoski <dmjpp at hotm>:
 * Version 1.5
 * Lot of stability fixes
 * Fixed compilation errors on various systems (Windows, FreeBSD)
 * Small performance improvement compared to 1.4.0
 * API is same as 1.4.

2016-04-29 Caolán McNamara <caolanm at LibO>:
 * deprecate old api and add new one
   old one remains implemented in terms of new one
   and will eventually be removed
 * shrink exposed api down to just hunspell.hxx
 * next major release is likely to require C++11
Comment on attachment 8812590 [details] [diff] [review]
Update Hunspell to version 1.5.0

I've updated the version in README.mozilla to 1.5.0 locally, sorry for forgetting to do that before submitting.
Comment on attachment 8812590 [details] [diff] [review]
Update Hunspell to version 1.5.0

>diff --git a/extensions/spellcheck/hunspell/glue/mozHunspell.cpp b/extensions/spellcheck/hunspell/glue/mozHunspell.cpp
>--- a/extensions/spellcheck/hunspell/glue/mozHunspell.cpp
>+++ b/extensions/spellcheck/hunspell/glue/mozHunspell.cpp
>@@ -195,17 +195,17 @@ NS_IMETHODIMP mozHunspell::SetDictionary
>   mDictionary = aDictionary;
>   mAffixFileName = affFileName;
> 
>   mHunspell = new Hunspell(affFileName.get(),
>                          dictFileName.get());
>   if (!mHunspell)
>     return NS_ERROR_OUT_OF_MEMORY;
> 
>-  nsDependentCString label(mHunspell->get_dic_encoding());
>+  nsCString label(mHunspell->get_dict_encoding().c_str());

Why don't you use nsAutoCString?

>-nsresult mozHunspell::ConvertCharset(const char16_t* aStr, char ** aDst)
>+nsresult mozHunspell::ConvertCharset(const char16_t* aStr, std::string* aDst)

nit: I'd like you to break the line after |nresult|.


I assume that the changes under extensions/spellcheck/hunspell/src comes from upstream. So, I don't check them, if I should do that, let me know.
Attachment #8812590 - Flags: review?(masayuki) → review+
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #3)
> I assume that the changes under extensions/spellcheck/hunspell/src comes
> from upstream. So, I don't check them, if I should do that, let me know.

Correct. You're certainly welcome to if you want, though! :)
Tyson, have you been fuzzing Hunspell lately?
Flags: needinfo?(twsmith)
Attached patch Update Hunspell to version 1.5.0 (obsolete) — Splinter Review
This addresses your review comments. Still green on Try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7248e0dcc8157529e36e66a4a77816451efa9c6&group_state=expanded

While changing the nsCString to nsAutoCString, I noticed a couple instances in mozHunspell.cpp as well. Is there any point in changing those while I'm at it? Looks OK on Try as well.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a79569a89fdb7f7257dd34382fed2bc43bf2ff98

Thanks for the quick review!
Attachment #8812590 - Attachment is obsolete: true
Flags: needinfo?(masayuki)
Attachment #8812650 - Flags: review+
The reason why I pointed it is, you changed from nsDependentCString to nsCString. So, up to you for other lines because you don't need to touch them for fixing this bug. Perhaps, we should fix it in another bug if there are a lot of points which should be replaced with nsAuto*String.
Flags: needinfo?(masayuki)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25c7efa29d66
Upgrade to Hunspell 1.5.0. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/25c7efa29d66
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> Tyson, have you been fuzzing Hunspell lately?

Not since August. I can likely add it to the continuous fuzzing system this quarter but I want to make sure we are targeting the correct APIs.
Flags: needinfo?(twsmith)
Depends on: 1320746
I decided to back this out for causing bug 1320746 and also because it causes some API compatibility issues for systems that use the system Hunspell. Those have been resolved upstream, so I'll try to land the future 1.5.x release that includes the Hungarian fix.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9444e271b391686b1238a20c20175b8bd7d7b400
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla53 → ---
backout also from central
https://hg.mozilla.org/mozilla-central/rev/9444e271b391
What is the status  for updating to Hunspell version 1.5.4?
Haven't had time to work on it yet. Will try as soon as I can.
2016-11-30 Dimitrij Mijoski <dmjpp at hotm>:
	* Version 1.5.4
	* Fixes the command COMPOUNDSYLLABLE used in Hungarian dictionary.

2016-11-28 Dimitrij Mijoski <dmjpp at hotm>:
	* Version 1.5.3
	* Removed a #include from hunspell.hxx that was creating trouble

2016-11-27 Dimitrij Mijoski <dmjpp at hotm>:
	* Version 1.5.2
	* Reverted full backward compatibility with 1.4 public API, again

2016-11-27 Dimitrij Mijoski <dmjpp at hotm>:
	* Version 1.5.1
	* Reverted full backward compatibility with 1.4 public API

2016-11-26 Dimitrij Mijoski <dmjpp at hotm>:
	* Version 1.5
	* Fixed the changelog notice bellow about the API. I had a mistake.
Summary: Upgrade to Hunspell 1.5.0 → Upgrade to Hunspell 1.5.4
Looks good on Try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86cf532c4a6f7db4896cd2ca8a4fe349843c6f0e

avada, can you please test a build from this Try push to confirm that everything works as expected for you?
https://archive.mozilla.org/pub/firefox/try-builds/ryanvm@gmail.com-86cf532c4a6f7db4896cd2ca8a4fe349843c6f0e/

Masayuki-san, did you want to take another look at this before I re-push it (assuming avada's testing goes well)? No changes on the Gecko side compared to the 1.5.0 patch you previously reviewed.
Attachment #8812650 - Attachment is obsolete: true
Flags: needinfo?(masayuki)
Attachment #8817444 - Flags: feedback?(dqeswn)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Masayuki-san, did you want to take another look at this before I re-push it
> (assuming avada's testing goes well)? No changes on the Gecko side compared
> to the 1.5.0 patch you previously reviewed.

No. Up to you.
Flags: needinfo?(masayuki)
Comment on attachment 8817444 [details] [diff] [review]
Update Hunspell to version 1.5.4

Review of attachment 8817444 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. No breakages in spellcheck with the hungarian dictionary.
Attachment #8817444 - Flags: feedback?(dqeswn) → feedback+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33044cd4c5db
Upgrade to Hunspell 1.5.4. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/33044cd4c5db
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1326277
Blocks: 691240
This requires a version check to be added to pkg_check_modules in moz.configure, because this breaks building with system version 1.4.
Seconded, i had a build failure w/ 53.0b1 using --with-system-hunspell - our system version is 1.3.2... (yeah ancient i know)
You need to log in before you can comment on or make changes to this bug.