Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1937203002:
Fixed hasty language selection in GetCorrespondingSpellCheckLanguage (Closed)
|
Created:
4 years, 7 months ago by Kevin Bailey Modified:
4 years, 7 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. |
DescriptionIn 'GetCorrespondingSpellCheckLanguage()', we were
finding the first language whose region matched the
incoming language. For example, it would find 'en-GB'
in the region and assume that the primary language
was 'en-AU'. Changed to continue searching to see if
there is a perfect match later in the language field.
BUG=608383
Committed: https://crrev.com/ca4eea63cf1d51f5d1e2ba4a6e3047f622438148
Cr-Commit-Position: refs/heads/master@{#391566}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added another unit test #
Total comments: 1
Messages
Total messages: 21 (9 generated)
Description was changed from ========== Fixed wrong language selection and added unit test. BUG=608383 ========== to ========== Fixed wrong language selection and added unit test. BUG=608383 ==========
krb@chromium.org changed reviewers: + rouslan@chromium.org
It was searching by value to find the key, so had to help it out a little. Passes by-hand test, and (new) unit test passes new code but fails on old code.
https://codereview.chromium.org/1937203002/diff/1/chrome/common/spellcheck_co... File chrome/common/spellcheck_common.cc (right): https://codereview.chromium.org/1937203002/diff/1/chrome/common/spellcheck_co... chrome/common/spellcheck_common.cc:158: best_match = g_supported_spellchecker_languages[i].language; If we're switching from returning the first element to returning the last matching element, would be simpler to change the order in g_supported_spellchecker_languages instead? https://codereview.chromium.org/1937203002/diff/1/chrome/common/spellcheck_co... chrome/common/spellcheck_common.cc:163: return best_match; Add a test case for blank best match.
https://codereview.chromium.org/1937203002/diff/1/chrome/common/spellcheck_co... File chrome/common/spellcheck_common.cc (right): https://codereview.chromium.org/1937203002/diff/1/chrome/common/spellcheck_co... chrome/common/spellcheck_common.cc:158: best_match = g_supported_spellchecker_languages[i].language; On 2016/05/02 18:02:28, Rouslan wrote: > If we're switching from returning the first element to returning the last > matching element, would be simpler to change the order in > g_supported_spellchecker_languages instead? The other functions depend on the current order. I'm not sure why you'd want to do that. https://codereview.chromium.org/1937203002/diff/1/chrome/common/spellcheck_co... chrome/common/spellcheck_common.cc:163: return best_match; On 2016/05/02 18:02:28, Rouslan wrote: > Add a test case for blank best match. I assume there already is one, since this is not new behavior, but since it's one line, I added one to the same file.
lgtm
groby@chromium.org changed reviewers: + groby@chromium.org
https://codereview.chromium.org/1937203002/diff/20001/chrome/common/spellchec... File chrome/common/spellcheck_common.cc (right): https://codereview.chromium.org/1937203002/diff/20001/chrome/common/spellchec... chrome/common/spellcheck_common.cc:156: if (spellcheck_language_region == language) { What specific behavior does this fix? Is this _just_ for the en_AU/en_GB case? We should at the very least call it out specifically - and consider rolling back the unification.
On 2016/05/02 19:07:49, groby wrote: > https://codereview.chromium.org/1937203002/diff/20001/chrome/common/spellchec... > File chrome/common/spellcheck_common.cc (right): > > https://codereview.chromium.org/1937203002/diff/20001/chrome/common/spellchec... > chrome/common/spellcheck_common.cc:156: if (spellcheck_language_region == > language) { > What specific behavior does this fix? Is this _just_ for the en_AU/en_GB case? > We should at the very least call it out specifically - and consider rolling back > the unification. And please do provide a slightly clearer description - ideally, people can guess why we're making a change based on description only and don't require navigating to the bug.
Description was changed from ========== Fixed wrong language selection and added unit test. BUG=608383 ========== to ========== In 'GetCorrespondingSpellCheckLanguage()', we were finding the first language whose region matched the incoming language. For example, it would find 'en-GB' in the region and assume that the primary language was 'en-AU'. Changed to continue searching to see if there is a perfect match later in the language field. BUG=608383 ==========
On 2016/05/02 19:07:49, groby wrote:
> What specific behavior does this fix? Is this _just_ for the en_AU/en_GB case?
> We should at the very least call it out specifically - and consider rolling
back
> the unification.
The change fixes what I would call a poor assumption made by the changed
function.
The function was doing:
for each key, value pair:
if target == key:
return key
if target == value:
return key
So it returns the first poor match (based on the value), instead of seeing if
there's a perfect match (based on the key) later.
If it was just for the GB/AU case, I would have just swapped the entries
(vertically). I wanted the next person who added an N:1 mapping to not run into
this.
> And please do provide a slightly clearer description - ideally, people can
guess
> why we're making a change based on description only and don't require
navigating
> to the bug.
I cheered up the description. I agree it was short, but I assume we don't
want to see a repetition of the symptom.
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/1937203002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1937203002/20001
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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1937203002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1937203002/20001
Message was sent while issue was closed.
Description was changed from ========== In 'GetCorrespondingSpellCheckLanguage()', we were finding the first language whose region matched the incoming language. For example, it would find 'en-GB' in the region and assume that the primary language was 'en-AU'. Changed to continue searching to see if there is a perfect match later in the language field. BUG=608383 ========== to ========== In 'GetCorrespondingSpellCheckLanguage()', we were finding the first language whose region matched the incoming language. For example, it would find 'en-GB' in the region and assume that the primary language was 'en-AU'. Changed to continue searching to see if there is a perfect match later in the language field. BUG=608383 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== In 'GetCorrespondingSpellCheckLanguage()', we were finding the first language whose region matched the incoming language. For example, it would find 'en-GB' in the region and assume that the primary language was 'en-AU'. Changed to continue searching to see if there is a perfect match later in the language field. BUG=608383 ========== to ========== In 'GetCorrespondingSpellCheckLanguage()', we were finding the first language whose region matched the incoming language. For example, it would find 'en-GB' in the region and assume that the primary language was 'en-AU'. Changed to continue searching to see if there is a perfect match later in the language field. BUG=608383 Committed: https://crrev.com/ca4eea63cf1d51f5d1e2ba4a6e3047f622438148 Cr-Commit-Position: refs/heads/master@{#391566} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ca4eea63cf1d51f5d1e2ba4a6e3047f622438148 Cr-Commit-Position: refs/heads/master@{#391566} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
