Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1814503003:
Spellchecking: Added use of Persian dictionary (Closed)
|
Created:
4 years, 9 months ago by Kevin Bailey Modified:
4 years, 8 months ago CC:
chromium-reviews, groby+spellwatch_chromium.org, rlp+watch_chromium.org, rouslan+spell_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSpellchecking: Added use of Persian dictionary
And see codereview/1810993003
BUG=565069, 568673
Committed: https://crrev.com/c6e9ccf2ccb95f2643f5a914e77d5217261878d0
Cr-Commit-Position: refs/heads/master@{#383908}
Patch Set 1 #Patch Set 2 : Added Persian to a couple unit-tests #
Total comments: 2
Patch Set 3 : fa-IR version and en-AU -> en-GB #Patch Set 4 : Bumped third_party/hunspell_dictionaries DEPS hash #
Messages
Total messages: 28 (10 generated)
Description was changed from ========== Spellchecking: Added use of Persian dictionary And see codereview/1810993003 BUG=565069, 568673 ========== to ========== Spellchecking: Added use of Persian dictionary And see codereview/1810993003 BUG=565069, 568673 ==========
krb@chromium.org changed reviewers: + groby@chromium.org, rouslan@chromium.org
A new unit test in spellcheck_unittest.cc seems to be missing. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/sp...
On 2016/03/17 21:00:02, Rouslan wrote: > A new unit test in spellcheck_unittest.cc seems to be missing. > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/sp... Maybe. What does that test actually check, though? Does that serve as our end-to-end test? It's technically testing hunspell & icu, unless you're on Mac, where this is actually at the mercy of the system spell checker.
On 2016/03/18 00:53:24, groby wrote: > On 2016/03/17 21:00:02, Rouslan wrote: > > A new unit test in spellcheck_unittest.cc seems to be missing. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/sp... > > Maybe. What does that test actually check, though? Does that serve as our > end-to-end test? It's technically testing hunspell & icu, unless you're on Mac, > where this is actually at the mercy of the system spell checker. I tend to agree that the value of a unit test that doesn't find an error in some text, or a unit test that finds a spelling error in total gibberish, is pretty low. I think what's more interesting is covering some old bug - such as if Chrome accidentally allowed English words when Persian was selected. Nevertheless, the back-stop is pretty cheap, so done.
On 2016/03/18 00:53:24, groby wrote: > On 2016/03/17 21:00:02, Rouslan wrote: > > A new unit test in spellcheck_unittest.cc seems to be missing. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/sp... > > Maybe. What does that test actually check, though? Does that serve as our > end-to-end test? It's technically testing hunspell & icu, unless you're on Mac, > where this is actually at the mercy of the system spell checker. That's the test which pointed me to the deficiencies in our persian/arabic word breaking rules. Therefore, it's valuable.
lgtm after fa-IR version is changed to 7-0. https://codereview.chromium.org/1814503003/diff/20001/chrome/common/spellchec... File chrome/common/spellcheck_common.cc (right): https://codereview.chromium.org/1814503003/diff/20001/chrome/common/spellchec... chrome/common/spellcheck_common.cc:124: {"fa-IR", "-2-0"}, fa-IR files should be versioned 7-0. See the comment above: "Use the same version for all the dictionaries that you add at the same time."
https://codereview.chromium.org/1814503003/diff/20001/chrome/common/spellchec... File chrome/common/spellcheck_common.cc (right): https://codereview.chromium.org/1814503003/diff/20001/chrome/common/spellchec... chrome/common/spellcheck_common.cc:124: {"fa-IR", "-2-0"}, On 2016/03/18 17:28:45, Rouslan wrote: > fa-IR files should be versioned 7-0. > > See the comment above: "Use the same version for all the dictionaries that you > add at the same time." Rather than blindly follow this advice, I'd like to understand why, in case it only applies to e.g. a bundle of en-* dictionaries. I think there's value in tracking the "official" fa_IR dictionary's version, for example.
Not sure why. I think the original thinking was to avoid thinking that fa-IR-2-0.bdic and en-US-2-0.bdic are the same age. We're using the freshest dictionaries for both en-US and fa-IR as of this month. Therefore, they should have the same version. Does that work?
My concern was that someone would have to check the date of a dictionary update to know if it was new (instead of Lilak's version number), but I guess they have to do that anyways with the hunspell stuff. So I made them all 7.0.
The CQ bit was checked by krb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814503003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814503003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
FWIW - this is failing because the 7.0 directories are not uploaded yet. (Mostly a note to my future self that'll wonder why this is red. Again :)
On 2016/03/29 19:40:44, groby wrote: > FWIW - this is failing because the 7.0 directories are not uploaded yet. (Mostly > a note to my future self that'll wonder why this is red. Again :) Bonus points for a CL that checks if all dictionaries are available before running the rest of the spellcheck test, so we don't have to parse test results/code
Btw, the CL needs to bump the hunspell_dictionaries version in src/DEPS <https://code.google.com/p/chromium/codesearch#chromium/src/DEPS&l=141>. On Tue, Mar 29, 2016 at 12:41 PM <groby@chromium.org> wrote: > On 2016/03/29 19:40:44, groby wrote: > > FWIW - this is failing because the 7.0 directories are not uploaded yet. > (Mostly > > a note to my future self that'll wonder why this is red. Again :) > > Bonus points for a CL that checks if all dictionaries are available before > running the rest of the spellcheck test, so we don't have to parse test > results/code > > https://codereview.chromium.org/1814503003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by krb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814503003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814503003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by krb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/1814503003/#ps60001 (title: "Bumped third_party/hunspell_dictionaries DEPS hash")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814503003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814503003/60001
Message was sent while issue was closed.
Description was changed from ========== Spellchecking: Added use of Persian dictionary And see codereview/1810993003 BUG=565069, 568673 ========== to ========== Spellchecking: Added use of Persian dictionary And see codereview/1810993003 BUG=565069, 568673 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Spellchecking: Added use of Persian dictionary And see codereview/1810993003 BUG=565069, 568673 ========== to ========== Spellchecking: Added use of Persian dictionary And see codereview/1810993003 BUG=565069, 568673 Committed: https://crrev.com/c6e9ccf2ccb95f2643f5a914e77d5217261878d0 Cr-Commit-Position: refs/heads/master@{#383908} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c6e9ccf2ccb95f2643f5a914e77d5217261878d0 Cr-Commit-Position: refs/heads/master@{#383908} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
