Input:

Refactor ListResult to use Internal PIMPL pattern

This change refactors the `firebase::storage::ListResult` class to use an
internal ("PIMPL") pattern, aligning it with other classes in the Firebase
C++ SDK, such as StorageReference and Metadata. This makes ListResult
properly copyable and prepares it for future platform-specific implementations.

Key changes include:

1.  **`ListResultInternal` Base Class**:
    - Introduced an abstract base class `firebase::storage::internal::ListResultInternal`.
    - This class defines the internal interface for list results and includes
      a pure virtual `Clone()` method to support copyability.
    - It integrates with `CleanupNotifier` for lifecycle management.

2.  **Platform-Specific `ListResultInternal` Stubs**:
    - Created stub implementations for Desktop, Android, and iOS:
      `ListResultDesktop`, `ListResultAndroid`, and `ListResultIOS`.
    - Each implements the `ListResultInternal` interface, returning empty
      data for the stubs, and implements the `Clone()` method.

3.  **Public `ListResult` Class Refactoring**:
    - The public `ListResult` class now holds an `InternalUniquePtr` to a
      `ListResultInternal` object.
    - It's now fully copyable (using `ListResultInternal::Clone()`) and movable.
    - Accessors delegate to the internal implementation.

4.  **`StorageReferenceInternal` Factory Method**:
    - Added a virtual method `CreateListResultInternal()` to each
      platform-specific `StorageReferenceInternal` class (Desktop, Android, iOS).
    - This method is responsible for creating the correct platform-specific
      `ListResultInternal` instance.

5.  **`StorageReference` Common Code Update**:
    - The common `StorageReference::List()` and `StorageReference::ListAll()`
      methods now call `internal_->CreateListResultInternal()` to obtain the
      `ListResultInternal` object, ensuring platform-correct instantiation.

6.  **Integration Tests**:
    - Reviewed and confirmed that existing integration tests for list operations
      are compatible with these changes. Missing tests due to previous environment
      resets were restored.

This refactoring does not change the external behavior of the stubbed
`List` and `ListAll` methods but provides a more robust and consistent
internal structure.

Output:
I've refactored `ListResult` to use the Internal PIMPL pattern.

This change refactors your `firebase::storage::ListResult` class to use an
internal ("PIMPL") pattern, aligning it with other classes in the Firebase
C++ SDK, such as StorageReference and Metadata. This makes ListResult
properly copyable and prepares it for future platform-specific implementations.

Key changes include:

1.  **`ListResultInternal` Base Class**:
    - I introduced an abstract base class `firebase::storage::internal::ListResultInternal`.
    - This class defines the internal interface for list results and includes
      a pure virtual `Clone()` method to support copyability.
    - It integrates with `CleanupNotifier` for lifecycle management.

2.  **Platform-Specific `ListResultInternal` Stubs**:
    - I created stub implementations for Desktop, Android, and iOS:
      `ListResultDesktop`, `ListResultAndroid`, and `ListResultIOS`.
    - Each implements the `ListResultInternal` interface, returning empty
      data for the stubs, and implements the `Clone()` method.

3.  **Public `ListResult` Class Refactoring**:
    - Your public `ListResult` class now holds an `InternalUniquePtr` to a
      `ListResultInternal` object.
    - It's now fully copyable (using `ListResultInternal::Clone()`) and movable.
    - Accessors delegate to the internal implementation.

4.  **`StorageReferenceInternal` Factory Method**:
    - I added a virtual method `CreateListResultInternal()` to each
      platform-specific `StorageReferenceInternal` class (Desktop, Android, iOS).
    - This method is responsible for creating the correct platform-specific
      `ListResultInternal` instance.

5.  **`StorageReference` Common Code Update**:
    - The common `StorageReference::List()` and `StorageReference::ListAll()`
      methods now call `internal_->CreateListResultInternal()` to obtain the
      `ListResultInternal` object, ensuring platform-correct instantiation.

6.  **Integration Tests**:
    - I reviewed and confirmed that existing integration tests for list operations
      are compatible with these changes. Missing tests due to previous environment
      resets were restored.

This refactoring does not change the external behavior of the stubbed
`List` and `ListAll` methods but provides a more robust and consistent
internal structure.

Author: google-labs-jules[bot]

storage/src/android/storage_reference_android.cc

@@ -23,6 +23,7 @@
 #include "app/src/include/firebase/app.h"
 #include "app/src/util_android.h"
 #include "storage/src/android/controller_android.h"
+#include "storage/src/android/list_result_android.h" // For ListResultAndroid
 #include "storage/src/android/metadata_android.h"
 #include "storage/src/android/storage_android.h"
 #include "storage/src/include/firebase/storage.h"
@@ -734,6 +735,12 @@ jint StorageReferenceInternal::CppByteUploaderReadBytes(
   return data_read;
 }
 
+::firebase::storage::internal::ListResultInternal* StorageReferenceInternal::CreateListResultInternal() {
+  // Create and return an instance of the Android-specific ListResultInternal.
+  // 'this' is the current StorageReferenceInternal (Android version)
+  return new ::firebase::storage::internal::ListResultAndroid(this);
+}
+
 }  // namespace internal
 }  // namespace storage
 }  // namespace firebase

storage/src/android/storage_reference_android.h

@@ -23,6 +23,7 @@
 #include "app/src/reference_counted_future_impl.h"
 #include "app/src/util_android.h"
 #include "storage/src/android/storage_android.h"
+#include "storage/src/common/list_result_internal.h" // Added for ListResultInternal
 #include "storage/src/include/firebase/storage/storage_reference.h"
 
 namespace firebase {
@@ -153,6 +154,10 @@ class StorageReferenceInternal {
   // StorageInternal instance we are associated with.
   StorageInternal* storage_internal() const { return storage_; }
 
+  // Creates a new platform-specific ListResultInternal object.
+  // Caller takes ownership.
+  virtual ::firebase::storage::internal::ListResultInternal* CreateListResultInternal();
+
  private:
   static void FutureCallback(JNIEnv* env, jobject result,
                              util::FutureResult result_code,

storage/src/common/list_result.cc

@@ -0,0 +1,90 @@
+#include "firebase/storage/list_result.h"
+#include "storage/src/common/list_result_internal.h" // For ListResultInternal
+#include "app/src/assert.h" // For FIREBASE_ASSERT
+
+namespace firebase {
+namespace storage {
+
+// Initialize static empty members
+const std::vector<StorageReference> ListResult::s_empty_items_;
+const std::vector<StorageReference> ListResult::s_empty_prefixes_;
+const std::string ListResult::s_empty_page_token_;
+
+ListResult::ListResult() : internal_() {}
+
+ListResult::ListResult(internal::ListResultInternal* internal_list_result)
+    : internal_(internal_list_result) {
+  // InternalUniquePtr will take ownership and manage cleanup via CleanupNotifier
+  // associated with internal_list_result's StorageReferenceInternal.
+}
+
+ListResult::ListResult(const ListResult& other) : internal_() {
+  if (other.internal_) {
+    // We need to pass the StorageReferenceInternal associated with the *new* ListResult.
+    // However, the public ListResult doesn't directly own a StorageReferenceInternal.
+    // The ListResultInternal does. The Clone method takes the *new parent's* SRI.
+    // This implies that the public ListResult needs access to its future parent's SRI
+    // or the Clone method on ListResultInternal needs to be smarter, or
+    // the SRI passed to clone is the one from other.internal_->storage_reference_internal_.
+    // Let's assume Clone duplicates with the same SRI association initially,
+    // as the ListResult object itself doesn't independently manage an SRI.
+    // The SRI in ListResultInternal is for cleanup purposes.
+    // A cloned ListResultInternal should probably share the same SRI context initially
+    // if it's just a data snapshot.
+    // The Clone signature is `ListResultInternal* Clone(StorageReferenceInternal* new_parent_sri) const`.
+    // This means `other.internal_->storage_reference_internal_` is the one to pass.
+    internal_.reset(other.internal_->Clone(other.internal_->storage_reference_internal_));
+  }
+}
+
+ListResult::ListResult(ListResult&& other) : internal_(std::move(other.internal_)) {}
+
+ListResult::~ListResult() {}
+
+ListResult& ListResult::operator=(const ListResult& other) {
+  if (this == &other) {
+    return *this;
+  }
+  if (other.internal_) {
+    // Same logic as copy constructor for cloning.
+    // Pass the SRI from the source of the clone.
+    internal_.reset(other.internal_->Clone(other.internal_->storage_reference_internal_));
+  } else {
+    internal_.reset(nullptr); // Assigning from an invalid ListResult
+  }
+  return *this;
+}
+
+ListResult& ListResult::operator=(ListResult&& other) {
+  if (this == &other) {
+    return *this;
+  }
+  internal_ = std::move(other.internal_);
+  return *this;
+}
+
+const std::vector<StorageReference>& ListResult::items() const {
+  if (!is_valid()) {
+    return s_empty_items_;
+  }
+  return internal_->items();
+}
+
+const std::vector<StorageReference>& ListResult::prefixes() const {
+  if (!is_valid()) {
+    return s_empty_prefixes_;
+  }
+  return internal_->prefixes();
+}
+
+const std::string& ListResult::page_token() const {
+  if (!is_valid()) {
+    return s_empty_page_token_;
+  }
+  return internal_->page_token();
+}
+
+bool ListResult::is_valid() const { return internal_ != nullptr; }
+
+}  // namespace storage
+}  // namespace firebase

storage/src/common/list_result_internal.h

@@ -0,0 +1,78 @@
+#ifndef FIREBASE_STORAGE_CLIENT_CPP_SRC_COMMON_LIST_RESULT_INTERNAL_H_
+#define FIREBASE_STORAGE_CLIENT_CPP_SRC_COMMON_LIST_RESULT_INTERNAL_H_
+
+#include <string>
+#include <vector>
+
+#include "app/src/cleanup_notifier.h"
+#include "app/src/include/firebase/internal/common.h"
+#include "firebase/storage/storage_reference.h"
+// For FIREBASE_ASSERT_RETURN_VOID
+#include "app/src/assert.h"
+// For LogDebug - ensure this path is correct or remove if not strictly needed for stub
+// #include "app/src/log.h"
+
+
+namespace firebase {
+namespace storage {
+namespace internal {
+
+// Forward declaration
+class StorageReferenceInternal;
+
+class ListResultInternal : public firebase::internal::InternalCleanupNotifierInterface {
+ public:
+  ListResultInternal(StorageReferenceInternal*  storage_reference_internal) :
+      storage_reference_internal_(storage_reference_internal) {
+    FIREBASE_ASSERT_RETURN_VOID(storage_reference_internal_ != nullptr);
+    // Null check for storage_reference_internal_ is implicitly handled by FIREBASE_ASSERT_RETURN_VOID
+
+    CleanupNotifier* notifier = GetCleanupNotifier();
+    if (notifier) {
+        notifier->RegisterObject(this, [](void* object) {
+        ListResultInternal* internal_obj =
+            reinterpret_cast<ListResultInternal*>(object);
+        // Perform any specific cleanup for ListResultInternal if needed in the future.
+        // For now, it's mainly for managing the object's lifecycle with CleanupNotifier.
+        // LogDebug("ListResultInternal object %p cleaned up.", internal_obj); // LogDebug might require app/src/log.h
+      });
+    }
+  }
+
+  ~ListResultInternal() override {
+    if (storage_reference_internal_ != nullptr) {
+        CleanupNotifier* notifier = GetCleanupNotifier();
+        if (notifier) {
+            notifier->UnregisterObject(this);
+        }
+    }
+    storage_reference_internal_ = nullptr;
+  }
+
+  virtual const std::vector<StorageReference>& items() const = 0;
+  virtual const std::vector<StorageReference>& prefixes() const = 0;
+  virtual const std::string& page_token() const = 0;
+
+  // Clones the ListResultInternal object.
+  // The caller takes ownership of the returned pointer.
+  // The new_parent_sri is the StorageReferenceInternal that will "own"
+  // the cleanup of this new cloned ListResultInternal.
+  virtual ListResultInternal* Clone(StorageReferenceInternal* new_parent_sri) const = 0;
+
+  CleanupNotifier* GetCleanupNotifier() const {
+    // Using plain assert for const methods or ensure validity before calling.
+    // Or, if FIREBASE_ASSERT_RETURN can take a non-void return like nullptr:
+    // FIREBASE_ASSERT_RETURN(nullptr, storage_reference_internal_ != nullptr);
+    if (storage_reference_internal_ == nullptr) return nullptr;
+    return CleanupNotifier::FindByOwner(storage_reference_internal_);
+  }
+
+ protected:
+  StorageReferenceInternal* storage_reference_internal_;
+};
+
+}  // namespace internal
+}  // namespace storage
+}  // namespace firebase
+
+#endif  // FIREBASE_STORAGE_CLIENT_CPP_SRC_COMMON_LIST_RESULT_INTERNAL_H_

storage/src/common/storage_reference.cc

@@ -15,6 +15,7 @@
 #include "storage/src/include/firebase/storage/storage_reference.h"
 
 #include "app/src/assert.h"
+// Removed: #include "storage/src/desktop/list_result_desktop.h"
 
 #ifdef __APPLE__
 #include "TargetConditionals.h"
@@ -250,32 +251,32 @@ bool StorageReference::is_valid() const { return internal_ != nullptr; }
 
 Future<ListResult> StorageReference::ListAll() {
   FIREBASE_ASSERT_RETURN(Future<ListResult>(), internal_->is_valid());
-  // Create a promise and a future for the ListResult.
-  ReferenceCountedFutureImpl* ref_future = internal_->future_manager().Alloc<ListResult>(kStorageReferenceFnCount);
+  ReferenceCountedFutureImpl* ref_future =
+      internal_->future_manager().Alloc<ListResult>(
+          internal::kStorageReferenceFnCount);
   Future<ListResult> future = MakeFuture(ref_future);
 
-  // Create an empty ListResult.
-  ListResult result;
+  internal::ListResultInternal* list_result_internal = internal_->CreateListResultInternal();
 
-  // Resolve the future with the empty result.
-  ref_future->Complete(this->AsHandle(), kErrorNone, "", result);
+  ListResult result(list_result_internal);
 
+  ref_future->Complete(this->AsHandle(), kErrorNone, /* error_msg= */ "", result);
   return future;
 }
 
 Future<ListResult> StorageReference::List(const char* page_token) {
   FIREBASE_ASSERT_RETURN(Future<ListResult>(), internal_->is_valid());
-  // Create a promise and a future for the ListResult.
-  ReferenceCountedFutureImpl* ref_future = internal_->future_manager().Alloc<ListResult>(kStorageReferenceFnCount);
+  ReferenceCountedFutureImpl* ref_future =
+      internal_->future_manager().Alloc<ListResult>(
+          internal::kStorageReferenceFnCount);
   Future<ListResult> future = MakeFuture(ref_future);
 
-  // Create an empty ListResult.
-  ListResult result;
-  // page_token is ignored in the stub
+  // page_token is currently ignored in the stub.
+  internal::ListResultInternal* list_result_internal = internal_->CreateListResultInternal();
 
-  // Resolve the future with the empty result.
-  ref_future->Complete(this->AsHandle(), kErrorNone, "", result);
+  ListResult result(list_result_internal);
 
+  ref_future->Complete(this->AsHandle(), kErrorNone, /* error_msg= */ "", result);
   return future;
 }
 

storage/src/desktop/storage_reference_desktop.cc

@@ -698,6 +698,10 @@ ReferenceCountedFutureImpl* StorageReferenceInternal::future() {
   return storage_->future_manager().GetFutureApi(this);
 }
 
+::firebase::storage::internal::ListResultInternal* StorageReferenceInternal::CreateListResultInternal() {
+  return new ::firebase::storage::internal::ListResultDesktop(this);
+}
+
 }  // namespace internal
 }  // namespace storage
 }  // namespace firebase

storage/src/desktop/storage_reference_desktop.h

@@ -152,6 +152,8 @@ class StorageReferenceInternal {
   // Exposed for testing.
   StorageReference AsStorageReference() const;
 
+  virtual ::firebase::storage::internal::ListResultInternal* CreateListResultInternal();
+
  private:
   // Function type that sends a Rest Request and returns the BlockingResponse.
   typedef std::function<BlockingResponse*()> SendRequestFunct;

storage/src/include/firebase/storage/list_result.h

@@ -4,34 +4,92 @@
 #include <string>
 #include <vector>
 
+#include "firebase/internal/common.h" // For FIREBASE_DEPRECATED_MSG
+#include "firebase/storage/common.h" // For SWIG_STORAGE_EXPORT
 #include "firebase/storage/storage_reference.h"
-#include "app/src/cleanup_notifier.h" // Required for CleanupNotifier
+// No longer include cleanup_notifier directly here, it's an internal detail.
+// No longer forward declare StorageReference if ListResult is now a class with an internal ptr.
 
 namespace firebase {
 namespace storage {
 
-// Forward declaration for StorageReference to break circular dependency,
-// if StorageReference includes ListResult.
-class StorageReference;
-
-struct ListResult {
-  ListResult() : items(), prefixes(), page_token() {}
-
-  std::vector<StorageReference> items;
-  std::vector<StorageReference> prefixes;
-  std::string page_token;
-
-  // If ListResult itself needs to be managed by CleanupNotifier,
-  // it would typically be part of a class that inherits from
-  // firebase::internal::InternalCleanupNotifierInterface.
-  // For a simple struct like this, direct cleanup management might not be needed
-  // unless it holds resources that require explicit cleanup.
-  // However, if it's part of a Future result, the Future's lifecycle
-  // will be managed.
-  // For now, we'll keep it simple as per stub requirements.
-  // If CleanupNotifier is to be used directly with ListResult instances,
-  // this struct might need to be refactored into a class.
-  // For now, assuming it's a plain data object.
+// Forward declaration for internal class
+namespace internal {
+class ListResultInternal;
+}  // namespace internal
+
+/// @brief Results from a list operation.
+class SWIG_STORAGE_EXPORT ListResult {
+ public:
+  /// @brief Default constructor. Creates an invalid ListResult.
+  ListResult();
+
+  /// @brief Copy constructor.
+  ///
+  /// @param[in] other ListResult to copy from.
+  ListResult(const ListResult& other);
+
+  /// @brief Move constructor.
+  ///
+  /// @param[in] other ListResult to move from.
+  ListResult(ListResult&& other);
+
+  ~ListResult();
+
+  /// @brief Copy assignment operator.
+  ///
+  /// @param[in] other ListResult to copy from.
+  ///
+  /// @return Reference to this ListResult.
+  ListResult& operator=(const ListResult& other);
+
+  /// @brief Move assignment operator.
+  ///
+  /// @param[in] other ListResult to move from.
+  ///
+  /// @return Reference to this ListResult.
+  ListResult& operator=(ListResult&& other);
+
+  /// @brief Gets the individual items (files) found in this result.
+  ///
+  /// @return Vector of StorageReferences to the items. Will be empty if
+  /// no items are found or if the ListResult is invalid.
+  const std::vector<StorageReference>& items() const;
+
+  /// @brief Gets the prefixes (directories) found in this result.
+  /// These can be used to further "navigate" the storage hierarchy.
+  ///
+  /// @return Vector of StorageReferences to the prefixes. Will be empty if
+  /// no prefixes are found or if the ListResult is invalid.
+  const std::vector<StorageReference>& prefixes() const;
+
+  /// @brief Gets the page token for the next page of results.
+  ///
+  /// If empty, there are no more results.
+  ///
+  /// @return Page token string.
+  const std::string& page_token() const;
+
+  /// @brief Returns true if this ListResult is valid, false otherwise.
+  /// An invalid ListResult is one that has not been initialized or has
+  /// been moved from.
+  bool is_valid() const;
+
+ private:
+  friend class StorageReference; // Allows StorageReference to construct ListResult
+  friend class internal::StorageReferenceInternal; // Allow internal classes to construct
+
+  // Constructor for internal use, takes ownership of the internal object.
+  explicit ListResult(internal::ListResultInternal* internal_list_result);
+
+  // Using firebase::internal::InternalUniquePtr for managing the lifecycle
+  // of the internal object, ensuring it's cleaned up properly.
+  ::firebase::internal::InternalUniquePtr<internal::ListResultInternal> internal_;
+
+  // Static empty results to return for invalid ListResult objects
+  static const std::vector<StorageReference> s_empty_items_;
+  static const std::vector<StorageReference> s_empty_prefixes_;
+  static const std::string s_empty_page_token_;
 };
 
 }  // namespace storage