|
|
Created:
9 years, 8 months ago by Yusuke Sato Modified:
9 years, 7 months ago Reviewers:
Erik does not do reviews, commit-bot: I haz the power, bryeung, Mihai Parparita -not on Chrome, Zachary Kuznia, Peng CC:
chromium-reviews, Aaron Boodman, pam+watch_chromium.org, mazda Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd 2 Extension APIs for handwriting: experimental.input.sendHandritingStroke and cancelHandWriting
This CL depends on http://codereview.chromium.org/6902067/.
BUG=chromium-os:14421
TEST=ran emerge
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85593
Patch Set 1 #
Total comments: 14
Patch Set 2 : review fix #Patch Set 3 : added TODOs #Patch Set 4 : fix windows build #
Total comments: 4
Patch Set 5 : review fix #
Total comments: 16
Patch Set 6 : review fix #
Messages
Total messages: 20 (0 generated)
lgtm http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_input_api.cc (right): http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_input_api.cc:52: const DictionaryValue* dict, const char* key, double* result) { nit: This should fit on the previous line
http://codereview.chromium.org/6905053/diff/1/chrome/common/extensions/api/ex... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6905053/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:2422: { I am thinking it is better to have a namespace chrome.input.focusedcontext or chrome.ime.focusedcontext. and then we may put some functions for interacting with current focused context in it. We could put those two function in it, maybe sendKeykeyboardEvent too.
Higher-level questions: what is the use-case for this? how will extensions be getting handwriting strokes in the first place? http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_function_dispatcher.cc (right): http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_function_dispatcher.cc:280: RegisterFunction<SendHandwritingStrokesFunction>(); Shouldn't this be in the "ChromeOS-specific part of the API." section later in the file? http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_input_api.cc (right): http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_input_api.cc:13: #include "chrome/browser/extensions/extension_tabs_module.h" Why do you need this include? http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_input_api.cc:51: bool GetDoubleValue( This is at least the third implementation of such a function (see ReadNumberByKey in extension_tts_api_util.cc and GetNumberValue/GetNumberFromDictionary in json_schema_validator.cc). I think adding this as a convenience GetNumber method to DictionaryValue is a good idea. http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_input_api.cc:149: chromeos::CrosLibrary::Get()->GetInputMethodLibrary()-> Other extensions that use CrosLibrary check if it's loaded (see http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/exten...). Do you need similar checks here?
Thanks for the review, Mihai and all. > Higher-level questions: what is the use-case for this? how will extensions be getting handwriting strokes in the first place? I'll implement a UI as a Chrome extension similar to the virtual keyboard extension for Chrome OS at http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/keyb..., which allows users to write a character using a finger or mouse. The extension will be bundled into Chrome OS. http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_function_dispatcher.cc (right): http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_function_dispatcher.cc:280: RegisterFunction<SendHandwritingStrokesFunction>(); On 2011/04/27 23:34:51, Mihai Parparita wrote: > Shouldn't this be in the "ChromeOS-specific part of the API." section later in > the file? Done. http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_input_api.cc (right): http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_input_api.cc:13: #include "chrome/browser/extensions/extension_tabs_module.h" Removed, thanks. On 2011/04/27 23:34:51, Mihai Parparita wrote: > Why do you need this include? http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_input_api.cc:51: bool GetDoubleValue( On 2011/04/27 23:34:51, Mihai Parparita wrote: > This is at least the third implementation of such a function (see > ReadNumberByKey in extension_tts_api_util.cc and > GetNumberValue/GetNumberFromDictionary in json_schema_validator.cc). I think > adding this as a convenience GetNumber method to DictionaryValue is a good idea. I did it in a separate CL. Please review http://codereview.chromium.org/6893089/ and http://codereview.chromium.org/6901084/ first. http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_input_api.cc:52: const DictionaryValue* dict, const char* key, double* result) { I'll remove the function. On 2011/04/27 10:25:09, Zachary Kuznia wrote: > nit: This should fit on the previous line http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_input_api.cc:149: chromeos::CrosLibrary::Get()->GetInputMethodLibrary()-> It's definitely better to have the check. Added. Thanks. On 2011/04/27 23:34:51, Mihai Parparita wrote: > Other extensions that use CrosLibrary check if it's loaded (see > http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/exten...). > Do you need similar checks here? http://codereview.chromium.org/6905053/diff/1/chrome/common/extensions/api/ex... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6905053/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:2422: { Are we planning to have an API which allows extension developers to send a key event to a specific input context? If not, I prefer chrome.input. I guess the sub namespace "focused context" looks too complicated for most VK extension developers. How about just rewriting the description to something like "Send a handwriting event to the focused input context in Chrome"? On 2011/04/27 15:13:44, Peng wrote: > I am thinking it is better to have a namespace chrome.input.focusedcontext or > chrome.ime.focusedcontext. and then we may put some functions for interacting > with current focused context in it. > We could put those two function in it, maybe sendKeykeyboardEvent too.
(try bots are red since dependent CLs are not submitted yet...) On 2011/04/28 10:57:07, Yusuke Sato wrote: > Thanks for the review, Mihai and all. > > > Higher-level questions: what is the use-case for this? how will extensions be > getting handwriting strokes in the first place? > > I'll implement a UI as a Chrome extension similar to the virtual keyboard > extension for Chrome OS at > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/keyb..., > which allows users to write a character using a finger or mouse. The extension > will be bundled into Chrome OS. > > http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... > File chrome/browser/extensions/extension_function_dispatcher.cc (right): > > http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... > chrome/browser/extensions/extension_function_dispatcher.cc:280: > RegisterFunction<SendHandwritingStrokesFunction>(); > On 2011/04/27 23:34:51, Mihai Parparita wrote: > > Shouldn't this be in the "ChromeOS-specific part of the API." section later in > > the file? > > Done. > > http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... > File chrome/browser/extensions/extension_input_api.cc (right): > > http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... > chrome/browser/extensions/extension_input_api.cc:13: #include > "chrome/browser/extensions/extension_tabs_module.h" > Removed, thanks. > > On 2011/04/27 23:34:51, Mihai Parparita wrote: > > Why do you need this include? > > http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... > chrome/browser/extensions/extension_input_api.cc:51: bool GetDoubleValue( > On 2011/04/27 23:34:51, Mihai Parparita wrote: > > This is at least the third implementation of such a function (see > > ReadNumberByKey in extension_tts_api_util.cc and > > GetNumberValue/GetNumberFromDictionary in json_schema_validator.cc). I think > > adding this as a convenience GetNumber method to DictionaryValue is a good > idea. > > I did it in a separate CL. Please review http://codereview.chromium.org/6893089/ > and http://codereview.chromium.org/6901084/ first. > > http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... > chrome/browser/extensions/extension_input_api.cc:52: const DictionaryValue* > dict, const char* key, double* result) { > I'll remove the function. > > On 2011/04/27 10:25:09, Zachary Kuznia wrote: > > nit: This should fit on the previous line > > http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... > chrome/browser/extensions/extension_input_api.cc:149: > chromeos::CrosLibrary::Get()->GetInputMethodLibrary()-> > It's definitely better to have the check. Added. Thanks. > > On 2011/04/27 23:34:51, Mihai Parparita wrote: > > Other extensions that use CrosLibrary check if it's loaded (see > > > http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/exten...). > > Do you need similar checks here? > > http://codereview.chromium.org/6905053/diff/1/chrome/common/extensions/api/ex... > File chrome/common/extensions/api/extension_api.json (right): > > http://codereview.chromium.org/6905053/diff/1/chrome/common/extensions/api/ex... > chrome/common/extensions/api/extension_api.json:2422: { > Are we planning to have an API which allows extension developers to send a key > event to a specific input context? If not, I prefer chrome.input. I guess the > sub namespace "focused context" looks too complicated for most VK extension > developers. > > How about just rewriting the description to something like "Send a handwriting > event to the focused input context in Chrome"? > > On 2011/04/27 15:13:44, Peng wrote: > > I am thinking it is better to have a namespace chrome.input.focusedcontext or > > chrome.ime.focusedcontext. and then we may put some functions for interacting > > with current focused context in it. > > We could put those two function in it, maybe sendKeykeyboardEvent too.
http://codereview.chromium.org/6905053/diff/1/chrome/common/extensions/api/ex... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6905053/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:2422: { On 2011/04/28 10:57:07, Yusuke Sato wrote: > Are we planning to have an API which allows extension developers to send a key > event to a specific input context? If not, I prefer chrome.input. I guess the > sub namespace "focused context" looks too complicated for most VK extension > developers. > > How about just rewriting the description to something like "Send a handwriting > event to the focused input context in Chrome"? > > On 2011/04/27 15:13:44, Peng wrote: > > I am thinking it is better to have a namespace chrome.input.focusedcontext or > > chrome.ime.focusedcontext. and then we may put some functions for interacting > > with current focused context in it. > > We could put those two function in it, maybe sendKeykeyboardEvent too. We don't have a plan to allow the VK interact with any input contexts. The VK is different with physical keyboard. It can only interact with the focused context (focused input control view). I think probably we will add more APIs, like get context input type (email, address, number and etc), sending key events, or inserting text, receiving some notifications from the context, and etc. I think have a namespace focusedcontext could make those APIs very clear. VK developers know they are playing with the focused context. If we put all APIs in chrome.input, it will hide the concept of context from developers, and the APIs meaning may be a little fuzzy? Or we have to put the detail in API document. Peng
Talked with Zach offline. > If we put all APIs in chrome.input, it will hide the concept of context from developers, and the APIs > meaning may be a little fuzzy? Then, how about adding a new function parameter, context_id, to the handwriting and sendKey functions and keep the functions in chrome.input? Although it's not possible for a VK extension to get an input context id right now, it'll become possible when Zach's IME extension APIs are implemented. I've added TODOs to extension_input_api.cc.
http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... File chrome/browser/extensions/extension_input_api.cc (right): http://codereview.chromium.org/6905053/diff/1/chrome/browser/extensions/exten... chrome/browser/extensions/extension_input_api.cc:51: bool GetDoubleValue( On 2011/04/27 23:34:51, Mihai Parparita wrote: > This is at least the third implementation of such a function (see > ReadNumberByKey in extension_tts_api_util.cc and > GetNumberValue/GetNumberFromDictionary in json_schema_validator.cc). I think > adding this as a convenience GetNumber method to DictionaryValue is a good idea. Done.
http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_function_dispatcher.cc (right): http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_function_dispatcher.cc:327: #if defined(TOOLKIT_VIEWS) && defined(TOUCH_UI) Is TOOLKIT_VIEWS necessary? http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_input_api.cc (right): http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_input_api.cc:43: const char kCrosLibraryNotLoadedError[] = "Cros shared library not loaded."; Maybe it should be in #if defined(xxx) too?
On 2011/05/02 06:52:30, Yusuke Sato wrote: > Talked with Zach offline. > > > If we put all APIs in chrome.input, it will hide the concept of context from > developers, and the APIs > > meaning may be a little fuzzy? > > Then, how about adding a new function parameter, context_id, to the handwriting > and sendKey functions and keep the functions in chrome.input? Although it's not > possible for a VK extension to get an input context id right now, it'll become > possible when Zach's IME extension APIs are implemented. > > I've added TODOs to extension_input_api.cc. I see. If necessary, we could discuss and change it later.
On Mon, May 2, 2011 at 9:59 AM, <penghuang@chromium.org> wrote: > > http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... > File chrome/browser/extensions/extension_function_dispatcher.cc (right): > > http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... > chrome/browser/extensions/extension_function_dispatcher.cc:327: #if > defined(TOOLKIT_VIEWS) && defined(TOUCH_UI) > Is TOOLKIT_VIEWS necessary? TOUCH_UI implies TOOLKIT_VIEWS, so you don't need both. Bryan
Thanks Bryan, Peng. Please take another look. http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_function_dispatcher.cc (right): http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_function_dispatcher.cc:327: #if defined(TOOLKIT_VIEWS) && defined(TOUCH_UI) On 2011/05/02 13:59:51, Peng wrote: > Is TOOLKIT_VIEWS necessary? Done. http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_input_api.cc (right): http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_input_api.cc:43: const char kCrosLibraryNotLoadedError[] = "Cros shared library not loaded."; On 2011/05/02 13:59:51, Peng wrote: > Maybe it should be in #if defined(xxx) too? Done.
LGTM On 2011/05/04 14:28:35, Yusuke Sato wrote: > Thanks Bryan, Peng. Please take another look. > > http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... > File chrome/browser/extensions/extension_function_dispatcher.cc (right): > > http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... > chrome/browser/extensions/extension_function_dispatcher.cc:327: #if > defined(TOOLKIT_VIEWS) && defined(TOUCH_UI) > On 2011/05/02 13:59:51, Peng wrote: > > Is TOOLKIT_VIEWS necessary? > > Done. > > http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... > File chrome/browser/extensions/extension_input_api.cc (right): > > http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... > chrome/browser/extensions/extension_input_api.cc:43: const char > kCrosLibraryNotLoadedError[] = "Cros shared library not loaded."; > On 2011/05/02 13:59:51, Peng wrote: > > Maybe it should be in #if defined(xxx) too? > > Done.
Hi Mihai, can I submit the change? On 2011/05/04 14:32:22, Peng wrote: > LGTM > > On 2011/05/04 14:28:35, Yusuke Sato wrote: > > Thanks Bryan, Peng. Please take another look. > > > > > http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... > > File chrome/browser/extensions/extension_function_dispatcher.cc (right): > > > > > http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... > > chrome/browser/extensions/extension_function_dispatcher.cc:327: #if > > defined(TOOLKIT_VIEWS) && defined(TOUCH_UI) > > On 2011/05/02 13:59:51, Peng wrote: > > > Is TOOLKIT_VIEWS necessary? > > > > Done. > > > > > http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... > > File chrome/browser/extensions/extension_input_api.cc (right): > > > > > http://codereview.chromium.org/6905053/diff/4006/chrome/browser/extensions/ex... > > chrome/browser/extensions/extension_input_api.cc:43: const char > > kCrosLibraryNotLoadedError[] = "Cros shared library not loaded."; > > On 2011/05/02 13:59:51, Peng wrote: > > > Maybe it should be in #if defined(xxx) too? > > > > Done.
Generally LGTM with the nits below. Note that I'm not in OWNERS for extensions (yet). http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_input_api.cc (right): http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_input_api.cc:144: chromeos::CrosLibrary* cros_library = chromeos::CrosLibrary::Get(); Shouldn't this be at the start of the function? http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_input_api.cc:159: chromeos::CrosLibrary* cros_library = chromeos::CrosLibrary::Get(); Ditto. http://codereview.chromium.org/6905053/diff/8001/chrome/common/extensions/api... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6905053/diff/8001/chrome/common/extensions/api... chrome/common/extensions/api/extension_api.json:2441: "name": "cancelHandwriting", Shouldn't this be called cancelHandwritingStrokes, to be consistent with the function that sends? http://codereview.chromium.org/6905053/diff/8001/chrome/common/extensions/api... chrome/common/extensions/api/extension_api.json:2446: "name": "n_strokes", The name of this should be strokeCount. You should also have a description indicating what omitting the parameter does.
Hi everyone. I've indicated to others in the past that I'd like to avoid #defined APIs. If they must exist, then they should be minimized. You don't need to have a working implementation unless it makes sense, but the functions should still exist as stubs and just returns errors when used on unsupported platforms. In practice, these functions won't actually be callable on these platforms, because we can restrict whether or not they're installable if necessary. While this development is in experimental, I'm not going to hold up your checkins, but I would like to see more effort to lay the groundwork to making this more general. For example, your implementation depends on libcros. OK, then that means you need to make a *_chromeos.cc file with that part of the implementation. My take is that it's easier to do this kind of work now, while you're developing the feature, than it is for you to come back months later to remember what you did and clean things up. Also, I don't want to be in a position where you're under time pressures to ship and you need to ask us to let it through without cleaning it up at all. Feel free to set up some time to chat with me in person about this if you have any questions. For the time being, with Mihai's and my comments addressed, LGTM. Please start to follow up soon with some cleanup to remove the #defines. http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_function_dispatcher.cc (right): http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_function_dispatcher.cc:327: #if defined(TOUCH_UI) why does this need to be in OS_CHROMEOS? isn't TOUCH_UI enough of a gate? http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_input_api.cc (right): http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_input_api.cc:164: cros_library->GetInputMethodLibrary()->CancelHandwriting(n_strokes); is passing a negative number (or a large number) bad here? If so, you should guard that with EXTENSION_FUNCTION_VALIDATE. http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_input_api.h (right): http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_input_api.h:36: #if defined(OS_CHROMEOS) && defined(TOUCH_UI) again, isn't TOUCH_UI enough? http://codereview.chromium.org/6905053/diff/8001/chrome/common/extensions/api... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6905053/diff/8001/chrome/common/extensions/api... chrome/common/extensions/api/extension_api.json:2448: "type": "integer" also, should this be a positive only int?
(sorry for the delay, I was ooo last week.) Fixed. Please take another look. > For the time being, with Mihai's and my comments addressed, LGTM. Please start > to follow up soon with some cleanup to remove the #defines. Sure. I've added a TODO to extension_input_api.cc. I'll file a bug on crosbug.com as well once this CL is submitted. http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_function_dispatcher.cc (right): http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_function_dispatcher.cc:327: #if defined(TOUCH_UI) If I understand correctly, OS_CHROMEOS and TOUCH_UI are orthogonal. For example, it seems to be possible to build TOUCH_UI version of Chromium Linux. On 2011/05/06 00:51:03, Erik Kay wrote: > why does this need to be in OS_CHROMEOS? isn't TOUCH_UI enough of a gate? http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_input_api.cc (right): http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_input_api.cc:144: chromeos::CrosLibrary* cros_library = chromeos::CrosLibrary::Get(); On 2011/05/05 22:47:49, Mihai Parparita wrote: > Shouldn't this be at the start of the function? Done. http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_input_api.cc:159: chromeos::CrosLibrary* cros_library = chromeos::CrosLibrary::Get(); On 2011/05/05 22:47:49, Mihai Parparita wrote: > Ditto. Done. http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_input_api.cc:164: cros_library->GetInputMethodLibrary()->CancelHandwriting(n_strokes); On 2011/05/06 00:51:03, Erik Kay wrote: > is passing a negative number (or a large number) bad here? If so, you should > guard that with EXTENSION_FUNCTION_VALIDATE. Done. http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_input_api.h (right): http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_input_api.h:36: #if defined(OS_CHROMEOS) && defined(TOUCH_UI) ditto. On 2011/05/06 00:51:03, Erik Kay wrote: > again, isn't TOUCH_UI enough? http://codereview.chromium.org/6905053/diff/8001/chrome/common/extensions/api... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/6905053/diff/8001/chrome/common/extensions/api... chrome/common/extensions/api/extension_api.json:2441: "name": "cancelHandwriting", On 2011/05/05 22:47:49, Mihai Parparita wrote: > Shouldn't this be called cancelHandwritingStrokes, to be consistent with the > function that sends? Done. http://codereview.chromium.org/6905053/diff/8001/chrome/common/extensions/api... chrome/common/extensions/api/extension_api.json:2446: "name": "n_strokes", On 2011/05/05 22:47:49, Mihai Parparita wrote: > The name of this should be strokeCount. You should also have a description > indicating what omitting the parameter does. Done. http://codereview.chromium.org/6905053/diff/8001/chrome/common/extensions/api... chrome/common/extensions/api/extension_api.json:2448: "type": "integer" Added "minimum": 0. On 2011/05/06 00:51:03, Erik Kay wrote: > also, should this be a positive only int?
LGTM. On 2011/05/16 15:47:38, Yusuke Sato wrote: > (sorry for the delay, I was ooo last week.) > Fixed. Please take another look. > > > For the time being, with Mihai's and my comments addressed, LGTM. Please > start > > to follow up soon with some cleanup to remove the #defines. > > Sure. I've added a TODO to extension_input_api.cc. I'll file a bug on > http://crosbug.com as well once this CL is submitted. > > http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... > File chrome/browser/extensions/extension_function_dispatcher.cc (right): > > http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... > chrome/browser/extensions/extension_function_dispatcher.cc:327: #if > defined(TOUCH_UI) > If I understand correctly, OS_CHROMEOS and TOUCH_UI are orthogonal. For example, > it seems to be possible to build TOUCH_UI version of Chromium Linux. > > On 2011/05/06 00:51:03, Erik Kay wrote: > > why does this need to be in OS_CHROMEOS? isn't TOUCH_UI enough of a gate? > > http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... > File chrome/browser/extensions/extension_input_api.cc (right): > > http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... > chrome/browser/extensions/extension_input_api.cc:144: chromeos::CrosLibrary* > cros_library = chromeos::CrosLibrary::Get(); > On 2011/05/05 22:47:49, Mihai Parparita wrote: > > Shouldn't this be at the start of the function? > > Done. > > http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... > chrome/browser/extensions/extension_input_api.cc:159: chromeos::CrosLibrary* > cros_library = chromeos::CrosLibrary::Get(); > On 2011/05/05 22:47:49, Mihai Parparita wrote: > > Ditto. > > Done. > > http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... > chrome/browser/extensions/extension_input_api.cc:164: > cros_library->GetInputMethodLibrary()->CancelHandwriting(n_strokes); > On 2011/05/06 00:51:03, Erik Kay wrote: > > is passing a negative number (or a large number) bad here? If so, you should > > guard that with EXTENSION_FUNCTION_VALIDATE. > > Done. > > http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... > File chrome/browser/extensions/extension_input_api.h (right): > > http://codereview.chromium.org/6905053/diff/8001/chrome/browser/extensions/ex... > chrome/browser/extensions/extension_input_api.h:36: #if defined(OS_CHROMEOS) && > defined(TOUCH_UI) > ditto. > > On 2011/05/06 00:51:03, Erik Kay wrote: > > again, isn't TOUCH_UI enough? > > http://codereview.chromium.org/6905053/diff/8001/chrome/common/extensions/api... > File chrome/common/extensions/api/extension_api.json (right): > > http://codereview.chromium.org/6905053/diff/8001/chrome/common/extensions/api... > chrome/common/extensions/api/extension_api.json:2441: "name": > "cancelHandwriting", > On 2011/05/05 22:47:49, Mihai Parparita wrote: > > Shouldn't this be called cancelHandwritingStrokes, to be consistent with the > > function that sends? > > Done. > > http://codereview.chromium.org/6905053/diff/8001/chrome/common/extensions/api... > chrome/common/extensions/api/extension_api.json:2446: "name": "n_strokes", > On 2011/05/05 22:47:49, Mihai Parparita wrote: > > The name of this should be strokeCount. You should also have a description > > indicating what omitting the parameter does. > > Done. > > http://codereview.chromium.org/6905053/diff/8001/chrome/common/extensions/api... > chrome/common/extensions/api/extension_api.json:2448: "type": "integer" > Added "minimum": 0. > > On 2011/05/06 00:51:03, Erik Kay wrote: > > also, should this be a positive only int?
Presubmit check for 6905053-15001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** This change modifies file(s) which the extension docs depend on. You must rebuild the extension docs. Build by running the build.py script in chrome/common/extensions/docs/build/. Be sure to include any modified resulting static files (/common/extension/docs/*.html) in your final changelist. |