Modernize EnumVariant and Remove Dependence on IUnknownImpl

BUG=1014283

Change-Id: Ieb4762c3c6b59cba40a55d60c980d76c0330180f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1860979Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Commit-Queue: Robert Liao <robliao@chromium.org>
Auto-Submit: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706485}

base/win/enum_variant.cc

@@ -4,6 +4,8 @@
 
 #include "base/win/enum_variant.h"
 
+#include <wrl/client.h>
+
 #include <algorithm>
 
 #include "base/logging.h"
@@ -26,27 +28,9 @@ VARIANT* EnumVariant::ItemAt(ULONG index) {
   return items_[index].AsInput();
 }
 
-ULONG STDMETHODCALLTYPE EnumVariant::AddRef() {
-  return IUnknownImpl::AddRef();
-}
-
-ULONG STDMETHODCALLTYPE EnumVariant::Release() {
-  return IUnknownImpl::Release();
-}
-
-STDMETHODIMP EnumVariant::QueryInterface(REFIID riid, void** ppv) {
-  if (riid == IID_IEnumVARIANT) {
-    *ppv = static_cast<IEnumVARIANT*>(this);
-    AddRef();
-    return S_OK;
-  }
-
-  return IUnknownImpl::QueryInterface(riid, ppv);
-}
-
-STDMETHODIMP EnumVariant::Next(ULONG requested_count,
-                               VARIANT* out_elements,
-                               ULONG* out_elements_received) {
+HRESULT EnumVariant::Next(ULONG requested_count,
+                          VARIANT* out_elements,
+                          ULONG* out_elements_received) {
   if (!out_elements)
     return E_INVALIDARG;
 
@@ -65,7 +49,7 @@ STDMETHODIMP EnumVariant::Next(ULONG requested_count,
   return (count == requested_count ? S_OK : S_FALSE);
 }
 
-STDMETHODIMP EnumVariant::Skip(ULONG skip_count) {
+HRESULT EnumVariant::Skip(ULONG skip_count) {
   ULONG count = skip_count;
   if (current_index_ + count > ULONG{items_.size()})
     count = ULONG{items_.size()} - current_index_;
@@ -74,24 +58,23 @@ STDMETHODIMP EnumVariant::Skip(ULONG skip_count) {
   return (count == skip_count ? S_OK : S_FALSE);
 }
 
-STDMETHODIMP EnumVariant::Reset() {
+HRESULT EnumVariant::Reset() {
   current_index_ = 0;
   return S_OK;
 }
 
-STDMETHODIMP EnumVariant::Clone(IEnumVARIANT** out_cloned_object) {
+HRESULT EnumVariant::Clone(IEnumVARIANT** out_cloned_object) {
   if (!out_cloned_object)
     return E_INVALIDARG;
 
   size_t count = items_.size();
-  EnumVariant* other = new EnumVariant(ULONG{count});
+  Microsoft::WRL::ComPtr<EnumVariant> other =
+      Microsoft::WRL::Make<EnumVariant>(ULONG{count});
   for (size_t i = 0; i < count; ++i)
     other->items_[i] = static_cast<const VARIANT&>(items_[i]);
 
   other->Skip(current_index_);
-  other->AddRef();
-  *out_cloned_object = other;
-  return S_OK;
+  return other.CopyTo(IID_PPV_ARGS(out_cloned_object));
 }
 
 }  // namespace win

base/win/enum_variant.h

@@ -5,12 +5,11 @@
 #ifndef BASE_WIN_ENUM_VARIANT_H_
 #define BASE_WIN_ENUM_VARIANT_H_
 
-#include <unknwn.h>
+#include <wrl/implements.h>
 
 #include <memory>
 #include <vector>
 
-#include "base/win/iunknown_impl.h"
 #include "base/win/scoped_variant.h"
 
 namespace base {
@@ -18,29 +17,25 @@ namespace win {
 
 // A simple implementation of IEnumVARIANT.
 class BASE_EXPORT EnumVariant
-  : public IEnumVARIANT,
-    public IUnknownImpl {
+    : public Microsoft::WRL::RuntimeClass<
+          Microsoft::WRL::RuntimeClassFlags<Microsoft::WRL::ClassicCom>,
+          IEnumVARIANT> {
  public:
   // The constructor allocates a vector of empty ScopedVariants of size |count|.
   // Use ItemAt to set the value of each item in the array.
   explicit EnumVariant(ULONG count);
 
+  // IEnumVARIANT:
+  IFACEMETHODIMP Next(ULONG requested_count,
+                      VARIANT* out_elements,
+                      ULONG* out_elements_received) override;
+  IFACEMETHODIMP Skip(ULONG skip_count) override;
+  IFACEMETHODIMP Reset() override;
+  IFACEMETHODIMP Clone(IEnumVARIANT** out_cloned_object) override;
+
   // Returns a mutable pointer to the item at position |index|.
   VARIANT* ItemAt(ULONG index);
 
-  // IUnknown.
-  ULONG STDMETHODCALLTYPE AddRef() override;
-  ULONG STDMETHODCALLTYPE Release() override;
-  STDMETHODIMP QueryInterface(REFIID riid, void** ppv) override;
-
-  // IEnumVARIANT.
-  STDMETHODIMP Next(ULONG requested_count,
-                    VARIANT* out_elements,
-                    ULONG* out_elements_received) override;
-  STDMETHODIMP Skip(ULONG skip_count) override;
-  STDMETHODIMP Reset() override;
-  STDMETHODIMP Clone(IEnumVARIANT** out_cloned_object) override;
-
  private:
   ~EnumVariant() override;
 

base/win/enum_variant_unittest.cc

@@ -4,7 +4,11 @@
 
 #include "base/win/enum_variant.h"
 
+#include <wrl/client.h>
+#include <wrl/implements.h>
+
 #include "base/win/scoped_com_initializer.h"
+#include "base/win/scoped_variant.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace base {
@@ -13,52 +17,34 @@ namespace win {
 TEST(EnumVariantTest, EmptyEnumVariant) {
   ScopedCOMInitializer com_initializer;
 
-  EnumVariant* ev = new EnumVariant(0);
-  ev->AddRef();
-
-  IUnknown* iunknown;
-  EXPECT_TRUE(SUCCEEDED(
-      ev->QueryInterface(IID_IUnknown, reinterpret_cast<void**>(&iunknown))));
-  iunknown->Release();
+  Microsoft::WRL::ComPtr<EnumVariant> ev = Microsoft::WRL::Make<EnumVariant>(0);
+  Microsoft::WRL::ComPtr<IEnumVARIANT> ienumvariant;
+  ASSERT_TRUE(SUCCEEDED(ev->QueryInterface(IID_PPV_ARGS(&ienumvariant))));
 
-  IEnumVARIANT* ienumvariant;
-  EXPECT_TRUE(SUCCEEDED(
-      ev->QueryInterface(IID_IEnumVARIANT,
-                         reinterpret_cast<void**>(&ienumvariant))));
-  EXPECT_EQ(ev, ienumvariant);
-  ienumvariant->Release();
-
-  VARIANT out_element;
-  ::VariantInit(&out_element);
-  ULONG out_received = 0;
-  EXPECT_EQ(S_FALSE, ev->Next(1, &out_element, &out_received));
-  EXPECT_EQ(0u, out_received);
-  ::VariantClear(&out_element);
+  {
+    base::win::ScopedVariant out_element;
+    ULONG out_received = 0;
+    EXPECT_EQ(S_FALSE, ev->Next(1, out_element.Receive(), &out_received));
+    EXPECT_EQ(0u, out_received);
+  }
 
   EXPECT_EQ(S_FALSE, ev->Skip(1));
 
   EXPECT_EQ(S_OK, ev->Reset());
 
-  IEnumVARIANT* ev2 = NULL;
+  Microsoft::WRL::ComPtr<IEnumVARIANT> ev2;
   EXPECT_EQ(S_OK, ev->Clone(&ev2));
 
-  EXPECT_NE(static_cast<IEnumVARIANT*>(NULL), ev2);
+  EXPECT_NE(nullptr, ev2);
   EXPECT_NE(ev, ev2);
   EXPECT_EQ(S_FALSE, ev2->Skip(1));
   EXPECT_EQ(S_OK, ev2->Reset());
-
-  ULONG ev2_finalrefcount = ev2->Release();
-  EXPECT_EQ(0u, ev2_finalrefcount);
-
-  ULONG ev_finalrefcount = ev->Release();
-  EXPECT_EQ(0u, ev_finalrefcount);
 }
 
 TEST(EnumVariantTest, SimpleEnumVariant) {
   ScopedCOMInitializer com_initializer;
 
-  EnumVariant* ev = new EnumVariant(3);
-  ev->AddRef();
+  Microsoft::WRL::ComPtr<EnumVariant> ev = Microsoft::WRL::Make<EnumVariant>(3);
   ev->ItemAt(0)->vt = VT_I4;
   ev->ItemAt(0)->lVal = 10;
   ev->ItemAt(1)->vt = VT_I4;
@@ -66,31 +52,36 @@ TEST(EnumVariantTest, SimpleEnumVariant) {
   ev->ItemAt(2)->vt = VT_I4;
   ev->ItemAt(2)->lVal = 30;
 
-  // Get elements one at a time.
-  VARIANT out_element;
-  ::VariantInit(&out_element);
-  ULONG out_received = 0;
-  EXPECT_EQ(S_OK, ev->Next(1, &out_element, &out_received));
-  EXPECT_EQ(1u, out_received);
-  EXPECT_EQ(VT_I4, out_element.vt);
-  EXPECT_EQ(10, out_element.lVal);
-  ::VariantClear(&out_element);
+  // Get elements one at a time from index 0 and 2.
+  base::win::ScopedVariant out_element_0;
+  ULONG out_received_0 = 0;
+  EXPECT_EQ(S_OK, ev->Next(1, out_element_0.Receive(), &out_received_0));
+  EXPECT_EQ(1u, out_received_0);
+  EXPECT_EQ(VT_I4, out_element_0.ptr()->vt);
+  EXPECT_EQ(10, out_element_0.ptr()->lVal);
+
   EXPECT_EQ(S_OK, ev->Skip(1));
-  EXPECT_EQ(S_OK, ev->Next(1, &out_element, &out_received));
-  EXPECT_EQ(1u, out_received);
-  EXPECT_EQ(VT_I4, out_element.vt);
-  EXPECT_EQ(30, out_element.lVal);
-  ::VariantClear(&out_element);
-  EXPECT_EQ(S_FALSE, ev->Next(1, &out_element, &out_received));
-  ::VariantClear(&out_element);
-
-  // Reset and get all elements at once.
+
+  base::win::ScopedVariant out_element_2;
+  ULONG out_received_2 = 0;
+  EXPECT_EQ(S_OK, ev->Next(1, out_element_2.Receive(), &out_received_2));
+  EXPECT_EQ(1u, out_received_2);
+  EXPECT_EQ(VT_I4, out_element_2.ptr()->vt);
+  EXPECT_EQ(30, out_element_2.ptr()->lVal);
+
+  base::win::ScopedVariant placeholder_variant;
+  EXPECT_EQ(S_FALSE, ev->Next(1, placeholder_variant.Receive(), nullptr));
+
+  // Verify the reset works for the next step.
+  ASSERT_EQ(S_OK, ev->Reset());
+
+  // Get all elements at once.
   VARIANT out_elements[3];
+  ULONG out_received_multiple;
   for (int i = 0; i < 3; ++i)
     ::VariantInit(&out_elements[i]);
-  EXPECT_EQ(S_OK, ev->Reset());
-  EXPECT_EQ(S_OK, ev->Next(3, out_elements, &out_received));
-  EXPECT_EQ(3u, out_received);
+  EXPECT_EQ(S_OK, ev->Next(3, out_elements, &out_received_multiple));
+  EXPECT_EQ(3u, out_received_multiple);
   EXPECT_EQ(VT_I4, out_elements[0].vt);
   EXPECT_EQ(10, out_elements[0].lVal);
   EXPECT_EQ(VT_I4, out_elements[1].vt);
@@ -99,16 +90,31 @@ TEST(EnumVariantTest, SimpleEnumVariant) {
   EXPECT_EQ(30, out_elements[2].lVal);
   for (int i = 0; i < 3; ++i)
     ::VariantClear(&out_elements[i]);
-  EXPECT_EQ(S_FALSE, ev->Next(1, &out_element, &out_received));
-  ::VariantClear(&out_element);
+
+  base::win::ScopedVariant placeholder_variant_multiple;
+  EXPECT_EQ(S_FALSE,
+            ev->Next(1, placeholder_variant_multiple.Receive(), nullptr));
+}
+
+TEST(EnumVariantTest, Clone) {
+  ScopedCOMInitializer com_initializer;
+
+  Microsoft::WRL::ComPtr<EnumVariant> ev = Microsoft::WRL::Make<EnumVariant>(3);
+  ev->ItemAt(0)->vt = VT_I4;
+  ev->ItemAt(0)->lVal = 10;
+  ev->ItemAt(1)->vt = VT_I4;
+  ev->ItemAt(1)->lVal = 20;
+  ev->ItemAt(2)->vt = VT_I4;
+  ev->ItemAt(2)->lVal = 30;
 
   // Clone it.
-  IEnumVARIANT* ev2 = NULL;
+  Microsoft::WRL::ComPtr<IEnumVARIANT> ev2;
   EXPECT_EQ(S_OK, ev->Clone(&ev2));
   EXPECT_TRUE(ev2 != NULL);
-  EXPECT_EQ(S_FALSE, ev->Next(1, &out_element, &out_received));
-  ::VariantClear(&out_element);
-  EXPECT_EQ(S_OK, ev2->Reset());
+
+  VARIANT out_elements[3];
+  for (int i = 0; i < 3; ++i)
+    ::VariantInit(&out_elements[i]);
   EXPECT_EQ(S_OK, ev2->Next(3, out_elements, nullptr));
   EXPECT_EQ(VT_I4, out_elements[0].vt);
   EXPECT_EQ(10, out_elements[0].lVal);
@@ -118,14 +124,6 @@ TEST(EnumVariantTest, SimpleEnumVariant) {
   EXPECT_EQ(30, out_elements[2].lVal);
   for (int i = 0; i < 3; ++i)
     ::VariantClear(&out_elements[i]);
-  EXPECT_EQ(S_FALSE, ev2->Next(1, &out_element, nullptr));
-  ::VariantClear(&out_element);
-
-  ULONG ev2_finalrefcount = ev2->Release();
-  EXPECT_EQ(0u, ev2_finalrefcount);
-
-  ULONG ev_finalrefcount = ev->Release();
-  EXPECT_EQ(0u, ev_finalrefcount);
 }
 
 }  // namespace win

ui/accessibility/platform/ax_platform_node_win.cc

@@ -5,6 +5,7 @@
 #include "ui/accessibility/platform/ax_platform_node_win.h"
 
 #include <wrl/client.h>
+#include <wrl/implements.h>
 
 #include <algorithm>
 #include <map>
@@ -1181,16 +1182,14 @@ IFACEMETHODIMP AXPlatformNodeWin::get_accSelection(VARIANT* selected) {
 
   // Multiple items are selected.
   LONG selected_count = static_cast<LONG>(selected_nodes.size());
-  auto* enum_variant = new base::win::EnumVariant(selected_count);
-  enum_variant->AddRef();
+  Microsoft::WRL::ComPtr<base::win::EnumVariant> enum_variant =
+      Microsoft::WRL::Make<base::win::EnumVariant>(selected_count);
   for (LONG i = 0; i < selected_count; ++i) {
     enum_variant->ItemAt(i)->vt = VT_DISPATCH;
     enum_variant->ItemAt(i)->pdispVal = selected_nodes[i].Detach();
   }
   selected->vt = VT_UNKNOWN;
-  HRESULT hr = enum_variant->QueryInterface(IID_PPV_ARGS(&V_UNKNOWN(selected)));
-  enum_variant->Release();
-  return hr;
+  return enum_variant.CopyTo(IID_PPV_ARGS(&V_UNKNOWN(selected)));
 }
 
 IFACEMETHODIMP AXPlatformNodeWin::accSelect(LONG flagsSelect, VARIANT var_id) {