WebShare: Styles and Stream Management in tests

Updating the various fake classes produced for Web Share to reflect
best practices brought up during other Web Share reviews.

This includes expanding system #defines for types, using 'final' where
appropriate, avoiding explicit AddRef calls where possible, including
checks on cleanup, and allowing calls to methods that don't require
additional functionality of the test class (even if they are not
currently in use).

The addition of some of these cleanup checks also prompted updates
to some of the unit tests that were not closing streams.

Bug: 1035527
Change-Id: Iea6c488b735d5877ba307d33c859114611c56dd5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2511629
Reviewed-by: Eric Willigers <ericwilligers@chromium.org>
Commit-Queue: Hoch Hochkeppel <mhochk@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#823052}

chrome/browser/webshare/win/fake_data_transfer_manager.h

@@ -15,7 +15,7 @@ namespace webshare {
 
 // Provides an implementation of IDataTransferManager for use in GTests.
 class __declspec(uuid("53CA4C00-6F19-40C1-A740-F66510E2DB40"))
-    FakeDataTransferManager
+    FakeDataTransferManager final
     : public Microsoft::WRL::RuntimeClass<
           Microsoft::WRL::RuntimeClassFlags<Microsoft::WRL::WinRtClassicComMix>,
           ABI::Windows::ApplicationModel::DataTransfer::IDataTransferManager> {
@@ -53,21 +53,25 @@ class __declspec(uuid("53CA4C00-6F19-40C1-A740-F66510E2DB40"))
   FakeDataTransferManager();
   FakeDataTransferManager(const FakeDataTransferManager&) = delete;
   FakeDataTransferManager& operator=(const FakeDataTransferManager&) = delete;
-  ~FakeDataTransferManager() override;
+  ~FakeDataTransferManager() final;
 
   // ABI::Windows::ApplicationModel::DataTransfer::IDataTransferManager:
   IFACEMETHODIMP add_DataRequested(
-      __FITypedEventHandler_2_Windows__CApplicationModel__CDataTransfer__CDataTransferManager_Windows__CApplicationModel__CDataTransfer__CDataRequestedEventArgs*
-          event_handler,
-      EventRegistrationToken* event_cookie) override;
+      ABI::Windows::Foundation::ITypedEventHandler<
+          ABI::Windows::ApplicationModel::DataTransfer::DataTransferManager*,
+          ABI::Windows::ApplicationModel::DataTransfer::
+              DataRequestedEventArgs*>* event_handler,
+      EventRegistrationToken* event_cookie) final;
   IFACEMETHODIMP
-  remove_DataRequested(EventRegistrationToken event_cookie) override;
+  remove_DataRequested(EventRegistrationToken event_cookie) final;
   IFACEMETHODIMP add_TargetApplicationChosen(
-      __FITypedEventHandler_2_Windows__CApplicationModel__CDataTransfer__CDataTransferManager_Windows__CApplicationModel__CDataTransfer__CTargetApplicationChosenEventArgs*
-          eventHandler,
-      EventRegistrationToken* event_cookie) override;
+      ABI::Windows::Foundation::ITypedEventHandler<
+          ABI::Windows::ApplicationModel::DataTransfer::DataTransferManager*,
+          ABI::Windows::ApplicationModel::DataTransfer::
+              TargetApplicationChosenEventArgs*>* eventHandler,
+      EventRegistrationToken* event_cookie) final;
   IFACEMETHODIMP
-  remove_TargetApplicationChosen(EventRegistrationToken event_cookie) override;
+  remove_TargetApplicationChosen(EventRegistrationToken event_cookie) final;
 
   // Returns a callback that captures a reference to the current DataRequested
   // event handler and, when invoked, triggers that handler.
@@ -89,10 +93,11 @@ class __declspec(uuid("53CA4C00-6F19-40C1-A740-F66510E2DB40"))
     DataRequestedHandlerEntry(DataRequestedHandlerEntry const& other);
     ~DataRequestedHandlerEntry();
 
-    Microsoft::WRL::ComPtr<
-        __FITypedEventHandler_2_Windows__CApplicationModel__CDataTransfer__CDataTransferManager_Windows__CApplicationModel__CDataTransfer__CDataRequestedEventArgs>
-        event_handler_;
-    int64_t token_value_;
+    Microsoft::WRL::ComPtr<ABI::Windows::Foundation::ITypedEventHandler<
+        ABI::Windows::ApplicationModel::DataTransfer::DataTransferManager*,
+        ABI::Windows::ApplicationModel::DataTransfer::DataRequestedEventArgs*>>
+        event_handler;
+    int64_t token_value;
   };
 
   std::vector<DataRequestedHandlerEntry> data_requested_event_handlers_;

chrome/browser/webshare/win/fake_data_transfer_manager_interop.h

@@ -19,7 +19,7 @@ class FakeDataTransferManager;
 //
 // Like the Windows implementation, this class cloaks its implementation of
 // IDataTransferManagerInterop to closely match casting behaviors.
-class FakeDataTransferManagerInterop
+class FakeDataTransferManagerInterop final
     : public Microsoft::WRL::RuntimeClass<
           Microsoft::WRL::RuntimeClassFlags<
               Microsoft::WRL::RuntimeClassType::WinRtClassicComMix>,
@@ -54,13 +54,13 @@ class FakeDataTransferManagerInterop
       delete;
   FakeDataTransferManagerInterop& operator=(
       const FakeDataTransferManagerInterop&) = delete;
-  ~FakeDataTransferManagerInterop() override;
+  ~FakeDataTransferManagerInterop() final;
 
   // IDataTransferManagerInterop:
   IFACEMETHODIMP GetForWindow(HWND app_window,
                               REFIID riid,
-                              void** data_transfer_manager) override;
-  IFACEMETHODIMP ShowShareUIForWindow(HWND app_window) override;
+                              void** data_transfer_manager) final;
+  IFACEMETHODIMP ShowShareUIForWindow(HWND app_window) final;
 
   // Returns a callback that captures a reference to the current DataRequested
   // event handler and, when invoked, triggers that handler.

chrome/browser/webshare/win/fake_data_writer_factory.cc

@@ -61,19 +61,15 @@ class FakeDataWriter final
     return E_NOTIMPL;
   }
   IFACEMETHODIMP put_UnicodeEncoding(UnicodeEncoding value) final {
-    NOTREACHED();
-    return E_NOTIMPL;
+    return S_OK;
   }
   IFACEMETHODIMP
   get_ByteOrder(ByteOrder* value) final {
     NOTREACHED();
     return E_NOTIMPL;
   }
   IFACEMETHODIMP
-  put_ByteOrder(ByteOrder value) final {
-    NOTREACHED();
-    return E_NOTIMPL;
-  }
+  put_ByteOrder(ByteOrder value) final { return S_OK; }
   IFACEMETHODIMP WriteByte(BYTE value) final {
     NOTREACHED();
     return E_NOTIMPL;

chrome/browser/webshare/win/fake_data_writer_factory_unittest.cc

@@ -71,6 +71,9 @@ TEST(FakeDataWriterFactoryTest, RacingStoreAsync) {
 
   // Expect a failure from the destructor due to the unflushed data
   EXPECT_NONFATAL_FAILURE(data_writer.Reset(), "FlushAsync");
+
+  // Cleanup
+  ASSERT_HRESULT_SUCCEEDED(stream->Close());
 }
 
 TEST(FakeDataWriterFactoryTest, UnstoredBytes) {
@@ -94,6 +97,9 @@ TEST(FakeDataWriterFactoryTest, UnstoredBytes) {
   EXPECT_NONFATAL_FAILURE(
       EXPECT_NONFATAL_FAILURE(data_writer.Reset(), "pending storage"),
       "FlushAsync");
+
+  // Cleanup
+  ASSERT_HRESULT_SUCCEEDED(stream->Close());
 }
 
 TEST(FakeDataWriterFactoryTest, WriteBytes) {
@@ -210,6 +216,9 @@ TEST(FakeDataWriterFactoryTest, WriteBytes) {
   ASSERT_EQ(raw_buffer[5], 'e');
   ASSERT_EQ(raw_buffer[6], 'r');
   ASSERT_EQ(raw_buffer[7], 'e');
+
+  // Cleanup
+  ASSERT_HRESULT_SUCCEEDED(stream->Close());
 }
 
 }  // namespace webshare

chrome/browser/webshare/win/fake_random_access_stream.cc

@@ -209,7 +209,14 @@ class StreamData final : public base::RefCountedThreadSafe<StreamData> {
  private:
   friend class base::RefCountedThreadSafe<StreamData>;
 
-  virtual ~StreamData() = default;
+  virtual ~StreamData() {
+    EXPECT_FALSE(flush_async_in_progress_)
+        << "StreamData destroyed while flush operation is in progress.";
+    EXPECT_FALSE(read_async_in_progress_)
+        << "StreamData destroyed while read operation is in progress.";
+    EXPECT_FALSE(write_async_in_progress_)
+        << "StreamData destroyed while write operation is in progress.";
+  }
 
   void OnFlushAsync(
       ComPtr<base::win::FakeIAsyncOperation<bool>> fake_iasync_operation) {
@@ -290,7 +297,10 @@ FakeRandomAccessStream::FakeRandomAccessStream() {
   position_ = base::MakeRefCounted<base::RefCountedData<UINT64>>();
   shared_data_ = base::MakeRefCounted<StreamData>();
 }
-FakeRandomAccessStream::~FakeRandomAccessStream() = default;
+FakeRandomAccessStream::~FakeRandomAccessStream() {
+  EXPECT_TRUE(is_closed_)
+      << "FakeRandomAccessStream destroyed without being closed.";
+}
 
 IFACEMETHODIMP FakeRandomAccessStream::get_Size(UINT64* value) {
   if (is_closed_) {

chrome/browser/webshare/win/fake_random_access_stream.h

@@ -18,7 +18,7 @@ class StreamData;
 
 // Provides an implementation of IRandomAccessStream for use in GTests.
 class __declspec(uuid("66DAD26A-BEDE-4A54-8316-088838CC65A0"))
-    FakeRandomAccessStream
+    FakeRandomAccessStream final
     : public Microsoft::WRL::RuntimeClass<
           Microsoft::WRL::RuntimeClassFlags<Microsoft::WRL::WinRtClassicComMix>,
           ABI::Windows::Storage::Streams::IRandomAccessStream,

chrome/browser/webshare/win/fake_random_access_stream_unittest.cc

@@ -18,6 +18,7 @@ using ABI::Windows::Foundation::IAsyncOperation;
 using ABI::Windows::Foundation::IAsyncOperationCompletedHandler;
 using ABI::Windows::Foundation::IAsyncOperationWithProgress;
 using ABI::Windows::Foundation::IAsyncOperationWithProgressCompletedHandler;
+using ABI::Windows::Foundation::IClosable;
 using ABI::Windows::Storage::Streams::IBuffer;
 using ABI::Windows::Storage::Streams::IInputStream;
 using ABI::Windows::Storage::Streams::InputStreamOptions;
@@ -32,6 +33,7 @@ TEST(FakeRandomAccessStreamTest, InvalidSeek) {
   auto stream = Make<FakeRandomAccessStream>();
   ASSERT_HRESULT_SUCCEEDED(stream->Seek(0));
   EXPECT_NONFATAL_FAILURE(ASSERT_HRESULT_FAILED(stream->Seek(1)), "Seek");
+  ASSERT_HRESULT_SUCCEEDED(stream->Close());
 }
 
 TEST(FakeRandomAccessStreamTest, UsageAfterClose) {
@@ -183,6 +185,7 @@ TEST(FakeRandomAccessStreamTest, CompetingAsyncCalls) {
                             "in progress");
     run_loop.Run();
   }
+  ASSERT_HRESULT_SUCCEEDED(stream->Close());
 }
 
 TEST(FakeRandomAccessStreamTest, BasicReadWrite) {
@@ -199,6 +202,7 @@ TEST(FakeRandomAccessStreamTest, BasicReadWrite) {
 
     ASSERT_HRESULT_SUCCEEDED(stream->GetInputStreamAt(0, &input_stream));
     ASSERT_HRESULT_SUCCEEDED(stream->GetOutputStreamAt(0, &output_stream));
+    ASSERT_HRESULT_SUCCEEDED(stream->Close());
   }
 
   // Create a filled buffer that reads "abcd"
@@ -278,6 +282,9 @@ TEST(FakeRandomAccessStreamTest, BasicReadWrite) {
             })
             .Get()));
     run_loop.Run();
+    ComPtr<IClosable> closable_stream;
+    ASSERT_HRESULT_SUCCEEDED(output_stream.As(&closable_stream));
+    ASSERT_HRESULT_SUCCEEDED(closable_stream->Close());
   }
 
   // Read the input stream to the buffer
@@ -332,6 +339,9 @@ TEST(FakeRandomAccessStreamTest, BasicReadWrite) {
             })
             .Get()));
     run_loop.Run();
+    ComPtr<IClosable> closable_stream;
+    ASSERT_HRESULT_SUCCEEDED(input_stream.As(&closable_stream));
+    ASSERT_HRESULT_SUCCEEDED(closable_stream->Close());
   }
 
   ASSERT_HRESULT_SUCCEEDED(buffer->get_Length(&length));

chrome/browser/webshare/win/fake_storage_file_statics.h

@@ -12,7 +12,7 @@ namespace webshare {
 
 // Provides an implementation of IStorageFileStatics for use in GTests.
 class __declspec(uuid("70A03B12-27C0-499A-B8AE-18E6060BDEDD"))
-    FakeStorageFileStatics
+    FakeStorageFileStatics final
     : public Microsoft::WRL::RuntimeClass<
           Microsoft::WRL::RuntimeClassFlags<Microsoft::WRL::WinRtClassicComMix>,
           ABI::Windows::Storage::IStorageFileStatics> {

chrome/browser/webshare/win/fake_storage_file_statics_unittest.cc

@@ -158,6 +158,9 @@ TEST(FakeStorageFileStaticsTest, CreateStreamedFileAsync) {
   ASSERT_HRESULT_SUCCEEDED(opened_stream->GetInputStreamAt(0, &input_stream));
   UINT64 size;
   ASSERT_HRESULT_SUCCEEDED(opened_stream->get_Size(&size));
+  ComPtr<IClosable> closable_opened_stream;
+  ASSERT_HRESULT_SUCCEEDED(opened_stream.As(&closable_opened_stream));
+  ASSERT_HRESULT_SUCCEEDED(closable_opened_stream->Close());
 
   // Read the opened stream
   auto buffer = Make<FakeBuffer>(size);
@@ -194,6 +197,11 @@ TEST(FakeStorageFileStaticsTest, CreateStreamedFileAsync) {
   ASSERT_EQ(raw_buffer[1], 'i');
   ASSERT_EQ(raw_buffer[2], 's');
   ASSERT_EQ(raw_buffer[3], 'h');
+
+  // Cleanup
+  ComPtr<IClosable> closable_input_stream;
+  ASSERT_HRESULT_SUCCEEDED(input_stream.As(&closable_input_stream));
+  ASSERT_HRESULT_SUCCEEDED(closable_input_stream->Close());
 }
 
 }  // namespace webshare

chrome/browser/webshare/win/fake_uri_runtime_class_factory.h

@@ -12,7 +12,7 @@ namespace webshare {
 
 // Provides an implementation of IUriRuntimeClassFactory for use in GTests.
 class __declspec(uuid("93741C10-A511-410F-B2CA-3F0A2B674ECE"))
-    FakeUriRuntimeClassFactory
+    FakeUriRuntimeClassFactory final
     : public Microsoft::WRL::RuntimeClass<
           Microsoft::WRL::RuntimeClassFlags<Microsoft::WRL::WinRtClassicComMix>,
           ABI::Windows::Foundation::IUriRuntimeClassFactory> {

chrome/browser/webshare/win/share_operation.cc

@@ -98,14 +98,14 @@ HRESULT GetActivationFactory(InterfaceType** factory) {
 }
 
 // Implements FileStreamWriter for an IDataWriter.
-class DataWriterFileStreamWriter : public storage::FileStreamWriter {
+class DataWriterFileStreamWriter final : public storage::FileStreamWriter {
  public:
   explicit DataWriterFileStreamWriter(
       ComPtr<IDataWriter> data_writer,
       scoped_refptr<base::RefCountedData<uint64_t>> file_bytes_shared)
       : data_writer_(data_writer), file_bytes_shared_(file_bytes_shared) {}
 
-  int Cancel(net::CompletionOnceCallback callback) override {
+  int Cancel(net::CompletionOnceCallback callback) final {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
     // If there is no async operation in progress, Cancel() should
     // return net::ERR_UNEXPECTED per file_stream_header.h
@@ -142,7 +142,7 @@ class DataWriterFileStreamWriter : public storage::FileStreamWriter {
     return net::OK;
   }
 
-  int Flush(net::CompletionOnceCallback callback) override {
+  int Flush(net::CompletionOnceCallback callback) final {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
     DCHECK(flush_callback_.is_null());
     DCHECK_EQ(flush_operation_, nullptr);
@@ -163,7 +163,7 @@ class DataWriterFileStreamWriter : public storage::FileStreamWriter {
 
   int Write(net::IOBuffer* buf,
             int buf_len,
-            net::CompletionOnceCallback callback) override {
+            net::CompletionOnceCallback callback) final {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
     DCHECK(flush_callback_.is_null());
     DCHECK_EQ(flush_operation_, nullptr);

chrome/browser/webshare/win/share_operation.h

@@ -44,7 +44,7 @@ namespace webshare {
 
 class ShowShareUIForWindowOperation;
 
-class ShareOperation : content::WebContentsObserver {
+class ShareOperation final : content::WebContentsObserver {
  public:
   static void SetMaxFileBytesForTesting(uint64_t max_file_bytes);
 
@@ -59,7 +59,7 @@ class ShareOperation : content::WebContentsObserver {
                  content::WebContents* web_contents);
   ShareOperation(const ShareOperation&) = delete;
   ShareOperation& operator=(const ShareOperation&) = delete;
-  ~ShareOperation() override;
+  ~ShareOperation() final;
 
   base::WeakPtr<ShareOperation> AsWeakPtr();
 

chrome/browser/webshare/win/share_operation_unittest.cc

@@ -35,6 +35,7 @@ using ABI::Windows::Foundation::IAsyncOperation;
 using ABI::Windows::Foundation::IAsyncOperationCompletedHandler;
 using ABI::Windows::Foundation::IAsyncOperationWithProgress;
 using ABI::Windows::Foundation::IAsyncOperationWithProgressCompletedHandler;
+using ABI::Windows::Foundation::IClosable;
 using ABI::Windows::Storage::FileAccessMode;
 using ABI::Windows::Storage::IStorageFile;
 using ABI::Windows::Storage::Streams::IBuffer;
@@ -101,6 +102,9 @@ class ShareOperationUnitTest : public ChromeRenderViewHostTestHarness {
 
     ComPtr<IInputStream> input_stream;
     ASSERT_HRESULT_SUCCEEDED(stream->GetInputStreamAt(0, &input_stream));
+    ComPtr<IClosable> closable_stream;
+    ASSERT_HRESULT_SUCCEEDED(stream.As(&closable_stream));
+    ASSERT_HRESULT_SUCCEEDED(closable_stream->Close());
 
     auto buffer = Make<FakeBuffer>(size);
     {
@@ -135,6 +139,11 @@ class ShareOperationUnitTest : public ChromeRenderViewHostTestHarness {
       bytes[i] = raw_buffer[i];
     }
     result = std::string(bytes.begin(), bytes.end());
+
+    // Cleanup
+    ComPtr<IClosable> closable_input_stream;
+    ASSERT_HRESULT_SUCCEEDED(input_stream.As(&closable_input_stream));
+    ASSERT_HRESULT_SUCCEEDED(closable_input_stream->Close());
   }
 
   blink::mojom::SharedFilePtr CreateSharedFile(const std::string& name,