Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1421473009:
Translate: Order language names in ASCII for locales ICU doesn't support (Closed)
|
Created:
5 years, 1 month ago by hajimehoshi Modified:
5 years, 1 month ago CC:
chromium-reviews, Miguel Garcia Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTranslate: Order language names in ASCII for locales ICU doesn't support
This CL fixes the crash at the first usage of an icu::Collator. It seems
like that icu::Collator can't be created for some locales especially on
Android. In this case, instead of using Collator, let's order the
language names in ASCII.
BUG=548355
TEST=n/a
Committed: https://crrev.com/005fcebeecbf198f2c94da4605e6b6c2b7fc0759
Cr-Commit-Position: refs/heads/master@{#357806}
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : Miguel's review #Patch Set 4 : #
Total comments: 7
Patch Set 5 : Miguel's review #
Total comments: 2
Patch Set 6 : Miguel's review #
Total comments: 1
Messages
Total messages: 16 (5 generated)
hajimehoshi@chromium.org changed reviewers: + andrewhayden@chromium.org
I wasn't able to reproduce this bug on my Linux environment. Andrew, would it be possible for you to reproduce the bug and confirm that this CL fixes this? Thanks.
Description was changed from ========== Translate: Order language names in ASCII for some locales This CL fixes the crash at the first usage of an icu::Collator. It seems like that icu::Collator can't be created for some locales especially on Android. In this case, instead of using Collator, let's order the language names in ASCII. BUG=548355 TEST=n/a ========== to ========== Translate: Order language names in ASCII for locales ICU doesn't support This CL fixes the crash at the first usage of an icu::Collator. It seems like that icu::Collator can't be created for some locales especially on Android. In this case, instead of using Collator, let's order the language names in ASCII. BUG=548355 TEST=n/a ==========
miguelg@chromium.org changed reviewers: + miguelg@chromium.org
Some by pass suggestions, I will let Andrew as the main reviewer l-g-t-m this. https://codereview.chromium.org/1421473009/diff/20001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1421473009/diff/20001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:57: icu::Collator* collator_instance = icu::Collator::createInstance(loc, error); this code is starting to be far from trivial and it happens in a constructor which is not great. Any chance we can move it to a static (or at least non virtual) method and write a test for it? https://codereview.chromium.org/1421473009/diff/20001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:63: } It seems you can get a collator object and still have an error code other than U_SUCCESS which will break the logic in line 76 and below. https://codereview.chromium.org/1421473009/diff/20001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:76: if (collator) { can you perhaps check if we should do ascii or icu sorting before the loop sort appropriately in the right method instead of using continue?
Thank you! https://codereview.chromium.org/1421473009/diff/20001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1421473009/diff/20001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:57: icu::Collator* collator_instance = icu::Collator::createInstance(loc, error); On 2015/11/02 12:09:26, Miguel Garcia wrote: > this code is starting to be far from trivial and it happens in a constructor > which is not great. Any chance we can move it to a static (or at least non > virtual) method and write a test for it? createInstance's behavior depends on platforms, so it seems hard to write a test. I left TODO comment. Done. https://codereview.chromium.org/1421473009/diff/20001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:63: } On 2015/11/02 12:09:26, Miguel Garcia wrote: > It seems you can get a collator object and still have an error code other than > U_SUCCESS which will break the logic in line 76 and below. Thanks. Changed to check if |collator_instance| is null or not instead. Done. https://codereview.chromium.org/1421473009/diff/20001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:76: if (collator) { On 2015/11/02 12:09:26, Miguel Garcia wrote: > can you perhaps check if we should do ascii or icu sorting before the loop sort > appropriately in the right method instead of using continue? Done.
https://codereview.chromium.org/1421473009/diff/60001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1421473009/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:33: scoped_ptr<icu::Collator> CreateCollator(const std::string& locale) { document the return value https://codereview.chromium.org/1421473009/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:37: if (!collator_instance) I think this should be if(!collator_instance || !U_SUCCESS(error)) so we account for the case where you get a "degraded" collator https://codereview.chromium.org/1421473009/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:39: scoped_ptr<icu::Collator> collator(collator_instance); can you make this atomic? i.e. just do scoped_ptr<icu::Collator> collator(icu::Collator::createInstance(loc, error)) in line 36. https://codereview.chromium.org/1421473009/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:89: // case, let's order the languages in ASCII. I guess this is actually ordering in UTF-8 which as a fallback is still fine but let's update the comment.
Thank you! https://codereview.chromium.org/1421473009/diff/60001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1421473009/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:33: scoped_ptr<icu::Collator> CreateCollator(const std::string& locale) { On 2015/11/04 09:14:58, Miguel Garcia wrote: > document the return value Done. https://codereview.chromium.org/1421473009/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:37: if (!collator_instance) On 2015/11/04 09:14:58, Miguel Garcia wrote: > I think this should be if(!collator_instance || !U_SUCCESS(error)) so we account > for the case where you get a "degraded" collator Ah, that's much better. Done. https://codereview.chromium.org/1421473009/diff/60001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:39: scoped_ptr<icu::Collator> collator(collator_instance); On 2015/11/04 09:14:58, Miguel Garcia wrote: > can you make this atomic? i.e. just do > scoped_ptr<icu::Collator> collator(icu::Collator::createInstance(loc, error)) in > line 36. Done.
lgtm I found out that Andrew is on vacation and since this is a M47 blocker let's submit it. I think I have looked at the code enough to ensure it's fine to land. In a subsequent CL I would encourage you to explore sorting the vector using stl sort (in <algorithm>) with a custom compare object that uses the collator when possible (or UTF8 otherwise). This would be much cleaner than the different for loops we have here. https://codereview.chromium.org/1421473009/diff/80001/components/translate/co... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1421473009/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:32: // Returns a Collator object which helps to sort strings in a given locale. nit: Add // or null if unable to find the right collator https://codereview.chromium.org/1421473009/diff/80001/components/translate/co... components/translate/core/browser/translate_ui_delegate.cc:90: // case, let's order the languages in ASCII. Can you change ASCII to UTF-8 here? The actual sorting we are doing does include UTF-8 characters
On 2015/11/04 10:30:53, Miguel Garcia wrote: > lgtm > > I found out that Andrew is on vacation and since this is a M47 blocker let's > submit it. I think I have looked at the code enough to ensure it's fine to land. Sure, I'll commit this while I've not reproduced this crash though. I believe this CL doesn't change the current correct behavior. > > In a subsequent CL I would encourage you to explore sorting the vector using stl > sort (in <algorithm>) with a custom compare object that uses the collator when > possible (or UTF8 otherwise). This would be much cleaner than the different for > loops we have here. I agree. I'd do this refactoring in another CL. > > https://codereview.chromium.org/1421473009/diff/80001/components/translate/co... > File components/translate/core/browser/translate_ui_delegate.cc (right): > > https://codereview.chromium.org/1421473009/diff/80001/components/translate/co... > components/translate/core/browser/translate_ui_delegate.cc:32: // Returns a > Collator object which helps to sort strings in a given locale. > nit: Add > // or null if unable to find the right collator > > https://codereview.chromium.org/1421473009/diff/80001/components/translate/co... > components/translate/core/browser/translate_ui_delegate.cc:90: // case, let's > order the languages in ASCII. > Can you change ASCII to UTF-8 here? The actual sorting we are doing does include > UTF-8 characters Done.
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miguelg@chromium.org Link to the patchset: https://codereview.chromium.org/1421473009/#ps100001 (title: "Miguel's review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1421473009/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1421473009/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/005fcebeecbf198f2c94da4605e6b6c2b7fc0759 Cr-Commit-Position: refs/heads/master@{#357806}
Message was sent while issue was closed.
I'm back. Thanks, everyone, for taking care of this. LGTM post-submit :) https://codereview.chromium.org/1421473009/diff/100001/components/translate/c... File components/translate/core/browser/translate_ui_delegate.cc (right): https://codereview.chromium.org/1421473009/diff/100001/components/translate/c... components/translate/core/browser/translate_ui_delegate.cc:41: return nullptr; It would have been nice to add a comment here indicating that the nullptr return case is actually expected and normal for some locales with a link to the bug. |
