Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1554983002:
Build chrome.input.ime.* API in GN mode. (Closed)
|
Created:
4 years, 11 months ago by Azure Wei Modified:
4 years, 11 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBuild chrome.input.ime.* API in GN mode.
This is to add input_ime_api_nonchromeos.cc to browser/extensions/BUILD.gn and list input_ime.json in schemas.gni for building in GN mode.
As Chrome desktop needs not to provide system level IME switching UI, the InputImeEventRouter and InputImeAPI related functions are different on Chrome OS and non Chrome OS. Thus, class extensions::InputImeEventRouterBase is added to reuse the common codes, and implementation of InputImeAPI functions are divided into input_ime_api_chromos.cc/input_ime_api_nonchromeos.cc.
BUG=573291, 517773
TEST=Verified bug not repro on local build.
Committed: https://crrev.com/aa01dab35c1efc257cc41235fd91267e0b16bb98
Cr-Commit-Position: refs/heads/master@{#368788}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rename chrome_browser_extensions_input_ime_nonchromeos_sources as chrome_browser_extensions_input_i… #Patch Set 3 : #Patch Set 4 : Remove class InputImeEventRouter from input_ime_api.h to input_ime_api_chromeos/nonchromeos.h #
Total comments: 24
Patch Set 5 : Add class extensions:InputImeEventRouterBase #Patch Set 6 : #
Total comments: 10
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Total comments: 28
Patch Set 9 : Address Devlin's comments #Patch Set 10 : Rebase on cl:1570723002 #
Messages
Total messages: 41 (15 generated)
Description was changed from ========== Build chrome.input.ime.* API in GN mode. BUG=573291 TEST=Verified bug not repro on local build. ========== to ========== Build chrome.input.ime.* API in GN mode. BUG=573291 TEST=Verified bug not repro on local build. ==========
azurewei@chromium.org changed reviewers: + rdevlin.cronin@chromium.org, shuchen@chromium.org
Hi Shu Chen, Devlin, can you please review this cl? Thanks!
https://codereview.chromium.org/1554983002/diff/1/chrome/chrome_browser_exten... File chrome/chrome_browser_extensions.gypi (right): https://codereview.chromium.org/1554983002/diff/1/chrome/chrome_browser_exten... chrome/chrome_browser_extensions.gypi:939: 'chrome_browser_extensions_input_ime_nonchromeos_sources': [ "nonchromeos" is not accurate as these files are (for now) just for windows & linux.
https://codereview.chromium.org/1554983002/diff/1/chrome/chrome_browser_exten... File chrome/chrome_browser_extensions.gypi (right): https://codereview.chromium.org/1554983002/diff/1/chrome/chrome_browser_exten... chrome/chrome_browser_extensions.gypi:939: 'chrome_browser_extensions_input_ime_nonchromeos_sources': [ On 2016/01/04 07:00:06, Shu Chen (OOO till Jan 6) wrote: > "nonchromeos" is not accurate as these files are (for now) just for windows & > linux. Renamed this as 'chrome_browser_extensions_input_ime_linux_win_sources'.
rdevlin.cronin@chromium.org changed reviewers: + rockot@chromium.org
I don't speak gn :/ punt to rockot@.
lgtm
lgtm
The CQ bit was checked by azurewei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1554983002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1554983002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, shuchen@chromium.org Link to the patchset: https://codereview.chromium.org/1554983002/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1554983002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1554983002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by azurewei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1554983002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1554983002/60001
The CQ bit was unchecked by azurewei@chromium.org
Hi Ken Rockot, Devlin and Shu Chen, can you please review this cl again? Thank you! More files was changed as I tried to remove the class extensions::InputComponents, which is only used on Chrome OS, from input_ime_api.cc. Class InputImeEventRouter is now defined in input_ime_api_chromeos.h and input_ime_api_nonchromeos.h respectively. Actually this cl is part of: https://codereview.chromium.org/1528483002/, in which the functions in InputImeEventRouter and InputImeAPI will be implemented.
https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right): https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api.cc:189: bool InputImeKeyEventHandledFunction::RunAsync() { nit: add blank line above. also, please move this block below. https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h (right): https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h:164: std::string AddRequest( I think AddRequest() and the request map logics can be shared between chromeos and non-chromeos. Can you please try to abstract it to a base class?
Just for future reference, I think this should probably have been two separate CLs. The GN changes could have been made separately from the code changes. Still LGTM
https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api.h (right): https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api.h:92: class InputImeKeyEventHandledFunction : public AsyncExtensionFunction { Why was this moved? https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h (right): https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h:164: std::string AddRequest( On 2016/01/05 15:10:48, Shu Chen wrote: > I think AddRequest() and the request map logics can be shared between chromeos > and non-chromeos. > > Can you please try to abstract it to a base class? +1. The goal is to have these platform-specific files be as small as possible. https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. Not 2015 https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:10: namespace extensions { \n https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:26: typedef std::map< using https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:26: typedef std::map< #include <map> https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:28: std::pair<std::string, #include <utility> https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:33: std::vector<std::string> extension_ids_; #include <string> https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:37: Profile* profile_; forward declare https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:38: base/macros
A new class extensions::InputImeEventRouterBase was added to abstract class InputImeEventRouter on different platforms. InputImeEventRouterBase was defined in new file input_ime_eventer_router_base.h rather than input_ime_api.h, because input_ime_api.h has to include input_ime_api_chromeos/nonchromeos.h first, and class InputImeEventRouter is needed by input_ime_api.cc. All functions in input_ime_api_chromeos.cc are just copy-paste. https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right): https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api.cc:189: bool InputImeKeyEventHandledFunction::RunAsync() { On 2016/01/05 15:10:47, Shu Chen wrote: > nit: add blank line above. > > also, please move this block below. Done. https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api.h (right): https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api.h:92: class InputImeKeyEventHandledFunction : public AsyncExtensionFunction { On 2016/01/05 18:39:06, Devlin wrote: > Why was this moved? Oh, I just thought it's more easy to read to move these InputImeXxxFunction classes before InputImeEventRouter. I've reverted this change. https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h (right): https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h:164: std::string AddRequest( On 2016/01/05 15:10:48, Shu Chen wrote: > I think AddRequest() and the request map logics can be shared between chromeos > and non-chromeos. > > Can you please try to abstract it to a base class? Done. Added class InputImeEventRouterBase. https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h:164: std::string AddRequest( On 2016/01/05 18:39:06, Devlin wrote: > On 2016/01/05 15:10:48, Shu Chen wrote: > > I think AddRequest() and the request map logics can be shared between chromeos > > and non-chromeos. > > > > Can you please try to abstract it to a base class? > > +1. The goal is to have these platform-specific files be as small as possible. Done. https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/05 18:39:07, Devlin wrote: > Not 2015 Done. https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:10: namespace extensions { On 2016/01/05 18:39:07, Devlin wrote: > \n Done. https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:26: typedef std::map< On 2016/01/05 18:39:07, Devlin wrote: > using Done. https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:26: typedef std::map< On 2016/01/05 18:39:07, Devlin wrote: > #include <map> Done. https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:28: std::pair<std::string, On 2016/01/05 18:39:07, Devlin wrote: > #include <utility> Done. https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:33: std::vector<std::string> extension_ids_; On 2016/01/05 18:39:07, Devlin wrote: > #include <string> Done. https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:37: Profile* profile_; On 2016/01/05 18:39:06, Devlin wrote: > forward declare Done. https://codereview.chromium.org/1554983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:38: On 2016/01/05 18:39:07, Devlin wrote: > base/macros Done.
https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h (right): https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h:157: ~InputImeEventRouter(); ~InputImeEventRouter() override; https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:22: ~InputImeEventRouter(); ~InputImeEventRouter() override; https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:26: std::vector<std::string> extension_ids_; This is not used. https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h (right): https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h:28: ~InputImeEventRouterBase(); virtual https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h:45: unsigned int next_request_id_; these should be private.
https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h (right): https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h:157: ~InputImeEventRouter(); On 2016/01/06 07:00:07, Shu Chen wrote: > ~InputImeEventRouter() override; Done. https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:22: ~InputImeEventRouter(); On 2016/01/06 07:00:08, Shu Chen wrote: > ~InputImeEventRouter() override; Done. https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:26: std::vector<std::string> extension_ids_; On 2016/01/06 07:00:07, Shu Chen wrote: > This is not used. Deleted. https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h (right): https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h:28: ~InputImeEventRouterBase(); On 2016/01/06 07:00:08, Shu Chen wrote: > virtual Done. https://codereview.chromium.org/1554983002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h:45: unsigned int next_request_id_; On 2016/01/06 07:00:08, Shu Chen wrote: > these should be private. Done.
lgtm https://codereview.chromium.org/1554983002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h (right): https://codereview.chromium.org/1554983002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h:48: protected: nit: the order should be public/protected/private.
https://codereview.chromium.org/1554983002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h (right): https://codereview.chromium.org/1554983002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h:48: protected: On 2016/01/06 08:02:22, Shu Chen wrote: > nit: the order should be public/protected/private. Done.
Description was changed from ========== Build chrome.input.ime.* API in GN mode. BUG=573291 TEST=Verified bug not repro on local build. ========== to ========== Build chrome.input.ime.* API in GN mode. This is to add input_ime_api_nonchromeos.cc to browser/extensions/BUILD.gn and list input_ime.json in schemas.gni for building in GN mode. As Chrome desktop needs not to provide system level IME switching UI, the InputImeEventRouter and InputImeAPI related functions are different on Chrome OS and non Chrome OS. Thus, class extensions::InputImeEventRouterBase is added to reuse the common codes, and implementation of InputImeAPI functions are divided into input_ime_api_chromos.cc/input_ime_api_nonchromeos.cc. BUG=573291,517773 TEST=Verified bug not repro on local build. ==========
https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc (right): https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc:722: extensions::InputComponents::GetInputComponents(extension); no need for extensions:: prefix. https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc:735: if (input_components->size() > 0) { cleaner as: if (input_components && !input_components->empty()) { // Unregister } https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc:749: if (engine) When is |engine| null? https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h (right): https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h:11: #include "chrome/browser/profiles/profile.h" ditto from line 13. https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h:13: #include "ui/base/ime/ime_engine_handler_interface.h" Do we need to include this, or do forward declaration work? https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h:161: const std::vector<extensions::InputComponentInfo>& input_components); #include <vector> https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h:171: std::map<std::string, ui::IMEEngineHandlerInterface*> engine_map_; #include <string> https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:8: #include <string> Not needed (ditto for vector) https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_event_router_base.cc (right): https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.cc:25: std::string component_id = request->second.first; why make the copy? In fact, is this even needed at all? https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.cc:26: (request->second.second).Run(handled); parens not needed. https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h (right): https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h:13: #include "chrome/browser/profiles/profile.h" forward declare https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h:14: #include "extensions/browser/extension_function.h" needed? https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h:39: protected: indentation. Also, probably prefer a profile() accessor and making the member private. https://codereview.chromium.org/1554983002/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/schemas.gni (right): https://codereview.chromium.org/1554983002/diff/140001/chrome/common/extensio... chrome/common/extensions/api/schemas.gni:20: sources += [ "input_ime.json" ] Shouldn't this be handled by the gypi?
https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc (right): https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc:722: extensions::InputComponents::GetInputComponents(extension); On 2016/01/07 23:08:29, Devlin wrote: > no need for extensions:: prefix. Done. https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc:735: if (input_components->size() > 0) { On 2016/01/07 23:08:29, Devlin wrote: > cleaner as: > if (input_components && !input_components->empty()) { > // Unregister > } Done. https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc:749: if (engine) On 2016/01/07 23:08:29, Devlin wrote: > When is |engine| null? The engine could be null if the current extension doesn't have an active engine. https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h (right): https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h:11: #include "chrome/browser/profiles/profile.h" On 2016/01/07 23:08:29, Devlin wrote: > ditto from line 13. Done. https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h:13: #include "ui/base/ime/ime_engine_handler_interface.h" On 2016/01/07 23:08:29, Devlin wrote: > Do we need to include this, or do forward declaration work? No need. I've removed these. https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h:161: const std::vector<extensions::InputComponentInfo>& input_components); On 2016/01/07 23:08:29, Devlin wrote: > #include <vector> Done. https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.h:171: std::map<std::string, ui::IMEEngineHandlerInterface*> engine_map_; On 2016/01/07 23:08:29, Devlin wrote: > #include <string> Done. https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h (right): https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.h:8: #include <string> On 2016/01/07 23:08:29, Devlin wrote: > Not needed (ditto for vector) Done. https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_event_router_base.cc (right): https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.cc:25: std::string component_id = request->second.first; On 2016/01/07 23:08:29, Devlin wrote: > why make the copy? In fact, is this even needed at all? Yeah, component_id was not used here. Deleted. https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.cc:26: (request->second.second).Run(handled); On 2016/01/07 23:08:29, Devlin wrote: > parens not needed. Done. https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h (right): https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h:13: #include "chrome/browser/profiles/profile.h" On 2016/01/07 23:08:29, Devlin wrote: > forward declare Done. https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h:14: #include "extensions/browser/extension_function.h" On 2016/01/07 23:08:29, Devlin wrote: > needed? Deleted. https://codereview.chromium.org/1554983002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/input_ime/input_ime_event_router_base.h:39: protected: On 2016/01/07 23:08:29, Devlin wrote: > indentation. Also, probably prefer a profile() accessor and making the member > private. Done. https://codereview.chromium.org/1554983002/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/schemas.gni (right): https://codereview.chromium.org/1554983002/diff/140001/chrome/common/extensio... chrome/common/extensions/api/schemas.gni:20: sources += [ "input_ime.json" ] On 2016/01/07 23:08:29, Devlin wrote: > Shouldn't this be handled by the gypi? Done. Updated it with gypi_values.input_ime_schema_file.
On 2016/01/05 16:51:08, Ken Rockot wrote: > Just for future reference, I think this should probably have been two separate > CLs. The GN changes could have been made separately from the code changes. > > Still LGTM Thanks for your kind comments. I've separated the InputImeEvent related code changes to a new CL: https://codereview.chromium.org/1570723002/. This CL will be rebased on that CL before submit.
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, shuchen@chromium.org Link to the patchset: https://codereview.chromium.org/1554983002/#ps180001 (title: "Rebase on cl:1570723002")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1554983002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1554983002/180001
Message was sent while issue was closed.
Description was changed from ========== Build chrome.input.ime.* API in GN mode. This is to add input_ime_api_nonchromeos.cc to browser/extensions/BUILD.gn and list input_ime.json in schemas.gni for building in GN mode. As Chrome desktop needs not to provide system level IME switching UI, the InputImeEventRouter and InputImeAPI related functions are different on Chrome OS and non Chrome OS. Thus, class extensions::InputImeEventRouterBase is added to reuse the common codes, and implementation of InputImeAPI functions are divided into input_ime_api_chromos.cc/input_ime_api_nonchromeos.cc. BUG=573291,517773 TEST=Verified bug not repro on local build. ========== to ========== Build chrome.input.ime.* API in GN mode. This is to add input_ime_api_nonchromeos.cc to browser/extensions/BUILD.gn and list input_ime.json in schemas.gni for building in GN mode. As Chrome desktop needs not to provide system level IME switching UI, the InputImeEventRouter and InputImeAPI related functions are different on Chrome OS and non Chrome OS. Thus, class extensions::InputImeEventRouterBase is added to reuse the common codes, and implementation of InputImeAPI functions are divided into input_ime_api_chromos.cc/input_ime_api_nonchromeos.cc. BUG=573291,517773 TEST=Verified bug not repro on local build. ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Build chrome.input.ime.* API in GN mode. This is to add input_ime_api_nonchromeos.cc to browser/extensions/BUILD.gn and list input_ime.json in schemas.gni for building in GN mode. As Chrome desktop needs not to provide system level IME switching UI, the InputImeEventRouter and InputImeAPI related functions are different on Chrome OS and non Chrome OS. Thus, class extensions::InputImeEventRouterBase is added to reuse the common codes, and implementation of InputImeAPI functions are divided into input_ime_api_chromos.cc/input_ime_api_nonchromeos.cc. BUG=573291,517773 TEST=Verified bug not repro on local build. ========== to ========== Build chrome.input.ime.* API in GN mode. This is to add input_ime_api_nonchromeos.cc to browser/extensions/BUILD.gn and list input_ime.json in schemas.gni for building in GN mode. As Chrome desktop needs not to provide system level IME switching UI, the InputImeEventRouter and InputImeAPI related functions are different on Chrome OS and non Chrome OS. Thus, class extensions::InputImeEventRouterBase is added to reuse the common codes, and implementation of InputImeAPI functions are divided into input_ime_api_chromos.cc/input_ime_api_nonchromeos.cc. BUG=573291,517773 TEST=Verified bug not repro on local build. Committed: https://crrev.com/aa01dab35c1efc257cc41235fd91267e0b16bb98 Cr-Commit-Position: refs/heads/master@{#368788} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/aa01dab35c1efc257cc41235fd91267e0b16bb98 Cr-Commit-Position: refs/heads/master@{#368788} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
