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

Issue 83233002: Enable basic IME functionality under Ash on Windows (Closed)

Created:
7 years, 1 month ago by yukawa
Modified:
7 years ago
CC:
chromium-reviews, sadrul, ben+aura_chromium.org, kalyank, msw
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Enable basic IME functionality under Ash on Windows With this CL, IMEs become functional under Ash environment on Windows. Some optional features for Chromium internal use (e.g., IME popup window detection, RTL language detection) will be covered by subsequent CLs. DesignDoc: https://docs.google.com/a/chromium.org/document/d/14TBE0LoamQz_MImcNWzeTIW_lo5EUrLJwNwmCi20V1o/edit# Note: In this CL, ui/base/ime/win/tsf_text_store.* is not moved but copied to win8/metro_driver/ime/text_store.* in order not to break non-Aura build. That said, hereafter we will use win8/metro_driver/ime/text_store.* as primary implementation of TSF TextStore. Anyway, ui/base/ime/win/tsf_text_store.* will be removed when Aura transition is successfully completed. I also found some style issues in win8/metro_driver/ime/text_store.cc but I'd like to fix them in subsequent CLs to keep this CL as minimum as possible. BUG=164964 TEST=manually done on Windows 8.1 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238921

Patch Set 1 : #

Patch Set 2 : More task delegation from ChromeAppViewAsh to TextServiceImpl #

Total comments: 12

Patch Set 3 : Address comment from ananta. Add text_service_delegate.h, which I forgot to upload. Style fixes. #

Total comments: 29

Patch Set 4 : rebase #

Patch Set 5 : Address comment from ananta. #

Total comments: 8

Patch Set 6 : Address comment from ananta@ (Logging HRESULT, etc) #

Patch Set 7 : rebase #

Patch Set 8 : Fix compile error caused by missing include. #

Patch Set 9 : Fix warnings (that are treated as an error) on 64-bit build #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1134 lines, -173 lines) Patch
M ui/aura/remote_root_window_host_win.h View 1 2 3 4 5 6 4 chunks +21 lines, -1 line 0 comments Download
M ui/aura/remote_root_window_host_win.cc View 1 2 3 4 5 6 7 chunks +88 lines, -0 lines 0 comments Download
M win8/metro_driver/chrome_app_view_ash.h View 1 5 chunks +29 lines, -1 line 0 comments Download
M win8/metro_driver/chrome_app_view_ash.cc View 1 2 3 7 chunks +54 lines, -0 lines 0 comments Download
A win8/metro_driver/ime/ime.gypi View 1 1 chunk +16 lines, -0 lines 0 comments Download
A win8/metro_driver/ime/input_scope.h View 1 chunk +22 lines, -0 lines 0 comments Download
A win8/metro_driver/ime/input_scope.cc View 1 2 3 4 5 6 7 8 1 chunk +87 lines, -0 lines 0 comments Download
A win8/metro_driver/ime/text_service.h View 1 2 3 4 5 6 7 1 chunk +53 lines, -0 lines 0 comments Download
A win8/metro_driver/ime/text_service.cc View 1 2 3 4 5 1 chunk +494 lines, -0 lines 0 comments Download
A win8/metro_driver/ime/text_service_delegate.h View 1 2 1 chunk +40 lines, -0 lines 0 comments Download
A + win8/metro_driver/ime/text_store.h View 1 2 3 4 5 6 7 8 7 chunks +48 lines, -37 lines 0 comments Download
A + win8/metro_driver/ime/text_store.cc View 1 2 3 4 5 6 7 8 25 chunks +124 lines, -134 lines 0 comments Download
A win8/metro_driver/ime/text_store_delegate.h View 1 chunk +55 lines, -0 lines 0 comments Download
M win8/metro_driver/metro_driver.gyp View 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 22 (0 generated)
yukawa
Hi reviewers, This is a bit big CL but could you kindly review it? cpu@: ...
7 years ago (2013-11-25 18:26:27 UTC) #1
sky
lgtm LGTM
7 years ago (2013-11-25 21:41:31 UTC) #2
horo
lgtm
7 years ago (2013-11-26 06:54:22 UTC) #3
ananta
https://codereview.chromium.org/83233002/diff/880001/win8/metro_driver/ime/input_scope.cc File win8/metro_driver/ime/input_scope.cc (right): https://codereview.chromium.org/83233002/diff/880001/win8/metro_driver/ime/input_scope.cc#newcode10 win8/metro_driver/ime/input_scope.cc:10: class InputScopeImpl : public ITfInputScope { Please add some ...
7 years ago (2013-11-27 02:27:44 UTC) #4
yukawa
https://codereview.chromium.org/83233002/diff/880001/win8/metro_driver/ime/input_scope.cc File win8/metro_driver/ime/input_scope.cc (right): https://codereview.chromium.org/83233002/diff/880001/win8/metro_driver/ime/input_scope.cc#newcode10 win8/metro_driver/ime/input_scope.cc:10: class InputScopeImpl : public ITfInputScope { On 2013/11/27 02:27:44, ...
7 years ago (2013-11-27 11:30:03 UTC) #5
ananta
Looking good. Mostly nits below. https://codereview.chromium.org/83233002/diff/940002/win8/metro_driver/chrome_app_view_ash.cc File win8/metro_driver/chrome_app_view_ash.cc (right): https://codereview.chromium.org/83233002/diff/940002/win8/metro_driver/chrome_app_view_ash.cc#newcode816 win8/metro_driver/chrome_app_view_ash.cc:816: if (!text_service_) Can text_service_ ...
7 years ago (2013-11-27 20:02:46 UTC) #6
yukawa
Thanks! PTAL? https://codereview.chromium.org/83233002/diff/940002/win8/metro_driver/chrome_app_view_ash.cc File win8/metro_driver/chrome_app_view_ash.cc (right): https://codereview.chromium.org/83233002/diff/940002/win8/metro_driver/chrome_app_view_ash.cc#newcode816 win8/metro_driver/chrome_app_view_ash.cc:816: if (!text_service_) On 2013/11/27 20:02:47, ananta wrote: ...
7 years ago (2013-11-28 06:06:20 UTC) #7
ananta
Looking good. Mostly nits. https://codereview.chromium.org/83233002/diff/940002/win8/metro_driver/chrome_app_view_ash.cc File win8/metro_driver/chrome_app_view_ash.cc (right): https://codereview.chromium.org/83233002/diff/940002/win8/metro_driver/chrome_app_view_ash.cc#newcode816 win8/metro_driver/chrome_app_view_ash.cc:816: if (!text_service_) On 2013/11/28 06:06:20, ...
7 years ago (2013-12-02 23:15:22 UTC) #8
cpu_(ooo_6.6-7.5)
only reviewed the metro bits and remote root window bits, lgtm
7 years ago (2013-12-02 23:44:32 UTC) #9
cpu_(ooo_6.6-7.5)
one more thing, please test snap view (on win 8.1) http://blog.laptopmag.com/use-split-screen-mode-windows-81
7 years ago (2013-12-02 23:46:20 UTC) #10
yukawa
ananta@: PTAL? cpu@: On 2013/12/02 23:46:20, cpu wrote: > one more thing, please test snap ...
7 years ago (2013-12-03 02:34:29 UTC) #11
yukawa
ananta@: ping?
7 years ago (2013-12-04 17:54:35 UTC) #12
ananta
lgtm
7 years ago (2013-12-04 19:34:02 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yukawa@chromium.org/83233002/1430001
7 years ago (2013-12-05 00:05:25 UTC) #14
commit-bot: I haz the power
Retried try job too often on win for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number=131470
7 years ago (2013-12-05 01:49:35 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yukawa@chromium.org/83233002/1470001
7 years ago (2013-12-05 02:13:04 UTC) #16
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=39529
7 years ago (2013-12-05 02:26:24 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yukawa@chromium.org/83233002/1470001
7 years ago (2013-12-05 02:48:03 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yukawa@chromium.org/83233002/1510001
7 years ago (2013-12-05 03:29:23 UTC) #19
commit-bot: I haz the power
Retried try job too often on win_x64_rel for step(s) base_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_rel&number=59112
7 years ago (2013-12-05 05:15:27 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yukawa@chromium.org/83233002/1530001
7 years ago (2013-12-05 05:47:33 UTC) #21
commit-bot: I haz the power
7 years ago (2013-12-05 08:31:42 UTC) #22
Message was sent while issue was closed.
Change committed as 238921

Powered by Google App Engine
This is Rietveld 408576698