Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
323023002:
aura: Supports text input on non UTF-8 environments. (Closed)
|
Created:
6 years, 6 months ago by Yuki Modified:
6 years, 6 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, jshin+watch_chromium.org, tdresser+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionaura: Supports text input on non UTF-8 environments.
We have assumed that XLookupString always returns a UTF-8 string, but it's not true. It could be any other encoding depending on the system locale and we have to support it.
The root cause of the Issue 376893 is
1. XLookupString returns a ISO-8859-1 string (for example)
2. We converts it by UTF8ToUTF16.
3. The resulting character code is zero.
4. Cannot input a (non-ASCII) character.
BUG=376893
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277003
Patch Set 1 #Patch Set 2 : Synced. #Patch Set 3 : Synced. #Patch Set 4 : Addressed review comments. #
Total comments: 2
Patch Set 5 : Synced. #Messages
Total messages: 17 (0 generated)
Could you guys review this CL? sadrul@: over all, especially in ui/events/ brettw@: as an owner of base/strings/
Is this only for Linux, or is this necessary/useful for Chrome OS too (because on Chrome OS, we are going to remove explicit glib dependency: crbug.com/170408). Is it possible to use Xutf8LookupString() instead of XLookupString()?
The current locale is supposed to be what the "NativeMB" variants do. So SysNativeMBToWide(...) and then I guess WideToUTF8(). That's pretty different than your implementation. But it should end up with the same result.
On 2014/06/11 20:54:58, sadrul wrote: > Is this only for Linux, or is this necessary/useful for Chrome OS too (because > on Chrome OS, we are going to remove explicit glib dependency: > crbug.com/170408). > > Is it possible to use Xutf8LookupString() instead of XLookupString()? IIUC, CrOS always use UTF-8, so we don't need this code for CrOS. I can add #if defined(OS_LINUX). Will do. I first tried to use Xutf8LookupString, but it's hard. It requires XIC as the first arg, and it indirectly XIM, etc. I don't want to introduce those things.
On 2014/06/11 21:38:03, brettw wrote: > The current locale is supposed to be what the "NativeMB" variants do. So > SysNativeMBToWide(...) and then I guess WideToUTF8(). > > That's pretty different than your implementation. But it should end up with the > same result. https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/sys_s... SysNativeMBToWide supports only UTF-8 on posix platforms. It doesn't work for non UTF-8 strings.
Interesting. Then I think the proper fix would be to implement that function properly and double-check that the existing callers (there aren't too many) can handle the new behavior. I did a quick look and in general it looks like callers are expecting the behavior you're proposing.
On 2014/06/12 04:20:57, brettw wrote: > Interesting. Then I think the proper fix would be to implement that function > properly and double-check that the existing callers (there aren't too many) can > handle the new behavior. I did a quick look and in general it looks like callers > are expecting the behavior you're proposing. I have some concerns. 1. My new code doesn't work on NaCl. Is it acceptable for SysNativeMBToWide? 2. For my purpose, I want a UTF-16 string, not a wide string. system locale -> UTF-8 -> UTF-16 is simpler than system locale -> UTF-8 -> Wide and Wide -> UTF-8 -> UTF-16. (I didn't find Wide -> UTF-16 conversion.)
Looking at this again, I think you misread the Posix implementation. The thing you linked to is the Android/ChromeOS version which correctly assumes UTF-8 for the native MB encoding. The real implementation is below for desktop Linux and that looks correct to me.
Thanks for useful review comments. Brett was right. I've updated the code and it became much simpler. Could you take another look?
Cool. This is much simpler. lgtm, but please also wait for a review from keycodes/ owner. +wez@
https://codereview.chromium.org/323023002/diff/60001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/323023002/diff/60001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:444: return result.length() == 1 ? result[0] : 0; What about characters outside the Basic Multilingual (i.e. 16-bit) Plane?
https://codereview.chromium.org/323023002/diff/60001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/323023002/diff/60001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:444: return result.length() == 1 ? result[0] : 0; On 2014/06/12 19:03:57, Wez wrote: > What about characters outside the Basic Multilingual (i.e. 16-bit) Plane? It's simply not supported by this function. Since this function returns uint16, we cannot return such characters out of 16-bit. Also the callers do not expect it. For example, text[i] = GetCharacterFromXEvent(native_event); doesn't expect such characters. Since this function is used for key events and AFAIK there is no such key, so it should be okay at least for now. If there were such keyboard, the resulting character code would be zero, and users cannot type it. If we really care such cases (I'm not really sure if there were the cases), we'd better revisit that point later. Can I go with this code for now? At least this should be good as same as the old implementation (the old code doesn't care about characters out of 16-bit, too).
lgtm
The CQ bit was checked by yukishiino@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yukishiino@chromium.org/323023002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
Message was sent while issue was closed.
Change committed as 277003 |
