Support intra-IME switching. Cros part.

Support intra-IME switching (e.g. "Japanese Hiragana mode" to "Japanese HalfWidthKatakana mode").
    
chromeos_language.h:
- Added new cros API: ActivateImeProperty.
- Added 2 monitor functions: LanguageRegisterImePropertiesFunction, LanguageUpdateImePropertyFunction.
    
chromeos_language.cc:
- Added DBus signal handlers for "RegisterProperties" and "UpdateProperty."

[Yusuke Sato, 2010-01-08 05:53:04]

http://codereview.chromium.org/521058/diff/3005/2002
File chromeos_language.cc (right):

http://codereview.chromium.org/521058/diff/3005/2002#newcode557
chromeos_language.cc:557: void SwitchToXKB(const char* name) {
Moved from public: to private:. Implementation is not changed.

http://codereview.chromium.org/521058/diff/3005/2002#newcode572
chromeos_language.cc:572: void SwitchToIME(const char* name) {
Ditto.

[satorux1, 2010-01-08 09:27:10]

http://codereview.chromium.org/521058/diff/3005/2002
File chromeos_language.cc (right):

http://codereview.chromium.org/521058/diff/3005/2002#newcode52
chromeos_language.cc:52: // converts IBus representation of a property,
|ibus_prop|, to our own. This
Returns true on success?

http://codereview.chromium.org/521058/diff/3005/2002#newcode60
chromeos_language.cc:60: int* in_out_radio_id) {
Could you add a comment about the parameter?

http://codereview.chromium.org/521058/diff/3005/2002#newcode109
chromeos_language.cc:109: result = false;
Why not return false here? Would be good to add a comment about why.

http://codereview.chromium.org/521058/diff/3005/2002#newcode122
chromeos_language.cc:122: bool is_radio_checked;
Would be nice to initialize it by false .

http://codereview.chromium.org/521058/diff/3005/2002#newcode157
chromeos_language.cc:157: const char* TypeToStr(int t) {
ToStr -> ToString ?

http://codereview.chromium.org/521058/diff/3005/2002#newcode173
chromeos_language.cc:173: const char* StateToStr(int state) {
Ditto

http://codereview.chromium.org/521058/diff/3005/2002#newcode190
chromeos_language.cc:190: void PrintProp(IBusProperty *prop, int tree_level) {
Probably does not matter here, but it's usually more useful to have a function
that returns a debug string like std::string PropToString(), than a function
that prints a debug string.

BTW, cannot we use protocol buffers?

http://codereview.chromium.org/521058/diff/3005/2002#newcode369
chromeos_language.cc:369: input_context_path_.c_str(),
ibus_bus_get_connection(ibus_));
would be safer to check if the returned context is NULL.

http://codereview.chromium.org/521058/diff/3005/2002#newcode415
chromeos_language.cc:415: //  |- Item-4
Can we assume that the top level items are not radio items?

http://codereview.chromium.org/521058/diff/3005/2002#newcode429
chromeos_language.cc:429: for (int i = 0;; ++i) {
Hmm, it seems that the top-level items in the property list are specially
handled here. I saw a similar loop in DoFlattenProperty()...

What about doing the flattening without recursion? It may be nicer to use STL's
std::stack to implement logic like this. It's not the case here, but recursion
is dangerous to be run in a thread with small stack space.

http://codereview.chromium.org/521058/diff/3005/2002#newcode445
chromeos_language.cc:445: // for details.
Please add a comment about the return value.

http://codereview.chromium.org/521058/diff/3005/2002#newcode620
chromeos_language.cc:620:
monitor_functions_.register_ime_properties(language_library_, prop_list);
Do we have to notify if ibus_prop_list is NULL?

http://codereview.chromium.org/521058/diff/3005/2002#newcode633
chromeos_language.cc:633:
monitor_functions_.update_ime_property(language_library_, prop_list);
Ditto.

http://codereview.chromium.org/521058/diff/3005/2002#newcode697
chromeos_language.cc:697: retval = ibus_message_get_args(message, &error,
You could declare retval here.

const gboolean retval =

http://codereview.chromium.org/521058/diff/3005/2003
File chromeos_language.h (right):

http://codereview.chromium.org/521058/diff/3005/2003#newcode25
chromeos_language.h:25: // A structure which represents a current IME language
or a XKB layout.
Why current? I think the struct can be used to represent input languages that
are not currently active.

http://codereview.chromium.org/521058/diff/3005/2003#newcode78
chromeos_language.h:78: : key(key),
I think it's legitimate code but looks a bit tricky. Maybe easier to read if
parameters are named like in_key.

http://codereview.chromium.org/521058/diff/3005/2003#newcode87
chromeos_language.h:87: ImeProperty() : is_radio(), is_radio_checked(),
radio_id(kInvalidRadioId) {
I didn't know the notation like is_radio() but maybe easier to read if it's
explicitly initialized like is_radio(false)

http://codereview.chromium.org/521058/diff/3005/2003#newcode92
chromeos_language.h:92: std::string icon;  // Path to an icon. Can be empty.
icon_path to make it a bit more descriptive?

http://codereview.chromium.org/521058/diff/3005/2003#newcode95
chromeos_language.h:95: bool is_radio;  // true if the property is a radio
button.
"radio" sounds like UI implementation details. What about naming it a bit more
generic like "selectional_item" as the struct is more like a model?

http://codereview.chromium.org/521058/diff/3005/2003#newcode133
chromeos_language.h:133: // ---------------------------
We don't have members like |type| and |state| in ImeProperty. Would be good to
make the examples consistent with the structure of ImeProperty .

http://codereview.chromium.org/521058/diff/3005/2003#newcode148
chromeos_language.h:148: // might send the following properties:
RegisterProperties and UpdateProperty look very similar. Could you comment
differences between these?

http://codereview.chromium.org/521058/diff/3005/2003#newcode220
chromeos_language.h:220: const char* key, bool active);
Would it be possible to have a separate function for deactivation? Using a
boolean flag for deactivation would make the code a bit hard to read. Besides,
having Deactive version is consistent with Activate/DeactivateLanguage().

http://codereview.chromium.org/521058/diff/3005/2005
File monitor_language.cc (right):

http://codereview.chromium.org/521058/diff/3005/2005#newcode37
monitor_language.cc:37: void DumpProperties(const chromeos::ImePropertyList&
prop_list) {
You might want to have ToString() in ImePropertyList in chromeos_language.h, so
you can use it in the client (chrome) world.

[satorux1, 2010-01-08 09:34:48]

http://codereview.chromium.org/521058/diff/3005/2003
File chromeos_language.h (right):

http://codereview.chromium.org/521058/diff/3005/2003#newcode95
chromeos_language.h:95: bool is_radio;  // true if the property is a radio
button.
On 2010/01/08 09:27:10, satorux1 wrote:
> "radio" sounds like UI implementation details. What about naming it a bit more
> generic like "selectional_item" as the struct is more like a model?
> 
> 

I meant "selection item"

[Yusuke Sato, 2010-01-10 16:55:06]

Addressed all issues. Thanks!

http://codereview.chromium.org/521058/diff/3005/2002
File chromeos_language.cc (right):

http://codereview.chromium.org/521058/diff/3005/2002#newcode52
chromeos_language.cc:52: // converts IBus representation of a property,
|ibus_prop|, to our own. This
On 2010/01/08 09:27:10, satorux1 wrote:
> Returns true on success?

Done.

http://codereview.chromium.org/521058/diff/3005/2002#newcode60
chromeos_language.cc:60: int* in_out_radio_id) {
On 2010/01/08 09:27:10, satorux1 wrote:
> Could you add a comment about the parameter?

Removed the param.

http://codereview.chromium.org/521058/diff/3005/2002#newcode109
chromeos_language.cc:109: result = false;
(The code is removed)

http://codereview.chromium.org/521058/diff/3005/2002#newcode122
chromeos_language.cc:122: bool is_radio_checked;
On 2010/01/08 09:27:10, satorux1 wrote:
> Would be nice to initialize it by false .

Done.

http://codereview.chromium.org/521058/diff/3005/2002#newcode157
chromeos_language.cc:157: const char* TypeToStr(int t) {
On 2010/01/08 09:27:10, satorux1 wrote:
> ToStr -> ToString ?

Done.

http://codereview.chromium.org/521058/diff/3005/2002#newcode173
chromeos_language.cc:173: const char* StateToStr(int state) {
On 2010/01/08 09:27:10, satorux1 wrote:
> Ditto

Done.

http://codereview.chromium.org/521058/diff/3005/2002#newcode190
chromeos_language.cc:190: void PrintProp(IBusProperty *prop, int tree_level) {
On 2010/01/08 09:27:10, satorux1 wrote:
> Probably does not matter here, but it's usually more useful to have a function
> that returns a debug string like std::string PropToString(), than a function
> that prints a debug string.
> 

Done.

> BTW, cannot we use protocol buffers?

AFAIK, Chromium OS does not use protobuf, but I agree that it would be nice if
we could use it. Added a TODO comment to chromeos_language.h for now. Thanks.

http://codereview.chromium.org/521058/diff/3005/2002#newcode369
chromeos_language.cc:369: input_context_path_.c_str(),
ibus_bus_get_connection(ibus_));
On 2010/01/08 09:27:10, satorux1 wrote:
> would be safer to check if the returned context is NULL.

Done.

http://codereview.chromium.org/521058/diff/3005/2002#newcode401
chromeos_language.cc:401: // THIS FUNCTION IS PUBLIC FOR UNIT TESTING. DO NOT
CALL THIS DIRECTLY.
I noticed that it's still not easy to test the function even if we make this
public: (since all functions in this file are compiled as a dynamically loaded
DSO, as you know...)

Moved FlattenXXX functions to the anonymous namespace for now.

http://codereview.chromium.org/521058/diff/3005/2002#newcode415
chromeos_language.cc:415: //  |- Item-4
On 2010/01/08 09:27:10, satorux1 wrote:
> Can we assume that the top level items are not radio items?

Revised the example.

http://codereview.chromium.org/521058/diff/3005/2002#newcode429
chromeos_language.cc:429: for (int i = 0;; ++i) {
ok, I agree that iterative traversing is better. Fixed.

http://codereview.chromium.org/521058/diff/3005/2002#newcode445
chromeos_language.cc:445: // for details.
On 2010/01/08 09:27:10, satorux1 wrote:
> Please add a comment about the return value.

Done.

http://codereview.chromium.org/521058/diff/3005/2002#newcode620
chromeos_language.cc:620:
monitor_functions_.register_ime_properties(language_library_, prop_list);
On 2010/01/08 09:27:10, satorux1 wrote:
> Do we have to notify if ibus_prop_list is NULL?

Yes, we have to do it in order to clear current properties.

http://codereview.chromium.org/521058/diff/3005/2002#newcode633
chromeos_language.cc:633:
monitor_functions_.update_ime_property(language_library_, prop_list);
On 2010/01/08 09:27:10, satorux1 wrote:
> Ditto.

Added if check. Thanks.

http://codereview.chromium.org/521058/diff/3005/2002#newcode697
chromeos_language.cc:697: retval = ibus_message_get_args(message, &error,
On 2010/01/08 09:27:10, satorux1 wrote:
> You could declare retval here.
> 
> const gboolean retval =

Done.

http://codereview.chromium.org/521058/diff/3005/2003
File chromeos_language.h (right):

http://codereview.chromium.org/521058/diff/3005/2003#newcode25
chromeos_language.h:25: // A structure which represents a current IME language
or a XKB layout.
Ah you're right. Fixed.

http://codereview.chromium.org/521058/diff/3005/2003#newcode78
chromeos_language.h:78: : key(key),
On 2010/01/08 09:27:10, satorux1 wrote:
> I think it's legitimate code but looks a bit tricky. Maybe easier to read if
> parameters are named like in_key.

Done.

http://codereview.chromium.org/521058/diff/3005/2003#newcode87
chromeos_language.h:87: ImeProperty() : is_radio(), is_radio_checked(),
radio_id(kInvalidRadioId) {
On 2010/01/08 09:27:10, satorux1 wrote:
> I didn't know the notation like is_radio() but maybe easier to read if it's
> explicitly initialized like is_radio(false)

Fixed.

http://codereview.chromium.org/521058/diff/3005/2003#newcode92
chromeos_language.h:92: std::string icon;  // Path to an icon. Can be empty.
On 2010/01/08 09:27:10, satorux1 wrote:
> icon_path to make it a bit more descriptive?

Done.

http://codereview.chromium.org/521058/diff/3005/2003#newcode95
chromeos_language.h:95: bool is_radio;  // true if the property is a radio
button.
On 2010/01/08 09:34:48, satorux1 wrote:
> On 2010/01/08 09:27:10, satorux1 wrote:
> > "radio" sounds like UI implementation details. What about naming it a bit
more
> > generic like "selectional_item" as the struct is more like a model?
> > 
> > 
> 
> I meant "selection item"

Done.

http://codereview.chromium.org/521058/diff/3005/2003#newcode95
chromeos_language.h:95: bool is_radio;  // true if the property is a radio
button.
On 2010/01/08 09:27:10, satorux1 wrote:
> "radio" sounds like UI implementation details. What about naming it a bit more
> generic like "selectional_item" as the struct is more like a model?
> 
> 

Done.

http://codereview.chromium.org/521058/diff/3005/2003#newcode133
chromeos_language.h:133: // ---------------------------
On 2010/01/08 09:27:10, satorux1 wrote:
> We don't have members like |type| and |state| in ImeProperty. Would be good to
> make the examples consistent with the structure of ImeProperty .

Done.

http://codereview.chromium.org/521058/diff/3005/2003#newcode148
chromeos_language.h:148: // might send the following properties:
On 2010/01/08 09:27:10, satorux1 wrote:
> RegisterProperties and UpdateProperty look very similar. Could you comment
> differences between these?

Done.

http://codereview.chromium.org/521058/diff/3005/2003#newcode220
chromeos_language.h:220: const char* key, bool active);
On 2010/01/08 09:27:10, satorux1 wrote:
> Would it be possible to have a separate function for deactivation? Using a
> boolean flag for deactivation would make the code a bit hard to read. Besides,
> having Deactive version is consistent with Activate/DeactivateLanguage().

Done.

http://codereview.chromium.org/521058/diff/3005/2005
File monitor_language.cc (right):

http://codereview.chromium.org/521058/diff/3005/2005#newcode37
monitor_language.cc:37: void DumpProperties(const chromeos::ImePropertyList&
prop_list) {
Nice idea. Moved.

[satorux1, 2010-01-12 10:07:02]

LGTM. Minor points below.

http://codereview.chromium.org/521058/diff/5001/5002
File chromeos_language.cc (right):

http://codereview.chromium.org/521058/diff/5001/5002#newcode55
chromeos_language.cc:55: IBusInputContext* GetInputContext(const std::string&
ic_path, IBusBus* ibus) {
input_context_path would be a bit more readable.

http://codereview.chromium.org/521058/diff/5001/5003
File chromeos_language.h (right):

http://codereview.chromium.org/521058/diff/5001/5003#newcode166
chromeos_language.h:166: // properties which is updated recently. Keys the
signal contains is a subset
is a subset -> are a subset ?

http://codereview.chromium.org/521058/diff/5001/5003#newcode167
chromeos_language.h:167: // of keys regstered by the "RegisterProperties" signal
above. For example,
regstered -> registered

http://codereview.chromium.org/521058/diff/5001/5005
File monitor_language.cc (right):

http://codereview.chromium.org/521058/diff/5001/5005#newcode30
monitor_language.cc:30: // 7. Verify that this tool automatically exits in a
second or so.
Shouldn't we update the comment to describe how to trigger RegisterProperties
and UpdateProperty signals?

[Yusuke Sato, 2010-01-19 02:33:44]

Fixed all.

http://codereview.chromium.org/521058/diff/5001/5002
File chromeos_language.cc (right):

http://codereview.chromium.org/521058/diff/5001/5002#newcode55
chromeos_language.cc:55: IBusInputContext* GetInputContext(const std::string&
ic_path, IBusBus* ibus) {
On 2010/01/12 10:07:02, satorux1 wrote:
> input_context_path would be a bit more readable.

Done.

http://codereview.chromium.org/521058/diff/5001/5003
File chromeos_language.h (right):

http://codereview.chromium.org/521058/diff/5001/5003#newcode166
chromeos_language.h:166: // properties which is updated recently. Keys the
signal contains is a subset
On 2010/01/12 10:07:02, satorux1 wrote:
> is a subset -> are a subset ?

Done.

http://codereview.chromium.org/521058/diff/5001/5003#newcode167
chromeos_language.h:167: // of keys regstered by the "RegisterProperties" signal
above. For example,
On 2010/01/12 10:07:02, satorux1 wrote:
> regstered -> registered

Done.

http://codereview.chromium.org/521058/diff/5001/5005
File monitor_language.cc (right):

http://codereview.chromium.org/521058/diff/5001/5005#newcode30
monitor_language.cc:30: // 7. Verify that this tool automatically exits in a
second or so.
On 2010/01/12 10:07:02, satorux1 wrote:
> Shouldn't we update the comment to describe how to trigger RegisterProperties
> and UpdateProperty signals?

Done.

[satorux1, 2010-01-19 02:39:33]

LGTM

On 2010/01/19 02:33:44, Yusuke Sato wrote:
> Fixed all.
> 
> http://codereview.chromium.org/521058/diff/5001/5002
> File chromeos_language.cc (right):
> 
> http://codereview.chromium.org/521058/diff/5001/5002#newcode55
> chromeos_language.cc:55: IBusInputContext* GetInputContext(const std::string&
> ic_path, IBusBus* ibus) {
> On 2010/01/12 10:07:02, satorux1 wrote:
> > input_context_path would be a bit more readable.
> 
> Done.
> 
> http://codereview.chromium.org/521058/diff/5001/5003
> File chromeos_language.h (right):
> 
> http://codereview.chromium.org/521058/diff/5001/5003#newcode166
> chromeos_language.h:166: // properties which is updated recently. Keys the
> signal contains is a subset
> On 2010/01/12 10:07:02, satorux1 wrote:
> > is a subset -> are a subset ?
> 
> Done.
> 
> http://codereview.chromium.org/521058/diff/5001/5003#newcode167
> chromeos_language.h:167: // of keys regstered by the "RegisterProperties"
signal
> above. For example,
> On 2010/01/12 10:07:02, satorux1 wrote:
> > regstered -> registered
> 
> Done.
> 
> http://codereview.chromium.org/521058/diff/5001/5005
> File monitor_language.cc (right):
> 
> http://codereview.chromium.org/521058/diff/5001/5005#newcode30
> monitor_language.cc:30: // 7. Verify that this tool automatically exits in a
> second or so.
> On 2010/01/12 10:07:02, satorux1 wrote:
> > Shouldn't we update the comment to describe how to trigger
RegisterProperties
> > and UpdateProperty signals?
> 
> Done.