chromeos networking: small cleanups

chromeos networking: small cleanups

Fix a typo in kProfileNotInitialized, and clarify requirements for
NetworkConfigurationHandler callbacks.

BUG=chromium:448600
TEST=build components_unittests for chromeos

Committed: https://crrev.com/643b79f603216d609450a9c708cdccb252070fdd
Cr-Commit-Position: refs/heads/master@{#311978}

[mukesh agrawal, 2015-01-14 00:43:44]

quiche@chromium.org changed reviewers:
+ pneubeck@chromium.org, stevenjb@chromium.org

[pneubeck (no reviews), 2015-01-14 10:14:35]

seems that there are a few places where |callback| must be not null. If there is
usage for it, i'd be fine with making callback optional.
For |error_callback|, I would agree.

[stevenjb, 2015-01-14 16:43:41]

On 2015/01/14 10:14:35, pneubeck wrote:
> seems that there are a few places where |callback| must be not null. If there
is
> usage for it, i'd be fine with making callback optional.
> For |error_callback|, I would agree.

Lets be consistent. It doesn't make sense for some methods to require a
callback, so I suggest we always make it optional, even if some methods like
SetProperties are pretty pointless without one.

[stevenjb, 2015-01-14 16:47:00]

On 2015/01/14 16:43:41, stevenjb wrote:
> On 2015/01/14 10:14:35, pneubeck wrote:
> > seems that there are a few places where |callback| must be not null. If
there
> is
> > usage for it, i'd be fine with making callback optional.
> > For |error_callback|, I would agree.
> 
> Lets be consistent. It doesn't make sense for some methods to require a
> callback, so I suggest we always make it optional, even if some methods like
> SetProperties are pretty pointless without one.

Actually, scanning the code and thinking about it, we should probably handle
this on a case-by-case basis. Apologies for the bad advise, I would remove the
top level comment for |callback| (As pneubeck@ noted, |error_callback| should
always be optional) and note the methods where |callback| is optional.

[mukesh agrawal, 2015-01-14 22:00:30]

I must be missing something, because I don't see any methods in
NetworkConfigurationHandler that require non-null callbacks.

GetProperties:
- binds |callback| as an argument to GetPropertiesCallback
- GetPropertiesCallback checks |callback.is_null()|

SetProperties:
- before a synchronous invocation of |callback|, checks |callback.is_null()|
- otherwise, binds |callback| as argument to SetPropertiesSuccessCallback
- SetPropertiesSuccessCallback checks |callback.is_null()|

ClearProperties:
- before a synchronous invocation of |callback|, checks |callback.is_null()|
- otherwise, binds |callback| as argument to ClearPropertiesSuccessCallback
- ClearPropertiesSuccessCallback checks |callback.is_null()|

CreateConfiguration:
- binds |callback| as an argument to RunCreateNetworkCallback
- RunCreateNetworkCallback checks |callback.is_null()|

RemoveConfiguration:
- passes |callback| to ProfileEntryDeleter ctor
- ProfileEntryDeleter uses |callback_| in ProfileEntryDeletedCallback
- ProfileEntryDeletedCallback checks |callback.is_null()|

SetNetworkProfile:
- binds |callback| as an argument to SetNetworkProfileCompleted
- SetNetworkProfileCompleted checks |callback.is_null()|

[mukesh agrawal, 2015-01-14 22:02:35]

Per comment #6, I believe all the NetworkConfigurationHandler methods deal
properly will null callbacks. Did I miss something? Or maybe I misunderstood the
concern? (Perhaps we concerned that we might add methods in the future, which
would require a non-null callback?)

[mukesh agrawal, 2015-01-15 03:04:59]

Hmm... Looking over the CL that motivated this
(https://codereview.chromium.org/836363002/), I see that the CL is actually
calling a method on ManagedNetworkConfigurationHandler.

So I should look over MNCH, and any other users of
NetworkConfigurationHandler::StringResultCallback. I'll take a crack at that
tomorrow.

[pneubeck (no reviews), 2015-01-15 09:15:05]

On 2015/01/15 03:04:59, mukesh agrawal wrote:
> Hmm... Looking over the CL that motivated this
> (https://codereview.chromium.org/836363002/), I see that the CL is actually
> calling a method on ManagedNetworkConfigurationHandler.
> 
> So I should look over MNCH, and any other users of
> NetworkConfigurationHandler::StringResultCallback. I'll take a crack at that
> tomorrow.

Uh, ok. I somehow assumed that you were modifying mnch.cc and mnch.h instead of
nch.h . sorry for the confusion.
lgtm as you checked each callback usage.

[mukesh agrawal, 2015-01-16 22:50:45]

The CQ bit was checked by quiche@chromium.org

[commit-bot: I haz the power, 2015-01-16 22:51:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/799313004/1

[commit-bot: I haz the power, 2015-01-16 23:39:45]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-01-16 23:40:32]

Patchset 1 (id:??) landed as
https://crrev.com/643b79f603216d609450a9c708cdccb252070fdd
Cr-Commit-Position: refs/heads/master@{#311978}