Hook up system clock update dbus signal to chrome and ash tray ui.

Hook up system clock update dbus signal to chrome and ash tray ui. cros will send tlsdate.TimeUpdated dbus signal when system clock is updated from server, chrome will listen to the signal and update ash tray ui for time.


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

[jennyz, 2013-03-12 23:27:15]

Please ignore the try bot compilation error for now, since the DEPS roll for
service_constants.h change is still lagging behind.
https://codereview.chromium.org/12562004/

[stevenjb, 2013-03-13 17:34:31]

https://codereview.chromium.org/12564007/diff/7001/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/12564007/diff/7001/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:270:
DBusThreadManager::Get()->GetSystemClockClient()->AddObserver(this);
What do you think of making TrayClock a SystemClockClient::Observer instead of
using the delegate? Eventually we want to move TimezoneSettings into
src/chromeos also and remove ClockObserver entirely. This means moving TrayClock
into ash/system/chromeos, which seems fine.

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
File chromeos/dbus/system_clock_client.cc (right):

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
chromeos/dbus/system_clock_client.cc:64: LOG_IF(WARNING, !success)
ERROR

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
chromeos/dbus/system_clock_client.cc:65: << "Failed to connect to TimeUpdated
signal.";
DISALLOW...

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
chromeos/dbus/system_clock_client.cc:80: class SystemClockClientStubImpl :
public SystemClockClient {
public destructor

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
chromeos/dbus/system_clock_client.cc:84: virtual bool HasObserver(Observer*
observer) OVERRIDE { return false; }
DISALLOW...

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
File chromeos/dbus/system_clock_client.h (right):

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
chromeos/dbus/system_clock_client.h:25: virtual void SystemClockUpdated() {}
Add:
 protected:
  virtual void ~Observer {}

(I know we don't do this everywhere, but we really should)

[jennyz, 2013-03-13 19:14:01]

Sorry, holding the patch until we confirmed about TrayDate moving direction.

https://codereview.chromium.org/12564007/diff/7001/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/12564007/diff/7001/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:270:
DBusThreadManager::Get()->GetSystemClockClient()->AddObserver(this);
On 2013/03/13 17:34:31, stevenjb (chromium) wrote:
> What do you think of making TrayClock a SystemClockClient::Observer instead of
> using the delegate? Eventually we want to move TimezoneSettings into
> src/chromeos also and remove ClockObserver entirely. This means moving
TrayClock
> into ash/system/chromeos, which seems fine.
I am not sure if TrayDate is chromeos only, or will it be used on other aura
enabled platform as well? If it is chromeos only, it makes sense to move it to
ash/system/chromeos and have it be chromeos::SystemClockClient::Observer. Can we
defer this task to a separate refactor bug? This is to fix the P1 bug which has
been lingering for a long time and needs to be ported back to R26.

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
File chromeos/dbus/system_clock_client.cc (right):

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
chromeos/dbus/system_clock_client.cc:64: LOG_IF(WARNING, !success)
On 2013/03/13 17:34:31, stevenjb (chromium) wrote:
> ERROR

Done.

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
chromeos/dbus/system_clock_client.cc:65: << "Failed to connect to TimeUpdated
signal.";
On 2013/03/13 17:34:31, stevenjb (chromium) wrote:
> DISALLOW...

which this refers to?

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
chromeos/dbus/system_clock_client.cc:80: class SystemClockClientStubImpl :
public SystemClockClient {
On 2013/03/13 17:34:31, stevenjb (chromium) wrote:
> public destructor

Done.

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
chromeos/dbus/system_clock_client.cc:84: virtual bool HasObserver(Observer*
observer) OVERRIDE { return false; }
On 2013/03/13 17:34:31, stevenjb (chromium) wrote:
> DISALLOW...

Done.

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
File chromeos/dbus/system_clock_client.h (right):

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
chromeos/dbus/system_clock_client.h:25: virtual void SystemClockUpdated() {}
On 2013/03/13 17:34:31, stevenjb (chromium) wrote:
> Add:
>  protected:
>   virtual void ~Observer {}
> 
> (I know we don't do this everywhere, but we really should)

Done.

[stevenjb, 2013-03-13 19:22:39]

https://codereview.chromium.org/12564007/diff/7001/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/12564007/diff/7001/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:270:
DBusThreadManager::Get()->GetSystemClockClient()->AddObserver(this);
On 2013/03/13 19:14:02, jennyz wrote:
> On 2013/03/13 17:34:31, stevenjb (chromium) wrote:
> > What do you think of making TrayClock a SystemClockClient::Observer instead
of
> > using the delegate? Eventually we want to move TimezoneSettings into
> > src/chromeos also and remove ClockObserver entirely. This means moving
> TrayClock
> > into ash/system/chromeos, which seems fine.
> I am not sure if TrayDate is chromeos only, or will it be used on other aura
> enabled platform as well? If it is chromeos only, it makes sense to move it to
> ash/system/chromeos and have it be chromeos::SystemClockClient::Observer. Can
we
> defer this task to a separate refactor bug? This is to fix the P1 bug which
has
> been lingering for a long time and needs to be ported back to R26.

So, TrayClock is already effectively chromeos specific, since as near as I can
tell system::TimezoneSettings is chromeos-only, as is SystemClockClient. If/when
we want to add one for other platforms we would rename the existing one as
TrayClockChromeos and add TrayClockWindows (or whatever), possibly with a common
TrayClock interface. We still shouldn't be calling into Chrome for OS specific
calls.

That said, if you would rather that be done in a separate pass I am OK with
that, but we are making additional work for ourselves later.

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
File chromeos/dbus/system_clock_client.cc (right):

https://codereview.chromium.org/12564007/diff/7001/chromeos/dbus/system_clock...
chromeos/dbus/system_clock_client.cc:65: << "Failed to connect to TimeUpdated
signal.";
Nevermind, I missed it below somehow.

[jennyz, 2013-03-13 20:47:15]

https://codereview.chromium.org/12564007/diff/7001/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/12564007/diff/7001/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:270:
DBusThreadManager::Get()->GetSystemClockClient()->AddObserver(this);
On 2013/03/13 19:22:39, stevenjb (chromium) wrote:
> On 2013/03/13 19:14:02, jennyz wrote:
> > On 2013/03/13 17:34:31, stevenjb (chromium) wrote:
> > > What do you think of making TrayClock a SystemClockClient::Observer
instead
> of
> > > using the delegate? Eventually we want to move TimezoneSettings into
> > > src/chromeos also and remove ClockObserver entirely. This means moving
> > TrayClock
> > > into ash/system/chromeos, which seems fine.
> > I am not sure if TrayDate is chromeos only, or will it be used on other aura
> > enabled platform as well? If it is chromeos only, it makes sense to move it
to
> > ash/system/chromeos and have it be chromeos::SystemClockClient::Observer.
Can
> we
> > defer this task to a separate refactor bug? This is to fix the P1 bug which
> has
> > been lingering for a long time and needs to be ported back to R26.
> 
> So, TrayClock is already effectively chromeos specific, since as near as I can
> tell system::TimezoneSettings is chromeos-only, as is SystemClockClient.
If/when
> we want to add one for other platforms we would rename the existing one as
> TrayClockChromeos and add TrayClockWindows (or whatever), possibly with a
common
> TrayClock interface. We still shouldn't be calling into Chrome for OS specific
> calls.
> 
> That said, if you would rather that be done in a separate pass I am OK with
> that, but we are making additional work for ourselves later.
I need to jump to audio work for M27. I filed crbug.com/190299 for the work
item, I will take this work when I got the audio work done. Thanks!

[stevenjb, 2013-03-13 21:25:53]

LGTM with minor fix

https://codereview.chromium.org/12564007/diff/15001/chromeos/dbus/system_cloc...
File chromeos/dbus/system_clock_client.cc (right):

https://codereview.chromium.org/12564007/diff/15001/chromeos/dbus/system_cloc...
chromeos/dbus/system_clock_client.cc:89: 
 private:

[jennyz, 2013-03-13 21:59:56]

https://codereview.chromium.org/12564007/diff/15001/chromeos/dbus/system_cloc...
File chromeos/dbus/system_clock_client.cc (right):

https://codereview.chromium.org/12564007/diff/15001/chromeos/dbus/system_cloc...
chromeos/dbus/system_clock_client.cc:89: 
On 2013/03/13 21:25:54, stevenjb (chromium) wrote:
>  private:

Done.

[commit-bot: I haz the power, 2013-03-14 02:35:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/12564007/22001

[commit-bot: I haz the power, 2013-03-14 14:54:21]

Change committed as 188070

[Dan Beam, 2016-11-28 21:42:50]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-11-28 21:42:50]

https://codereview.chromium.org/12564007/diff/22001/chromeos/dbus/system_cloc...
File chromeos/dbus/system_clock_client.cc (right):

https://codereview.chromium.org/12564007/diff/22001/chromeos/dbus/system_cloc...
chromeos/dbus/system_clock_client.cc:55: VLOG(1) << "TimeUpdated signal
received: " << signal->ToString();
is |signal| guaranteed to be non-null here?

[stevenjb, 2016-11-28 21:58:32]

https://codereview.chromium.org/12564007/diff/22001/chromeos/dbus/system_cloc...
File chromeos/dbus/system_clock_client.cc (right):

https://codereview.chromium.org/12564007/diff/22001/chromeos/dbus/system_cloc...
chromeos/dbus/system_clock_client.cc:55: VLOG(1) << "TimeUpdated signal
received: " << signal->ToString();
On 2016/11/28 21:42:50, Dan Beam wrote:
> is |signal| guaranteed to be non-null here?

Yes. ObjectProxy::HandleMessage calls RunMethod with a constructed signal.