Don't treat Shill ClearProperty errors as failures.

Don't treat Shill ClearProperty errors as failures.

BUG=362303
R=pneubeck@chromium.org

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

[stevenjb, 2014-04-21 19:25:07]

ping

[pneubeck (no reviews), 2014-04-22 07:16:41]

lgtm with comments.

https://codereview.chromium.org/243103004/diff/1/chromeos/network/network_con...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/243103004/diff/1/chromeos/network/network_con...
chromeos/network/network_configuration_handler.cc:407: // If we try to clear a
property that hasn't been set the clear will
can this error be distinguished from other errors?
if so, you might want to set some_failed in the other cases.

https://codereview.chromium.org/243103004/diff/1/chromeos/network/network_con...
chromeos/network/network_configuration_handler.cc:413: callback.Run();
seems unintentional to remove 
 if (!callback.is_null())


depending on your answer to the above question, you should remove error_callback
if its never being used.

[stevenjb, 2014-04-22 19:06:23]

https://codereview.chromium.org/243103004/diff/1/chromeos/network/network_con...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/243103004/diff/1/chromeos/network/network_con...
chromeos/network/network_configuration_handler.cc:407: // If we try to clear a
property that hasn't been set the clear will
On 2014/04/22 07:16:41, pneubeck wrote:
> can this error be distinguished from other errors?
> if so, you might want to set some_failed in the other cases.
I'm not sure what you mean by "this error". We do not keep track of which
properties have been explicitly set (and I don't think we should try to). 
This is really a "warning", but I intentionally didn't distinguish between
"error" and "warning" in NET_LOG since they are effectively the same.

https://codereview.chromium.org/243103004/diff/1/chromeos/network/network_con...
chromeos/network/network_configuration_handler.cc:413: callback.Run();
On 2014/04/22 07:16:41, pneubeck wrote:
> seems unintentional to remove 
>  if (!callback.is_null())
> 
> 
> depending on your answer to the above question, you should remove
error_callback
> if its never being used.

Good catch, and you are correct, will do.

[stevenjb, 2014-04-22 19:06:26]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-04-22 19:06:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/243103004/50001

[pneubeck (no reviews), 2014-04-22 19:12:10]

https://codereview.chromium.org/243103004/diff/1/chromeos/network/network_con...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/243103004/diff/1/chromeos/network/network_con...
chromeos/network/network_configuration_handler.cc:407: // If we try to clear a
property that hasn't been set the clear will
On 2014/04/22 19:06:23, stevenjb wrote:
> On 2014/04/22 07:16:41, pneubeck wrote:
> > can this error be distinguished from other errors?
> > if so, you might want to set some_failed in the other cases.
> I'm not sure what you mean by "this error". We do not keep track of which
> properties have been explicitly set (and I don't think we should try to). 
> This is really a "warning", but I intentionally didn't distinguish between
> "error" and "warning" in NET_LOG since they are effectively the same.

Oh, I meant whether the error 'tried to clear a not-existent property' reported
by Shill can be distinguished from other Shill errors like 'service unknown'.
If so, you might want to log the error only in the former case and set
'some_failed = true' in the latter cases.

[stevenjb, 2014-04-22 19:39:53]

On 2014/04/22 19:12:10, pneubeck wrote:
>
https://codereview.chromium.org/243103004/diff/1/chromeos/network/network_con...
> File chromeos/network/network_configuration_handler.cc (right):
> 
>
https://codereview.chromium.org/243103004/diff/1/chromeos/network/network_con...
> chromeos/network/network_configuration_handler.cc:407: // If we try to clear a
> property that hasn't been set the clear will
> On 2014/04/22 19:06:23, stevenjb wrote:
> > On 2014/04/22 07:16:41, pneubeck wrote:
> > > can this error be distinguished from other errors?
> > > if so, you might want to set some_failed in the other cases.
> > I'm not sure what you mean by "this error". We do not keep track of which
> > properties have been explicitly set (and I don't think we should try to). 
> > This is really a "warning", but I intentionally didn't distinguish between
> > "error" and "warning" in NET_LOG since they are effectively the same.
> 
> Oh, I meant whether the error 'tried to clear a not-existent property'
reported
> by Shill can be distinguished from other Shill errors like 'service unknown'.
> If so, you might want to log the error only in the former case and set
> 'some_failed = true' in the latter cases.

Other Shill errors will trigger ClearPropertiesErrorCallback, so we have that
covered.

[pneubeck (no reviews), 2014-04-22 20:16:59]

https://codereview.chromium.org/243103004/diff/50001/chromeos/network/network...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/243103004/diff/50001/chromeos/network/network...
chromeos/network/network_configuration_handler.cc:398: const std::string
kClearPropertiesFailedError("Error.ClearPropertiesFailed");
unused. (didn't see it earlier)

[pneubeck (no reviews), 2014-04-22 20:17:23]

On 2014/04/22 19:39:53, stevenjb wrote:
> On 2014/04/22 19:12:10, pneubeck wrote:
> >
>
https://codereview.chromium.org/243103004/diff/1/chromeos/network/network_con...
> > File chromeos/network/network_configuration_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/243103004/diff/1/chromeos/network/network_con...
> > chromeos/network/network_configuration_handler.cc:407: // If we try to clear
a
> > property that hasn't been set the clear will
> > On 2014/04/22 19:06:23, stevenjb wrote:
> > > On 2014/04/22 07:16:41, pneubeck wrote:
> > > > can this error be distinguished from other errors?
> > > > if so, you might want to set some_failed in the other cases.
> > > I'm not sure what you mean by "this error". We do not keep track of which
> > > properties have been explicitly set (and I don't think we should try to). 
> > > This is really a "warning", but I intentionally didn't distinguish between
> > > "error" and "warning" in NET_LOG since they are effectively the same.
> > 
> > Oh, I meant whether the error 'tried to clear a not-existent property'
> reported
> > by Shill can be distinguished from other Shill errors like 'service
unknown'.
> > If so, you might want to log the error only in the former case and set
> > 'some_failed = true' in the latter cases.
> 
> Other Shill errors will trigger ClearPropertiesErrorCallback, so we have that
> covered.

Ok. All good then.

[commit-bot: I haz the power, 2014-04-22 20:17:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-22 20:17:47]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[stevenjb, 2014-04-22 21:37:45]

Committed patchset #5 manually as r265370 (presubmit successful).