chromeos: Don't spam logs with tlsdate D-Bus errors at boot.

chromeos: Don't spam logs with tlsdate D-Bus errors at boot.

Make SystemClockClientImpl avoid making its initial call to
tlsdate to check if the time can be set until D-Bus has
notified it that tlsdate is available.

BUG=636554
Committed: https://crrev.com/709228f565b22a2115bdbbac2f6b804055699d07
Cr-Commit-Position: refs/heads/master@{#411369}

[Daniel Erat, 2016-08-10 21:34:57]

derat@chromium.org changed reviewers:
+ jennyz@chromium.org, michaelpg@chromium.org

[Daniel Erat, 2016-08-10 21:34:58]

any suggestions about what i should be doing to test this change, apart from
running with vmodule=1 and verifying that we're getting notified when the
service becomes available?

[Daniel Erat, 2016-08-10 21:35:05]

The CQ bit was checked by derat@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 21:35:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 22:17:41]

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

[commit-bot: I haz the power, 2016-08-10 22:17:42]

Dry run: This issue passed the CQ dry run.

[Daniel Erat, 2016-08-11 01:15:46]

https://codereview.chromium.org/2236713002/diff/1/chromeos/dbus/system_clock_...
File chromeos/dbus/system_clock_client.cc (right):

https://codereview.chromium.org/2236713002/diff/1/chromeos/dbus/system_clock_...
chromeos/dbus/system_clock_client.cc:73: void ProxyAvailabilityChanged(bool
service_is_available) {
i should rename this and change the code. this is actually only called once,
when the service becomes available. (it's unlikely that it'll ever see a false
argument.)

[Daniel Erat, 2016-08-11 02:19:08]

The CQ bit was checked by derat@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 02:19:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 02:59:33]

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

[commit-bot: I haz the power, 2016-08-11 02:59:34]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2016-08-11 03:21:08]

lgtm

On 2016/08/10 21:34:58, Daniel Erat wrote:
> any suggestions about what i should be doing to test this change, apart from
> running with vmodule=1 and verifying that we're getting notified when the
> service becomes available?

You could try having/not having a network on boot, and check whether the set
time UI can be launched from the date in the tray -- that's still the only thing
this is for, right?

https://codereview.chromium.org/2236713002/diff/1/chromeos/dbus/system_clock_...
File chromeos/dbus/system_clock_client.cc (right):

https://codereview.chromium.org/2236713002/diff/1/chromeos/dbus/system_clock_...
chromeos/dbus/system_clock_client.cc:73: void ProxyAvailabilityChanged(bool
service_is_available) {
On 2016/08/11 01:15:46, Daniel Erat wrote:
> i should rename this and change the code. this is actually only called once,
> when the service becomes available. (it's unlikely that it'll ever see a false
> argument.)

what would make the service unavailable after the timeout? (and how long is the
timeout?)

[Daniel Erat, 2016-08-11 14:18:44]

On 2016/08/11 03:21:08, michaelpg wrote:
> lgtm
> 
> On 2016/08/10 21:34:58, Daniel Erat wrote:
> > any suggestions about what i should be doing to test this change, apart from
> > running with vmodule=1 and verifying that we're getting notified when the
> > service becomes available?
> 
> You could try having/not having a network on boot, and check whether the set
> time UI can be launched from the date in the tray -- that's still the only
thing
> this is for, right?

thanks, i'll give this a try!

>
https://codereview.chromium.org/2236713002/diff/1/chromeos/dbus/system_clock_...
> File chromeos/dbus/system_clock_client.cc (right):
> 
>
https://codereview.chromium.org/2236713002/diff/1/chromeos/dbus/system_clock_...
> chromeos/dbus/system_clock_client.cc:73: void ProxyAvailabilityChanged(bool
> service_is_available) {
> On 2016/08/11 01:15:46, Daniel Erat wrote:
> > i should rename this and change the code. this is actually only called once,
> > when the service becomes available. (it's unlikely that it'll ever see a
false
> > argument.)
> 
> what would make the service unavailable after the timeout? (and how long is
the
> timeout?)

it's actually weirder than that; there's no timeout. the callback is only run
with false if our d-bus library fails to wire up the name-owner-changed signals
(so naming the arg "service_is_available" is a bit misleading).

it would be clearer if the callback didn't take an argument, and the d-bus
method instead returned true when the callback is registered successfully and
false when an issue occurs. the method is used all over the place and is hard to
change now, though. :-(

[Daniel Erat, 2016-08-11 17:20:56]

yep, confirmed that i can set the time at the login screen without a network
connection but not with.

[Daniel Erat, 2016-08-11 17:21:00]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2016-08-11 17:21:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 17:29:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-11 17:32:05]

Description was changed from

==========
chromeos: Don't spam logs with tlsdate D-Bus errors at boot.

Make SystemClockClientImpl avoid making its initial call to
tlsdate to check if the time can be set until D-Bus has
notified it that tlsdate is available.

BUG=636554
==========

to

==========
chromeos: Don't spam logs with tlsdate D-Bus errors at boot.

Make SystemClockClientImpl avoid making its initial call to
tlsdate to check if the time can be set until D-Bus has
notified it that tlsdate is available.

BUG=636554

Committed: https://crrev.com/709228f565b22a2115bdbbac2f6b804055699d07
Cr-Commit-Position: refs/heads/master@{#411369}
==========

[commit-bot: I haz the power, 2016-08-11 17:32:05]

Patchset 2 (id:??) landed as
https://crrev.com/709228f565b22a2115bdbbac2f6b804055699d07
Cr-Commit-Position: refs/heads/master@{#411369}