Adding IBus support to cros library.

Adding IBus support to the cros library. The following 4 APIs will be added:

MonitorLanguageStatus()
DisconnectLanguageStatus()
GetLanguages()
ChangeLanguage()

Basic structure of the code is almost the same as chromeos_power.cc. As with other cros modules, you can test the 4 interfaces above by running monitor_language.cc test tool.

This change is for http://codereview.chromium.org/449050

BUG=494
TEST=Run monitor_language command and verify that the command can detect IME engine switches.

[Yusuke Sato, 2009-12-08 07:01:54]

Charlie:
Can you please check if the structure of this change is good enough as well?
Thanks.

[Randall Spangler, 2009-12-08 07:17:54]

lgtm

[satorux1, 2009-12-08 08:41:14]

The code looks good but it's inherently tricky due to complexities coming from
libcros (dlopen) and DBus. I would like to see more comments so we can
understand the code more easily.

http://codereview.chromium.org/460107/diff/3001/3003
File chromeos_language.cc (right):

http://codereview.chromium.org/460107/diff/3001/3003#newcode102
chromeos_language.cc:102: LOG(INFO) << "Established private D-Bus connection to:
'" << address << "'";
maybe VLOG?

http://codereview.chromium.org/460107/diff/3001/3003#newcode233
chromeos_language.cc:233: if (is_ime_enabled) {
just a matter of taste, but "if (ime_is_enabled)" might be a bit more readable.

http://codereview.chromium.org/460107/diff/3001/3003#newcode238
chromeos_language.cc:238: DCHECK(engine_desc);
maybe we need to handle the error here, like:

 if (!engine_desc) { return; }

http://codereview.chromium.org/460107/diff/3001/3003#newcode262
chromeos_language.cc:262: LanguageStatusMonitor monitor_;
I thought it's an object of some class, but realized it's just a function
pointer. It may be nice to name it like 
LanguageStatusMonitorFunction monitor_function_ ?

http://codereview.chromium.org/460107/diff/3001/3003#newcode263
chromeos_language.cc:263: void* language_library_;
Please add a comment about this. What do we do with this?

http://codereview.chromium.org/460107/diff/3001/3003#newcode281
chromeos_language.cc:281: LanguageStatusConnection
ChromeOSMonitorLanguageStatus(
I'm confused why it needs to be in the C linkage. Would be nice to add a comment
explaining why

// The function will be bound to chromeos::MonitorLanguageStatus with dlsym() in
load.cc so it needs to be in the C linkage, so the symbol name does not get
mangled.

http://codereview.chromium.org/460107/diff/3001/3004
File chromeos_language.h (right):

http://codereview.chromium.org/460107/diff/3001/3004#newcode55
chromeos_language.h:55: std::string id;
What is id? Please add a comment about it. I would like examples. :)

http://codereview.chromium.org/460107/diff/3001/3004#newcode56
chromeos_language.h:56: std::string display_name;
Please also add a comment with an example.

http://codereview.chromium.org/460107/diff/3001/3004#newcode61
chromeos_language.h:61: inline InputLanguageList*
CreateFallbackInputLanguageList() {
Would be nice to have a function comment, like "Creates ..."

http://codereview.chromium.org/460107/diff/3001/3004#newcode63
chromeos_language.h:63: // language_library.cc when EnsureLoaded() fails.
You can make it part of the function comment.

http://codereview.chromium.org/460107/diff/3001/3004#newcode74
chromeos_language.h:74: typedef OpaqueLanguageStatusConnection*
LanguageStatusConnection;
I'm confused. Why don't we just live with:

  class LanguageStatusConnection;

I don't see a benefit of having OpaqueLanguageStatusConnection.

http://codereview.chromium.org/460107/diff/3001/3007
File monitor_language.cc (right):

http://codereview.chromium.org/460107/diff/3001/3007#newcode30
monitor_language.cc:30: chromeos::LanguageStatusConnection connection = NULL;
maybe global_connection?

http://codereview.chromium.org/460107/diff/3001/3007#newcode52
monitor_language.cc:52: ChangeLanguage(connection,
chromeos::ChangeLanguage?

I'm confused why it can be called without chromeos:: namespace. This looks like
an example of ADL. Please add chromeos:: to be explicit.


If I understand right, ChromeOSChangeLanguage() in chromeos_language.cc is
called here, right? Would be nice to add a comment. The libcros tricks are hard
to follow. :)

http://codereview.chromium.org/460107/diff/3001/3007#newcode94
monitor_language.cc:94: GetLanguages(connection));
chromeos::GetLanguages() ?

[Yusuke Sato, 2009-12-09 06:25:56]

Thanks! Addressed all issues except VLOG (please check my comment).

http://codereview.chromium.org/460107/diff/3001/3003
File chromeos_language.cc (right):

http://codereview.chromium.org/460107/diff/3001/3003#newcode102
chromeos_language.cc:102: LOG(INFO) << "Established private D-Bus connection to:
'" << address << "'";
On Chromium OS tree, VLOG(n) seems to be defined as LOG(INFO)...

http://git.chromium.org/cgi-bin/gitweb.cgi?p=chromiumos.git;a=blob_plain;f=sr...

http://codereview.chromium.org/460107/diff/3001/3003#newcode233
chromeos_language.cc:233: if (is_ime_enabled) {
On 2009/12/08 08:41:14, satorux1 wrote:
> just a matter of taste, but "if (ime_is_enabled)" might be a bit more
readable.

Done.

http://codereview.chromium.org/460107/diff/3001/3003#newcode238
chromeos_language.cc:238: DCHECK(engine_desc);
On 2009/12/08 08:41:14, satorux1 wrote:
> maybe we need to handle the error here, like:
> 
>  if (!engine_desc) { return; }

Done.

http://codereview.chromium.org/460107/diff/3001/3003#newcode262
chromeos_language.cc:262: LanguageStatusMonitor monitor_;
On 2009/12/08 08:41:14, satorux1 wrote:
> I thought it's an object of some class, but realized it's just a function
> pointer. It may be nice to name it like 
> LanguageStatusMonitorFunction monitor_function_ ?
> 

Done.

http://codereview.chromium.org/460107/diff/3001/3003#newcode263
chromeos_language.cc:263: void* language_library_;
On 2009/12/08 08:41:14, satorux1 wrote:
> Please add a comment about this. What do we do with this?

Done.

http://codereview.chromium.org/460107/diff/3001/3003#newcode281
chromeos_language.cc:281: LanguageStatusConnection
ChromeOSMonitorLanguageStatus(
Thanks, added.

http://codereview.chromium.org/460107/diff/3001/3004
File chromeos_language.h (right):

http://codereview.chromium.org/460107/diff/3001/3004#newcode55
chromeos_language.h:55: std::string id;
On 2009/12/08 08:41:14, satorux1 wrote:
> What is id? Please add a comment about it. I would like examples. :)

Done.

http://codereview.chromium.org/460107/diff/3001/3004#newcode56
chromeos_language.h:56: std::string display_name;
On 2009/12/08 08:41:14, satorux1 wrote:
> Please also add a comment with an example.

Done.

http://codereview.chromium.org/460107/diff/3001/3004#newcode61
chromeos_language.h:61: inline InputLanguageList*
CreateFallbackInputLanguageList() {
On 2009/12/08 08:41:14, satorux1 wrote:
> Would be nice to have a function comment, like "Creates ..."

Done.

http://codereview.chromium.org/460107/diff/3001/3004#newcode63
chromeos_language.h:63: // language_library.cc when EnsureLoaded() fails.
On 2009/12/08 08:41:14, satorux1 wrote:
> You can make it part of the function comment.

Done.

http://codereview.chromium.org/460107/diff/3001/3004#newcode74
chromeos_language.h:74: typedef OpaqueLanguageStatusConnection*
LanguageStatusConnection;
On 2009/12/08 08:41:14, satorux1 wrote:
> I'm confused. Why don't we just live with:
> 
>   class LanguageStatusConnection;
> 
> I don't see a benefit of having OpaqueLanguageStatusConnection.

Done.

http://codereview.chromium.org/460107/diff/3001/3007
File monitor_language.cc (right):

http://codereview.chromium.org/460107/diff/3001/3007#newcode30
monitor_language.cc:30: chromeos::LanguageStatusConnection connection = NULL;
On 2009/12/08 08:41:14, satorux1 wrote:
> maybe global_connection?

Done.

http://codereview.chromium.org/460107/diff/3001/3007#newcode52
monitor_language.cc:52: ChangeLanguage(connection,
On 2009/12/08 08:41:14, satorux1 wrote:
> chromeos::ChangeLanguage?
> 
> I'm confused why it can be called without chromeos:: namespace. This looks
like
> an example of ADL. Please add chromeos:: to be explicit.

Done. I didn't notice that I'd been using ADL :(
 
> If I understand right, ChromeOSChangeLanguage() in chromeos_language.cc is
> called here, right? 

Yes.

> Would be nice to add a comment. The libcros tricks are hard
> to follow. :)

Sure, added 6-lines comment. thanks.

http://codereview.chromium.org/460107/diff/3001/3007#newcode94
monitor_language.cc:94: GetLanguages(connection));
On 2009/12/08 08:41:14, satorux1 wrote:
> chromeos::GetLanguages() ?

Done.

[satorux1, 2009-12-09 08:49:04]

LGTM

http://codereview.chromium.org/460107/diff/1004/1006
File chromeos_language.cc (right):

http://codereview.chromium.org/460107/diff/1004/1006#newcode266
chromeos_language.cc:266: // Points a chromeos::LanguageLibrary object.
|language_library_| is used as
Points to?

http://codereview.chromium.org/460107/diff/1004/1006#newcode267
chromeos_language.cc:267: // a first argument of the |monitor_function_|
function.
a first -> the first ?

http://codereview.chromium.org/460107/diff/1004/1010
File monitor_language.cc (right):

http://codereview.chromium.org/460107/diff/1004/1010#newcode56
monitor_language.cc:56: // here, "StateChanged" method in candidate_window will
be called by
I thought it first calls "SetEngine" method in ibubus-daemon. Would be nice to
describe what will happen a bit more in detail. This would be useful
documentation for us.

  1. Calls "SetEngine" method in ibus-daemon
  2. ...
  3. ...

[Yusuke Sato, 2009-12-09 11:05:28]

Thanks! Fixed all.

http://codereview.chromium.org/460107/diff/1004/1006
File chromeos_language.cc (right):

http://codereview.chromium.org/460107/diff/1004/1006#newcode266
chromeos_language.cc:266: // Points a chromeos::LanguageLibrary object.
|language_library_| is used as
On 2009/12/09 08:49:04, satorux1 wrote:
> Points to?

Done.

http://codereview.chromium.org/460107/diff/1004/1006#newcode267
chromeos_language.cc:267: // a first argument of the |monitor_function_|
function.
On 2009/12/09 08:49:04, satorux1 wrote:
> a first -> the first ?

Done.

http://codereview.chromium.org/460107/diff/1004/1010
File monitor_language.cc (right):

http://codereview.chromium.org/460107/diff/1004/1010#newcode56
monitor_language.cc:56: // here, "StateChanged" method in candidate_window will
be called by
On 2009/12/09 08:49:04, satorux1 wrote:
> I thought it first calls "SetEngine" method in ibubus-daemon. Would be nice to
> describe what will happen a bit more in detail. This would be useful
> documentation for us.
> 
>   1. Calls "SetEngine" method in ibus-daemon
>   2. ...
>   3. ...
> 

Done.

[satorux1, 2009-12-09 12:07:19]

LGTM.

Thank you for adding comments.

[Charlie Lee, 2009-12-09 23:26:11]

lgtm

http://codereview.chromium.org/460107/diff/2009/7002
File chromeos_language.cc (right):

http://codereview.chromium.org/460107/diff/2009/7002#newcode294
chromeos_language.cc:294: if (!connection->Init()) {
You should probably log a warning and figure out why this failed.

http://codereview.chromium.org/460107/diff/2009/7002#newcode309
chromeos_language.cc:309: if (!connection) {
We should log this when this happen. And maybe have it be a DCHECK

http://codereview.chromium.org/460107/diff/2009/7002#newcode320
chromeos_language.cc:320: if (!connection) {
same comment as above

[Yusuke Sato, 2009-12-10 04:40:40]

Thanks. Fixed all.

http://codereview.chromium.org/460107/diff/2009/7002
File chromeos_language.cc (right):

http://codereview.chromium.org/460107/diff/2009/7002#newcode294
chromeos_language.cc:294: if (!connection->Init()) {
On 2009/12/09 23:26:11, chocobo wrote:
> You should probably log a warning and figure out why this failed.

Done. Added LOG(WARNING) here.
The reason of failure is logged in Init().

http://codereview.chromium.org/460107/diff/2009/7002#newcode309
chromeos_language.cc:309: if (!connection) {
On 2009/12/09 23:26:11, chocobo wrote:
> We should log this when this happen. And maybe have it be a DCHECK

Done.

http://codereview.chromium.org/460107/diff/2009/7002#newcode320
chromeos_language.cc:320: if (!connection) {
On 2009/12/09 23:26:11, chocobo wrote:
> same comment as above

Done.

[Charlie Lee, 2009-12-11 00:48:29]

Dave, do you mind taking a quick look at this also. Thanks.

[DaveMoore, 2009-12-11 20:16:59]

LGTM