Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(50)

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
Reviewers:
brettw, sadrul, Wez
CC:
chromium-reviews, erikwright+watch_chromium.org, jshin+watch_chromium.org, tdresser+watch_chromium.org
Visibility:
Public.

Description

aura: 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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -3 lines) Patch
M ui/events/keycodes/keyboard_code_conversion_x.cc View 1 2 3 2 chunks +6 lines, -3 lines 0 comments Download

Messages

Total messages: 17 (0 generated)
Yuki
Could you guys review this CL? sadrul@: over all, especially in ui/events/ brettw@: as an ...
6 years, 6 months ago (2014-06-10 06:24:00 UTC) #1
sadrul
Is this only for Linux, or is this necessary/useful for Chrome OS too (because on ...
6 years, 6 months ago (2014-06-11 20:54:58 UTC) #2
brettw
The current locale is supposed to be what the "NativeMB" variants do. So SysNativeMBToWide(...) and ...
6 years, 6 months ago (2014-06-11 21:38:03 UTC) #3
Yuki
On 2014/06/11 20:54:58, sadrul wrote: > Is this only for Linux, or is this necessary/useful ...
6 years, 6 months ago (2014-06-12 03:33:14 UTC) #4
Yuki
On 2014/06/11 21:38:03, brettw wrote: > The current locale is supposed to be what the ...
6 years, 6 months ago (2014-06-12 03:34:56 UTC) #5
brettw
Interesting. Then I think the proper fix would be to implement that function properly and ...
6 years, 6 months ago (2014-06-12 04:20:57 UTC) #6
Yuki
On 2014/06/12 04:20:57, brettw wrote: > Interesting. Then I think the proper fix would be ...
6 years, 6 months ago (2014-06-12 04:41:45 UTC) #7
brettw
Looking at this again, I think you misread the Posix implementation. The thing you linked ...
6 years, 6 months ago (2014-06-12 05:08:08 UTC) #8
Yuki
Thanks for useful review comments. Brett was right. I've updated the code and it became ...
6 years, 6 months ago (2014-06-12 05:57:02 UTC) #9
sadrul
Cool. This is much simpler. lgtm, but please also wait for a review from keycodes/ ...
6 years, 6 months ago (2014-06-12 11:31:14 UTC) #10
Wez
https://codereview.chromium.org/323023002/diff/60001/ui/events/keycodes/keyboard_code_conversion_x.cc File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/323023002/diff/60001/ui/events/keycodes/keyboard_code_conversion_x.cc#newcode444 ui/events/keycodes/keyboard_code_conversion_x.cc:444: return result.length() == 1 ? result[0] : 0; What ...
6 years, 6 months ago (2014-06-12 19:03:58 UTC) #11
Yuki
https://codereview.chromium.org/323023002/diff/60001/ui/events/keycodes/keyboard_code_conversion_x.cc File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/323023002/diff/60001/ui/events/keycodes/keyboard_code_conversion_x.cc#newcode444 ui/events/keycodes/keyboard_code_conversion_x.cc:444: return result.length() == 1 ? result[0] : 0; On ...
6 years, 6 months ago (2014-06-13 02:58:31 UTC) #12
Wez
lgtm
6 years, 6 months ago (2014-06-13 04:55:28 UTC) #13
Yuki
The CQ bit was checked by yukishiino@chromium.org
6 years, 6 months ago (2014-06-13 05:59:24 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yukishiino@chromium.org/323023002/80001
6 years, 6 months ago (2014-06-13 06:01:15 UTC) #15
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium ...
6 years, 6 months ago (2014-06-13 10:34:40 UTC) #16
commit-bot: I haz the power
6 years, 6 months ago (2014-06-13 12:59:16 UTC) #17
Message was sent while issue was closed.
Change committed as 277003

Powered by Google App Engine
This is Rietveld 408576698