Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
305533002:
Adds IME switching private APIs. (Closed)
|
Created:
6 years, 7 months ago by Shu Chen Modified:
6 years, 6 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, asvitkine+watch_chromium.org, nona+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, jar (doing other things), davemoore+watch_chromium.org, nkostylev+watch_chromium.org, bshe, kevers Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdds IME switching private APIs.
BUG=372291
TEST=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276570
Patch Set 1 #Patch Set 2 : rebased. #Patch Set 3 : #Patch Set 4 : #
Total comments: 15
Patch Set 5 : #
Total comments: 5
Patch Set 6 : #Patch Set 7 : rebased. #Patch Set 8 : #
Messages
Total messages: 32 (0 generated)
Can you please review this cl? isherman@ for approval of histogram changes. kalman@ for private API changes. Thanks, Shu
you should try to get somebody more familiar with IME to review this as well. https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/input_method_api.cc (right): https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:34: base::Value::CreateStringValue(manager->GetCurrentInputMethod().id())); use new base::StringValue not CreateStringValue https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:40: #if !defined(OS_CHROMEOS) better here (and everywhere) is just EXTENSION_FUNCTION_VALIDATE(false). https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:54: SetResult(new base::FundamentalValue(true)); i.e. just return true here https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:59: return true; then here set _error (like "cannot find ime with id *" or whatever) and return false. https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:80: output->Append(val); what you have here is totally fine. but a fun class is extensions::DictionaryBuilder (in extensions/common/value_builder.h: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...) which would let you write just extensions::ListBuilder output; ... for (...) { output.Append(DictionaryBuilder() .Set("id", input_method.id()) .Set("name", input_method.name()) .Set("indicator", input_method.indicator())); } SetResult(output.Build()); https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:118: registry->RegisterFunction<StartImeFunction>(); you shouldn't actually need these RegisterFunction calls because the schema compiler will do it automatically. https://codereview.chromium.org/305533002/diff/50001/chrome/common/extensions... File chrome/common/extensions/api/input_method_private.json (right): https://codereview.chromium.org/305533002/diff/50001/chrome/common/extensions... chrome/common/extensions/api/input_method_private.json:76: "type": "boolean", more idiomatic would be a void return value, but set an error on failure (which would end up on chrome.runtime.lastError).
https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/input_method_api.cc (right): https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:118: registry->RegisterFunction<StartImeFunction>(); On 2014/06/06 16:36:04, kalman wrote: > you shouldn't actually need these RegisterFunction calls because the schema > compiler will do it automatically. never mind, looks like the ime private API doesn't go through the schema compiler.
histograms change lgtm
+nona@ for reviewing input method related stuff. Thanks, Shu https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/input_method_api.cc (right): https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:34: base::Value::CreateStringValue(manager->GetCurrentInputMethod().id())); On 2014/06/06 16:36:04, kalman wrote: > use new base::StringValue not CreateStringValue Done. https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:40: #if !defined(OS_CHROMEOS) On 2014/06/06 16:36:04, kalman wrote: > better here (and everywhere) is just EXTENSION_FUNCTION_VALIDATE(false). Done. https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:54: SetResult(new base::FundamentalValue(true)); On 2014/06/06 16:36:04, kalman wrote: > i.e. just return true here Done. https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:59: return true; On 2014/06/06 16:36:04, kalman wrote: > then here set _error (like "cannot find ime with id *" or whatever) and return > false. Done. https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:80: output->Append(val); On 2014/06/06 16:36:04, kalman wrote: > what you have here is totally fine. but a fun class is > extensions::DictionaryBuilder (in extensions/common/value_builder.h: > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...) > which would let you write just > > extensions::ListBuilder output; > ... > for (...) { > output.Append(DictionaryBuilder() > .Set("id", input_method.id()) > .Set("name", input_method.name()) > .Set("indicator", input_method.indicator())); > } > SetResult(output.Build()); Just found that value_builder.cc is only for test purpose for now. So I'd like to remain the current implementation. https://codereview.chromium.org/305533002/diff/50001/chrome/common/extensions... File chrome/common/extensions/api/input_method_private.json (right): https://codereview.chromium.org/305533002/diff/50001/chrome/common/extensions... chrome/common/extensions/api/input_method_private.json:76: "type": "boolean", On 2014/06/06 16:36:04, kalman wrote: > more idiomatic would be a void return value, but set an error on failure (which > would end up on chrome.runtime.lastError). Done.
cc'ed bshe@/kevers@ who will use these APIs in virtual keyboard.
lgtm https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/input_method_api.cc (right): https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:80: output->Append(val); On 2014/06/07 05:31:09, Shu Chen wrote: > On 2014/06/06 16:36:04, kalman wrote: > > what you have here is totally fine. but a fun class is > > extensions::DictionaryBuilder (in extensions/common/value_builder.h: > > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...) > > which would let you write just > > > > extensions::ListBuilder output; > > ... > > for (...) { > > output.Append(DictionaryBuilder() > > .Set("id", input_method.id()) > > .Set("name", input_method.name()) > > .Set("indicator", input_method.indicator())); > > } > > SetResult(output.Build()); > > Just found that value_builder.cc is only for test purpose for now. So I'd like > to remain the current implementation. bummer. ok, there's actually no reason that should be test-only. if you want you could move it from the test-only gyp to the common-gyp and use it, but I understand if you don't want to do that for this change.
On 2014/06/10 19:48:27, kalman wrote: > lgtm > > https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... > File chrome/browser/chromeos/extensions/input_method_api.cc (right): > > https://codereview.chromium.org/305533002/diff/50001/chrome/browser/chromeos/... > chrome/browser/chromeos/extensions/input_method_api.cc:80: output->Append(val); > On 2014/06/07 05:31:09, Shu Chen wrote: > > On 2014/06/06 16:36:04, kalman wrote: > > > what you have here is totally fine. but a fun class is > > > extensions::DictionaryBuilder (in extensions/common/value_builder.h: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...) > > > which would let you write just > > > > > > extensions::ListBuilder output; > > > ... > > > for (...) { > > > output.Append(DictionaryBuilder() > > > .Set("id", input_method.id()) > > > .Set("name", input_method.name()) > > > .Set("indicator", input_method.indicator())); > > > } > > > SetResult(output.Build()); > > > > Just found that value_builder.cc is only for test purpose for now. So I'd like > > to remain the current implementation. > > bummer. ok, there's actually no reason that should be test-only. if you want you > could move it from the test-only gyp to the common-gyp and use it, but I > understand if you don't want to do that for this change. Thanks. I suppose that should be done in a separated cl.
https://codereview.chromium.org/305533002/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/input_method_api.cc (right): https://codereview.chromium.org/305533002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:87: ExtensionFunction::ResponseAction StartImeFunction::Run() { I guess this function is no longer necessary. This API is used for http://crbug.com/242864 as the workaround. However you implement the OnListnerAdded in InputImeApi which fixes the issue properly so let's remove this unnecessary function. https://codereview.chromium.org/305533002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:89: EXTENSION_FUNCTION_VALIDATE(false); I think NOTREACHED is still fine since this is Private API which will not be called from third party, right? So reaching here means simply a bug of Chrome implementation.
https://codereview.chromium.org/305533002/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/input_method_api.cc (right): https://codereview.chromium.org/305533002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:87: ExtensionFunction::ResponseAction StartImeFunction::Run() { On 2014/06/11 03:18:04, Seigo Nonaka wrote: > I guess this function is no longer necessary. > This API is used for http://crbug.com/242864 as the workaround. However you > implement the OnListnerAdded in InputImeApi which fixes the issue properly so > let's remove this unnecessary function. Done. https://codereview.chromium.org/305533002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:89: EXTENSION_FUNCTION_VALIDATE(false); On 2014/06/11 03:18:04, Seigo Nonaka wrote: > I think NOTREACHED is still fine since this is Private API which will not be > called from third party, right? So reaching here means simply a bug of Chrome > implementation. The return value changed. EXTENSION_FUNCTION_VALIDATE(false) looks much cleaner than: NOTREACHED(); return RespondNow(Error("..."));
lgtm https://codereview.chromium.org/305533002/diff/70001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/input_method_api.cc (right): https://codereview.chromium.org/305533002/diff/70001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/input_method_api.cc:89: EXTENSION_FUNCTION_VALIDATE(false); Oh, I didn't know NOTREACHED does not always causes crash. Seems new implementation is fine. On 2014/06/11 03:31:15, Shu Chen wrote: > On 2014/06/11 03:18:04, Seigo Nonaka wrote: > > I think NOTREACHED is still fine since this is Private API which will not be > > called from third party, right? So reaching here means simply a bug of Chrome > > implementation. > > The return value changed. EXTENSION_FUNCTION_VALIDATE(false) looks much cleaner > than: > NOTREACHED(); > return RespondNow(Error("..."));
The CQ bit was checked by shuchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shuchen@chromium.org/305533002/90001
The CQ bit was unchecked by shuchen@chromium.org
The CQ bit was checked by shuchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shuchen@chromium.org/305533002/110001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by shuchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shuchen@chromium.org/305533002/110001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by shuchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shuchen@chromium.org/305533002/110001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by shuchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shuchen@chromium.org/305533002/130001
Message was sent while issue was closed.
Change committed as 276570 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
