Add FlimflamIPConfigClient

Add FlimflamIPConfigClient

BUG=chromium-os:16557
TEST=build success


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129756

[stevenjb, 2012-03-29 00:28:14]

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc (right):

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:21: class
FlimflamIPConfigClientImpl : public FlimflamIPConfigClient {
For complex classes like these, it is easier to read when the class definition
is separated from the implementation. I know we inline Impl classes a lot, but
it is a trend that we should reverse.

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:70: if (value.GetType()
== base::Value::TYPE_LIST) {
I think this would be better as a switch(value.GetType()), with the supported
types spelled out. e.g. How does AppendBasicTypeValueData handle DictionaryValue
or BinaryValue?

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:106: virtual void
Remove(const VoidCallback& callback) OVERRIDE {
I think this should be named RemoveConfig.

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:119: bool successed) {
'success' or 'succeeded'

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:121: signal << "
failed.";
nit: start second line with <<

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:128: const bool
name_popped = reader.PopString(&name);
nit: I think I prefer
if (!reader.PopString(&name))
  return;

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:152:
callback.Run(FAILURE, result);
callback.Run(FAILURE, base::DictionaryValue())

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:160:
callback.Run(FAILURE, result);
callback.Run(FAILURE, base::DictionaryValue())

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:188: virtual void
GetProperties(const DictionaryValueCallback& callback) OVERRIDE {
Stub should invoke callback here and elsewhere.

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/flimflam_ipconfig_client.h (right):

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.h:54: // Factory function,
creates a new instance and returns ownership.
nit: 'and returns ownership' is confusing. Prefer 'which is owned by the
caller.'

[hashimoto, 2012-03-29 03:04:45]

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc (right):

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:21: class
FlimflamIPConfigClientImpl : public FlimflamIPConfigClient {
On 2012/03/29 00:28:14, stevenjb (chromium) wrote:
> For complex classes like these, it is easier to read when the class definition
> is separated from the implementation. I know we inline Impl classes a lot, but
> it is a trend that we should reverse.

Done.

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:70: if (value.GetType()
== base::Value::TYPE_LIST) {
On 2012/03/29 00:28:14, stevenjb (chromium) wrote:
> I think this would be better as a switch(value.GetType()), with the supported
> types spelled out. e.g. How does AppendBasicTypeValueData handle
DictionaryValue
> or BinaryValue?

Done.

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:106: virtual void
Remove(const VoidCallback& callback) OVERRIDE {
On 2012/03/29 00:28:14, stevenjb (chromium) wrote:
> I think this should be named RemoveConfig.
The actual method name is "Remove".
(http://git.chromium.org/gitweb/?p=chromiumos/platform/system_api.git;a=blob;f...)
This constant is named "kRemoveConfigFunction" because there are two "Remove" in
the same namespace.  (I think there should be only one constant named
kRemoveFunction, though.)

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:119: bool successed) {
On 2012/03/29 00:28:14, stevenjb (chromium) wrote:
> 'success' or 'succeeded'

Done.

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:121: signal << "
failed.";
On 2012/03/29 00:28:14, stevenjb (chromium) wrote:
> nit: start second line with <<

Done.

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:128: const bool
name_popped = reader.PopString(&name);
On 2012/03/29 00:28:14, stevenjb (chromium) wrote:
> nit: I think I prefer
> if (!reader.PopString(&name))
>   return;

Done.

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:152:
callback.Run(FAILURE, result);
On 2012/03/29 00:28:14, stevenjb (chromium) wrote:
> callback.Run(FAILURE, base::DictionaryValue())
I tried it before, but clang said "error: C++98 requires an accessible copy
constructor for class 'base::DictionaryValue' when binding a reference to a
temporary; was private"

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:160:
callback.Run(FAILURE, result);
On 2012/03/29 00:28:14, stevenjb (chromium) wrote:
> callback.Run(FAILURE, base::DictionaryValue())

ditto.

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:188: virtual void
GetProperties(const DictionaryValueCallback& callback) OVERRIDE {
On 2012/03/29 00:28:14, stevenjb (chromium) wrote:
> Stub should invoke callback here and elsewhere.

Done.

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/flimflam_ipconfig_client.h (right):

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.h:54: // Factory function,
creates a new instance and returns ownership.
On 2012/03/29 00:28:14, stevenjb (chromium) wrote:
> nit: 'and returns ownership' is confusing. Prefer 'which is owned by the
> caller.'

Done.

[satorux1, 2012-03-29 17:58:44]

LGTM

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc (right):

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:21: class
FlimflamIPConfigClientImpl : public FlimflamIPConfigClient {
On 2012/03/29 03:04:46, hashimoto wrote:
> On 2012/03/29 00:28:14, stevenjb (chromium) wrote:
> > For complex classes like these, it is easier to read when the class
definition
> > is separated from the implementation. I know we inline Impl classes a lot,
but
> > it is a trend that we should reverse.
> 
> Done.

I personally prefer not to separate the implementation from the class
definition, mostly because I hate to type more and touch two places when I
change function signatures. Besides, we inline functions inside classes in every
other language, so I'm used to it.

I think things like the are up to the author's taste. Since Ryo agreed to make
them separate, we should go with separation in this file. :)

http://codereview.chromium.org/9875013/diff/8001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc (right):

http://codereview.chromium.org/9875013/diff/8001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:75: }
nit: add blank line.

[hashimoto, 2012-03-29 18:12:16]

http://codereview.chromium.org/9875013/diff/8001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc (right):

http://codereview.chromium.org/9875013/diff/8001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:75: }
On 2012/03/29 17:58:44, satorux1 wrote:
> nit: add blank line.

Done.

[commit-bot: I haz the power, 2012-03-29 18:13:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/9875013/8002

[commit-bot: I haz the power, 2012-03-29 18:53:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/9875013/15006

[stevenjb, 2012-03-29 18:56:16]

lgtm

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc (right):

http://codereview.chromium.org/9875013/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/flimflam_ipconfig_client.cc:106: virtual void
Remove(const VoidCallback& callback) OVERRIDE {
On 2012/03/29 03:04:46, hashimoto wrote:
> On 2012/03/29 00:28:14, stevenjb (chromium) wrote:
> > I think this should be named RemoveConfig.
> The actual method name is "Remove".
>
(http://git.chromium.org/gitweb/?p=chromiumos/platform/system_api.git;a=blob;f...)
> This constant is named "kRemoveConfigFunction" because there are two "Remove"
in
> the same namespace.  (I think there should be only one constant named
> kRemoveFunction, though.)
Well, I think flimflam::kRemoveConfigFunction and RemoveConfig is more clear
(the other should probably also be more specific - functions like 'Add' and
'Remove' are harder to search for, etc), but this is fine.

http://codereview.chromium.org/9875013/diff/8002/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/dbus_thread_manager.cc (right):

http://codereview.chromium.org/9875013/diff/8002/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/dbus_thread_manager.cc:149: 
future nit: We should have one comment for all DBusThreadManger overrides
(preferred), or one per function. I'm guessing this was a merge glitch.

[commit-bot: I haz the power, 2012-03-29 20:40:39]

Try job failure for 9875013-15006 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-03-29 22:07:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/9875013/15006

[commit-bot: I haz the power, 2012-03-29 23:36:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/9875013/28001

[commit-bot: I haz the power, 2012-03-30 01:37:25]

Change committed as 129756