Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21157006:
change tsf_input_scopes::SetInputScopeForTsfUnawareWindow to SetInputScopesForTsfUnawareWindow (Closed)
|
Created:
7 years, 4 months ago by yoichio Modified:
7 years, 4 months ago CC:
chromium-reviews, nona+watch_chromium.org, James Su, penghuang+watch_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionchange tsf_input_scopes::SetInputScopeForTsfUnawareWindow to SetInputScopesForTsfUnawareWindow
function to set InputScopes representing both TextInputType and TextInputMode.
implementation:
change GetSetInputScope to GetSetInputScopes function to get SetInputScopes procedure address from msctf.dll.
BUG=244688
TEST=Manually done on Windows 8 with on-screen keyboard
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215880
Patch Set 1 #Patch Set 2 : update #
Total comments: 13
Patch Set 3 : modify #
Total comments: 2
Patch Set 4 : rename functions to avoid overloading #
Total comments: 2
Patch Set 5 : rename args #
Total comments: 8
Patch Set 6 : Thread safe #
Total comments: 4
Patch Set 7 : Remove singleton and add thread checking #
Total comments: 4
Patch Set 8 : modify comments #Patch Set 9 : Rebase #
Messages
Total messages: 26 (0 generated)
On 2013/07/31 04:20:11, yoichio wrote: This cl is just adding a new function. Please review.
https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... File ui/base/ime/win/tsf_input_scope.cc (right): https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... ui/base/ime/win/tsf_input_scope.cc:91: Proc GetTSFProcedure(LPCSTR procedure_name) { We prefer to use |const wchar_t *| instead of |procedure_name| unless the function is a synonym of Win32 API. https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... ui/base/ime/win/tsf_input_scope.cc:104: &module)) { nit: indent. https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... ui/base/ime/win/tsf_input_scope.cc:105: return NULL; nit: indent. https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... ui/base/ime/win/tsf_input_scope.cc:146: case TEXT_INPUT_MODE_VERBATIM: Have you tested if IS_ALPHANUMERIC_HALFWIDTH is suitable for TEXT_INPUT_MODE_VERBATIM? Is built-in MS-IME turned on with IS_ALPHANUMERIC_HALFWIDTH? https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... ui/base/ime/win/tsf_input_scope.cc:179: GetTSFProcedure<SetInputScopeFunc> ("SetInputScope"); GetTSFProcedure<SetInputScopeFunc>("SetInputScope"); https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... ui/base/ime/win/tsf_input_scope.cc:192: GetInputScopeType(text_input_type), We should consider if |GetInputScopeType(text_input_type)| is the same to |GetInputScopeType(text_input_mode)|.
https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... File ui/base/ime/win/tsf_input_scope.cc (right): https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... ui/base/ime/win/tsf_input_scope.cc:91: Proc GetTSFProcedure(LPCSTR procedure_name) { On 2013/07/31 05:47:35, Yohei Yukawa wrote: > We prefer to use |const wchar_t *| instead of |procedure_name| > unless the function is a synonym of Win32 API. Done. https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... ui/base/ime/win/tsf_input_scope.cc:104: &module)) { On 2013/07/31 05:47:35, Yohei Yukawa wrote: > nit: indent. Done. https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... ui/base/ime/win/tsf_input_scope.cc:105: return NULL; On 2013/07/31 05:47:35, Yohei Yukawa wrote: > nit: indent. Done. https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... ui/base/ime/win/tsf_input_scope.cc:146: case TEXT_INPUT_MODE_VERBATIM: On 2013/07/31 05:47:35, Yohei Yukawa wrote: > Have you tested if IS_ALPHANUMERIC_HALFWIDTH is suitable for > TEXT_INPUT_MODE_VERBATIM? > Is built-in MS-IME turned on with IS_ALPHANUMERIC_HALFWIDTH? TEXT_INPUT_MODE_VERBATIM corresponds to inputmode="verbatim", which means "Alphanumeric Latin-script input of non-prose content, e.g. usernames, passwords, product codes" in draft. http://www.w3.org/html/wg/drafts/html/master/single-page.html#input-modalitie... https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... ui/base/ime/win/tsf_input_scope.cc:179: GetTSFProcedure<SetInputScopeFunc> ("SetInputScope"); On 2013/07/31 05:47:35, Yohei Yukawa wrote: > GetTSFProcedure<SetInputScopeFunc>("SetInputScope"); Done. https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... ui/base/ime/win/tsf_input_scope.cc:192: GetInputScopeType(text_input_type), On 2013/07/31 05:47:35, Yohei Yukawa wrote: > We should consider if |GetInputScopeType(text_input_type)| is the > same to |GetInputScopeType(text_input_mode)|. Done.
https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... File ui/base/ime/win/tsf_input_scope.cc (right): https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... ui/base/ime/win/tsf_input_scope.cc:146: case TEXT_INPUT_MODE_VERBATIM: So what happens when MS-IME finds IS_ALPHANUMERIC_HALFWIDTH? On 2013/07/31 06:13:54, yoichio wrote: > On 2013/07/31 05:47:35, Yohei Yukawa wrote: > > Have you tested if IS_ALPHANUMERIC_HALFWIDTH is suitable for > > TEXT_INPUT_MODE_VERBATIM? > > Is built-in MS-IME turned on with IS_ALPHANUMERIC_HALFWIDTH? > > TEXT_INPUT_MODE_VERBATIM corresponds to inputmode="verbatim", > which means "Alphanumeric Latin-script input of non-prose content, e.g. > usernames, passwords, product codes" in draft. > http://www.w3.org/html/wg/drafts/html/master/single-page.html#input-modalitie...
On 2013/07/31 06:23:19, Yohei Yukawa wrote: > https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... > File ui/base/ime/win/tsf_input_scope.cc (right): > > https://chromiumcodereview.appspot.com/21157006/diff/2001/ui/base/ime/win/tsf... > ui/base/ime/win/tsf_input_scope.cc:146: case TEXT_INPUT_MODE_VERBATIM: > So what happens when MS-IME finds IS_ALPHANUMERIC_HALFWIDTH? > MS-IME changes its mode to "半角英数".
https://chromiumcodereview.appspot.com/21157006/diff/11001/ui/base/ime/win/ts... File ui/base/ime/win/tsf_input_scope.cc (right): https://chromiumcodereview.appspot.com/21157006/diff/11001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:144: InputScope GetInputScopeType(TextInputMode text_input_mode) { We prefer to avoid function overloading.
https://chromiumcodereview.appspot.com/21157006/diff/11001/ui/base/ime/win/ts... File ui/base/ime/win/tsf_input_scope.cc (right): https://chromiumcodereview.appspot.com/21157006/diff/11001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:144: InputScope GetInputScopeType(TextInputMode text_input_mode) { On 2013/07/31 06:44:15, Yohei Yukawa wrote: > We prefer to avoid function overloading. Done.
https://chromiumcodereview.appspot.com/21157006/diff/18001/ui/base/ime/win/ts... File ui/base/ime/win/tsf_input_scope.cc (right): https://chromiumcodereview.appspot.com/21157006/diff/18001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:137: const InputScope *pInputScopes, Please keep consistency about naming with SetInputScopeFunc.
https://chromiumcodereview.appspot.com/21157006/diff/18001/ui/base/ime/win/ts... File ui/base/ime/win/tsf_input_scope.cc (right): https://chromiumcodereview.appspot.com/21157006/diff/18001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:137: const InputScope *pInputScopes, On 2013/07/31 06:54:15, Yohei Yukawa wrote: > Please keep consistency about naming with SetInputScopeFunc. Done.
lgtm
+nona@ as OWNER.
https://chromiumcodereview.appspot.com/21157006/diff/23001/ui/base/ime/win/ts... File ui/base/ime/win/tsf_input_scope.cc (right): https://chromiumcodereview.appspot.com/21157006/diff/23001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:94: // Thread safety is not required. Sorry this is not introduced by this CL but please let me know again why thread safety is not required. I guess, only UI thread is allowed to call this function, right? If so, how about adding thread check explicitly? https://chromiumcodereview.appspot.com/21157006/diff/23001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:113: typedef HRESULT (WINAPI *SetInputScopeFunc)(HWND window_handle, Can we use SetInputScopesFunc for both case? https://chromiumcodereview.appspot.com/21157006/diff/23001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:191: if (set_input_scopes) { nit: I prefer early exit, but up to you. https://chromiumcodereview.appspot.com/21157006/diff/23001/ui/base/ime/win/ts... File ui/base/ime/win/tsf_input_scope.h (right): https://chromiumcodereview.appspot.com/21157006/diff/23001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.h:40: UI_EXPORT void SetInputScopesForTsfUnawareWindow( Why don't you add argument into SetInputScopeForTsfUnawareWindow?
+yukawa. I changed large part of this CL so appreciate if you could re-review. Thanks https://chromiumcodereview.appspot.com/21157006/diff/23001/ui/base/ime/win/ts... File ui/base/ime/win/tsf_input_scope.cc (right): https://chromiumcodereview.appspot.com/21157006/diff/23001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:94: // Thread safety is not required. On 2013/07/31 13:23:09, Seigo Nonaka wrote: > Sorry this is not introduced by this CL but please let me know again why thread > safety is not required. > I guess, only UI thread is allowed to call this function, right? > If so, how about adding thread check explicitly? Done. https://chromiumcodereview.appspot.com/21157006/diff/23001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:113: typedef HRESULT (WINAPI *SetInputScopeFunc)(HWND window_handle, On 2013/07/31 13:23:09, Seigo Nonaka wrote: > Can we use SetInputScopesFunc for both case? I think yes and manually tested. https://chromiumcodereview.appspot.com/21157006/diff/23001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:191: if (set_input_scopes) { On 2013/07/31 13:23:09, Seigo Nonaka wrote: > nit: I prefer early exit, but up to you. Done.
https://chromiumcodereview.appspot.com/21157006/diff/23001/ui/base/ime/win/ts... File ui/base/ime/win/tsf_input_scope.cc (right): https://chromiumcodereview.appspot.com/21157006/diff/23001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:94: // Thread safety is not required. Sorry for confusing you. I'd not like to say making this function as thread safe. If this function should be called from only UI thread, I think good to adding following thread boundary check explicitly. DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI); On 2013/08/01 10:29:22, yoichio wrote: > On 2013/07/31 13:23:09, Seigo Nonaka wrote: > > Sorry this is not introduced by this CL but please let me know again why > thread > > safety is not required. > > I guess, only UI thread is allowed to call this function, right? > > If so, how about adding thread check explicitly? > > Done. https://chromiumcodereview.appspot.com/21157006/diff/30001/ui/base/ime/win/ts... File ui/base/ime/win/tsf_input_scope.cc (right): https://chromiumcodereview.appspot.com/21157006/diff/30001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:99: class TSFProceduresSingleton { I think thread check is enough but if you want to make GetSetInputScope function as thread safe, seems singleton is overkill. How about using LazyInstance for SetInputScopeFunc. https://chromiumcodereview.appspot.com/21157006/diff/30001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:112: nit: no need this blank line.
After all, CL looks simplified. https://chromiumcodereview.appspot.com/21157006/diff/30001/ui/base/ime/win/ts... File ui/base/ime/win/tsf_input_scope.cc (right): https://chromiumcodereview.appspot.com/21157006/diff/30001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:99: class TSFProceduresSingleton { On 2013/08/01 17:07:59, Seigo Nonaka wrote: > I think thread check is enough but if you want to make GetSetInputScope function > as thread safe, seems singleton is overkill. > How about using LazyInstance for SetInputScopeFunc. Add only thread checking. https://chromiumcodereview.appspot.com/21157006/diff/30001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:112: On 2013/08/01 17:07:59, Seigo Nonaka wrote: > nit: no need this blank line. Done.
https://chromiumcodereview.appspot.com/21157006/diff/37001/ui/base/ime/win/ts... File ui/base/ime/win/tsf_input_scope.cc (right): https://chromiumcodereview.appspot.com/21157006/diff/37001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:104: // Thread safety is not required because this function is under ui thread. s/ui/UI/ https://chromiumcodereview.appspot.com/21157006/diff/37001/ui/base/ime/win/ts... File ui/base/ime/win/tsf_input_scope.h (right): https://chromiumcodereview.appspot.com/21157006/diff/37001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.h:28: // http://msdn.microsoft.com/en-us/library/windows/desktop/ I prefer http://msdn.microsoft.com/en-us/library/windows/desktop/ms629026.aspx rather than http://msdn.microsoft.com/en-us/library/windows/desktop/ms629026(v=vs.85).aspx
https://chromiumcodereview.appspot.com/21157006/diff/37001/ui/base/ime/win/ts... File ui/base/ime/win/tsf_input_scope.cc (right): https://chromiumcodereview.appspot.com/21157006/diff/37001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.cc:104: // Thread safety is not required because this function is under ui thread. On 2013/08/05 08:25:50, Yohei Yukawa wrote: > s/ui/UI/ Done. https://chromiumcodereview.appspot.com/21157006/diff/37001/ui/base/ime/win/ts... File ui/base/ime/win/tsf_input_scope.h (right): https://chromiumcodereview.appspot.com/21157006/diff/37001/ui/base/ime/win/ts... ui/base/ime/win/tsf_input_scope.h:28: // http://msdn.microsoft.com/en-us/library/windows/desktop/ On 2013/08/05 08:25:50, Yohei Yukawa wrote: > I prefer http://msdn.microsoft.com/en-us/library/windows/desktop/ms629026.aspx > rather than > http://msdn.microsoft.com/en-us/library/windows/desktop/ms629026%28v=vs.85%29... Done.
lgtm
lgtm
+jochen@ as OWNER.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/21157006/45001
Failed to apply patch for ui/base/ime/win/tsf_input_scope.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file ui/base/ime/win/tsf_input_scope.cc
Hunk #1 succeeded at 9 (offset 1 line).
Hunk #2 succeeded at 91 (offset 3 lines).
Hunk #3 succeeded at 117 (offset 3 lines).
Hunk #4 FAILED at 140.
1 out of 4 hunks FAILED -- saving rejects to file
ui/base/ime/win/tsf_input_scope.cc.rej
Patch: ui/base/ime/win/tsf_input_scope.cc
Index: ui/base/ime/win/tsf_input_scope.cc
diff --git a/ui/base/ime/win/tsf_input_scope.cc
b/ui/base/ime/win/tsf_input_scope.cc
index
dac701b0a8b85e41694515b1a90ec0affc81653f..8666dd4244c693a280976a325f1824a1fe5ff70e
100644
--- a/ui/base/ime/win/tsf_input_scope.cc
+++ b/ui/base/ime/win/tsf_input_scope.cc
@@ -8,6 +8,7 @@
#include "base/compiler_specific.h"
#include "base/logging.h"
+#include "base/message_loop/message_loop.h"
#include "base/win/windows_version.h"
namespace ui {
@@ -87,14 +88,20 @@ class TSFInputScope : public ITfInputScope {
DISALLOW_COPY_AND_ASSIGN(TSFInputScope);
};
-typedef HRESULT (WINAPI *SetInputScopeFunc)(HWND window_handle,
- InputScope input_scope);
+typedef HRESULT (WINAPI *SetInputScopesFunc)(HWND window_handle,
+ const InputScope*
input_scope_list,
+ UINT num_input_scopes,
+ WCHAR**, /* unused */
+ UINT, /* unused */
+ WCHAR*, /* unused */
+ WCHAR* /* unused */);
-SetInputScopeFunc g_set_input_scope = NULL;
+SetInputScopesFunc g_set_input_scopes = NULL;
bool g_get_proc_done = false;
-SetInputScopeFunc GetSetInputScope() {
- // Thread safety is not required.
+SetInputScopesFunc GetSetInputScopes() {
+ DCHECK_EQ(base::MessageLoop::TYPE_UI, base::MessageLoop::current()->type());
+ // Thread safety is not required because this function is under UI thread.
if (!g_get_proc_done) {
g_get_proc_done = true;
@@ -107,13 +114,13 @@ SetInputScopeFunc GetSetInputScope() {
&module)) {
return NULL;
}
- g_set_input_scope = reinterpret_cast<SetInputScopeFunc>(
- GetProcAddress(module, "SetInputScope"));
+ g_set_input_scopes = reinterpret_cast<SetInputScopesFunc>(
+ GetProcAddress(module, "SetInputScopes"));
}
- return g_set_input_scope;
+ return g_set_input_scopes;
}
-InputScope GetInputScopeType(TextInputType text_input_type) {
+InputScope ConvertTextInputTypeToInputScope(TextInputType text_input_type) {
// Following mapping is based in IE10 on Windows 8.
switch (text_input_type) {
case TEXT_INPUT_TYPE_PASSWORD:
@@ -133,17 +140,54 @@ InputScope GetInputScopeType(TextInputType
text_input_type) {
}
}
+InputScope ConvertTextInputModeToInputScope(TextInputMode text_input_mode) {
+ switch (text_input_mode) {
+ case TEXT_INPUT_MODE_VERBATIM:
+ case TEXT_INPUT_MODE_LATIN:
+ case TEXT_INPUT_MODE_LATIN_NAME:
+ case TEXT_INPUT_MODE_LATIN_PROSE:
+ return IS_ALPHANUMERIC_HALFWIDTH;
+ case TEXT_INPUT_MODE_FULL_WIDTH_LATIN:
+ return IS_ALPHANUMERIC_FULLWIDTH;
+ case TEXT_INPUT_MODE_KANA:
+ return IS_HIRAGANA;
+ case TEXT_INPUT_MODE_KATAKANA:
+ return IS_KATAKANA_FULLWIDTH;
+ case TEXT_INPUT_MODE_NUMERIC:
+ return IS_NUMBER;
+ case TEXT_INPUT_MODE_TEL:
+ return IS_TELEPHONE_FULLTELEPHONENUMBER;
+ case TEXT_INPUT_MODE_EMAIL:
+ return IS_EMAIL_SMTPEMAILADDRESS;
+ case TEXT_INPUT_MODE_URL:
+ return IS_URL;
+ default:
+ return IS_DEFAULT;
+ }
+}
+
} // namespace
ITfInputScope* CreateInputScope(TextInputType text_input_type) {
- return new TSFInputScope(GetInputScopeType(text_input_type));
+ return new TSFInputScope(ConvertTextInputTypeToInputScope(text_input_type));
}
-void SetInputScopeForTsfUnawareWindow(HWND window_handle,
- TextInputType text_input_type) {
- SetInputScopeFunc set_input_scope = GetSetInputScope();
- if (set_input_scope)
- set_input_scope(window_handle, GetInputScopeType(text_input_type));
+void SetInputScopeForTsfUnawareWindow(
+ HWND window_handle,
+ TextInputType text_input_type,
+ TextInputMode text_input_mode) {
+ SetInputScopesFunc set_input_scopes = GetSetInputScopes();
+ if (!set_input_scopes)
+ return;
+
+ InputScope input_scopes[] = {
+ ConvertTextInputTypeToInputScope(text_input_type),
+ ConvertTextInputModeToInputScope(text_input_mode),
+ };
+
+ set_input_scopes(window_handle, input_scopes,
+ (input_scopes[0] == input_scopes[1] ? 1 : 2), NULL, 0, NULL,
+ NULL);
}
} // namespace tsf_inputscope
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/21157006/67001
Message was sent while issue was closed.
Change committed as 215880 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
