Implement IBusLookupTable.

Implement IBusLookupTable.

IBusLookupTable is one of representations an obeject used in communication with
ibus-daemon.

According to this CL, ibus_lookup_table will be comipled and tested but not in
used production binary at thi moment.

BUG=chromium-os:26334
TEST=chromeos_unittests, unit_tests, dbus_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138507

[satorux1, 2012-05-11 22:55:28]

http://codereview.chromium.org/10392039/diff/6001/chromeos/dbus/ibus/ibus_loo...
File chromeos/dbus/ibus/ibus_lookup_table.cc (right):

http://codereview.chromium.org/10392039/diff/6001/chromeos/dbus/ibus/ibus_loo...
chromeos/dbus/ibus/ibus_lookup_table.cc:150: }
Please incline simpler getters/setters in .h file.

http://codereview.chromium.org/10392039/diff/6001/chromeos/dbus/ibus/ibus_loo...
File chromeos/dbus/ibus/ibus_lookup_table.h (right):

http://codereview.chromium.org/10392039/diff/6001/chromeos/dbus/ibus/ibus_loo...
chromeos/dbus/ibus/ibus_lookup_table.h:117: std::vector<std::string> labels_;
I thought candidates_ and labels_ should have the same length. If so, wouldn't
it be nicer to have them in a single vector?

struct Entry {
  candidate_;
  label_;
}
std::vector<Entry> entries_;

http://codereview.chromium.org/10392039/diff/6001/chromeos/dbus/ibus/ibus_loo...
File chromeos/dbus/ibus/ibus_lookup_table_unittest.cc (right):

http://codereview.chromium.org/10392039/diff/6001/chromeos/dbus/ibus/ibus_loo...
chromeos/dbus/ibus/ibus_lookup_table_unittest.cc:59: EXPECT_EQ(kSampleText1,
target_lookup_table.candidates().at(0));
We should ASSERT_EQ(2U, arget_lookup_table.candidates().size())

[satorux1, 2012-05-18 04:50:17]

ping

[Seigo Nonaka, 2012-05-18 17:57:46]

http://codereview.chromium.org/10392039/diff/6001/chromeos/dbus/ibus/ibus_loo...
File chromeos/dbus/ibus/ibus_lookup_table.cc (right):

http://codereview.chromium.org/10392039/diff/6001/chromeos/dbus/ibus/ibus_loo...
chromeos/dbus/ibus/ibus_lookup_table.cc:150: }
On 2012/05/11 22:55:29, satorux1 wrote:
> Please incline simpler getters/setters in .h file.

Done.

http://codereview.chromium.org/10392039/diff/6001/chromeos/dbus/ibus/ibus_loo...
File chromeos/dbus/ibus/ibus_lookup_table.h (right):

http://codereview.chromium.org/10392039/diff/6001/chromeos/dbus/ibus/ibus_loo...
chromeos/dbus/ibus/ibus_lookup_table.h:117: std::vector<std::string> labels_;
On 2012/05/11 22:55:29, satorux1 wrote:
> I thought candidates_ and labels_ should have the same length. If so, wouldn't
> it be nicer to have them in a single vector?
> 
> struct Entry {
>   candidate_;
>   label_;
> }
> std::vector<Entry> entries_;

Done.

http://codereview.chromium.org/10392039/diff/6001/chromeos/dbus/ibus/ibus_loo...
File chromeos/dbus/ibus/ibus_lookup_table_unittest.cc (right):

http://codereview.chromium.org/10392039/diff/6001/chromeos/dbus/ibus/ibus_loo...
chromeos/dbus/ibus/ibus_lookup_table_unittest.cc:59: EXPECT_EQ(kSampleText1,
target_lookup_table.candidates().at(0));
On 2012/05/11 22:55:29, satorux1 wrote:
> We should ASSERT_EQ(2U, arget_lookup_table.candidates().size())

Done.

[satorux1, 2012-05-18 18:27:22]

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
File chromeos/dbus/ibus/ibus_lookup_table.cc (right):

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.cc:18: using
chromeos::ibus::PopStringFromIBusText;
I think we should use 'ibus' namespace here too.

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.cc:34: bool write_label = false;
write_label -> have_labels

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.cc:46: // If there are not any labels, do
not write empty string.
If there are no labels, don't append labels.

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
File chromeos/dbus/ibus/ibus_lookup_table.h (right):

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.h:18: namespace chromeos {
shoudn't we put this in 'ibus' namespace for consistency?

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.h:38: //      string "青"
having non-ASCII character is dangerous. please make it "\xE9\x9D\x92"

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.h:80: // Corresponding to each one
candidate entry. In dbus communication, each
 Corresponding to each one candidate entry.

Represents a candidate entry.

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.h:82: // So just simple string is enough in
this case.
enough -> sufficient?

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.h:139: // A vector of candidate entries.
don't need this comment.

[Seigo Nonaka, 2012-05-18 19:53:16]

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
File chromeos/dbus/ibus/ibus_lookup_table.cc (right):

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.cc:18: using
chromeos::ibus::PopStringFromIBusText;
On 2012/05/18 18:27:22, satorux1 wrote:
> I think we should use 'ibus' namespace here too.

Done.

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.cc:34: bool write_label = false;
On 2012/05/18 18:27:22, satorux1 wrote:
> write_label -> have_labels

Done.

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.cc:46: // If there are not any labels, do
not write empty string.
On 2012/05/18 18:27:22, satorux1 wrote:
> If there are no labels, don't append labels.

Done.

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
File chromeos/dbus/ibus/ibus_lookup_table.h (right):

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.h:18: namespace chromeos {
On 2012/05/18 18:27:22, satorux1 wrote:
> shoudn't we put this in 'ibus' namespace for consistency? 

Yes! thanks.

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.h:38: //      string "青"
On 2012/05/18 18:27:22, satorux1 wrote:
> having non-ASCII character is dangerous. please make it "\xE9\x9D\x92"

Done.

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.h:80: // Corresponding to each one
candidate entry. In dbus communication, each
On 2012/05/18 18:27:22, satorux1 wrote:
>  Corresponding to each one candidate entry.
> 
> Represents a candidate entry.

Done.

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.h:82: // So just simple string is enough in
this case.
On 2012/05/18 18:27:22, satorux1 wrote:
> enough -> sufficient?

Done.

http://codereview.chromium.org/10392039/diff/15001/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.h:139: // A vector of candidate entries.
On 2012/05/18 18:27:22, satorux1 wrote:
> don't need this comment.

Done.

[satorux1, 2012-05-21 19:59:56]

LGTM

[satorux1, 2012-05-21 20:59:35]

http://codereview.chromium.org/10392039/diff/12005/chromeos/dbus/ibus/ibus_lo...
File chromeos/dbus/ibus/ibus_lookup_table.h (right):

http://codereview.chromium.org/10392039/diff/12005/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.h:93: // Accessors.
Please document the members.

[Seigo Nonaka, 2012-05-21 21:16:46]

https://chromiumcodereview.appspot.com/10392039/diff/12005/chromeos/dbus/ibus...
File chromeos/dbus/ibus/ibus_lookup_table.h (right):

https://chromiumcodereview.appspot.com/10392039/diff/12005/chromeos/dbus/ibus...
chromeos/dbus/ibus/ibus_lookup_table.h:93: // Accessors.
On 2012/05/21 20:59:35, satorux1 wrote:
> Please document the members.

Done, wrote in member variable filed.

[satorux1, 2012-05-22 22:17:12]

LGTM with a nit

http://codereview.chromium.org/10392039/diff/10002/chromeos/dbus/ibus/ibus_lo...
File chromeos/dbus/ibus/ibus_lookup_table.h (right):

http://codereview.chromium.org/10392039/diff/10002/chromeos/dbus/ibus/ibus_lo...
chromeos/dbus/ibus/ibus_lookup_table.h:135: // The number of candidates in one
page.
I think we should move comments like this to getters, so users of the class can
read the documentation in the public section, not private section.

[Seigo Nonaka, 2012-05-23 02:40:50]

Thanks!

https://chromiumcodereview.appspot.com/10392039/diff/10002/chromeos/dbus/ibus...
File chromeos/dbus/ibus/ibus_lookup_table.h (right):

https://chromiumcodereview.appspot.com/10392039/diff/10002/chromeos/dbus/ibus...
chromeos/dbus/ibus/ibus_lookup_table.h:135: // The number of candidates in one
page.
I see. Done.
On 2012/05/22 22:17:12, satorux1 wrote:
> I think we should move comments like this to getters, so users of the class
can
> read the documentation in the public section, not private section.

[commit-bot: I haz the power, 2012-05-23 15:58:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nona@chromium.org/10392039/7004

[commit-bot: I haz the power, 2012-05-23 16:23:36]

Try job failure for 10392039-7004 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-05-23 16:31:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nona@chromium.org/10392039/7004

[tfarina, 2012-05-24 16:37:39]

https://chromiumcodereview.appspot.com/10392039/diff/7009/chromeos/dbus/ibus/...
File chromeos/dbus/ibus/ibus_lookup_table.h (right):

https://chromiumcodereview.appspot.com/10392039/diff/7009/chromeos/dbus/ibus/...
chromeos/dbus/ibus/ibus_lookup_table.h:7: 
#pragma once

https://chromiumcodereview.appspot.com/10392039/diff/7009/chromeos/dbus/ibus/...
chromeos/dbus/ibus/ibus_lookup_table.h:67: bool CHROMEOS_EXPORT
PopIBusLookupTable(dbus::MessageReader* reader,
the way I've seen is:

FOO_EXPORT return-type FunctionName(parameter-list);

but probably not a big deal though.