Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2377873002:
Initial definition of HeliumSyncManager and SyncRegistration objects. (Closed)
|
Created:
4 years, 2 months ago by Kevin M Modified:
4 years, 2 months ago CC:
anandc+watch-blimp_chromium.org, bgoldman+watch-blimp_chromium.org, cbentzel+watch_chromium.org, chromium-reviews, dtrainor+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, scf+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, steimel+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial definition of HeliumSyncManager and SyncRegistration objects.
This CL contains an abstract interface for SyncManager and a private
class skeleton SyncManagerImpl with the method bodies stubbed out.
R=steimel@chromium.org,scf@chromium.org
CC=wez@chromium.org
BUG=650469
Committed: https://crrev.com/35a16a011800bf25273c58f0c937b0140b55d9bd
Cr-Commit-Position: refs/heads/master@{#423660}
Patch Set 1 #
Total comments: 26
Patch Set 2 : code review feedback #Patch Set 3 : Remove review cruft from CL #
Total comments: 6
Messages
Total messages: 21 (5 generated)
Description was changed from ========== Initial definition of HeliumSyncManager and SyncRegistration objects. Initial definition of HeliumSyncManager and SyncRegistration objects. This CL contains an abstract interface for SyncManager and a private class skeleton SyncManagerImpl with the method bodies stubbed out. R=steimel@chromium.org,scf@chromium.org CC=wez@chromium.org BUG=650469 ========== to ========== Initial definition of HeliumSyncManager and SyncRegistration objects. This CL contains an abstract interface for SyncManager and a private class skeleton SyncManagerImpl with the method bodies stubbed out. R=steimel@chromium.org,scf@chromium.org CC=wez@chromium.org BUG=650469 ==========
https://codereview.chromium.org/2377873002/diff/1/blimp/common/proto/helium.p... File blimp/common/proto/helium.proto (right): https://codereview.chromium.org/2377873002/diff/1/blimp/common/proto/helium.p... blimp/common/proto/helium.proto:8: syntax = "proto2"; use proto3 as you describe no? https://codereview.chromium.org/2377873002/diff/1/blimp/common/proto/helium.p... blimp/common/proto/helium.proto:14: package blimp.proto; i've noticed other proto files use "package blimp" https://codereview.chromium.org/2377873002/diff/1/blimp/common/proto/helium.p... blimp/common/proto/helium.proto:18: message TransitionalChangeset { rename it to LegacyChangeset? https://codereview.chromium.org/2377873002/diff/1/blimp/common/proto/helium.p... blimp/common/proto/helium.proto:22: message HeliumRevision { Call it HeliumVectorClock so that it matches the c++ impl? https://codereview.chromium.org/2377873002/diff/1/blimp/common/proto/helium.p... blimp/common/proto/helium.proto:23: optional int32 local_revision = 1; uint32 so that matches the VectorClock https://codereview.chromium.org/2377873002/diff/1/blimp/common/proto/helium.p... blimp/common/proto/helium.proto:38: optional HeliumRevision reference_revision = 1; i like calling from/to as it matches the sync_manager definition. but not feeling super strong about this https://codereview.chromium.org/2377873002/diff/1/blimp/common/proto/helium.p... blimp/common/proto/helium.proto:50: optional int32 object_id = 3; change to uint32 for consistency with HeliumObjectId? https://codereview.chromium.org/2377873002/diff/1/blimp/net/BUILD.gn File blimp/net/BUILD.gn (right): https://codereview.chromium.org/2377873002/diff/1/blimp/net/BUILD.gn#newcode55 blimp/net/BUILD.gn:55: "helium/helium_sync_manager.cc", might want to move to source_set("helium") once cl#2372903002 lands https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... File blimp/net/helium/helium_sync_manager.cc (right): https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... blimp/net/helium/helium_sync_manager.cc:15: class ScopedSyncRegistration; unused? https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... blimp/net/helium/helium_sync_manager.cc:19: explicit HeliumSyncManagerImpl(std::unique_ptr<HeliumTransport> transport); Don't you also need the "RunningAs" enum (Client/Server) to know whether you are running as server or client so that you can generate ids differently? https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... blimp/net/helium/helium_sync_manager.cc:38: DCHECK(transport_); change to CHECK? if we don't have transport we are in really bad shape https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... File blimp/net/helium/helium_sync_manager.h (right): https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... blimp/net/helium/helium_sync_manager.h:21: // TODO(kmarshall): Define this type. put bug#? https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... blimp/net/helium/helium_sync_manager.h:60: std::unique_ptr<HeliumTransport> transport); don't we need an api to get the current HeliumSyncManager? The same way notifications services has Create()/current() methods? https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... blimp/net/helium/helium_sync_manager.h:78: virtual void Pause(HeliumObjectId id, bool paused) = 0; move this under the protected section too?
One major question on Helium Message vs Blimp Message wrapping. https://codereview.chromium.org/2377873002/diff/1/blimp/common/proto/BUILD.gn File blimp/common/proto/BUILD.gn (right): https://codereview.chromium.org/2377873002/diff/1/blimp/common/proto/BUILD.gn... blimp/common/proto/BUILD.gn:34: "helium.proto", Could you start a new proto library so it's easier to see what's existing/new? https://codereview.chromium.org/2377873002/diff/1/blimp/common/proto/helium.p... File blimp/common/proto/helium.proto (right): https://codereview.chromium.org/2377873002/diff/1/blimp/common/proto/helium.p... blimp/common/proto/helium.proto:18: message TransitionalChangeset { I am confused. Wouldn't you want to wrap a HeliumMessage inside a BlimpMessage to begin with? Otherwise, how would this even work with HeliumTransport on top of BlimpTransport? i.e. BlimpMessage { oneof { InputFeature; NavigationFeature; ... HeliumFeature; // Wrap Helium message here. } } https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... File blimp/net/helium/helium_sync_manager.h (right): https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... blimp/net/helium/helium_sync_manager.h:59: static std::unique_ptr<HeliumSyncManager> Create( Consider: A member function such as HeliumTransport::CreateSyncManager() instead of a static function in a class that is being created?
Sorry, please disregard helium.proto. I forked from an earlier local experimental branch for this CL but it looks like I didn't prune everything away.
https://codereview.chromium.org/2377873002/diff/1/blimp/common/proto/BUILD.gn File blimp/common/proto/BUILD.gn (right): https://codereview.chromium.org/2377873002/diff/1/blimp/common/proto/BUILD.gn... blimp/common/proto/BUILD.gn:34: "helium.proto", On 2016/09/28 23:31:30, perumaal wrote: > Could you start a new proto library so it's easier to see what's existing/new? Acknowledged. https://codereview.chromium.org/2377873002/diff/1/blimp/net/BUILD.gn File blimp/net/BUILD.gn (right): https://codereview.chromium.org/2377873002/diff/1/blimp/net/BUILD.gn#newcode55 blimp/net/BUILD.gn:55: "helium/helium_sync_manager.cc", On 2016/09/28 17:18:00, scf wrote: > might want to move to source_set("helium") once cl#2372903002 lands Done. https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... File blimp/net/helium/helium_sync_manager.cc (right): https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... blimp/net/helium/helium_sync_manager.cc:15: class ScopedSyncRegistration; On 2016/09/28 17:18:00, scf wrote: > unused? Done. https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... blimp/net/helium/helium_sync_manager.cc:19: explicit HeliumSyncManagerImpl(std::unique_ptr<HeliumTransport> transport); On 2016/09/28 17:18:00, scf wrote: > Don't you also need the "RunningAs" enum (Client/Server) to know whether you are > running as server or client so that you can generate ids differently? Was waiting on the common enums to be landed. https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... blimp/net/helium/helium_sync_manager.cc:38: DCHECK(transport_); On 2016/09/28 17:18:00, scf wrote: > change to CHECK? if we don't have transport we are in really bad shape Coding errors (dev messed up) are typically DCHECKs whereas runtime errors (e.g. resource unavailable) are CHECKs. Null checks are always DCHECKs. https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... File blimp/net/helium/helium_sync_manager.h (right): https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... blimp/net/helium/helium_sync_manager.h:59: static std::unique_ptr<HeliumSyncManager> Create( On 2016/09/28 23:31:30, perumaal wrote: > Consider: A member function such as HeliumTransport::CreateSyncManager() instead > of a static function in a class that is being created? I think that would invert the dependency order. SyncManager will depend on HeliumTransport; HeliumTransport has no idea what SyncManagers are. Having a static Create() method within an interface when there is only one actual impl class is not an uncommon pattern. https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... blimp/net/helium/helium_sync_manager.h:60: std::unique_ptr<HeliumTransport> transport); On 2016/09/28 17:18:00, scf wrote: > don't we need an api to get the current HeliumSyncManager? The same way > notifications services has Create()/current() methods? Which classes would rely on such an accessor? Is there a reason they couldn't be passed a pointer to it directly? https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... blimp/net/helium/helium_sync_manager.h:78: virtual void Pause(HeliumObjectId id, bool paused) = 0; On 2016/09/28 17:18:00, scf wrote: > move this under the protected section too? OK. In the future we might want to make it public so that Objects can control the synchronization of other objects, too - e.g. the Tablist knows that a tab became invisible, so it might want to pause synchronization of said tab. But I can see the wisdom in keeping the interface narrow for now and widening it only as needed.
kmarshall@chromium.org changed reviewers: + wez@chromium.org
https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... File blimp/net/helium/helium_sync_manager.h (right): https://codereview.chromium.org/2377873002/diff/1/blimp/net/helium/helium_syn... blimp/net/helium/helium_sync_manager.h:60: std::unique_ptr<HeliumTransport> transport); On 2016/09/29 00:06:28, Kevin M wrote: > On 2016/09/28 17:18:00, scf wrote: > > don't we need an api to get the current HeliumSyncManager? The same way > > notifications services has Create()/current() methods? > > Which classes would rely on such an accessor? Is there a reason they couldn't be > passed a pointer to it directly? make sense
¡Friendly ping!
lgtm
lgtm https://codereview.chromium.org/2377873002/diff/40001/blimp/net/helium/helium... File blimp/net/helium/helium_sync_manager.h (right): https://codereview.chromium.org/2377873002/diff/40001/blimp/net/helium/helium... blimp/net/helium/helium_sync_manager.h:19: using HeliumObjectId = uint32_t; I with proto allowed typedefs so we could define this at the proto file definition https://codereview.chromium.org/2377873002/diff/40001/blimp/net/helium/helium... blimp/net/helium/helium_sync_manager.h:32: // object from the SyncManager. Who owns the SyncRegistration? Should we add to the TwoPhaseSyncable so that it can enforce destruction?
lgtm
https://codereview.chromium.org/2377873002/diff/40001/blimp/net/helium/helium... File blimp/net/helium/helium_sync_manager.h (right): https://codereview.chromium.org/2377873002/diff/40001/blimp/net/helium/helium... blimp/net/helium/helium_sync_manager.h:32: // object from the SyncManager. On 2016/10/06 17:37:49, scf wrote: > Who owns the SyncRegistration? Should we add to the TwoPhaseSyncable so that it > can enforce destruction? The Object, which is the TwoPhaseSyncable. So yeah, adding a non-virtual SyncRegistration setter to the TwoPhaseSyncable seems like a good idea.
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Initial definition of HeliumSyncManager and SyncRegistration objects. This CL contains an abstract interface for SyncManager and a private class skeleton SyncManagerImpl with the method bodies stubbed out. R=steimel@chromium.org,scf@chromium.org CC=wez@chromium.org BUG=650469 ========== to ========== Initial definition of HeliumSyncManager and SyncRegistration objects. This CL contains an abstract interface for SyncManager and a private class skeleton SyncManagerImpl with the method bodies stubbed out. R=steimel@chromium.org,scf@chromium.org CC=wez@chromium.org BUG=650469 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Initial definition of HeliumSyncManager and SyncRegistration objects. This CL contains an abstract interface for SyncManager and a private class skeleton SyncManagerImpl with the method bodies stubbed out. R=steimel@chromium.org,scf@chromium.org CC=wez@chromium.org BUG=650469 ========== to ========== Initial definition of HeliumSyncManager and SyncRegistration objects. This CL contains an abstract interface for SyncManager and a private class skeleton SyncManagerImpl with the method bodies stubbed out. R=steimel@chromium.org,scf@chromium.org CC=wez@chromium.org BUG=650469 Committed: https://crrev.com/35a16a011800bf25273c58f0c937b0140b55d9bd Cr-Commit-Position: refs/heads/master@{#423660} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/35a16a011800bf25273c58f0c937b0140b55d9bd Cr-Commit-Position: refs/heads/master@{#423660}
Message was sent while issue was closed.
Very late to the game; just a couple of things to follow-up on - otherwise LGTM https://codereview.chromium.org/2377873002/diff/40001/blimp/net/helium/helium... File blimp/net/helium/helium_sync_manager.h (right): https://codereview.chromium.org/2377873002/diff/40001/blimp/net/helium/helium... blimp/net/helium/helium_sync_manager.h:32: // object from the SyncManager. On 2016/10/06 18:41:09, Kevin M wrote: > On 2016/10/06 17:37:49, scf wrote: > > Who owns the SyncRegistration? Should we add to the TwoPhaseSyncable so that > it > > can enforce destruction? > > The Object, which is the TwoPhaseSyncable. So yeah, adding a non-virtual > SyncRegistration setter to the TwoPhaseSyncable seems like a good idea. It would be preferable to keep a clean separation between interfaces and base-classes, though. It would also be good to avoid "polluting" Syncable primitives with knowledge of registration, while still allowing them to be synchronized directly. https://codereview.chromium.org/2377873002/diff/40001/blimp/net/helium/helium... blimp/net/helium/helium_sync_manager.h:60: std::unique_ptr<HeliumTransport> transport); Given that the impl in this CL is a no-op, and that we may well have a SyncManager shim to run on 0.5, I'd suggest omitting this for now and adding it once we have an impl to return. :) https://codereview.chromium.org/2377873002/diff/40001/blimp/net/helium/helium... blimp/net/helium/helium_sync_manager.h:81: virtual void Pause(HeliumObjectId id, bool paused) = 0; Let's leave out stuff like this until we get to implementing it :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
