Merge remote-tracking branch 'origin/master' into ghm

a-maurice · a-maurice · commit 9a874a7b3154 · 2020-06-12T16:23:25.000-07:00
diff --git a/app/src/include/firebase/internal/future_impl.h b/app/src/include/firebase/internal/future_impl.h
@@ -204,6 +204,8 @@ inline FutureBase::FutureBase(detail::FutureApiInterface* api,
                               const FutureHandle& handle)
     : api_(api), handle_(handle) {
   api_->ReferenceFuture(handle_);
+  // Once the FutureBase has reference, we don't need extra handle reference.
+  handle_.Detach();
   detail::RegisterForCleanup(api_, this);
 }
 
diff --git a/app/src/reference_counted_future_impl.cc b/app/src/reference_counted_future_impl.cc
@@ -701,10 +701,9 @@ bool ReferenceCountedFutureImpl::IsReferencedExternally() const {
   }
   for (int i = 0; i < last_results_.size(); i++) {
     if (last_results_[i].status() != kFutureStatusInvalid) {
-      // If the status is not invalid, this entry is using up 2 references,
-      // one for the Future and one for the FutureHandle it holds.
+      // If the status is not invalid, this entry is using up a reference.
       // Count up the internal references.
-      internal_references += 2;
+      internal_references++;
     }
   }
   // If there are more references than the internal ones, someone is holding
diff --git a/cmake/external/firestore.cmake b/cmake/external/firestore.cmake
@@ -18,7 +18,11 @@ if(TARGET firestore)
   return()
 endif()
 
-set(version Firestore-1.13.0)
+# Pin to the first revision that includes these changes:
+# https://github.com/firebase/firebase-ios-sdk/pull/5807.
+#
+# This should be released with the next version after Firestore-1.15.0.
+set(version 05026f3df92cdee7b91fccc72a7195be2618acdf)
 
 ExternalProject_Add(
   firestore
diff --git a/firestore/src/android/document_reference_android.h b/firestore/src/android/document_reference_android.h
@@ -167,7 +167,7 @@ class DocumentReferenceInternal
    * only DocumentSnapshot.getMetadata() changed) should trigger snapshot
    * events.
    * @param[in] callback function or lambda to call. When this function is
-   * called, snapshot value is valid if and only if error is Error::kOk.
+   * called, snapshot value is valid if and only if error is Error::kErrorOk.
    *
    * @return A registration object that can be used to remove the listener.
    *
diff --git a/firestore/src/android/event_listener_android.cc b/firestore/src/android/event_listener_android.cc
@@ -57,7 +57,7 @@ void EventListenerInternal::DocumentEventListenerNativeOnEvent(
       reinterpret_cast<EventListener<DocumentSnapshot>*>(listener_ptr);
   Error error_code =
       FirebaseFirestoreExceptionInternal::ToErrorCode(env, error);
-  if (error_code != Error::kOk) {
+  if (error_code != Error::kErrorOk) {
     listener->OnEvent(DocumentSnapshot{}, error_code);
     return;
   }
@@ -79,7 +79,7 @@ void EventListenerInternal::QueryEventListenerNativeOnEvent(
       reinterpret_cast<EventListener<QuerySnapshot>*>(listener_ptr);
   Error error_code =
       FirebaseFirestoreExceptionInternal::ToErrorCode(env, error);
-  if (error_code != Error::kOk) {
+  if (error_code != Error::kErrorOk) {
     listener->OnEvent(QuerySnapshot{}, error_code);
     return;
   }
@@ -100,7 +100,7 @@ void EventListenerInternal::VoidEventListenerNativeOnEvent(JNIEnv* env,
   EventListener<void>* listener =
       reinterpret_cast<EventListener<void>*>(listener_ptr);
 
-  listener->OnEvent(Error::kOk);
+  listener->OnEvent(Error::kErrorOk);
 }
 
 /* static */
diff --git a/firestore/src/android/firebase_firestore_exception_android.cc b/firestore/src/android/firebase_firestore_exception_android.cc
@@ -51,16 +51,16 @@ METHOD_LOOKUP_DEFINITION(illegal_state_exception,
 Error FirebaseFirestoreExceptionInternal::ToErrorCode(JNIEnv* env,
                                                       jobject exception) {
   if (exception == nullptr) {
-    return Error::kOk;
+    return Error::kErrorOk;
   }
 
   // Some of the Precondition failure is thrown as IllegalStateException instead
   // of a FirebaseFirestoreException. So we convert them into a more meaningful
   // code.
   if (env->IsInstanceOf(exception, illegal_state_exception::GetClass())) {
-    return Error::kFailedPrecondition;
+    return Error::kErrorFailedPrecondition;
   } else if (!IsInstance(env, exception)) {
-    return Error::kUnknown;
+    return Error::kErrorUnknown;
   }
 
   jobject code = env->CallObjectMethod(
@@ -72,8 +72,9 @@ Error FirebaseFirestoreExceptionInternal::ToErrorCode(JNIEnv* env,
   env->DeleteLocalRef(code);
   CheckAndClearJniExceptions(env);
 
-  if (code_value > Error::kUnauthenticated || code_value < Error::kOk) {
-    return Error::kUnknown;
+  if (code_value > Error::kErrorUnauthenticated ||
+      code_value < Error::kErrorOk) {
+    return Error::kErrorUnknown;
   }
   return static_cast<Error>(code_value);
 }
@@ -87,7 +88,7 @@ std::string FirebaseFirestoreExceptionInternal::ToString(JNIEnv* env,
 /* static */
 jthrowable FirebaseFirestoreExceptionInternal::ToException(
     JNIEnv* env, Error code, const char* message) {
-  if (code == Error::kOk) {
+  if (code == Error::kErrorOk) {
     return nullptr;
   }
   // FirebaseFirestoreException requires message to be non-empty. If the caller
diff --git a/firestore/src/android/promise_android.h b/firestore/src/android/promise_android.h
@@ -86,18 +86,18 @@ class Promise {
         return;
       }
 
-      Error error_code = Error::kUnknown;
+      Error error_code = Error::kErrorUnknown;
       switch (result_code) {
         case util::kFutureResultFailure:
           // When failed, result is the exception raised.
           error_code = FirebaseFirestoreExceptionInternal::ToErrorCode(
               this->firestore_->app()->GetJNIEnv(), result);
           break;
         case util::kFutureResultCancelled:
-          error_code = Error::kCancelled;
+          error_code = Error::kErrorCancelled;
           break;
         default:
-          error_code = Error::kUnknown;
+          error_code = Error::kErrorUnknown;
           FIREBASE_ASSERT_MESSAGE(false, "unknown FutureResult %d",
                                   result_code);
           break;
@@ -130,10 +130,10 @@ class Promise {
       PublicT future_result = FirestoreInternal::Wrap<InternalT>(
           new InternalT(this->firestore_, result));
 
-      this->impl_->CompleteWithResult(this->handle_, Error::kOk,
+      this->impl_->CompleteWithResult(this->handle_, Error::kErrorOk,
                                       /*error_msg=*/"", future_result);
       if (this->completion_ != nullptr) {
-        this->completion_->CompleteWith(Error::kOk, /*error_message*/ "",
+        this->completion_->CompleteWith(Error::kErrorOk, /*error_message*/ "",
                                         &future_result);
       }
       delete this;
@@ -146,9 +146,9 @@ class Promise {
     using CompleterBase<void>::CompleterBase;
 
     void SucceedWithResult(jobject result) override {
-      this->impl_->Complete(this->handle_, Error::kOk, /*error_msg=*/"");
+      this->impl_->Complete(this->handle_, Error::kErrorOk, /*error_msg=*/"");
       if (this->completion_ != nullptr) {
-        this->completion_->CompleteWith(Error::kOk, /*error_message*/ "",
+        this->completion_->CompleteWith(Error::kErrorOk, /*error_message*/ "",
                                         nullptr);
       }
       delete this;
diff --git a/firestore/src/android/transaction_android.cc b/firestore/src/android/transaction_android.cc
@@ -142,7 +142,7 @@ DocumentSnapshot TransactionInternal::Get(const DocumentReference& document,
     return DocumentSnapshot{};
   } else {
     if (error_code != nullptr) {
-      *error_code = Error::kOk;
+      *error_code = Error::kErrorOk;
     }
     if (error_message != nullptr) {
       *error_message = "";
diff --git a/firestore/src/common/futures.h b/firestore/src/common/futures.h
@@ -18,7 +18,7 @@ Future<T> CreateFailedFuture(
     ReferenceCountedFutureImpl* ref_counted_future_impl) {
   SafeFutureHandle<T> handle = ref_counted_future_impl->SafeAlloc<T>();
   ref_counted_future_impl->Complete(
-      handle, Error::kFailedPrecondition,
+      handle, Error::kErrorFailedPrecondition,
       "This instance is in an invalid state. This could either because the "
       "underlying Firestore instance has been destructed or because you're "
       "running on an unsupported platform. Currently the Firestore C++/Unity "
diff --git a/firestore/src/include/firebase/csharp/snapshots_in_sync_listener.cc b/firestore/src/include/firebase/csharp/snapshots_in_sync_listener.cc
@@ -18,7 +18,7 @@ ListenerRegistration SnapshotsInSyncListener::AddListenerTo(
   SnapshotsInSyncListener listener(callback_id, callback);
 
   return firestore->AddSnapshotsInSyncListener(
-      [listener]() mutable { listener.OnEvent(Error::kOk); });
+      [listener]() mutable { listener.OnEvent(Error::kErrorOk); });
 }
 
 }  // namespace csharp
diff --git a/firestore/src/include/firebase/csharp/typemap_helper.h b/firestore/src/include/firebase/csharp/typemap_helper.h
@@ -18,7 +18,7 @@ namespace csharp {
 
 struct TransactionGetResult {
   DocumentSnapshot snapshot;
-  Error error_code = Error::kUnknown;
+  Error error_code = Error::kErrorUnknown;
   std::string error_message;
 };
 
diff --git a/firestore/src/include/firebase/csharp/unity_transaction_function.cc b/firestore/src/include/firebase/csharp/unity_transaction_function.cc
@@ -19,7 +19,7 @@ Error UnityTransactionFunction::Apply(Transaction& transaction,
   } else {
     FIREBASE_ASSERT_MESSAGE(
         false, "C++ transaction callback called before C# registered.");
-    return Error::kOk;
+    return Error::kErrorOk;
   }
 }
 
diff --git a/firestore/src/include/firebase/firestore/document_reference.h b/firestore/src/include/firebase/firestore/document_reference.h
@@ -256,7 +256,7 @@ class DocumentReference {
    * DocumentReference.
    *
    * @param[in] callback The std::function to call. When this function is
-   * called, snapshot value is valid if and only if error is Error::kOk.
+   * called, snapshot value is valid if and only if error is Error::kErrorOk.
    *
    * @return A registration object that can be used to remove the listener.
    *
@@ -274,7 +274,7 @@ class DocumentReference {
    * is, only DocumentSnapshot::metadata() changed) should trigger snapshot
    * events.
    * @param[in] callback The std::function to call. When this function is
-   * called, snapshot value is valid if and only if error is Error::kOk.
+   * called, snapshot value is valid if and only if error is Error::kErrorOk.
    *
    * @return A registration object that can be used to remove the listener.
    *
diff --git a/firestore/src/include/firebase/firestore/event_listener.h b/firestore/src/include/firebase/firestore/event_listener.h
@@ -41,10 +41,11 @@ class EventListener {
    * @brief OnEvent will be called with the new value or the error if an error
    * occurred.
    *
-   * It's guaranteed that value is valid if and only if error is Error::kOk.
+   * It's guaranteed that value is valid if and only if error is
+   * Error::kErrorOk.
    *
    * @param value The value of the event. Invalid if there was an error.
-   * @param error The error if there was error. Error::kOk otherwise.
+   * @param error The error if there was error. Error::kErrorOk otherwise.
    */
   virtual void OnEvent(const T& value, Error error) = 0;
 };
@@ -67,7 +68,7 @@ class EventListener<void> {
   /**
    * @brief OnEvent will be called with the error if an error occurred.
    *
-   * @param error The error if there was error. Error::kOk otherwise.
+   * @param error The error if there was error. Error::kErrorOk otherwise.
    */
   virtual void OnEvent(Error error) = 0;
 };
diff --git a/firestore/src/include/firebase/firestore/query.h b/firestore/src/include/firebase/firestore/query.h
@@ -530,7 +530,7 @@ class Query {
    * query.
    *
    * @param[in] callback The std::function to call. When this function is
-   * called, snapshot value is valid if and only if error is Error::kOk.
+   * called, snapshot value is valid if and only if error is Error::kErrorOk.
    *
    * @return A registration object that can be used to remove the listener.
    *
@@ -548,7 +548,7 @@ class Query {
    * is, only DocumentSnapshot::metadata() changed) should trigger snapshot
    * events.
    * @param[in] callback The std::function to call. When this function is
-   * called, snapshot value is valid if and only if error is Error::kOk.
+   * called, snapshot value is valid if and only if error is Error::kErrorOk.
    *
    * @return A registration object that can be used to remove the listener.
    *
diff --git a/firestore/src/include/firebase/firestore/transaction.h b/firestore/src/include/firebase/firestore/transaction.h
@@ -150,8 +150,8 @@ class TransactionFunction {
    *
    * @param[in] transaction The transaction to run this function with.
    * @param[out] error_message You can set error message with this parameter.
-   * @return Either Error::kOk if successful or the error code from Error that
-   * most closely matches the failure.
+   * @return Either Error::kErrorOk if successful or the error code from Error
+   * that most closely matches the failure.
    */
   virtual Error Apply(Transaction& transaction, std::string& error_message) = 0;
 };
diff --git a/firestore/src/ios/credentials_provider_ios.cc b/firestore/src/ios/credentials_provider_ios.cc
@@ -29,7 +29,7 @@ User GetCurrentUser(Auth* firebase_auth) {
 
 StatusOr<Token> ConvertToken(const Future<std::string>& future,
                              Auth* firebase_auth) {
-  if (future.error() != Error::kOk) {
+  if (future.error() != Error::kErrorOk) {
     return Status(static_cast<Error>(future.error()), future.error_message());
   }
 
@@ -51,8 +51,8 @@ void OnToken(const Future<std::string>& future_token, Auth* firebase_auth,
     // Cancel the request since the user may have changed while the request was
     // outstanding, so the response is likely for a previous user (which user,
     // we can't be sure).
-    listener(
-        Status(Error::kAborted, "GetToken() aborted due to token change."));
+    listener(Status(Error::kErrorAborted,
+                    "GetToken() aborted due to token change."));
     return;
   }
 
diff --git a/firestore/src/ios/firestore_ios.cc b/firestore/src/ios/firestore_ios.cc
@@ -60,10 +60,13 @@ FirestoreInternal::FirestoreInternal(
 }
 
 FirestoreInternal::~FirestoreInternal() {
-  std::lock_guard<std::mutex> lock(listeners_mutex_);
-  HARD_ASSERT_IOS(listeners_.empty(),
-                  "Expected all listeners to be unregistered by the time "
-                  "FirestoreInternal is destroyed.");
+  {
+    std::lock_guard<std::mutex> lock(listeners_mutex_);
+    HARD_ASSERT_IOS(listeners_.empty(),
+                    "Expected all listeners to be unregistered by the time "
+                    "FirestoreInternal is destroyed.");
+  }
+  firestore_core_->Dispose();
 }
 
 std::shared_ptr<api::Firestore> FirestoreInternal::CreateFirestore(
@@ -153,7 +156,7 @@ Future<void> FirestoreInternal::RunTransaction(
               Transaction transaction{transaction_internal};
 
               Error error_code = update(transaction, error_message);
-              if (error_code == Error::kOk) {
+              if (error_code == Error::kErrorOk) {
                 eventual_result_callback(Status::OK());
               } else {
                 // TODO(varconst): port this from iOS
@@ -252,7 +255,7 @@ void FirestoreInternal::ClearListeners() {
 ListenerRegistration FirestoreInternal::AddSnapshotsInSyncListener(
     EventListener<void>* listener) {
   std::function<void()> listener_function = [listener] {
-    listener->OnEvent(Error::kOk);
+    listener->OnEvent(Error::kErrorOk);
   };
   auto result = firestore_core_->AddSnapshotsInSyncListener(
       ListenerWithCallback(std::move(listener_function)));
diff --git a/firestore/src/ios/listener_ios.h b/firestore/src/ios/listener_ios.h
@@ -35,8 +35,9 @@ std::unique_ptr<core::EventListener<From>> ListenerWithPromise(
 }
 
 // Creates an `EventListener` that will:
-// - invoke the given `callback` with either a valid value and `Error::kOk`, or
-//   a default-constructed value and an error indicating the failure;
+// - invoke the given `callback` with either a valid value and
+//   `Error::kErrorOk`, or a default-constructed value and an error indicating
+//   the failure;
 // - convert the Core API value given to it upon invocation (`From`) into
 //   a public API type (`To`).
 template <typename From, typename To>
@@ -47,7 +48,7 @@ std::unique_ptr<core::EventListener<From>> ListenerWithCallback(
         if (maybe_value.ok()) {
           From from = std::move(maybe_value).ValueOrDie();
           To to = MakePublic(std::move(from));
-          callback(std::move(to), Error::kOk);
+          callback(std::move(to), Error::kErrorOk);
 
         } else {
           callback(To{}, maybe_value.status().code());
diff --git a/firestore/src/ios/promise_ios.h b/firestore/src/ios/promise_ios.h
@@ -147,7 +147,7 @@ class Promise {
  private:
   Promise() = default;
 
-  int NoError() const { return static_cast<int>(Error::kOk); }
+  int NoError() const { return static_cast<int>(Error::kErrorOk); }
 
   // Note: `CleanupFn` is not used because `Promise` is a header-only class, to
   // avoid a circular dependency between headers.
diff --git a/firestore/src/ios/transaction_ios.cc b/firestore/src/ios/transaction_ios.cc
@@ -134,7 +134,7 @@ DocumentSnapshot TransactionInternal::Get(const DocumentReference& document,
 
   if (result.ok()) {
     if (error_code != nullptr) {
-      *error_code = Error::kOk;
+      *error_code = Error::kErrorOk;
     }
     if (error_message != nullptr) {
       *error_message = "";
diff --git a/firestore/src/tests/util/event_accumulator.h b/firestore/src/tests/util/event_accumulator.h
@@ -17,7 +17,7 @@ class EventAccumulator {
   std::vector<T> Await(int num_events) {
     max_events_ += num_events;
     FirestoreIntegrationTest::Await(listener_, max_events_);
-    EXPECT_EQ(Error::kOk, listener_.first_error());
+    EXPECT_EQ(Error::kErrorOk, listener_.first_error());
 
     std::vector<T> result;
     // We cannot use listener.last_result() as it goes backward and can

Original file line number	Diff line number	Diff line change
`@@ -204,6 +204,8 @@ inline FutureBase::FutureBase(detail::FutureApiInterface* api,`
`204`	`204`	`const FutureHandle& handle)`
`205`	`205`	`: api_(api), handle_(handle) {`
`206`	`206`	`api_->ReferenceFuture(handle_);`
	`207`	`+ // Once the FutureBase has reference, we don't need extra handle reference.`
	`208`	`+ handle_.Detach();`
`207`	`209`	`detail::RegisterForCleanup(api_, this);`
`208`	`210`	`}`
`209`	`211`
Original file line number	Diff line number	Diff line change
`@@ -701,10 +701,9 @@ bool ReferenceCountedFutureImpl::IsReferencedExternally() const {`
`701`	`701`	`}`
`702`	`702`	`for (int i = 0; i < last_results_.size(); i++) {`
`703`	`703`	`if (last_results_[i].status() != kFutureStatusInvalid) {`
`704`		`- // If the status is not invalid, this entry is using up 2 references,`
`705`		`- // one for the Future and one for the FutureHandle it holds.`
	`704`	`+ // If the status is not invalid, this entry is using up a reference.`
`706`	`705`	`// Count up the internal references.`
`707`		`- internal_references += 2;`
	`706`	`+ internal_references++;`
`708`	`707`	`}`
`709`	`708`	`}`
`710`	`709`	`// If there are more references than the internal ones, someone is holding`
Original file line number	Diff line number	Diff line change
`@@ -167,7 +167,7 @@ class DocumentReferenceInternal`
`167`	`167`	`* only DocumentSnapshot.getMetadata() changed) should trigger snapshot`
`168`	`168`	`* events.`
`169`	`169`	`* @param[in] callback function or lambda to call. When this function is`
`170`		`- * called, snapshot value is valid if and only if error is Error::kOk.`
	`170`	`+ * called, snapshot value is valid if and only if error is Error::kErrorOk.`
`171`	`171`	`*`
`172`	`172`	`* @return A registration object that can be used to remove the listener.`
`173`	`173`	`*`
Original file line number	Diff line number	Diff line change
`@@ -142,7 +142,7 @@ DocumentSnapshot TransactionInternal::Get(const DocumentReference& document,`
`142`	`142`	`return DocumentSnapshot{};`
`143`	`143`	`} else {`
`144`	`144`	`if (error_code != nullptr) {`
`145`		`- *error_code = Error::kOk;`
	`145`	`+ *error_code = Error::kErrorOk;`
`146`	`146`	`}`
`147`	`147`	`if (error_message != nullptr) {`
`148`	`148`	`*error_message = "";`
Original file line number	Diff line number	Diff line change
`@@ -18,7 +18,7 @@ ListenerRegistration SnapshotsInSyncListener::AddListenerTo(`
`18`	`18`	`SnapshotsInSyncListener listener(callback_id, callback);`
`19`	`19`
`20`	`20`	`return firestore->AddSnapshotsInSyncListener(`
`21`		`- [listener]() mutable { listener.OnEvent(Error::kOk); });`
	`21`	`+ [listener]() mutable { listener.OnEvent(Error::kErrorOk); });`
`22`	`22`	`}`
`23`	`23`
`24`	`24`	`} // namespace csharp`