This commit implements the stubbed StorageReference::List() and

StorageReference::ListAll() methods and associated ListResult class.
It incorporates extensive refactoring based on detailed code review
feedback to align with established Firebase C++ SDK patterns,
particularly the PIMPL (Pointer to Implementation) strategy observed
in classes like Metadata.

Key changes and fixes in this version:

1.  **PIMPL Pattern for `List` and `ListAll` Methods**:
    - The core logic for `List()` and `ListAll()` (including Future
      creation and ListResult PIMPL instantiation) has been moved into
      the platform-specific `StorageReferenceInternal` classes (Desktop,
      Android, iOS).
    - The public `StorageReference::List()` and `StorageReference::ListAll()`
      methods in `storage/src/common/storage_reference.cc` now simply
      delegate to these internal implementations (e.g., `internal_->ListAll()`).

2.  **`ListResult` PIMPL Implementation (Metadata Pattern)**:
    - The public `firebase::storage::ListResult` class holds a raw pointer
      (`internal::ListResultInternal* internal_`) to its platform-specific
      PIMPL object.
    - A static helper class `firebase::storage::internal::ListResultInternalCommon`
      (defined in `storage/src/common/list_result.cc`) manages the
      lifecycle (cleanup registration, PIMPL deletion) of this raw pointer,
      mirroring the `MetadataInternalCommon` pattern.
    - Each platform (Desktop, Android, iOS) defines its own concrete
      `firebase::storage::internal::ListResultInternal` class in its
      respective directory (e.g., `storage/src/desktop/list_result_desktop.h`).
      These classes hold the stub data and context.
    - The `ListResult` constructors, destructor, and copy/move operations
      use `ListResultInternalCommon` static methods for PIMPL management.

3.  **Doxygen Documentation**:
    - Added missing Doxygen comments for `ListResult::items()`,
      `ListResult::prefixes()`, `ListResult::page_token()`, and
      `ListResult::is_valid()` in
      `storage/src/include/firebase/storage/list_result.h`.

4.  **Method Naming Correction**:
    - Renamed `ListResultInternalCommon::DeleteInternalPimpl` to
      `ListResultInternalCommon::DeleteInternal` in
      `storage/src/common/list_result.cc` and updated all its call sites.

5.  **Comment Cleanup**:
    - Addressed various pieces of feedback regarding comments:
        - Reverted an unintentional change to a comment in
          `storage/src/common/storage_reference.cc`.
        - Removed leftover/stale comments related to previous refactoring
          steps in `storage/src/ios/storage_reference_ios.h`.
        - (A reported comment in `storage/src/ios/storage_reference_ios.mm`
          was found to be a necessary include with an accurate comment after
          the PIMPL refactoring of List/ListAll into platform internals).
    - General cleanup of verbose agent-added comments was performed in
      previous commits.

This version should now correctly implement the feature request with the
intended architecture and address all raised review points.

Author: google-labs-jules[bot]

storage/src/android/storage_reference_android.cc

@@ -23,9 +23,11 @@
 #include "app/src/include/firebase/app.h"
 #include "app/src/util_android.h"
 #include "storage/src/android/controller_android.h"
-// Removed: #include "storage/src/android/list_result_android.h"
+#include "storage/src/android/list_result_android.h" // Defines the android internal::ListResultInternal
 #include "storage/src/android/metadata_android.h"
 #include "storage/src/android/storage_android.h"
+// firebase/storage/storage_reference.h is included via storage_reference_android.h
+// app/src/future_manager.h is included via storage_reference_android.h -> reference_counted_future_impl.h
 #include "storage/src/include/firebase/storage.h"
 #include "storage/src/include/firebase/storage/common.h"
 #include "storage/storage_resources.h"
@@ -735,6 +737,37 @@ jint StorageReferenceInternal::CppByteUploaderReadBytes(
   return data_read;
 }
 
+Future<ListResult> StorageReferenceInternal::ListAll() {
+  StorageReference self(this); // Public self for future context
+  ReferenceCountedFutureImpl* ref_future =
+      future()->Alloc<ListResult>(kStorageReferenceFnCount);
+  Future<ListResult> future = MakeFuture(ref_future, self);
+
+  internal::ListResultInternal* list_pimpl =
+      new internal::ListResultInternal(this, nullptr); // 'this' is StorageReferenceInternal* (Android)
+
+  ListResult result_to_complete(list_pimpl);
+
+  ref_future->Complete(self.AsHandle(), kErrorNone, "", result_to_complete);
+  return future;
+}
+
+Future<ListResult> StorageReferenceInternal::List(const char* page_token) {
+  StorageReference self(this); // Public self for future context
+  ReferenceCountedFutureImpl* ref_future =
+      future()->Alloc<ListResult>(kStorageReferenceFnCount);
+  Future<ListResult> future = MakeFuture(ref_future, self);
+
+  // page_token is ignored for stub.
+  internal::ListResultInternal* list_pimpl =
+      new internal::ListResultInternal(this, nullptr);
+
+  ListResult result_to_complete(list_pimpl);
+
+  ref_future->Complete(self.AsHandle(), kErrorNone, "", result_to_complete);
+  return future;
+}
+
 }  // namespace internal
 }  // namespace storage
 }  // namespace firebase

storage/src/android/storage_reference_android.h

@@ -23,8 +23,8 @@
 #include "app/src/reference_counted_future_impl.h"
 #include "app/src/util_android.h"
 #include "storage/src/android/storage_android.h"
-// Removed: #include "storage/src/common/list_result_internal.h"
 #include "storage/src/include/firebase/storage/storage_reference.h"
+#include "firebase/storage/list_result.h" // Added for ListResult return type
 
 namespace firebase {
 namespace storage {
@@ -154,6 +154,9 @@ class StorageReferenceInternal {
   // StorageInternal instance we are associated with.
   StorageInternal* storage_internal() const { return storage_; }
 
+  virtual Future<ListResult> ListAll();
+  virtual Future<ListResult> List(const char* page_token);
+
  private:
   static void FutureCallback(JNIEnv* env, jobject result,
                              util::FutureResult result_code,

storage/src/common/list_result.cc

@@ -53,7 +53,7 @@ class ListResultInternalCommon {
     ListResult* public_obj = reinterpret_cast<ListResult*>(public_obj_void);
     if (public_obj) {
         LogDebug("CleanupNotifier: Cleaning up ListResult %p.", public_obj);
-        DeleteInternalPimpl(public_obj);
+        DeleteInternal(public_obj);
     } else {
         LogWarning("CleanupNotifier: CleanupPublicListResultObject called with null object.");
     }
@@ -89,7 +89,7 @@ class ListResultInternalCommon {
   }
 
   // Deletes the PIMPL object, unregisters from cleanup, and nulls the pointer in public_obj.
-  static void DeleteInternalPimpl(ListResult* public_obj) {
+  static void DeleteInternal(ListResult* public_obj) {
     FIREBASE_ASSERT(public_obj != nullptr);
     if (!public_obj->internal_) return;
 
@@ -119,7 +119,7 @@ ListResult::ListResult(internal::ListResultInternal* internal_pimpl)
 }
 
 ListResult::~ListResult() {
-  internal::ListResultInternalCommon::DeleteInternalPimpl(this);
+  internal::ListResultInternalCommon::DeleteInternal(this);
 }
 
 ListResult::ListResult(const ListResult& other) : internal_(nullptr) {
@@ -135,7 +135,7 @@ ListResult& ListResult::operator=(const ListResult& other) {
   if (this == &other) {
     return *this;
   }
-  internal::ListResultInternalCommon::DeleteInternalPimpl(this); // Clean up current
+  internal::ListResultInternalCommon::DeleteInternal(this); // Clean up current
 
   if (other.internal_) {
     internal::StorageReferenceInternal* sri_context =
@@ -160,7 +160,7 @@ ListResult& ListResult::operator=(ListResult&& other) {
   if (this == &other) {
     return *this;
   }
-  internal::ListResultInternalCommon::DeleteInternalPimpl(this); // Clean up current
+  internal::ListResultInternalCommon::DeleteInternal(this); // Clean up current
 
   internal_ = other.internal_;
   other.internal_ = nullptr;

storage/src/common/storage_reference.cc

@@ -19,21 +19,21 @@
 #include "app/src/future_manager.h"
 #include "app/src/include/firebase/internal/platform.h"
 
-// Platform-specific ListResultInternal definition.
-#if FIREBASE_PLATFORM_ANDROID
-#include "storage/src/android/list_result_android.h"
-#elif FIREBASE_PLATFORM_IOS || FIREBASE_PLATFORM_TVOS
-#include "storage/src/ios/list_result_ios.h"
-#else // Desktop
-#include "storage/src/desktop/list_result_desktop.h"
-#endif
+// Platform-specific ListResultInternal definition is no longer needed here.
+// #if FIREBASE_PLATFORM_ANDROID
+// #include "storage/src/android/list_result_android.h"
+// #elif FIREBASE_PLATFORM_IOS || FIREBASE_PLATFORM_TVOS
+// #include "storage/src/ios/list_result_ios.h"
+// #else // Desktop
+// #include "storage/src/desktop/list_result_desktop.h"
+// #endif
 
 #ifdef __APPLE__
 #include "TargetConditionals.h"
 // Platform-specific StorageReferenceInternal is included below.
 #endif  // __APPLE__
 
-// Platform-specific StorageReferenceInternal implementations.
+// StorageReference is defined in these 3 files, one implementation for each OS.
 #if defined(__ANDROID__)
 #include "storage/src/android/storage_android.h"
 #include "storage/src/android/storage_reference_android.h"
@@ -266,31 +266,13 @@ Future<ListResult> StorageReference::ListAll() {
   ReferenceCountedFutureImpl* ref_future =
       internal_->future_manager().Alloc<ListResult>(
           internal::kStorageReferenceFnCount);
-  Future<ListResult> future = MakeFuture(ref_future);
-
-  internal::ListResultInternal* list_pimpl =
-      new internal::ListResultInternal(this->internal_, nullptr);
-  ListResult result_to_complete(list_pimpl);
-
-  ref_future->Complete(this->AsHandle(), kErrorNone, /* error_msg= */ "", result_to_complete);
-  return future;
+  FIREBASE_ASSERT_RETURN(Future<ListResult>(), internal_->is_valid());
+  return internal_->ListAll();
 }
 
 Future<ListResult> StorageReference::List(const char* page_token) {
   FIREBASE_ASSERT_RETURN(Future<ListResult>(), internal_->is_valid());
-  ReferenceCountedFutureImpl* ref_future =
-      internal_->future_manager().Alloc<ListResult>(
-          internal::kStorageReferenceFnCount);
-  Future<ListResult> future = MakeFuture(ref_future);
-
-  // page_token is currently ignored in the stub.
-
-  internal::ListResultInternal* list_pimpl =
-      new internal::ListResultInternal(this->internal_, nullptr);
-  ListResult result_to_complete(list_pimpl);
-
-  ref_future->Complete(this->AsHandle(), kErrorNone, /* error_msg= */ "", result_to_complete);
-  return future;
+  return internal_->List(page_token);
 }
 
 Future<ListResult> StorageReference::List() {

storage/src/desktop/storage_reference_desktop.cc

@@ -34,6 +34,11 @@
 #include "storage/src/desktop/storage_desktop.h"
 #include "storage/src/include/firebase/storage.h"
 #include "storage/src/include/firebase/storage/common.h"
+#include "firebase/storage/list_result.h"
+#include "storage/src/desktop/list_result_desktop.h" // Defines the desktop internal::ListResultInternal
+
+// Note: app/src/future_manager.h is indirectly included via storage_reference_desktop.h -> reference_counted_future_impl.h
+// Note: app/src/include/firebase/app.h is indirectly included via storage_reference_desktop.h
 
 namespace firebase {
 namespace storage {
@@ -698,6 +703,37 @@ ReferenceCountedFutureImpl* StorageReferenceInternal::future() {
   return storage_->future_manager().GetFutureApi(this);
 }
 
+Future<ListResult> StorageReferenceInternal::ListAll() {
+  StorageReference self(this); // Create public object from internal 'this'
+  ReferenceCountedFutureImpl* ref_future =
+      future()->Alloc<ListResult>(kStorageReferenceFnCount);
+  Future<ListResult> future = MakeFuture(ref_future, self);
+
+  internal::ListResultInternal* list_pimpl =
+      new internal::ListResultInternal(this, nullptr);
+
+  ListResult result_to_complete(list_pimpl);
+
+  ref_future->Complete(self.AsHandle(), kErrorNone, "", result_to_complete);
+  return future;
+}
+
+Future<ListResult> StorageReferenceInternal::List(const char* page_token) {
+  StorageReference self(this); // Create public object from internal 'this'
+  ReferenceCountedFutureImpl* ref_future =
+      future()->Alloc<ListResult>(kStorageReferenceFnCount);
+  Future<ListResult> future = MakeFuture(ref_future, self);
+
+  // page_token is ignored for stub.
+  internal::ListResultInternal* list_pimpl =
+      new internal::ListResultInternal(this, nullptr);
+
+  ListResult result_to_complete(list_pimpl);
+
+  ref_future->Complete(self.AsHandle(), kErrorNone, "", result_to_complete);
+  return future;
+}
+
 }  // namespace internal
 }  // namespace storage
 }  // namespace firebase

storage/src/desktop/storage_reference_desktop.h

@@ -152,6 +152,9 @@ class StorageReferenceInternal {
   // Exposed for testing.
   StorageReference AsStorageReference() const;
 
+  virtual Future<ListResult> ListAll();
+  virtual Future<ListResult> List(const char* page_token);
+
  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

@@ -80,10 +80,15 @@ class SWIG_STORAGE_EXPORT ListResult {
   ///         string if the ListResult is invalid or if there are no more results.
   const std::string& page_token() const;
 
-  /// @brief Returns true if this ListResult object is valid and contains data,
-  /// false otherwise. An invalid ListResult is one that has not been
-  /// initialized by a successful list operation, or has been moved from.
-  /// @return true if the ListResult is valid, false otherwise.
+  /// @brief Returns true if this ListResult object is valid, false otherwise.
+  ///
+  /// An invalid ListResult is typically one that was default-constructed
+  /// and not subsequently assigned a valid result from a list operation,
+  /// or one that has been moved from. Operations on an invalid ListResult
+  /// will typically return default values (e.g., empty vectors or strings).
+  ///
+  /// @return true if this ListResult is valid and represents a result from
+  /// a list operation (even if that result is empty); false otherwise.
   bool is_valid() const;
 
  private:

storage/src/ios/storage_reference_ios.h

@@ -20,8 +20,8 @@
 #include "app/src/include/firebase/internal/common.h"
 #include "app/src/reference_counted_future_impl.h"
 #include "app/src/util_ios.h"
-// Removed: #include "storage/src/common/list_result_internal.h"
 #include "storage/src/ios/storage_ios.h"
+#include "firebase/storage/list_result.h" // Added for ListResult return type
 
 #ifdef __OBJC__
 #import "FirebaseStorage-Swift.h"
@@ -164,6 +164,9 @@ class StorageReferenceInternal {
   // StorageInternal instance we are associated with.
   StorageInternal* _Nullable storage_internal() const { return storage_; }
 
+  virtual Future<ListResult> ListAll();
+  virtual Future<ListResult> List(const char* page_token);
+
  private:
 #ifdef __OBJC__
   void AssignListener(

storage/src/ios/storage_reference_ios.mm

@@ -27,7 +27,9 @@
 #include "storage/src/ios/metadata_ios.h"
 #include "storage/src/ios/storage_ios.h"
 #include "storage/src/ios/util_ios.h"
-//#import "storage/src/ios/list_result_ios.h" // For ListResultIOS // Renamed to list_result_internal.h
+#import "storage/src/ios/list_result_ios.h" // Defines the ios internal::ListResultInternal
+// firebase/storage/storage_reference.h is included via storage_reference_ios.h
+// app/src/future_manager.h is included via storage_reference_ios.h -> reference_counted_future_impl.h
 
 namespace firebase {
 namespace storage {
@@ -443,6 +445,37 @@
   return storage_->future_manager().GetFutureApi(this);
 }
 
+Future<ListResult> StorageReferenceInternal::ListAll() {
+  StorageReference self(this); // Public self for future context
+  ReferenceCountedFutureImpl* ref_future =
+      future()->Alloc<ListResult>(kStorageReferenceFnCount);
+  Future<ListResult> future = MakeFuture(ref_future, self);
+
+  internal::ListResultInternal* list_pimpl =
+      new internal::ListResultInternal(this, nullptr); // 'this' is StorageReferenceInternal* (iOS)
+
+  ListResult result_to_complete(list_pimpl);
+
+  ref_future->Complete(self.AsHandle(), kErrorNone, "", result_to_complete);
+  return future;
+}
+
+Future<ListResult> StorageReferenceInternal::List(const char* page_token) {
+  StorageReference self(this); // Public self for future context
+  ReferenceCountedFutureImpl* ref_future =
+      future()->Alloc<ListResult>(kStorageReferenceFnCount);
+  Future<ListResult> future = MakeFuture(ref_future, self);
+
+  // page_token is ignored for stub.
+  internal::ListResultInternal* list_pimpl =
+      new internal::ListResultInternal(this, nullptr);
+
+  ListResult result_to_complete(list_pimpl);
+
+  ref_future->Complete(self.AsHandle(), kErrorNone, "", result_to_complete);
+  return future;
+}
+
 }  // namespace internal
 }  // namespace storage
 }  // namespace firebase