This converts the Shill clients to allow propagation of shill errors

This converts the Shill clients to allow propagation of shill errors
back from the function calls.  The existing implentation completely ignores most shill errors, and the javascript implementation will want to be able to receive them to aid in diagnosis.

BUG=chromium:147620, chromium:146616
TEST=Ran unit tests, ran on device.


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

[Greg Spencer (Chromium), 2012-09-20 18:22:00]

Hashimoto, could you please review this?

Satoru, I need OWNERS approval from you for this.

[hashimoto, 2012-09-21 11:52:01]

Could you split this into two changes?
One for observer and another for ErrorCallback.

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
File chrome/browser/chromeos/cros/cros_network_functions.cc (right):

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
chrome/browser/chromeos/cros/cros_network_functions.cc:56:
ShillServiceClient::PropertyChangedObserver
Add public, no new line before '{'

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
chrome/browser/chromeos/cros/cros_network_functions.cc:76: private:
nit: Add new line before 'private'

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
chrome/browser/chromeos/cros/cros_network_functions.cc:211: void DoNothing() {}
You can use base::DoNothing() in base/bind_helpers.h

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
chrome/browser/chromeos/cros/cros_network_functions.cc:245: 
nit: Remove this blank line.

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
chrome/browser/chromeos/cros/cros_network_functions.cc:381:
base::Bind(&IgnoreErrors));
nit: align.

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
chrome/browser/chromeos/cros/cros_network_functions.cc:565:
dbus::ObjectPath(service_path), base::Bind(DoNothing),
nit: Add '&' before DoNothing for consistency.

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
chrome/browser/chromeos/cros/cros_network_functions.cc:571:
dbus::ObjectPath(service_path), base::Bind(DoNothing),
ditto.

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
File chrome/browser/chromeos/cros/cros_network_functions_unittest.cc (right):

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
chrome/browser/chromeos/cros/cros_network_functions_unittest.cc:326:
ShillClientHelper::PropertyChangedObserver* observer;
Please initialize with NULL.

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
chrome/browser/chromeos/cros/cros_network_functions_unittest.cc:346:
ShillClientHelper::PropertyChangedObserver* observer;
ditto.

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
chrome/browser/chromeos/cros/cros_network_functions_unittest.cc:368:
ShillClientHelper::PropertyChangedObserver* observer;
ditto.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
File chromeos/dbus/shill_client_helper.cc (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
chromeos/dbus/shill_client_helper.cc:27: PropertyChangedObserver* obs) {
Rename this to |observer|

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
chromeos/dbus/shill_client_helper.cc:32: PropertyChangedObserver* obs) {
ditto.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
chromeos/dbus/shill_client_helper.cc:211: if
(!observer_list_.might_have_observers())
This early exit was necessary since calling Run() against NULL callback results
in SIGSEGV.
With observer list, we no longer have to check this.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
File chromeos/dbus/shill_client_helper.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
chromeos/dbus/shill_client_helper.h:60: class PropertyChangedObserver {
Please add class comment, especially about the event in which this observer is
interested.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
chromeos/dbus/shill_client_helper.h:70: void
AddPropertyChangedObserver(PropertyChangedObserver* observer);
Please add method comments.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
chromeos/dbus/shill_client_helper.h:87: const ObjectPathDBusMethodCallback&
callback,
Can we introduce something like DictionaryValueCallbackWithoutStatus for
ObjectPath? (say, ObjectPathCallbackWithoutStatus)

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_uni...
File chromeos/dbus/shill_client_unittest_base.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_uni...
chromeos/dbus/shill_client_unittest_base.h:50: MatchResultListener* listener)
const {
Could you move these method definitions to .cc file and add OVERRIDEs?

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_device_cli...
File chromeos/dbus/shill_device_client.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_device_cli...
chromeos/dbus/shill_device_client.h:49: virtual void AddPropertyChangedObserver(
Please add method comments.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_ipconfig_c...
File chromeos/dbus/shill_ipconfig_client.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_ipconfig_c...
chromeos/dbus/shill_ipconfig_client.h:47: virtual void
AddPropertyChangedObserver(
Please add method comments.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_manager_cl...
File chromeos/dbus/shill_manager_client.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_manager_cl...
chromeos/dbus/shill_manager_client.h:41: virtual void
AddPropertyChangedObserver(
Please add method comments.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_network_cl...
File chromeos/dbus/shill_network_client.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_network_cl...
chromeos/dbus/shill_network_client.h:48: virtual void
AddPropertyChangedObserver(
Please add method comments.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_profile_cl...
File chromeos/dbus/shill_profile_client.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_profile_cl...
chromeos/dbus/shill_profile_client.h:50: virtual void
AddPropertyChangedObserver(
Please add method comments.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_service_cl...
File chromeos/dbus/shill_service_client.cc (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_service_cl...
chromeos/dbus/shill_service_client.cc:260: void PostClosure(const base::Closure&
callback) {
Since this method became very simple, we no longer need this?

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_service_cl...
File chromeos/dbus/shill_service_client.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_service_cl...
chromeos/dbus/shill_service_client.h:50: virtual void
AddPropertyChangedObserver(
Please add method comments.

[Greg Spencer (Chromium), 2012-09-21 22:03:47]

OK, I've split this into two changes.  This change will now be just for the
error callback changes, and I'll send you another change for the observer
change.

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
File chrome/browser/chromeos/cros/cros_network_functions.cc (right):

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
chrome/browser/chromeos/cros/cros_network_functions.cc:56:
ShillServiceClient::PropertyChangedObserver
On 2012/09/21 11:52:01, hashimoto wrote:
> Add public, no new line before '{'

Done.

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
chrome/browser/chromeos/cros/cros_network_functions.cc:76: private:
On 2012/09/21 11:52:01, hashimoto wrote:
> nit: Add new line before 'private'

Done.

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
File chrome/browser/chromeos/cros/cros_network_functions_unittest.cc (right):

http://codereview.chromium.org/10949030/diff/1/chrome/browser/chromeos/cros/c...
chrome/browser/chromeos/cros/cros_network_functions_unittest.cc:326:
ShillClientHelper::PropertyChangedObserver* observer;
On 2012/09/21 11:52:01, hashimoto wrote:
> Please initialize with NULL.

Done, and the ones below.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
File chromeos/dbus/shill_client_helper.cc (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
chromeos/dbus/shill_client_helper.cc:27: PropertyChangedObserver* obs) {
On 2012/09/21 11:52:01, hashimoto wrote:
> Rename this to |observer|

Done.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
chromeos/dbus/shill_client_helper.cc:32: PropertyChangedObserver* obs) {
On 2012/09/21 11:52:01, hashimoto wrote:
> ditto.

Done.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
chromeos/dbus/shill_client_helper.cc:211: if
(!observer_list_.might_have_observers())
On 2012/09/21 11:52:01, hashimoto wrote:
> This early exit was necessary since calling Run() against NULL callback
results
> in SIGSEGV.
> With observer list, we no longer have to check this.

Sure, but why not early exit anyhow, to avoid having to create the reader and
pop the values?

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
File chromeos/dbus/shill_client_helper.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
chromeos/dbus/shill_client_helper.h:60: class PropertyChangedObserver {
On 2012/09/21 11:52:01, hashimoto wrote:
> Please add class comment, especially about the event in which this observer is
> interested.

Done.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
chromeos/dbus/shill_client_helper.h:70: void
AddPropertyChangedObserver(PropertyChangedObserver* observer);
On 2012/09/21 11:52:01, hashimoto wrote:
> Please add method comments.

Done.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_hel...
chromeos/dbus/shill_client_helper.h:87: const ObjectPathDBusMethodCallback&
callback,
On 2012/09/21 11:52:01, hashimoto wrote:
> Can we introduce something like DictionaryValueCallbackWithoutStatus for
> ObjectPath? (say, ObjectPathCallbackWithoutStatus)

Done.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_uni...
File chromeos/dbus/shill_client_unittest_base.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_client_uni...
chromeos/dbus/shill_client_unittest_base.h:50: MatchResultListener* listener)
const {
On 2012/09/21 11:52:01, hashimoto wrote:
> Could you move these method definitions to .cc file and add OVERRIDEs?

Done.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_device_cli...
File chromeos/dbus/shill_device_client.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_device_cli...
chromeos/dbus/shill_device_client.h:49: virtual void AddPropertyChangedObserver(
On 2012/09/21 11:52:01, hashimoto wrote:
> Please add method comments.

Done.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_ipconfig_c...
File chromeos/dbus/shill_ipconfig_client.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_ipconfig_c...
chromeos/dbus/shill_ipconfig_client.h:47: virtual void
AddPropertyChangedObserver(
On 2012/09/21 11:52:01, hashimoto wrote:
> Please add method comments.

Done.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_manager_cl...
File chromeos/dbus/shill_manager_client.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_manager_cl...
chromeos/dbus/shill_manager_client.h:41: virtual void
AddPropertyChangedObserver(
On 2012/09/21 11:52:01, hashimoto wrote:
> Please add method comments.

Done.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_network_cl...
File chromeos/dbus/shill_network_client.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_network_cl...
chromeos/dbus/shill_network_client.h:48: virtual void
AddPropertyChangedObserver(
On 2012/09/21 11:52:01, hashimoto wrote:
> Please add method comments.

Done.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_profile_cl...
File chromeos/dbus/shill_profile_client.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_profile_cl...
chromeos/dbus/shill_profile_client.h:50: virtual void
AddPropertyChangedObserver(
On 2012/09/21 11:52:01, hashimoto wrote:
> Please add method comments.

Done.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_service_cl...
File chromeos/dbus/shill_service_client.cc (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_service_cl...
chromeos/dbus/shill_service_client.cc:260: void PostClosure(const base::Closure&
callback) {
On 2012/09/21 11:52:01, hashimoto wrote:
> Since this method became very simple, we no longer need this?

Removed.

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_service_cl...
File chromeos/dbus/shill_service_client.h (right):

http://codereview.chromium.org/10949030/diff/1/chromeos/dbus/shill_service_cl...
chromeos/dbus/shill_service_client.h:50: virtual void
AddPropertyChangedObserver(
On 2012/09/21 11:52:01, hashimoto wrote:
> Please add method comments.

Done.

[hashimoto, 2012-09-24 02:28:56]

http://codereview.chromium.org/10949030/diff/3003/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/cros_network_functions.cc (right):

http://codereview.chromium.org/10949030/diff/3003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/cros_network_functions.cc:218: 
nit: Remove this blank line.

http://codereview.chromium.org/10949030/diff/3003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/cros_network_functions.cc:458:
base::Bind(RunCallbackWithDictionaryValueError, callback, profile_path));
Add '&' for consistency.

http://codereview.chromium.org/10949030/diff/3003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/cros_network_functions.cc:471:
base::Bind(RunCallbackWithDictionaryValueError,
Add '&' for consistency.

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/dbus_method_c...
File chromeos/dbus/dbus_method_call_status.h (right):

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/dbus_method_c...
chromeos/dbus/dbus_method_call_status.h:45: const dbus::ObjectPath& result)>
ObjectPathCallbackWithoutStatus;
Just 'ObjectPathCallback'?
ShillClientHelper::DictionaryValueCallbackWithoutStatus was named
'-WithoutStatus' because 'DictionaryValueCallback' was already there.
If you place this callback here, I think you don't have to add 'WithoutStatus'.

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_client_...
File chromeos/dbus/shill_client_helper.cc (right):

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_client_...
chromeos/dbus/shill_client_helper.cc:252: callback.Run(dbus::ObjectPath());
Please run ErrorCallback on error cases. (see
OnDictionaryValueMethodWithErrorCallback)

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_client_...
chromeos/dbus/shill_client_helper.cc:258: callback.Run(dbus::ObjectPath());
ditto.

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_client_...
File chromeos/dbus/shill_client_unittest_base.h (right):

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_client_...
chromeos/dbus/shill_client_unittest_base.h:81: const base::Value& value);
nit: Please keep a blank line here.

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_client_...
chromeos/dbus/shill_client_unittest_base.h:116: static void
ExpectDictionaryValueResultWithoutStatus(
Please add a blank line and a method comment before this method.

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_manager...
File chromeos/dbus/shill_manager_client_unittest.cc (right):

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_manager...
chromeos/dbus/shill_manager_client_unittest.cc:308: MockErrorCallback
error_callback;
Move this to the line before the actual method call and set expectations.

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_profile...
File chromeos/dbus/shill_profile_client.cc (right):

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_profile...
chromeos/dbus/shill_profile_client.cc:33: 
nit: Remove this line.

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_service...
File chromeos/dbus/shill_service_client.cc (right):

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_service...
chromeos/dbus/shill_service_client.cc:194: // ShillServiceClient override
nit: Revert this line to make it easy to merge changes in your observer patch.

[Greg Spencer (Chromium), 2012-09-24 21:50:54]

http://codereview.chromium.org/10949030/diff/3003/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/cros_network_functions.cc (right):

http://codereview.chromium.org/10949030/diff/3003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/cros_network_functions.cc:218: 
On 2012/09/24 02:28:56, hashimoto wrote:
> nit: Remove this blank line.

Done.

http://codereview.chromium.org/10949030/diff/3003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/cros_network_functions.cc:458:
base::Bind(RunCallbackWithDictionaryValueError, callback, profile_path));
On 2012/09/24 02:28:56, hashimoto wrote:
> Add '&' for consistency.

Done.

http://codereview.chromium.org/10949030/diff/3003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/cros_network_functions.cc:471:
base::Bind(RunCallbackWithDictionaryValueError,
On 2012/09/24 02:28:56, hashimoto wrote:
> Add '&' for consistency.

Done.

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/dbus_method_c...
File chromeos/dbus/dbus_method_call_status.h (right):

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/dbus_method_c...
chromeos/dbus/dbus_method_call_status.h:45: const dbus::ObjectPath& result)>
ObjectPathCallbackWithoutStatus;
On 2012/09/24 02:28:56, hashimoto wrote:
> Just 'ObjectPathCallback'?
> ShillClientHelper::DictionaryValueCallbackWithoutStatus was named
> '-WithoutStatus' because 'DictionaryValueCallback' was already there.
> If you place this callback here, I think you don't have to add
'WithoutStatus'.

OK, or should I rename ObjectPathDBusMethodCallback to just be
ObjectPathCallback?  Or should the new one be called
ObjectPathDBusMethodCallbackWithoutStatus?

I think I'll do the first one, since that results in shorter names all around,
and I don't think that the "DBusMethod" part of the name adds anything.

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_client_...
File chromeos/dbus/shill_client_helper.cc (right):

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_client_...
chromeos/dbus/shill_client_helper.cc:252: callback.Run(dbus::ObjectPath());
On 2012/09/24 02:28:56, hashimoto wrote:
> Please run ErrorCallback on error cases. (see
> OnDictionaryValueMethodWithErrorCallback)

Done.

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_client_...
chromeos/dbus/shill_client_helper.cc:258: callback.Run(dbus::ObjectPath());
On 2012/09/24 02:28:56, hashimoto wrote:
> ditto.

Done.

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_client_...
File chromeos/dbus/shill_client_unittest_base.h (right):

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_client_...
chromeos/dbus/shill_client_unittest_base.h:81: const base::Value& value);
On 2012/09/24 02:28:56, hashimoto wrote:
> nit: Please keep a blank line here.

Done.

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_client_...
chromeos/dbus/shill_client_unittest_base.h:116: static void
ExpectDictionaryValueResultWithoutStatus(
On 2012/09/24 02:28:56, hashimoto wrote:
> Please add a blank line and a method comment before this method.

Done.

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_manager...
File chromeos/dbus/shill_manager_client_unittest.cc (right):

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_manager...
chromeos/dbus/shill_manager_client_unittest.cc:308: MockErrorCallback
error_callback;
On 2012/09/24 02:28:56, hashimoto wrote:
> Move this to the line before the actual method call and set expectations.

Done.

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_service...
File chromeos/dbus/shill_service_client.cc (right):

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/shill_service...
chromeos/dbus/shill_service_client.cc:194: // ShillServiceClient override
On 2012/09/24 02:28:56, hashimoto wrote:
> nit: Revert this line to make it easy to merge changes in your observer patch.

Done.

[hashimoto, 2012-09-25 08:08:19]

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/dbus_method_c...
File chromeos/dbus/dbus_method_call_status.h (right):

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/dbus_method_c...
chromeos/dbus/dbus_method_call_status.h:45: const dbus::ObjectPath& result)>
ObjectPathCallbackWithoutStatus;
On 2012/09/24 21:50:54, Greg Spencer (Chromium) wrote:
> On 2012/09/24 02:28:56, hashimoto wrote:
> > Just 'ObjectPathCallback'?
> > ShillClientHelper::DictionaryValueCallbackWithoutStatus was named
> > '-WithoutStatus' because 'DictionaryValueCallback' was already there.
> > If you place this callback here, I think you don't have to add
> 'WithoutStatus'.
> 
> OK, or should I rename ObjectPathDBusMethodCallback to just be
> ObjectPathCallback?  Or should the new one be called
> ObjectPathDBusMethodCallbackWithoutStatus?
> 
> I think I'll do the first one, since that results in shorter names all around,
> and I don't think that the "DBusMethod" part of the name adds anything.
> 

I think we should keep the old one's name unchanged because its first argument
is DBusMethodCallStatus.

I also think 'ObjectPathCallback' is a very generic name and it should be a name
of a simple type like base::Callback<void(const dbus::ObjectPath&)>.

http://codereview.chromium.org/10949030/diff/19001/chromeos/dbus/shill_client...
File chromeos/dbus/shill_client_unittest_base.h (right):

http://codereview.chromium.org/10949030/diff/19001/chromeos/dbus/shill_client...
chromeos/dbus/shill_client_unittest_base.h:112: // Checks the result and expects
the call status to be SUCCESS
nit: Add period.

http://codereview.chromium.org/10949030/diff/19001/chromeos/dbus/shill_profil...
File chromeos/dbus/shill_profile_client_unittest.cc (right):

http://codereview.chromium.org/10949030/diff/19001/chromeos/dbus/shill_profil...
chromeos/dbus/shill_profile_client_unittest.cc:106: MockErrorCallback
error_callback;
Add expectation

[Greg Spencer (Chromium), 2012-09-25 18:40:08]

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/dbus_method_c...
File chromeos/dbus/dbus_method_call_status.h (right):

http://codereview.chromium.org/10949030/diff/3003/chromeos/dbus/dbus_method_c...
chromeos/dbus/dbus_method_call_status.h:45: const dbus::ObjectPath& result)>
ObjectPathCallbackWithoutStatus;
On 2012/09/25 08:08:19, hashimoto wrote:
> On 2012/09/24 21:50:54, Greg Spencer (Chromium) wrote:
> > On 2012/09/24 02:28:56, hashimoto wrote:
> > > Just 'ObjectPathCallback'?
> > > ShillClientHelper::DictionaryValueCallbackWithoutStatus was named
> > > '-WithoutStatus' because 'DictionaryValueCallback' was already there.
> > > If you place this callback here, I think you don't have to add
> > 'WithoutStatus'.
> > 
> > OK, or should I rename ObjectPathDBusMethodCallback to just be
> > ObjectPathCallback?  Or should the new one be called
> > ObjectPathDBusMethodCallbackWithoutStatus?
> > 
> > I think I'll do the first one, since that results in shorter names all
around,
> > and I don't think that the "DBusMethod" part of the name adds anything.
> > 
> 
> I think we should keep the old one's name unchanged because its first argument
> is DBusMethodCallStatus.
> 
> I also think 'ObjectPathCallback' is a very generic name and it should be a
name
> of a simple type like base::Callback<void(const dbus::ObjectPath&)>.

OK, I kept the original one and renamed the new one to be ObjectPathCallback.

http://codereview.chromium.org/10949030/diff/19001/chromeos/dbus/shill_client...
File chromeos/dbus/shill_client_unittest_base.h (right):

http://codereview.chromium.org/10949030/diff/19001/chromeos/dbus/shill_client...
chromeos/dbus/shill_client_unittest_base.h:112: // Checks the result and expects
the call status to be SUCCESS
On 2012/09/25 08:08:19, hashimoto wrote:
> nit: Add period.

Done.

http://codereview.chromium.org/10949030/diff/19001/chromeos/dbus/shill_profil...
File chromeos/dbus/shill_profile_client_unittest.cc (right):

http://codereview.chromium.org/10949030/diff/19001/chromeos/dbus/shill_profil...
chromeos/dbus/shill_profile_client_unittest.cc:106: MockErrorCallback
error_callback;
On 2012/09/25 08:08:19, hashimoto wrote:
> Add expectation

Done.

[satorux1, 2012-09-25 21:58:41]

LGTM with a nit

http://codereview.chromium.org/10949030/diff/29001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/cros_network_functions.cc (right):

http://codereview.chromium.org/10949030/diff/29001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/cros_network_functions.cc:349:
base::Bind(&IgnoreErrors));
nit: indentation is off

[hashimoto, 2012-09-26 00:17:34]

lgtm with nits

http://codereview.chromium.org/10949030/diff/29001/chromeos/dbus/dbus_method_...
File chromeos/dbus/dbus_method_call_status.h (right):

http://codereview.chromium.org/10949030/diff/29001/chromeos/dbus/dbus_method_...
chromeos/dbus/dbus_method_call_status.h:44: typedef base::Callback<void(
nit: No need to newline?

http://codereview.chromium.org/10949030/diff/29001/chromeos/dbus/mock_shill_m...
File chromeos/dbus/mock_shill_manager_client.h (right):

http://codereview.chromium.org/10949030/diff/29001/chromeos/dbus/mock_shill_m...
chromeos/dbus/mock_shill_manager_client.h:40: MOCK_METHOD3(GetService, void(
nit: No need to newline?

http://codereview.chromium.org/10949030/diff/29001/chromeos/dbus/shill_manage...
File chromeos/dbus/shill_manager_client.h (right):

http://codereview.chromium.org/10949030/diff/29001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client.h:92: virtual void GetService(
nit: No need to newline?

[Greg Spencer (Chromium), 2012-09-26 23:36:33]

http://codereview.chromium.org/10949030/diff/29001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/cros_network_functions.cc (right):

http://codereview.chromium.org/10949030/diff/29001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/cros_network_functions.cc:349:
base::Bind(&IgnoreErrors));
On 2012/09/25 21:58:41, satorux1 wrote:
> nit: indentation is off

Done.

http://codereview.chromium.org/10949030/diff/29001/chromeos/dbus/dbus_method_...
File chromeos/dbus/dbus_method_call_status.h (right):

http://codereview.chromium.org/10949030/diff/29001/chromeos/dbus/dbus_method_...
chromeos/dbus/dbus_method_call_status.h:44: typedef base::Callback<void(
On 2012/09/26 00:17:34, hashimoto wrote:
> nit: No need to newline?

Done.

http://codereview.chromium.org/10949030/diff/29001/chromeos/dbus/mock_shill_m...
File chromeos/dbus/mock_shill_manager_client.h (right):

http://codereview.chromium.org/10949030/diff/29001/chromeos/dbus/mock_shill_m...
chromeos/dbus/mock_shill_manager_client.h:40: MOCK_METHOD3(GetService, void(
On 2012/09/26 00:17:34, hashimoto wrote:
> nit: No need to newline?

Done.

http://codereview.chromium.org/10949030/diff/29001/chromeos/dbus/shill_manage...
File chromeos/dbus/shill_manager_client.h (right):

http://codereview.chromium.org/10949030/diff/29001/chromeos/dbus/shill_manage...
chromeos/dbus/shill_manager_client.h:92: virtual void GetService(
On 2012/09/26 00:17:34, hashimoto wrote:
> nit: No need to newline?

Done.

[commit-bot: I haz the power, 2012-09-28 16:46:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/10949030/18004

[commit-bot: I haz the power, 2012-09-28 17:20:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-28 17:29:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/10949030/25006

[commit-bot: I haz the power, 2012-09-28 20:34:21]

Change committed as 159325