C++ migration: port watch stream-related part of FSTRemoteStore
#2331
Conversation
var-const
changed the base branch from
master
to
varconst/fst-online-state-tracker
| std::function<void(model::OnlineState)> online_state_handler); | ||
|
|
||
| // TODO(varconst): remove the getters and setters | ||
| id<FSTRemoteSyncer> sync_engine() { |
There was a problem hiding this comment.
These are only necessary for the short transitional period, until the rest of FSTRemoteStore is migrated.
| const model::SnapshotVersion& snapshot_version) override; | ||
| void OnWatchStreamClose(const util::Status& status) override; | ||
|
|
||
| // TODO(varconst): make the following methods private. |
There was a problem hiding this comment.
Similarly, these are only public until the rest of FSTRemoteStore is ported.
|
|
||
| void RemoteStore::StartWatchStream() { | ||
| HARD_ASSERT(ShouldStartWatchStream(), | ||
| "StartWatchStream called when ShouldStartWatchStream: is false."); |
var-const
changed the base branch from
varconst/fst-online-state-tracker
to
master
| @@ -0,0 +1,216 @@ | |||
| /* | |||
There was a problem hiding this comment.
A suggestion (for later PRs): It's impossible to tell which lines in this file were "moved" from FSTRemoteStore, and which have been changed. Separating that into multiple commits (or even multiple PRs) would make reviewing much easier, since your reviewers could just skim the 'move' commit, and focus on the 'changed' stuff. (Especially useful when we're trying to keep the various ports consistent.)
| */ | ||
| class WatchStream : public Stream { | ||
| public: | ||
| WatchStream(util::AsyncQueue* async_queue, | ||
| auth::CredentialsProvider* credentials_provider, | ||
| FSTSerializerBeta* serializer, | ||
| GrpcConnection* grpc_connection, | ||
| id<FSTWatchStreamDelegate> delegate); | ||
| WatchStreamCallback* callback); |
There was a problem hiding this comment.
Consider adding something like the following on the docstring:
@param callback Called when open/change/etc events occur on this WatchStream. Must not be null[*]. Must remain valid for the lifetime of this object.
[*] - Or 'Allowed to be null, in which case, the callbacks won't be invoked'.
(I really only care about the middle sentence; you could omit the other two, as they seem sufficiently obvious.)
Due to obj-c's nil dispatch, you'd get the [*] behaviour for free. We should decide whether we explicitly want this or not.
| @@ -73,7 +73,7 @@ | |||
| } | |||
|
|
|||
| void WatchStream::NotifyStreamOpen() { | |||
| delegate_bridge_.NotifyDelegateOnOpen(); | |||
| callback_->OnWatchStreamOpen(); | |||
There was a problem hiding this comment.
Judging by the implementation, callback_ needs to be notnull. Consider asserting that in the ctor.
| @@ -309,7 +311,7 @@ class RemoteEvent { | |||
| class WatchChangeAggregator { | |||
| public: | |||
| explicit WatchChangeAggregator( | |||
| id<FSTTargetMetadataProvider> target_metadata_provider) | |||
| TargetMetadataProvider* target_metadata_provider) | |||
There was a problem hiding this comment.
Similar to my other comment, consider making nullability explicit here. (i.e. docstring and notnull assertion if relevant.)
(Similarly here, you've technically changed the behaviour, since obj-c is quite happy to allow nils without segfaulting. But again, you're probably now more in line with the other ports, so this is entirely fine.)
Update: there's a bunch of other functions that just pipe this value through. Adding docstrings to those functions too might have some value in a public api, but personally, I think that would just be noise for internal apis. You could even omit the docstring here... the assertion itself would be a stronger form of documentation. :)
* master: (27 commits) Pass FSTMutations using a vector (#2357) Update CI to use CocoaPods 1.6.0 (#2360) Add NS_ASSUME_NONNULL_NOTATION for game center sign in (#2359) C++ migration: make all methods of `FSTRemoteStore` delegate to C++ (#2337) C++ migration: port write stream-related part of `FSTRemoteStore` (#2335) Resolve hard dependency of GameKit (#2355) Update gRPC certificate bundles locations for Firebase 5.16 (#2353) Start pod lib lint CI for IAM (#2347) Update library name to `fire-fiam`. (#2352) Bump FirebaseAnalyticsInterop version (#2315) Add IAM headless to CI (#2341) C++ migration: port watch stream-related part of `FSTRemoteStore` (#2331) Open source FIAM headless SDK (#2312) Port flaky test fix from web. (#2332) Make scripts/style.sh compatible with newer swiftformat versions (0.38) (#2334) C++ migration: port `FSTOnlineStateTracker` (#2325) Don't build fuzz tests target on Travis (#2330) Remove FSTMutationQueue (#2319) C++ migration: port `FSTRemoteEvent` (#2320) C++ migration: port `FSTTargetChange` (#2318) ...
FSTRemoteStoreis quite large, so to make the PRs more reviewable, I decided to have an intermediate state where only some of the methods are delegated to the C++ implementation.