Reimplement ibus-gconf so it does not depend on GConf-2 database

Reimplement ibus-gconf so it does not depend on GConf-2 database.

(Please review the README.chromium first.)

BUG=crosbug.com/1638
TEST=run autotest/files/config/site_tests/desktopui_IBusTest

[satorux1, 2010-03-29 03:48:32]

http://codereview.chromium.org/1539001/diff/2001/3001
File README.chromium (right):

http://codereview.chromium.org/1539001/diff/2001/3001#newcode11
README.chromium:11: - gconf/main.cc
Any reason we should colocate the code in the gconf directory? We are adding a
version that doesn't use gconf so I think it would be cleaner to create a
separate directory for the new version, like memconf. The separation would make
it easier for us to sync with the upstream changes.

Would be also nicer if we could send this to the upstream, if they like the idea
of the in-memory version.

[Yusuke Sato, 2010-03-29 05:15:47]

http://codereview.chromium.org/1539001/diff/2001/3001
File README.chromium (right):

http://codereview.chromium.org/1539001/diff/2001/3001#newcode11
README.chromium:11: - gconf/main.cc
Moved new files in gconf/ to a new directory, memconf/, and changed autotools
scripts accordingly. The contents of C++ files are not changed at all.

Thanks.

On 2010/03/29 03:48:32, satorux1 wrote:
> Any reason we should colocate the code in the gconf directory? We are adding a
> version that doesn't use gconf so I think it would be cleaner to create a
> separate directory for the new version, like memconf. The separation would
make
> it easier for us to sync with the upstream changes.
> 
> Would be also nicer if we could send this to the upstream, if they like the
idea
> of the in-memory version.

[satorux1, 2010-03-29 05:19:50]

http://codereview.chromium.org/1539001/diff/2001/3003
File gconf/config.proto (right):

http://codereview.chromium.org/1539001/diff/2001/3003#newcode1
gconf/config.proto:1: // A protobuf definition which represents an entry of IBus
configuration. Only
Please add a copyright notice to all files.

http://codereview.chromium.org/1539001/diff/2001/3004
File gconf/config_mem.cc (right):

http://codereview.chromium.org/1539001/diff/2001/3004#newcode3
gconf/config_mem.cc:3: static void
ibus_config_gconf_class_init(IBusConfigGConfClass* klass);
gconf -> memconf everywhere?

http://codereview.chromium.org/1539001/diff/2001/3004#newcode100
gconf/config_mem.cc:100: return false;  // We handle the type in
ibus_config_gconf_set_value.
Wouldn't it be nicer to handle array types here, rather than partially handling
these in another place? If needed,

http://codereview.chromium.org/1539001/diff/2001/3004#newcode206
gconf/config_mem.cc:206: from_proto(entry, 0, value);
ditto. would be nicer if from_proto does evertyhing.

http://codereview.chromium.org/1539001/diff/2001/3006
File gconf/main.cc (right):

http://codereview.chromium.org/1539001/diff/2001/3006#newcode9
gconf/main.cc:9: static IBusConfigGConf *config = NULL;
Would be nice to add global_ prefix to global variables.

http://codereview.chromium.org/1539001/diff/2001/3006#newcode27
gconf/main.cc:27: g_debug ("bus disconnected");
do we want to have a space before '('? Looks we don't have that space in
config_mem.cc

http://codereview.chromium.org/1539001/diff/2001/3006#newcode32
gconf/main.cc:32: ibus_gconf_start (void)
ibus_gconf -> ibus_memconf ?

http://codereview.chromium.org/1539001/diff/2001/3006#newcode49
gconf/main.cc:49: GOptionContext *context;
initialize by NULL just in case?

[satorux1, 2010-03-29 05:51:16]

Not sure if it's worth doing but we might want to add --enable-memconf and
--disable-gconf to configure.ac

http://codereview.chromium.org/1539001/diff/7001/8001
File Makefile.am (right):

http://codereview.chromium.org/1539001/diff/7001/8001#newcode25
Makefile.am:25: memconf     \
We could add --enable-memconf and --disable-gconf in configure.ac. Then, we
could handle these like ENABLE_PYTHON.

http://codereview.chromium.org/1539001/diff/7001/8003
File configure.ac (right):

http://codereview.chromium.org/1539001/diff/7001/8003#newcode115
configure.ac:115: # local change for Chromium OS. We don't use GConf-2.
What about adding --disable-gconf just like we did for python?

http://codereview.chromium.org/1539001/diff/7001/8004
File data/Makefile.am (left):

http://codereview.chromium.org/1539001/diff/7001/8004#oldcode28
data/Makefile.am:28: schemasdir = $(GCONF_SCHEMA_FILE_DIR)
This could also be done by "if ENABLE_GCONF"

http://codereview.chromium.org/1539001/diff/7001/8005
File memconf/Makefile.am (right):

http://codereview.chromium.org/1539001/diff/7001/8005#newcode92
memconf/Makefile.am:92: $(NULL)
add a blank line here?

[Yusuke Sato, 2010-03-29 09:05:24]

Thanks for the review.

- Addressed all issues you pointed out.
- Applied your --disable-gconf patch.
- Replaced all g_assert() with g_return_val_if_fail() for safety.

PTAL.

http://codereview.chromium.org/1539001/diff/2001/3003
File gconf/config.proto (right):

http://codereview.chromium.org/1539001/diff/2001/3003#newcode1
gconf/config.proto:1: // A protobuf definition which represents an entry of IBus
configuration. Only
On 2010/03/29 05:19:50, satorux1 wrote:
> Please add a copyright notice to all files.

Done. Added the same copyright notices as you added to src/ibuspanelservice.c to
config.proto, config.cc, config.h, and Makefile.am.

http://codereview.chromium.org/1539001/diff/2001/3004
File gconf/config_mem.cc (right):

http://codereview.chromium.org/1539001/diff/2001/3004#newcode3
gconf/config_mem.cc:3: static void
ibus_config_gconf_class_init(IBusConfigGConfClass* klass);
On 2010/03/29 05:19:50, satorux1 wrote:
> gconf -> memconf everywhere?

Oh I forgot to do this :)
Done.

http://codereview.chromium.org/1539001/diff/2001/3004#newcode100
gconf/config_mem.cc:100: return false;  // We handle the type in
ibus_config_gconf_set_value.
On 2010/03/29 05:19:50, satorux1 wrote:
> Wouldn't it be nicer to handle array types here, rather than partially
handling
> these in another place? If needed, 

Done.

http://codereview.chromium.org/1539001/diff/2001/3004#newcode206
gconf/config_mem.cc:206: from_proto(entry, 0, value);
On 2010/03/29 05:19:50, satorux1 wrote:
> ditto. would be nicer if from_proto does evertyhing.

Done.

http://codereview.chromium.org/1539001/diff/2001/3006
File gconf/main.cc (right):

http://codereview.chromium.org/1539001/diff/2001/3006#newcode9
gconf/main.cc:9: static IBusConfigGConf *config = NULL;
As I told offline, I've removed this file from git. Modified Makefile.am so the
file is copied from gconf/main.c on compile time.

http://codereview.chromium.org/1539001/diff/2001/3006#newcode27
gconf/main.cc:27: g_debug ("bus disconnected");
(Wont fix)

On 2010/03/29 05:19:50, satorux1 wrote:
> do we want to have a space before '('? Looks we don't have that space in
> config_mem.cc
>

http://codereview.chromium.org/1539001/diff/2001/3006#newcode32
gconf/main.cc:32: ibus_gconf_start (void)
(Wont fix)

On 2010/03/29 05:19:50, satorux1 wrote:
> ibus_gconf -> ibus_memconf ?

http://codereview.chromium.org/1539001/diff/2001/3006#newcode49
gconf/main.cc:49: GOptionContext *context;
(Wont fix)

On 2010/03/29 05:19:50, satorux1 wrote:
> initialize by NULL just in case?

http://codereview.chromium.org/1539001/diff/7001/8001
File Makefile.am (right):

http://codereview.chromium.org/1539001/diff/7001/8001#newcode25
Makefile.am:25: memconf     \
On 2010/03/29 05:51:16, satorux1 wrote:
> We could add --enable-memconf and --disable-gconf in configure.ac. Then, we
> could handle these like ENABLE_PYTHON.

As we talked offline, applied your --disable-gconf patch.

http://codereview.chromium.org/1539001/diff/7001/8003
File configure.ac (right):

http://codereview.chromium.org/1539001/diff/7001/8003#newcode115
configure.ac:115: # local change for Chromium OS. We don't use GConf-2.
Applied your patch.

http://codereview.chromium.org/1539001/diff/7001/8004
File data/Makefile.am (left):

http://codereview.chromium.org/1539001/diff/7001/8004#oldcode28
data/Makefile.am:28: schemasdir = $(GCONF_SCHEMA_FILE_DIR)
On 2010/03/29 05:51:16, satorux1 wrote:
> This could also be done by "if ENABLE_GCONF"

Applied your patch.

http://codereview.chromium.org/1539001/diff/7001/8005
File memconf/Makefile.am (right):

http://codereview.chromium.org/1539001/diff/7001/8005#newcode92
memconf/Makefile.am:92: $(NULL)
On 2010/03/29 05:51:16, satorux1 wrote:
> add a blank line here?

Done.

[satorux1, 2010-03-29 09:57:55]

http://codereview.chromium.org/1539001/diff/22001/23005
File memconf/config.cc (right):

http://codereview.chromium.org/1539001/diff/22001/23005#newcode161
memconf/config.cc:161: const std::string key = std::string(section) + name;
This fails if section or name is NULL. We might want to have something like:

g_return_if_fail(config, false);
g_return_if_fail(section, false);
g_return_if_fail(name, false);

http://codereview.chromium.org/1539001/diff/22001/23005#newcode195
memconf/config.cc:195: g_return_val_if_fail(size > 0, false);
This looks tricky. What about counting these first?

vector<int> counts;
counts.push_back(config.string_values_size());
counts.push_back(config.boolean_values_size());
counts.push_back(config.int_values_size());
counts.push_back(config.double_values_size());

g_return_if_fail(std::cont(counts.begin(), counts.end(), 0), counts.size() - 1)
size = std::max(counts.begin(), coutns.end())

http://codereview.chromium.org/1539001/diff/22001/23005#newcode236
memconf/config.cc:236: const std::string key = std::string(section) + name;
ditto. This fails if section or name is NULL.

http://codereview.chromium.org/1539001/diff/22001/23005#newcode248
memconf/config.cc:248: if (!from_proto(entry, value)) {
Wait a moment. Why are we using the protocol buffers here? Can we just use
GValue* in the values of "entries" map? Besides, if we get rid of Protobuf, it
would be easy to write code in C, and not in C++. This would make it easy to
upstream the code if they like the idea of memconf.

http://codereview.chromium.org/1539001/diff/22001/23006
File memconf/config.h (right):

http://codereview.chromium.org/1539001/diff/22001/23006#newcode20
memconf/config.h:20: #ifndef CONFIG_H_
MEMCONF_SERVER_H_ ?

[satorux1, 2010-03-29 09:59:18]

http://codereview.chromium.org/1539001/diff/22001/23006
File memconf/config.h (right):

http://codereview.chromium.org/1539001/diff/22001/23006#newcode20
memconf/config.h:20: #ifndef CONFIG_H_
On 2010/03/29 09:57:55, satorux1 wrote:
> MEMCONF_SERVER_H_ ?

I meant MEMCONF_CONFIG_H_

[Yusuke Sato, 2010-03-29 13:36:21]

http://codereview.chromium.org/1539001/diff/22001/23005
File memconf/config.cc (right):

http://codereview.chromium.org/1539001/diff/22001/23005#newcode161
memconf/config.cc:161: const std::string key = std::string(section) + name;
On 2010/03/29 09:57:55, satorux1 wrote:
> This fails if section or name is NULL. We might want to have something like:
> 
> g_return_if_fail(config, false);
> g_return_if_fail(section, false);
> g_return_if_fail(name, false);
> 

Done.

http://codereview.chromium.org/1539001/diff/22001/23005#newcode195
memconf/config.cc:195: g_return_val_if_fail(size > 0, false);
Thanks, but the code is removed.

http://codereview.chromium.org/1539001/diff/22001/23005#newcode236
memconf/config.cc:236: const std::string key = std::string(section) + name;
On 2010/03/29 09:57:55, satorux1 wrote:
> ditto. This fails if section or name is NULL.

Done.

http://codereview.chromium.org/1539001/diff/22001/23005#newcode248
memconf/config.cc:248: if (!from_proto(entry, value)) {
I totally forgot why I started to use protobuf last week, but you're absolutely
right!!! Removed protobuf code, thanks so much.

http://codereview.chromium.org/1539001/diff/22001/23006
File memconf/config.h (right):

http://codereview.chromium.org/1539001/diff/22001/23006#newcode20
memconf/config.h:20: #ifndef CONFIG_H_
On 2010/03/29 09:57:55, satorux1 wrote:
> MEMCONF_SERVER_H_ ?

Done.

http://codereview.chromium.org/1539001/diff/22001/23006#newcode20
memconf/config.h:20: #ifndef CONFIG_H_
On 2010/03/29 09:59:18, satorux1 wrote:
> On 2010/03/29 09:57:55, satorux1 wrote:
> > MEMCONF_SERVER_H_ ?
> 
> I meant MEMCONF_CONFIG_H_

Done.

[satorux1, 2010-03-30 02:45:17]

lgtm. please add some comments before checking this in.

http://codereview.chromium.org/1539001/diff/29001/21013
File memconf/config.cc (right):

http://codereview.chromium.org/1539001/diff/29001/21013#newcode110
memconf/config.cc:110: static gboolean do_unset(IBusConfigMemConf* memconf,
const std::string& key) {
Please write a function comment.

http://codereview.chromium.org/1539001/diff/29001/21013#newcode209
memconf/config.cc:209: // with zero-cleared GValue. See src/ibusconfigservice.c
for details.
Is this explaining why we don't call bus_config_service_value_changed () here?
If so, would be nice to make the comment more explicit like:

... zero-clered GValue, so we don't call the function here.

[Yusuke Sato, 2010-03-30 04:47:29]

thanks. fixed & submitted.

http://codereview.chromium.org/1539001/diff/29001/21013
File memconf/config.cc (right):

http://codereview.chromium.org/1539001/diff/29001/21013#newcode110
memconf/config.cc:110: static gboolean do_unset(IBusConfigMemConf* memconf,
const std::string& key) {
On 2010/03/30 02:45:17, satorux1 wrote:
> Please write a function comment.

Done.

http://codereview.chromium.org/1539001/diff/29001/21013#newcode209
memconf/config.cc:209: // with zero-cleared GValue. See src/ibusconfigservice.c
for details.
On 2010/03/30 02:45:17, satorux1 wrote:
> Is this explaining why we don't call bus_config_service_value_changed () here?
> If so, would be nice to make the comment more explicit like:
> 
> ... zero-clered GValue, so we don't call the function here.
> 

Done.