Addressing comemnts #1

Author: wu-hui

Firestore/Example/Tests/Integration/FSTDatastoreTests.mm

@@ -168,27 +168,27 @@ void ExpectListenEvent(NSString *description) {
     [underlying_capture_ expectListenEventWithDescription:description];
   }
 
-  void ApplyRemoteEvent(const remote::RemoteEvent &remote_event) override {
+  void ApplyRemoteEvent(const RemoteEvent &remote_event) override {
     [underlying_capture_ applyRemoteEvent:remote_event];
   }
 
-  void HandleRejectedListen(model::TargetId target_id, util::Status error) override {
+  void HandleRejectedListen(TargetId target_id, Status error) override {
     [underlying_capture_ rejectListenWithTargetID:target_id error:error.ToNSError()];
   }
 
-  void HandleSuccessfulWrite(const model::MutationBatchResult &batch_result) override {
+  void HandleSuccessfulWrite(const MutationBatchResult &batch_result) override {
     [underlying_capture_ applySuccessfulWriteWithResult:batch_result];
   }
 
-  void HandleRejectedWrite(model::BatchId batch_id, util::Status error) override {
+  void HandleRejectedWrite(BatchId batch_id, Status error) override {
     [underlying_capture_ rejectFailedWriteWithBatchID:batch_id error:error.ToNSError()];
   }
 
-  void HandleOnlineStateChange(model::OnlineState online_state) override {
+  void HandleOnlineStateChange(OnlineState online_state) override {
     HARD_FAIL("Not implemented");
   }
 
-  model::DocumentKeySet GetRemoteKeys(model::TargetId target_id) const override {
+  model::DocumentKeySet GetRemoteKeys(TargetId target_id) const override {
     return [underlying_capture_ remoteKeysForTarget:target_id];
   }
 

Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.mm

@@ -200,10 +200,8 @@ - (instancetype)initWithPersistence:(id<FSTPersistence>)persistence
     _datastore = std::make_shared<MockDatastore>(_databaseInfo, _workerQueue,
                                                  std::make_shared<EmptyCredentialsProvider>());
     _remoteStore = absl::make_unique<RemoteStore>(
-        _localStore, _datastore, _workerQueue, [self](OnlineState onlineState) {
-          _syncEngine->HandleOnlineStateChange(onlineState);
-          // _eventManager->HandleOnlineStateChange(onlineState);
-        });
+        _localStore, _datastore, _workerQueue,
+        [self](OnlineState onlineState) { _syncEngine->HandleOnlineStateChange(onlineState); });
     ;
 
     _syncEngine = absl::make_unique<SyncEngine>(_localStore, _remoteStore.get(), initialUser);

Firestore/core/src/firebase/firestore/core/event_manager.h

@@ -97,7 +97,7 @@ class EventManager : public QueryEventCallback {
     absl::optional<ViewSnapshot> snapshot_;
   };
 
-  QueryEventSource* query_event_source_;
+  QueryEventSource* query_event_source_ = nullptr;
   model::OnlineState online_state_ = model::OnlineState::Unknown;
   std::unordered_map<core::Query, QueryListenersInfo> queries_;
 };

Firestore/core/src/firebase/firestore/core/firestore_client.h

@@ -17,6 +17,10 @@
 #ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_CORE_FIRESTORE_CLIENT_H_
 #define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_CORE_FIRESTORE_CLIENT_H_
 
+#if !defined(__OBJC__)
+#error "This header only supports Objective-C++"
+#endif  // !defined(__OBJC__)
+
 #import <Foundation/Foundation.h>
 
 #include <memory>
@@ -48,15 +52,11 @@ namespace firebase {
 namespace firestore {
 
 namespace remote {
-// Forward declaration to make CMAKE build pass as it does not support
-// Objective-C protos.
 class RemoteStore;
 }  // namespace remote
 
 namespace core {
 
-// Forward declaration to make CMAKE build pass as it does not support
-// Objective-C protos.
 class EventManager;
 class SyncEngine;
 

Firestore/core/src/firebase/firestore/core/firestore_client.mm

@@ -429,7 +429,8 @@ QuerySnapshot result(query.firestore(), query.query(), std::move(snapshot),
           std::move(mutations), [callback, shared_this](Status error) {
             // Dispatch the result back onto the user dispatch queue.
             if (callback) {
-              shared_this->user_executor()->Execute([=] { callback(error); });
+              shared_this->user_executor()->Execute(
+                  [=] { callback(std::move(error)); });
             }
           });
     }

Firestore/core/src/firebase/firestore/core/sync_engine.h

@@ -52,24 +52,28 @@ namespace core {
  */
 class QueryEventCallback {
  public:
+  virtual ~QueryEventCallback() = default;
+
   /** Handles a change in online state. */
   virtual void HandleOnlineStateChange(model::OnlineState online_state) = 0;
   /** Handles new view snapshots. */
   virtual void OnViewSnapshots(std::vector<core::ViewSnapshot>&& snapshots) = 0;
   /** Handles the failure of a query. */
   virtual void OnError(const core::Query& query, util::Status error) = 0;
-
-  virtual ~QueryEventCallback() = default;
 };
 
 /**
- * Interface implemented by `SyncEngine` receive requests from `EventManager`.
+ * Interface implemented by `SyncEngine` to receive requests from
+ * `EventManager`.
  */
 class QueryEventSource {
  public:
+  virtual ~QueryEventSource() = default;
+
   virtual void SetCallback(QueryEventCallback* callback) = 0;
+
   /**
-   * Initiates a new listen. The FSTLocalStore will be queried for initial data
+   * Initiates a new listen. The LocalStore will be queried for initial data
    * and the listen will be sent to the `RemoteStore` to get remote data. The
    * registered SyncEngineCallback will be notified of resulting view
    * snapshots and/or listen errors.
@@ -80,8 +84,6 @@ class QueryEventSource {
 
   /** Stops listening to a query previously listened to via `Listen`. */
   virtual void StopListening(const Query& query) = 0;
-
-  virtual ~QueryEventSource() = default;
 };
 
 /**

Firestore/core/src/firebase/firestore/remote/remote_store.h

@@ -56,11 +56,12 @@ class RemoteStoreCallback {
    * visible because of the snapshot version the remote event contains.
    */
   virtual void ApplyRemoteEvent(const RemoteEvent& remote_event) = 0;
+
   /**
    * Handles rejection of listen for the given targetId. This can be triggered
    * by the backend for any active target.
    *
-   * @param target_id The targetId corresponding to a listen initiated via
+   * @param target_id The target ID corresponding to a listen initiated via
    * RemoteStore::Listen()
    * @param error A description of the condition that has forced the rejection.
    * Nearly always this will be an indication that the user is no longer
@@ -85,8 +86,10 @@ class RemoteStoreCallback {
   virtual void HandleRejectedWrite(model::BatchId batch_id,
                                    util::Status error) = 0;
 
-  /** Applies an OnlineState change to the sync engine and notifies any views of
-   * the change. */
+  /**
+   * Applies an OnlineState change to the sync engine and notifies any views of
+   * the change.
+   */
   virtual void HandleOnlineStateChange(model::OnlineState online_state) = 0;
 
   /**

Firestore/core/src/firebase/firestore/remote/remote_store.mm

@@ -326,9 +326,7 @@ QueryData request_query_data(query_data.query(), target_id,
   }
 
   // Finally handle remote event
-  if (sync_engine_) {
-    sync_engine_->ApplyRemoteEvent(remote_event);
-  }
+  sync_engine_->ApplyRemoteEvent(remote_event);
 }
 
 void RemoteStore::ProcessTargetError(const WatchTargetChange& change) {
@@ -340,9 +338,7 @@ QueryData request_query_data(query_data.query(), target_id,
     if (found != listen_targets_.end()) {
       listen_targets_.erase(found);
       watch_change_aggregator_->RemoveTarget(target_id);
-      if (sync_engine_) {
-        sync_engine_->HandleRejectedListen(target_id, change.cause());
-      }
+      sync_engine_->HandleRejectedListen(target_id, change.cause());
     }
   }
 }
@@ -424,9 +420,7 @@ QueryData request_query_data(query_data.query(), target_id,
   MutationBatchResult batch_result(std::move(batch), commit_version,
                                    std::move(mutation_results),
                                    write_stream_->GetLastStreamToken());
-  if (sync_engine_) {
-    sync_engine_->HandleSuccessfulWrite(batch_result);
-  }
+  sync_engine_->HandleSuccessfulWrite(batch_result);
 
   // It's possible that with the completion of this mutation another slot has
   // freed up.
@@ -502,9 +496,7 @@ MutationBatchResult batch_result(std::move(batch), commit_version,
   // down--this was just a bad request so inhibit backoff on the next restart.
   write_stream_->InhibitBackoff();
 
-  if (sync_engine_) {
-    sync_engine_->HandleRejectedWrite(batch.batch_id(), status);
-  }
+  sync_engine_->HandleRejectedWrite(batch.batch_id(), status);
 
   // It's possible that with the completion of this mutation another slot has
   // freed up.
@@ -522,10 +514,7 @@ MutationBatchResult batch_result(std::move(batch), commit_version,
 }
 
 DocumentKeySet RemoteStore::GetRemoteKeysForTarget(TargetId target_id) const {
-  if (sync_engine_) {
-    return sync_engine_->GetRemoteKeys(target_id);
-  }
-  return DocumentKeySet{};
+  return sync_engine_->GetRemoteKeys(target_id);
 }
 
 absl::optional<QueryData> RemoteStore::GetQueryDataForTarget(

Firestore/core/test/firebase/firestore/core/event_manager_test.mm

@@ -29,20 +29,20 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
-using firebase::firestore::core::EventListener;
-using firebase::firestore::core::EventManager;
-using firebase::firestore::core::ListenOptions;
-using firebase::firestore::core::QueryListener;
-using firebase::firestore::core::QueryEventCallback;
-using firebase::firestore::core::ViewSnapshot;
-using firebase::firestore::model::DocumentKeySet;
-using firebase::firestore::model::DocumentSet;
-using firebase::firestore::model::OnlineState;
-using firebase::firestore::util::StatusOr;
-using firebase::firestore::util::StatusOrCallback;
-using firebase::firestore::testutil::Query;
+namespace firebase {
+namespace firestore {
+namespace core {
+namespace {
+
+using model::DocumentKeySet;
+using model::DocumentSet;
+using model::OnlineState;
+using util::StatusOr;
+using util::StatusOrCallback;
+using testutil::Query;
 using testing::_;
 using testing::ElementsAre;
+using testing::StrictMock;
 
 ViewSnapshot::Listener NoopViewSnapshotHandler() {
   return EventListener<ViewSnapshot>::Create(
@@ -67,17 +67,15 @@
   auto listener1 = NoopQueryListener(query);
   auto listener2 = NoopQueryListener(query);
 
-  MockEventSource mock_event_source;
+  StrictMock<MockEventSource> mock_event_source;
   EXPECT_CALL(mock_event_source, SetCallback(_));
   EventManager event_manager(&mock_event_source);
 
   EXPECT_CALL(mock_event_source, Listen(query));
   event_manager.AddQueryListener(listener1);
 
-  EXPECT_CALL(mock_event_source, Listen(_)).Times(0);
+  // Expecting no activity from mock_event_source.
   event_manager.AddQueryListener(listener2);
-
-  EXPECT_CALL(mock_event_source, StopListening(_)).Times(0);
   event_manager.RemoveQueryListener(listener2);
 
   EXPECT_CALL(mock_event_source, StopListening(query));
@@ -112,33 +110,34 @@ ViewSnapshot make_empty_view_snapshot(const core::Query& query) {
 TEST(EventManagerTest, NotifiesListenersInTheRightOrder) {
   core::Query query1 = Query("foo/bar");
   core::Query query2 = Query("bar/baz");
-  std::vector<std::string> eventOrder;
-
-  auto listener1 =
-      QueryListener::Create(query1, [&eventOrder](StatusOr<ViewSnapshot>) {
-        eventOrder.push_back("listener1");
-      });
-  auto listener2 =
-      QueryListener::Create(query2, [&eventOrder](StatusOr<ViewSnapshot>) {
-        eventOrder.push_back("listener2");
-      });
-  auto listener3 =
-      QueryListener::Create(query1, [&eventOrder](StatusOr<ViewSnapshot>) {
-        eventOrder.push_back("listener3");
-      });
+  std::vector<std::string> event_order;
+
+  auto listener1 = QueryListener::Create(query1, [&](StatusOr<ViewSnapshot>) {
+    event_order.push_back("listener1");
+  });
+  auto listener2 = QueryListener::Create(query2, [&](StatusOr<ViewSnapshot>) {
+    event_order.push_back("listener2");
+  });
+  auto listener3 = QueryListener::Create(query1, [&](StatusOr<ViewSnapshot>) {
+    event_order.push_back("listener3");
+  });
 
   MockEventSource mock_event_source;
   EventManager event_manager(&mock_event_source);
 
+  EXPECT_CALL(mock_event_source, Listen(query1));
   event_manager.AddQueryListener(listener1);
+
+  EXPECT_CALL(mock_event_source, Listen(query2));
   event_manager.AddQueryListener(listener2);
+
   event_manager.AddQueryListener(listener3);
 
   ViewSnapshot snapshot1 = make_empty_view_snapshot(query1);
   ViewSnapshot snapshot2 = make_empty_view_snapshot(query2);
   event_manager.OnViewSnapshots({snapshot1, snapshot2});
 
-  ASSERT_THAT(eventOrder, ElementsAre("listener1", "listener3", "listener2"));
+  ASSERT_THAT(event_order, ElementsAre("listener1", "listener3", "listener2"));
 }
 
 TEST(EventManagerTest, WillForwardOnlineStateChanges) {
@@ -171,3 +170,8 @@ void OnOnlineStateChanged(OnlineState online_state) override {
   ASSERT_THAT(fake_listener->events,
               ElementsAre(OnlineState::Unknown, OnlineState::Online));
 }
+
+}  // namespace
+}  // namespace core
+}  // namespace firestore
+}  // namespace firebase