Improve support for multiselect list box accessibility on Windows.

base/win/enum_variant.h

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_WIN_ENUM_VARIANT_H_
+#define BASE_WIN_ENUM_VARIANT_H_
+#pragma once
+
+#include <unknwn.h>
+
+#include "base/memory/scoped_ptr.h"
+
+namespace base {
+namespace win {
+
+// A simple implementation of IEnumVARIANT.
+class BASE_EXPORT EnumVariant : public IEnumVARIANT {

[darin (slow to review), 2011/11/22 01:16:52]

I know you are naming this based on the interface type, but EnumVariant is a
very strange name for a class that represents an array of VARIANT objects. 
Maybe VariantArray would be better?  On the other hand, giving this a name that
resembles the interface would help re-inforce the idea that this class only
exists to provide an implementation of IEnumVARIANT.  Hmm...

[dmazzoni, 2011/11/22 08:08:14]

In this case, I favor something closer to the interface name, because there's no
possible reason anyone would ever choose to look for this except when an
IEnumVARIANT is required.

+ public:
+  // The constructor allocates an array of size |count|. Then use
+  // itemAt to set the value of each item in the array to initialize it.

[darin (slow to review), 2011/11/22 01:16:52]

This comment refers to a function named "itemAt" that doesn't exist.

[dmazzoni, 2011/11/22 08:08:14]

Renamed get() -> ItemAt() as per your suggestion below.

+  EnumVariant(unsigned long count);
+  virtual ~EnumVariant();

[darin (slow to review), 2011/11/22 01:16:52]

The destructor should not be public as deleting a EnumVariant instance directly
would result in a memory error.

[dmazzoni, 2011/11/22 08:08:14]

Done.

+
+  VARIANT* get(unsigned long index);

[darin (slow to review), 2011/11/22 01:16:52]

nit: non-inline functions should be given CamelCase names.  also, the function
name "get" is not super descriptive.  maybe GetItem would be better or ItemAt?

[dmazzoni, 2011/11/22 08:08:14]

Changed back to ItemAt (as you can see from the comment above, I waffled between
those two). :)

+
+  // IUnknown.
+  ULONG STDMETHODCALLTYPE AddRef();
+

[darin (slow to review), 2011/11/22 01:16:52]

nit: nuke the extra new lines between method declarations for implementations of
a specific interface.

[dmazzoni, 2011/11/22 08:08:14]

Done.

+  ULONG STDMETHODCALLTYPE Release();
+
+  STDMETHODIMP QueryInterface(REFIID riid, void** ppv);
+
+  // IEnumVARIANT.
+  STDMETHODIMP Next(ULONG requested_count,
+                    VARIANT* out_elements,
+                    ULONG* out_elements_received);
+
+  STDMETHODIMP Skip(ULONG skip_count);
+
+  STDMETHODIMP Reset();
+
+  STDMETHODIMP Clone(IEnumVARIANT** out_cloned_object);
+
+ private:
+  scoped_array<VARIANT> items_;
+  ULONG count_;
+  ULONG current_index_;
+
+  // IUnknown ref count.
+  ULONG iunknown_ref_count_;

[darin (slow to review), 2011/11/22 01:16:52]

nit: why bother with the iunknown_ prefix here?  it seems like this could be
named ref_count_ without ambiguity.

also, did you consider just deriving from RefCountedThreadSafe?  it seems better
to reuse that than to reimplement it.

[dmazzoni, 2011/11/22 08:08:14]

Unfortunately RefCountedThreadSafe has its own AddRef and Release methods that
are similar to the ones in IUnknown, but not the same. Mixing them up sounds
like a recipe for disaster.

Having a generic implementation of IUnknown might not be a bad idea, though. If
I end up needing an IUnknown implementation for another class I'll refactor it
then.

+};
+
+}  // namespace win
+}  // namespace base
+
+#endif  // BASE_WIN_ENUM_VARIANT_H_