Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
935933002:
Adds metronome time sync dbus client (Closed)
|
Created:
5 years, 10 months ago by varkha Modified:
5 years, 9 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, hashimoto+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds metronome time sync dbus client
BUG=395878
TEST = run with "--dbus-stub --test-metronome-timer" in command line to test with a timer-based implementation.
Committed: https://crrev.com/2aba422641731a4e8a4186da5a0866e5313b4359
Cr-Commit-Position: refs/heads/master@{#319322}
Patch Set 1 : Adds metronome time sync dbus client #
Total comments: 14
Patch Set 2 : Added ability to notify multiple observers #Patch Set 3 : Defaults to Metronome client #
Total comments: 12
Patch Set 4 : Addresses nits #
Total comments: 6
Patch Set 5 : Makes sure Metronome proxy is only connected once #Patch Set 6 : Rebase #
Total comments: 2
Patch Set 7 : Nits #Patch Set 8 : Adds do-nothing stub implementation and rebase #
Total comments: 6
Patch Set 9 : Adds a command line flag to use a timer-based fake implementation #
Messages
Total messages: 59 (32 generated)
Patchset #1 (id:1) has been deleted
varkha@chromium.org changed reviewers: + benchan@chromium.org, dtapuska@chromium.org
benchan@, Can you please take an early look?
https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... File chromeos/dbus/metronome_client.cc (right): https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. Copyrights should be 2015. https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:40: virtual void SetTimestampUpdatedHandler( Remove the virtual https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:51: virtual void Init(dbus::Bus* bus) override { Remove the virtual https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:111: base::Time now_time = base::Time::Now(); I don't get why we are using the wall clock anywhere... Wall clock is subject to offset and adjustment by other processes. https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:120: base::WeakPtrFactory<MetronomeClientStubImpl> weak_ptr_factory_; I thought Weak Ptr Factories should be listed towards the end of the class so they are destructed first. https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... File chromeos/dbus/metronome_client.h (right): https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.h:26: virtual void SetTimestampUpdatedHandler( I don't know if you want to use an Observer class instead of a Callback here much like I used in the leadership and peerd.. I was asked to follow the Bluetooth client example; as you might want to become aware that the metronome dbus service has crashed or become unavailable? Ben might have an opinion.
On 2015/02/18 13:59:13, Dave Tapuska wrote: > https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... > File chromeos/dbus/metronome_client.cc (right): > > https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... > chromeos/dbus/metronome_client.cc:1: // Copyright 2014 The Chromium Authors. All > rights reserved. > Copyrights should be 2015. > > https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... > chromeos/dbus/metronome_client.cc:40: virtual void SetTimestampUpdatedHandler( > Remove the virtual > > https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... > chromeos/dbus/metronome_client.cc:51: virtual void Init(dbus::Bus* bus) override > { > Remove the virtual > > https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... > chromeos/dbus/metronome_client.cc:111: base::Time now_time = base::Time::Now(); > I don't get why we are using the wall clock anywhere... Wall clock is subject to > offset and adjustment by other processes. > > https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... > chromeos/dbus/metronome_client.cc:120: > base::WeakPtrFactory<MetronomeClientStubImpl> weak_ptr_factory_; > I thought Weak Ptr Factories should be listed towards the end of the class so > they are destructed first. > > https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... > File chromeos/dbus/metronome_client.h (right): > > https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... > chromeos/dbus/metronome_client.h:26: virtual void SetTimestampUpdatedHandler( > I don't know if you want to use an Observer class instead of a Callback here > much like I used in the leadership and peerd.. > > I was asked to follow the Bluetooth client example; as you might want to become > aware that the metronome dbus service has crashed or become unavailable? > > Ben might have an opinion. oops, I also have a CL: https://codereview.chromium.org/904163005. I'm still thinking about whether to change metronome to use a DBus FileDescriptor instead of DBus signal to deliver the timestamps. I'm wondering if metronome should use a descriptor to pass
> oops, I also have a CL: https://codereview.chromium.org/904163005. +benchan, Do you want to keep this CL alive (those are doing the same thing)? > I'm still > thinking about whether to change metronome to use a DBus FileDescriptor instead > of DBus signal to deliver the timestamps. > > I'm wondering if metronome should use a descriptor to pass Are you concerned about latency of DBus signals?
https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... File chromeos/dbus/metronome_client.cc (right): https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/02/18 13:59:12, Dave Tapuska wrote: > Copyrights should be 2015. Done. https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:40: virtual void SetTimestampUpdatedHandler( On 2015/02/18 13:59:12, Dave Tapuska wrote: > Remove the virtual Done. https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:51: virtual void Init(dbus::Bus* bus) override { On 2015/02/18 13:59:12, Dave Tapuska wrote: > Remove the virtual Done. https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:111: base::Time now_time = base::Time::Now(); On 2015/02/18 13:59:12, Dave Tapuska wrote: > I don't get why we are using the wall clock anywhere... Wall clock is subject to > offset and adjustment by other processes. Right. For now this is just a stub implementation to allow debugging although we could wire that to the ptp and maybe implement software sync this way. https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:120: base::WeakPtrFactory<MetronomeClientStubImpl> weak_ptr_factory_; On 2015/02/18 13:59:12, Dave Tapuska wrote: > I thought Weak Ptr Factories should be listed towards the end of the class so > they are destructed first. Done. https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... File chromeos/dbus/metronome_client.h (right): https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.h:26: virtual void SetTimestampUpdatedHandler( On 2015/02/18 13:59:12, Dave Tapuska wrote: > I don't know if you want to use an Observer class instead of a Callback here > much like I used in the leadership and peerd.. > > I was asked to follow the Bluetooth client example; as you might want to become > aware that the metronome dbus service has crashed or become unavailable? > > Ben might have an opinion. Done (multiple observers part).
https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... File chromeos/dbus/metronome_client.h (right): https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.h:26: virtual void SetTimestampUpdatedHandler( On 2015/02/18 13:59:12, Dave Tapuska wrote: > I don't know if you want to use an Observer class instead of a Callback here > much like I used in the leadership and peerd.. > > I was asked to follow the Bluetooth client example; as you might want to become > aware that the metronome dbus service has crashed or become unavailable? > > Ben might have an opinion. It depends. I generally use Delegate for situation where there should only be one client observing the event, and use Observer for situation where there would be multiple observers. As we may do another DBus-to-mojo bridge, it may not necessarily to have multiple observers here.
benchan@, Wouldn't using dbus::FileDescriptor require an I/O capable thread on the receiving end? https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... File chromeos/dbus/metronome_client.h (right): https://codereview.chromium.org/935933002/diff/20001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.h:26: virtual void SetTimestampUpdatedHandler( On 2015/02/18 18:39:22, Ben Chan wrote: > On 2015/02/18 13:59:12, Dave Tapuska wrote: > > I don't know if you want to use an Observer class instead of a Callback here > > much like I used in the leadership and peerd.. > > > > I was asked to follow the Bluetooth client example; as you might want to > become > > aware that the metronome dbus service has crashed or become unavailable? > > > > Ben might have an opinion. > > It depends. I generally use Delegate for situation where there should only be > one client observing the event, and use Observer for situation where there would > be multiple observers. As we may do another DBus-to-mojo bridge, it may not > necessarily to have multiple observers here. Yes, this is why I've added observers.
benchan@, are you still looking at this? I would like to make sure you are OK with that before I go with the OWNERS.
benchan@, are you still looking at this? I would like to make sure you are OK with that before I go with the OWNERS.
benchan@, are you still looking at this? I would like to make sure you are OK with that before I go with the OWNERS.
lgtm with nits https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... File chromeos/dbus/metronome_client.cc (right): https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:39: virtual ~MetronomeClientImpl() {} ~MetronomeClientImpl() override {} https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:120: virtual ~MetronomeClientStubImpl() { timer_.Stop(); } ~MetronomeClientImpl() override { ... } https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:123: virtual void Init(dbus::Bus* bus) override { drop virtual https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:129: void AddObserver(Observer* observer) { override https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:134: void RemoveObserver(Observer* observer) { override https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... File chromeos/dbus/metronome_client.h (right): https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.h:23: uint64 mac_timestamp) = 0; |beacon_timestamp| and |mac_timestamp| kinda refers to similar thing. |mac_timestamp| should be renamed to something else like |local_timestamp|
New patchsets have been uploaded after l-g-t-m from benchan@chromium.org
varkha@chromium.org changed reviewers: + stevenjb@chromium.org
+stevenjb for OWNERS. https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... File chromeos/dbus/metronome_client.cc (right): https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:39: virtual ~MetronomeClientImpl() {} On 2015/02/19 19:03:14, Ben Chan wrote: > ~MetronomeClientImpl() override {} Done. https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:120: virtual ~MetronomeClientStubImpl() { timer_.Stop(); } On 2015/02/19 19:03:14, Ben Chan wrote: > ~MetronomeClientImpl() override { ... } Done. https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:123: virtual void Init(dbus::Bus* bus) override { On 2015/02/19 19:03:14, Ben Chan wrote: > drop virtual Done. https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:129: void AddObserver(Observer* observer) { On 2015/02/19 19:03:14, Ben Chan wrote: > override Done. https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:134: void RemoveObserver(Observer* observer) { On 2015/02/19 19:03:14, Ben Chan wrote: > override Done. https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... File chromeos/dbus/metronome_client.h (right): https://codereview.chromium.org/935933002/diff/60001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.h:23: uint64 mac_timestamp) = 0; On 2015/02/19 19:03:14, Ben Chan wrote: > |beacon_timestamp| and |mac_timestamp| kinda refers to similar thing. > > |mac_timestamp| should be renamed to something else like |local_timestamp| > Done.
https://codereview.chromium.org/935933002/diff/80001/chromeos/dbus/metronome_... File chromeos/dbus/metronome_client.cc (right): https://codereview.chromium.org/935933002/diff/80001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:57: dbus::ObjectProxy* proxy_; WS https://codereview.chromium.org/935933002/diff/80001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:76: weak_ptr_factory_.GetWeakPtr())); Is it OK to do this more than once? If not we need a better way to track this, since a client could add, remove, then add an observer. https://codereview.chromium.org/935933002/diff/80001/chromeos/dbus/metronome_... File chromeos/dbus/metronome_client.h (right): https://codereview.chromium.org/935933002/diff/80001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.h:23: uint64 local_timestamp) = 0; Needs a virtual destructor (can be inlined).
Patchset #5 (id:100001) has been deleted
PTAL https://codereview.chromium.org/935933002/diff/80001/chromeos/dbus/metronome_... File chromeos/dbus/metronome_client.cc (right): https://codereview.chromium.org/935933002/diff/80001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:57: dbus::ObjectProxy* proxy_; On 2015/02/24 18:34:50, stevenjb wrote: > WS Done. https://codereview.chromium.org/935933002/diff/80001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.cc:76: weak_ptr_factory_.GetWeakPtr())); On 2015/02/24 18:34:49, stevenjb wrote: > Is it OK to do this more than once? If not we need a better way to track this, > since a client could add, remove, then add an observer. Done. https://codereview.chromium.org/935933002/diff/80001/chromeos/dbus/metronome_... File chromeos/dbus/metronome_client.h (right): https://codereview.chromium.org/935933002/diff/80001/chromeos/dbus/metronome_... chromeos/dbus/metronome_client.h:23: uint64 local_timestamp) = 0; On 2015/02/24 18:34:50, stevenjb wrote: > Needs a virtual destructor (can be inlined). Done.
owner lgtm https://codereview.chromium.org/935933002/diff/140001/chromeos/dbus/metronome... File chromeos/dbus/metronome_client.cc (right): https://codereview.chromium.org/935933002/diff/140001/chromeos/dbus/metronome... chromeos/dbus/metronome_client.cc:60: bool once_; nit: Use a more descriptive variable name, e.g. signal_connected_
Patchset #7 (id:160001) has been deleted
Thanks! https://codereview.chromium.org/935933002/diff/140001/chromeos/dbus/metronome... File chromeos/dbus/metronome_client.cc (right): https://codereview.chromium.org/935933002/diff/140001/chromeos/dbus/metronome... chromeos/dbus/metronome_client.cc:60: bool once_; On 2015/02/24 23:42:54, stevenjb wrote: > nit: Use a more descriptive variable name, e.g. signal_connected_ Done.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benchan@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/935933002/#ps180001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/935933002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/935933002/180001
Patchset #10 (id:240001) has been deleted
Patchset #8 (id:200001) has been deleted
Patchset #8 (id:220001) has been deleted
Patchset #8 (id:260001) has been deleted
Patchset #8 (id:280001) has been deleted
Patchset #8 (id:300001) has been deleted
Patchset #8 (id:310001) has been deleted
Patchset #8 (id:320001) has been deleted
Patchset #8 (id:340001) has been deleted
Patchset #9 (id:380001) has been deleted
Patchset #8 (id:360001) has been deleted
Patchset #9 (id:420001) has been deleted
Patchset #8 (id:400001) has been deleted
Patchset #8 (id:440001) has been deleted
Patchset #8 (id:460001) has been deleted
Patchset #8 (id:480001) has been deleted
Patchset #8 (id:500001) has been deleted
Patchset #8 (id:520001) has been deleted
Patchset #8 (id:540001) has been deleted
Patchset #8 (id:560001) has been deleted
benchan@, can you PTAL? The stub running a timer did not work well in unit tests so I've made up a do-nothing stub and put what used to be stub under ifdef.
benchan@, can you PTAL? The stub running a timer did not work well in unit tests so I've made up a do-nothing stub and put what used to be stub under ifdef.
https://codereview.chromium.org/935933002/diff/580001/chromeos/dbus/metronome... File chromeos/dbus/metronome_client.cc (right): https://codereview.chromium.org/935933002/diff/580001/chromeos/dbus/metronome... chromeos/dbus/metronome_client.cc:46: protected: wrong indent https://codereview.chromium.org/935933002/diff/580001/chromeos/dbus/metronome... chromeos/dbus/metronome_client.cc:47: // DBusClient: // DBusClient overrides https://codereview.chromium.org/935933002/diff/580001/chromeos/dbus/metronome... chromeos/dbus/metronome_client.cc:121: #ifdef USE_TIMER_BASED_METRONOME_DEMO_IMPLEMENTATION can this be a parameter on the stub implementation (probably configure via command line) instead of a compile time ifdef?
https://codereview.chromium.org/935933002/diff/580001/chromeos/dbus/metronome... File chromeos/dbus/metronome_client.cc (right): https://codereview.chromium.org/935933002/diff/580001/chromeos/dbus/metronome... chromeos/dbus/metronome_client.cc:46: protected: On 2015/03/05 18:20:37, Ben Chan wrote: > wrong indent Done. https://codereview.chromium.org/935933002/diff/580001/chromeos/dbus/metronome... chromeos/dbus/metronome_client.cc:47: // DBusClient: On 2015/03/05 18:20:37, Ben Chan wrote: > // DBusClient overrides This style seems to be a more dominant variant in chromium these days and is preferred by the code owners. https://codereview.chromium.org/935933002/diff/580001/chromeos/dbus/metronome... chromeos/dbus/metronome_client.cc:121: #ifdef USE_TIMER_BASED_METRONOME_DEMO_IMPLEMENTATION On 2015/03/05 18:20:37, Ben Chan wrote: > can this be a parameter on the stub implementation (probably configure via > command line) instead of a compile time ifdef? Done.
still lgtm
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benchan@chromium.org Link to the patchset: https://codereview.chromium.org/935933002/#ps600001 (title: "Adds a command line flag to use a timer-based fake implementation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/935933002/600001
Message was sent while issue was closed.
Committed patchset #9 (id:600001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/2aba422641731a4e8a4186da5a0866e5313b4359 Cr-Commit-Position: refs/heads/master@{#319322} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
