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

Issue 645683002: Get IME's to work in Chrome OS mode on Windows 7. (Closed)

Created:
6 years, 2 months ago by ananta
Modified:
6 years, 2 months ago
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, tfarina, penghuang+watch_chromium.org, nona+watch_chromium.org, James Su
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Get IME's to work in Chrome OS mode on Windows 7. IME support in Chrome OS mode/Windows 8 mode is currently implemented in two parts. 1. Metro driver (Viewer process): Here we rely on the Text services framework for IME notifications and relay those to the browser process. 2. Chrome Browser: The browser initializes the input method via the RemoteInputMethodWin class which initializes the input method if the viewer is an immersive process. Fixes as below:- 1. The metro driver initializes the text services framework by instantiating the CLSID_TF_ThreadMgr COM object and requests the ITfThreadMgr2 interface. This interface is not implemented for Windows 7. Fix is to use the ITfThreadMgr interface instead for Windows 7 and up. This provides all the functionality we need. 2. The metro driver was instantiating a MTA COM apartment. The text services COM objects expect to be instantiated in a STA. 3. The AppListService object on Windows attempts to bring up the app list bubble in ASH mode even if we are in desktop mode. Fix is to avoid that. 4. The IsRemoteInputMethodWinRequired function initialized the remote input method if the viewer was an immersive process. To ensure that it also works on Windows 7 we check whether the browser process is launched with the kViewerConnect switch. BUG=421980 Committed: https://crrev.com/dc1615b4060ed2a879ee74e0a7c8c4a811b22ec6 Cr-Commit-Position: refs/heads/master@{#299203}

Patch Set 1 #

Patch Set 2 : Reverted unneeded changes #

Patch Set 3 : Fix build error #

Total comments: 3

Patch Set 4 : Code review comments from tapted and duplicate remote IME creation #

Patch Set 5 : Added a comment #

Total comments: 4

Patch Set 6 : Code review comments #

Patch Set 7 : Fix presubmit errors #

Unified diffs Side-by-side diffs Delta from patch set Stats (+27 lines, -13 lines) Patch
M chrome/browser/ui/views/app_list/win/app_list_service_win.cc View 1 2 3 4 5 6 2 chunks +8 lines, -2 lines 0 comments Download
M ui/base/ime/remote_input_method_win.cc View 1 2 3 4 4 chunks +9 lines, -1 line 0 comments Download
M win8/metro_driver/ime/text_service.cc View 1 8 chunks +9 lines, -9 lines 0 comments Download
M win8/metro_driver/metro_driver_win7.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 29 (10 generated)
ananta
cpu: Please review everything. tapted:- For applist changes.
6 years, 2 months ago (2014-10-09 18:52:24 UTC) #3
cpu_(ooo_6.6-7.5)
lgtm
6 years, 2 months ago (2014-10-09 19:34:56 UTC) #4
tapted
Was the app launcher change specific to the IME stuff? (i.e. it might be better ...
6 years, 2 months ago (2014-10-09 22:33:09 UTC) #5
ananta
https://codereview.chromium.org/645683002/diff/80001/chrome/browser/ui/views/app_list/win/app_list_service_win.cc File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (right): https://codereview.chromium.org/645683002/diff/80001/chrome/browser/ui/views/app_list/win/app_list_service_win.cc#newcode54 chrome/browser/ui/views/app_list/win/app_list_service_win.cc:54: (CommandLine::ForCurrentProcess()->HasSwitch(switches::kViewerConnect))) On 2014/10/09 22:33:09, tapted wrote: > This doesn't ...
6 years, 2 months ago (2014-10-09 22:55:02 UTC) #6
tapted
On 2014/10/09 22:55:02, ananta wrote: > https://codereview.chromium.org/645683002/diff/80001/chrome/browser/ui/views/app_list/win/app_list_service_win.cc > File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (right): > > https://codereview.chromium.org/645683002/diff/80001/chrome/browser/ui/views/app_list/win/app_list_service_win.cc#newcode54 > ...
6 years, 2 months ago (2014-10-09 23:17:58 UTC) #7
ananta
On 2014/10/09 23:17:58, tapted wrote: > On 2014/10/09 22:55:02, ananta wrote: > > > https://codereview.chromium.org/645683002/diff/80001/chrome/browser/ui/views/app_list/win/app_list_service_win.cc ...
6 years, 2 months ago (2014-10-10 00:55:32 UTC) #8
tapted
lgtm. Don't forget to update the CL description too. Thanks! https://codereview.chromium.org/645683002/diff/290001/chrome/browser/ui/views/app_list/win/app_list_service_win.cc File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (right): https://codereview.chromium.org/645683002/diff/290001/chrome/browser/ui/views/app_list/win/app_list_service_win.cc#newcode53 ...
6 years, 2 months ago (2014-10-10 01:23:46 UTC) #9
ananta
https://codereview.chromium.org/645683002/diff/290001/chrome/browser/ui/views/app_list/win/app_list_service_win.cc File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (right): https://codereview.chromium.org/645683002/diff/290001/chrome/browser/ui/views/app_list/win/app_list_service_win.cc#newcode53 chrome/browser/ui/views/app_list/win/app_list_service_win.cc:53: if (desktop_type == chrome::HOST_DESKTOP_TYPE_ASH) On 2014/10/10 01:23:46, tapted wrote: ...
6 years, 2 months ago (2014-10-10 01:28:53 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/645683002/370001
6 years, 2 months ago (2014-10-10 01:39:41 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/16824)
6 years, 2 months ago (2014-10-10 01:53:11 UTC) #14
ananta
+yukawa for ui/base/ime
6 years, 2 months ago (2014-10-10 02:04:43 UTC) #16
yukawa
lgtm lgtm for ui/base/ime
6 years, 2 months ago (2014-10-10 02:10:05 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/645683002/370001
6 years, 2 months ago (2014-10-10 02:11:22 UTC) #19
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/16834)
6 years, 2 months ago (2014-10-10 02:22:10 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/645683002/370001
6 years, 2 months ago (2014-10-10 02:56:21 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/16841)
6 years, 2 months ago (2014-10-10 03:04:35 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/645683002/710001
6 years, 2 months ago (2014-10-10 18:03:27 UTC) #27
commit-bot: I haz the power
Committed patchset #7 (id:710001)
6 years, 2 months ago (2014-10-10 22:24:58 UTC) #28
commit-bot: I haz the power
6 years, 2 months ago (2014-10-10 22:25:43 UTC) #29
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/dc1615b4060ed2a879ee74e0a7c8c4a811b22ec6
Cr-Commit-Position: refs/heads/master@{#299203}

Powered by Google App Engine
This is Rietveld 408576698