Improve support for multiselect list box accessibility on Windows.

Improve support for multiselect list box accessibility on Windows.

Exposes several new states and attributes and fires new notifications
when the selection changes. Adds general-purpose implementations of
IUnknown and IEnumVARIANT to base/win/, which will be used in the future
as we remote ATL from accessibility code.

BUG=5027, 98984
TEST=manual testing with JAWS and NVDA

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112870

[dmazzoni, 2011-11-21 23:57:05]

No rush, and happy Thanksgiving!

dtseng: accessibility code
thakis: base/win (or feel free to delegate)
tony: webkit/glue

[Nico, 2011-11-22 00:02:43]

I'm not that great with windows (I could review the patch, but I can't judge if
this is something useful enough that it should be in base).

Since brettw is out, darin is probably the best base owner we have for this.
Ping him liberally :-)

[tony, 2011-11-22 00:05:10]

rubber stamp LGTM for webkit/glue

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

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.cc
File base/win/enum_variant.cc (right):

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.cc#new...
base/win/enum_variant.cc:81: *other->get(i) = items_[i];
it is interesting that there is no memory management required here.  this code
is creating multiple references to the same VARIANT.  if the lifetime of the
resulting clone is longer than the object being cloned, then isn't there the
potential for the VARIANT items to reference deleted memory?  i'm assuming that
it is possible for the VARIANTs to reference heap allocated memory.  although, i
don't see any special cleanup happening in ~EnumVariant.

is there some restriction on the type of VARIANTs that should be used with
EnumVariant?

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h
File base/win/enum_variant.h (right):

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h#newc...
base/win/enum_variant.h:17: class BASE_EXPORT EnumVariant : public IEnumVARIANT
{
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...

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h#newc...
base/win/enum_variant.h:20: // itemAt to set the value of each item in the array
to initialize it.
This comment refers to a function named "itemAt" that doesn't exist.

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h#newc...
base/win/enum_variant.h:22: virtual ~EnumVariant();
The destructor should not be public as deleting a EnumVariant instance directly
would result in a memory error.

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h#newc...
base/win/enum_variant.h:24: VARIANT* get(unsigned long index);
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?

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h#newc...
base/win/enum_variant.h:28: 
nit: nuke the extra new lines between method declarations for implementations of
a specific interface.

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h#newc...
base/win/enum_variant.h:50: ULONG iunknown_ref_count_;
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.

http://codereview.chromium.org/8588036/diff/2001/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/8588036/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:537: if
((children_[i]->state() >> WebAccessibility::STATE_SELECTED) & 1)
nit: A HasState method might be nice here.

  if (children_[i]->HasState(WebAccessibility::STATE_SELECTED))
    ...

^^^ more readable

http://codereview.chromium.org/8588036/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:565:
children_[i]->toBrowserAccessibilityWin()->NewReference();
NewReference appears to take a reference count.  It calls AddRef().  How does
that reference ever get released?

http://codereview.chromium.org/8588036/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:2400:
string16(L"setsize:") + base::IntToString16(parent_->child_count()));
nit: is it necessary to wrap those constants with string16() constructors?  we
should have an operator+ that is happy provided either the LHS or RHS is a
string16.

http://codereview.chromium.org/8588036/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:2546: old_ia_state_ =
ia_state_;
do we have to worry about |this| being deleted upon return from NotifyWinEvent? 
since you are calling into crazy hooker code, i worry about that foreign code
doing things like trying to close the browser that might cause the
BrowserAccessibilityWin object to get deleted before returning from
NotifyWinEvent.  is that a valid concern?

http://codereview.chromium.org/8588036/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:2898: if ((state_ >>
WebAccessibility::STATE_FOCUSED) & 1)
again, it feels like a HasState type function might help with readability.

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

Thanks!

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.cc
File base/win/enum_variant.cc (right):

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.cc#new...
base/win/enum_variant.cc:81: *other->get(i) = items_[i];
On 2011/11/22 01:16:52, darin wrote:
> it is interesting that there is no memory management required here.  this code
> is creating multiple references to the same VARIANT.  if the lifetime of the
> resulting clone is longer than the object being cloned, then isn't there the
> potential for the VARIANT items to reference deleted memory?  i'm assuming
that
> it is possible for the VARIANTs to reference heap allocated memory.  although,
i
> don't see any special cleanup happening in ~EnumVariant.
> 
> is there some restriction on the type of VARIANTs that should be used with
> EnumVariant?

Good question.

A VARIANT isn't an object, it's data. It might contain a pointer, yes - but the
fact that the pointer is contained inside a VARIANT has no impact on its
reference count or ownership semantics. The VARIANT itself does not "own" a
reference to an object that it points to, if it contains a pointer.

Suppose you're a client and you call a COM method that returns an array of
results in an IEnumVARIANT. If any of those items are of type VT_DISPATCH, then
the method has just given you a new reference to that result, and it's now your
responsibility to Release it, just as if that same method had returned a single
VARIANT.

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h
File base/win/enum_variant.h (right):

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h#newc...
base/win/enum_variant.h:17: class BASE_EXPORT EnumVariant : public IEnumVARIANT
{
On 2011/11/22 01:16:52, darin wrote:
> 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...

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.

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h#newc...
base/win/enum_variant.h:20: // itemAt to set the value of each item in the array
to initialize it.
On 2011/11/22 01:16:52, darin wrote:
> This comment refers to a function named "itemAt" that doesn't exist.

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

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h#newc...
base/win/enum_variant.h:22: virtual ~EnumVariant();
On 2011/11/22 01:16:52, darin wrote:
> The destructor should not be public as deleting a EnumVariant instance
directly
> would result in a memory error.

Done.

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h#newc...
base/win/enum_variant.h:24: VARIANT* get(unsigned long index);
On 2011/11/22 01:16:52, darin wrote:
> 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?

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

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h#newc...
base/win/enum_variant.h:28: 
On 2011/11/22 01:16:52, darin wrote:
> nit: nuke the extra new lines between method declarations for implementations
of
> a specific interface.

Done.

http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h#newc...
base/win/enum_variant.h:50: ULONG iunknown_ref_count_;
On 2011/11/22 01:16:52, darin wrote:
> 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.

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.

http://codereview.chromium.org/8588036/diff/2001/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/8588036/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:537: if
((children_[i]->state() >> WebAccessibility::STATE_SELECTED) & 1)
On 2011/11/22 01:16:52, darin wrote:
> nit: A HasState method might be nice here.
> 
>   if (children_[i]->HasState(WebAccessibility::STATE_SELECTED))
>     ...
> 
> ^^^ more readable

Good idea, done.

http://codereview.chromium.org/8588036/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:565:
children_[i]->toBrowserAccessibilityWin()->NewReference();
On 2011/11/22 01:16:52, darin wrote:
> NewReference appears to take a reference count.  It calls AddRef().  How does
> that reference ever get released?

It's the responsibility of the client who calls this method. 

See the memory management rules here:
http://msdn.microsoft.com/en-us/library/ms810016.aspx - normally the rules are
pretty simple, in this class we're implementing a COM server so whenever we
return a pointer, we always increment the reference count.

IEnumVARIANT seems confusing at first but you should just think of it as an
array. If the items in the array are pointers, the same semantics apply - we
increment the reference count, and it's the client's responsibility to release
it.

http://codereview.chromium.org/8588036/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:2400:
string16(L"setsize:") + base::IntToString16(parent_->child_count()));
On 2011/11/22 01:16:52, darin wrote:
> nit: is it necessary to wrap those constants with string16() constructors?  we
> should have an operator+ that is happy provided either the LHS or RHS is a
> string16.

You're right, thanks.

http://codereview.chromium.org/8588036/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:2546: old_ia_state_ =
ia_state_;
On 2011/11/22 01:16:52, darin wrote:
> do we have to worry about |this| being deleted upon return from
NotifyWinEvent? 
> since you are calling into crazy hooker code, i worry about that foreign code
> doing things like trying to close the browser that might cause the
> BrowserAccessibilityWin object to get deleted before returning from
> NotifyWinEvent.  is that a valid concern?

I don't think we have to worry about that. It'd do that by sending a WM_CLOSE or
WM_DESTROY to the window, which would get caught in the main message loop, not
inside this call stack.

The main thing we do have to worry about here is that some client that's hooked
NotifyWinEvent might query this object and maybe even explore the a11y tree
before NotifyWinEvent returns. That's why these notifications are sent in a
separate SendNodeUpdateEvents method, which happens after the tree is connected
up, rather than during initialization.

http://codereview.chromium.org/8588036/diff/2001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:2898: if ((state_ >>
WebAccessibility::STATE_FOCUSED) & 1)
On 2011/11/22 01:16:52, darin wrote:
> again, it feels like a HasState type function might help with readability.

Done.

[M-A Ruel, 2011-11-22 14:42:30]

I'd highly prefer a enum_variant_test.cc.

Also you can probably add 5027 to the list of bugs involved.

http://codereview.chromium.org/8588036/diff/9001/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/8588036/diff/9001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:573: return
E_NOTIMPL;
The error code isn't correct now. If clidren_size() == 0, S_FALSE would make
more sense, and setting the variant to empty? And I'd vote for an early exit
instead of whole indentation unless it would be sensible to add other role_
implementation.

http://codereview.chromium.org/8588036/diff/9001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:740: return
E_NOTIMPL;
same

[dmazzoni, 2011-11-23 06:18:10]

I'll add a test. There will probably be several other COM classes like this
going into base/win/ as I try to replace ATL, so it'll be a good idea to have
test coverage for all of these.

I'll probably finish this after the holiday. Happy Thanksgiving!

http://codereview.chromium.org/8588036/diff/9001/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/8588036/diff/9001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:573: return
E_NOTIMPL;
On 2011/11/22 14:42:30, Marc-Antoine Ruel wrote:
> The error code isn't correct now. If clidren_size() == 0, S_FALSE would make
> more sense, and setting the variant to empty? And I'd vote for an early exit
> instead of whole indentation unless it would be sensible to add other role_
> implementation.

You're right, it doesn't make sense to return E_NOTIMPL for an empty list. I
changed it to do an early exit of E_NOTIMPL if it's any other role. (We may add
support for other roles as needed.)

While it's common to return S_FALSE for an empty result, the get_accSelection
docs specifically say to return S_OK. Also, this matches Firefox's impl (I
checked the source).

Returning E_NOTIMPL rather than empty for roles we don't support yet is very
important, because assistive technology may try another method to figure out
information if it gets E_NOTIMPL. If it get S_FALSE it might assume this method
is supported but this object has no selection, which may be incorrect and may be
a regression from the current behavior.

http://codereview.chromium.org/8588036/diff/9001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:740: return
E_NOTIMPL;
On 2011/11/22 14:42:30, Marc-Antoine Ruel wrote:
> same

I think this one is right - see reasoning above.

[darin (slow to review), 2011-11-23 16:55:41]

On Tue, Nov 22, 2011 at 12:08 AM, <dmazzoni@chromium.org> wrote:

> Thanks!
>
>
>
> http://codereview.chromium.**org/8588036/diff/2001/base/**
>
win/enum_variant.cc<http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.cc>
> File base/win/enum_variant.cc (right):
>
> http://codereview.chromium.**org/8588036/diff/2001/base/**
>
win/enum_variant.cc#newcode81<http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.cc#newcode81>
> base/win/enum_variant.cc:81: *other->get(i) = items_[i];
> On 2011/11/22 01:16:52, darin wrote:
>
>> it is interesting that there is no memory management required here.
>>
> this code
>
>> is creating multiple references to the same VARIANT.  if the lifetime
>>
> of the
>
>> resulting clone is longer than the object being cloned, then isn't
>>
> there the
>
>> potential for the VARIANT items to reference deleted memory?  i'm
>>
> assuming that
>
>> it is possible for the VARIANTs to reference heap allocated memory.
>>
> although, i
>
>> don't see any special cleanup happening in ~EnumVariant.
>>
>
>  is there some restriction on the type of VARIANTs that should be used
>>
> with
>
>> EnumVariant?
>>
>
> Good question.
>
> A VARIANT isn't an object, it's data. It might contain a pointer, yes -
> but the fact that the pointer is contained inside a VARIANT has no
> impact on its reference count or ownership semantics. The VARIANT itself
> does not "own" a reference to an object that it points to, if it
> contains a pointer.
>
> Suppose you're a client and you call a COM method that returns an array
> of results in an IEnumVARIANT. If any of those items are of type
> VT_DISPATCH, then the method has just given you a new reference to that
> result, and it's now your responsibility to Release it, just as if that
> same method had returned a single VARIANT.


OK



>
>
> http://codereview.chromium.**org/8588036/diff/2001/base/**
>
win/enum_variant.h<http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h>
> File base/win/enum_variant.h (right):
>
> http://codereview.chromium.**org/8588036/diff/2001/base/**
>
win/enum_variant.h#newcode17<http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h#newcode17>
> base/win/enum_variant.h:17: class BASE_EXPORT EnumVariant : public
> IEnumVARIANT {
> On 2011/11/22 01:16:52, darin wrote:
>
>> 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...
>>
>
> 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.
>
>
OK


>
> http://codereview.chromium.**org/8588036/diff/2001/base/**
>
win/enum_variant.h#newcode50<http://codereview.chromium.org/8588036/diff/2001/base/win/enum_variant.h#newcode50>
> base/win/enum_variant.h:50: ULONG iunknown_ref_count_;
> On 2011/11/22 01:16:52, darin wrote:
>
>> 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.
>>
>
> 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.
>
>
I think you should at the very least use AtomicRefCountInc and
AtomicRefCountDec from base/atomic_ref_count.h. However, I still think you
can directly use RefCountedThreadSafe here.  The fact that the method
signatures are the same should be a non-issue.  What problem are you
worried about?



>
> http://codereview.chromium.**org/8588036/diff/2001/content/**
>
browser/accessibility/browser_**accessibility_win.cc#**newcode565<http://codereview.chromium.org/8588036/diff/2001/content/browser/accessibility/browser_accessibility_win.cc#newcode565>
> content/browser/accessibility/**browser_accessibility_win.cc:**565:
> children_[i]->**toBrowserAccessibilityWin()->**NewReference();
> On 2011/11/22 01:16:52, darin wrote:
>
>> NewReference appears to take a reference count.  It calls AddRef().
>>
> How does
>
>> that reference ever get released?
>>
>
> It's the responsibility of the client who calls this method.
>

Oh right... I have tried hard to forget everything I ever knew about COM ;-)

>
>
> http://codereview.chromium.**org/8588036/diff/2001/content/**
>
browser/accessibility/browser_**accessibility_win.cc#**newcode2546<http://codereview.chromium.org/8588036/diff/2001/content/browser/accessibility/browser_accessibility_win.cc#newcode2546>
> content/browser/accessibility/**browser_accessibility_win.cc:**2546:
> old_ia_state_ = ia_state_;
> On 2011/11/22 01:16:52, darin wrote:
>
>> do we have to worry about |this| being deleted upon return from
>>
> NotifyWinEvent?
>
>> since you are calling into crazy hooker code, i worry about that
>>
> foreign code
>
>> doing things like trying to close the browser that might cause the
>> BrowserAccessibilityWin object to get deleted before returning from
>> NotifyWinEvent.  is that a valid concern?
>>
>
> I don't think we have to worry about that. It'd do that by sending a
> WM_CLOSE or WM_DESTROY to the window, which would get caught in the main
> message loop, not inside this call stack.
>
> The main thing we do have to worry about here is that some client that's
> hooked NotifyWinEvent might query this object and maybe even explore the
> a11y tree before NotifyWinEvent returns. That's why these notifications
> are sent in a separate SendNodeUpdateEvents method, which happens after
> the tree is connected up, rather than during initialization.
>
>
OK... but it still makes me nervous to call out to foreign code and then
directly manipulate member variables after that.

-Darin



>
http://codereview.chromium.**org/8588036/<http://codereview.chromium.org/8588...
>

[dmazzoni, 2011-11-23 17:50:12]

On Wed, Nov 23, 2011 at 8:55 AM, Darin Fisher <darin@chromium.org> wrote:

> I think you should at the very least use AtomicRefCountInc and
> AtomicRefCountDec from base/atomic_ref_count.h. However, I still think you
> can directly use RefCountedThreadSafe here.  The fact that the method
> signatures are the same should be a non-issue.  What problem are you
> worried about?
>

Multiple inheritance of two classes with similar but not identical method
signatures can be dangerous - you can cast an instance to one of the parent
types and call the wrong method with no compile error. But I guess I'm
being too paranoid in this case.

OK, I'll add a ref count accessor to RefCountedThreadSafe and derive from
it.

- Dominic

[dmazzoni, 2011-11-28 22:37:43]

Ready for another look. I added a unit test and made it derive from
RefCountedThreadSafe.

Please take a close look at the changes to AtomicRefCount and
RefCountedThreadSafe - I had to modify them to return the ref count rather than
just a boolean, because that's required by the IUnknown methods. In particular,
I'm worried that this required adding some extra barriers that weren't there
before, which could have performance implications. I could create variants of
every method that returns the ref count and leave the old implementations alone,
but then it makes me wonder whether there's any benefit to using
RefCountedThreadSafe at all...

[darin (slow to review), 2011-11-28 23:07:31]

Wow... COM says that AddRef and Release don't necessarily need to return
the actual ref-count of the object.  Maybe you should make them always lie
and return 1?  Or, if you are implementing in terms of AtomicRefCount, then
just return the AtomicRefCount value.  It is OK for it to be stale, right?

If you really need them to return the actual ref-count, then I agree that
changing atomic_ref_count.h and ref_counted.h seems unfortunate, and we
should probably do something else.  If you are going to use raw
Interlocked* functions, then you may need to add ANNOTATE_ macros.  I defer
to timurrrr, et. al.

-Darin


On Mon, Nov 28, 2011 at 2:37 PM, <dmazzoni@chromium.org> wrote:

> Ready for another look. I added a unit test and made it derive from
> RefCountedThreadSafe.
>
> Please take a close look at the changes to AtomicRefCount and
> RefCountedThreadSafe - I had to modify them to return the ref count rather
> than
> just a boolean, because that's required by the IUnknown methods. In
> particular,
> I'm worried that this required adding some extra barriers that weren't
> there
> before, which could have performance implications. I could create variants
> of
> every method that returns the ref count and leave the old implementations
> alone,
> but then it makes me wonder whether there's any benefit to using
> RefCountedThreadSafe at all...
>
>
>
http://codereview.chromium.**org/8588036/<http://codereview.chromium.org/8588...
>

[dmazzoni, 2011-11-29 00:08:37]

On Mon, Nov 28, 2011 at 3:07 PM, Darin Fisher <darin@chromium.org> wrote:

> Wow... COM says that AddRef and Release don't necessarily need to return
> the actual ref-count of the object.  Maybe you should make them always lie
> and return 1?  Or, if you are implementing in terms of AtomicRefCount, then
> just return the AtomicRefCount value.  It is OK for it to be stale, right?


Where did you see it say that it doesn't have to return the actual ref
count? I was looking here:

http://msdn.microsoft.com/en-us/library/windows/desktop/ms691379(v=vs.85).aspx

I'd hate to fake the value of the ref count and discover 6 months from now
it's causing an obscure failure in some random tool we want to interoperate
with. It might be okay if the value returned is stale, but that makes me
think inheriting from RefCountedThreadSafe is misleading, because the
operation of [incrementing or decrementing and returning the new ref count]
wouldn't actually be atomic.

If you really need them to return the actual ref-count, then I agree that
> changing atomic_ref_count.h and ref_counted.h seems unfortunate, and we
> should probably do something else.  If you are going to use raw
> Interlocked* functions, then you may need to add ANNOTATE_ macros.  I defer
> to timurrrr, et. al.
>

Alternative idea: how about I add new methods to atomic_ref_count (that
return the new ref count) and use those directly instead of modifying
RefCountedThreadSafe?

Either way, how about I create a new IUnknownImpl class (or suggest a
better name?) that just implements AddRef and Release as needed by
IUnknown; this would be a parent class of EnumVariant instead of
RefCountedThreadSafe. I'll probably need to reuse this 3 - 4 more times as
I get rid of ATL from accessibility code.

- Dominic

[darin (slow to review), 2011-11-29 00:27:26]

On Mon, Nov 28, 2011 at 4:08 PM, Dominic Mazzoni <dmazzoni@chromium.org>wrote:

> On Mon, Nov 28, 2011 at 3:07 PM, Darin Fisher <darin@chromium.org> wrote:
>
>> Wow... COM says that AddRef and Release don't necessarily need to return
>> the actual ref-count of the object.  Maybe you should make them always lie
>> and return 1?  Or, if you are implementing in terms of AtomicRefCount, then
>> just return the AtomicRefCount value.  It is OK for it to be stale, right?
>
>
> Where did you see it say that it doesn't have to return the actual ref
> count? I was looking here:
>
>
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms691379(v=vs.85).aspx
>
> I'd hate to fake the value of the ref count and discover 6 months from now
> it's causing an obscure failure in some random tool we want to interoperate
> with. It might be okay if the value returned is stale, but that makes me
> think inheriting from RefCountedThreadSafe is misleading, because the
> operation of [incrementing or decrementing and returning the new ref count]
> wouldn't actually be atomic.
>

Hmm, yeah... MSDN doesn't make the claim quite as strongly as I recall.
 Maybe I read it in a book.  Yeah... page 70 of Inside
COM<http://www.amazon.com/Inside-Microsoft-Programming-Dale-Rogerson/dp/1572313498>
.

I think the point is that the value is racy.  Once you put it on the stack
to return it, it may already be out-of-date.  All you can promise to
someone is that if they have a reference, then the ref-count is >= 1.



>
> If you really need them to return the actual ref-count, then I agree that
>> changing atomic_ref_count.h and ref_counted.h seems unfortunate, and we
>> should probably do something else.  If you are going to use raw
>> Interlocked* functions, then you may need to add ANNOTATE_ macros.  I defer
>> to timurrrr, et. al.
>>
>
> Alternative idea: how about I add new methods to atomic_ref_count (that
> return the new ref count) and use those directly instead of modifying
> RefCountedThreadSafe?
>

You can just read the Atomic32 value directly, right?


>
> Either way, how about I create a new IUnknownImpl class (or suggest a
> better name?) that just implements AddRef and Release as needed by
> IUnknown; this would be a parent class of EnumVariant instead of
> RefCountedThreadSafe. I'll probably need to reuse this 3 - 4 more times
> as I get rid of ATL from accessibility code.
>

+1 for IUnknownImpl


>
> - Dominic
>
>

[dmazzoni, 2011-11-29 19:57:02]

On 2011/11/29 00:27:26, darin wrote:
> +1 for IUnknownImpl

Ready for another look. I created IUnknownImpl and made EnumVariant derive from
it, with unit tests for both.

I added new methods to atomic_ref_count because otherwise there'd be a race
condition where two clients could both get 0 back from Release(), making them
both think they were the ones to release the object. Plus even if we didn't care
about that case, implementing AddRef and Release non-atomically could trigger
warnings in data-race-detection tools, right?

One new ugliness is that due to the multiple inheritance of IUnknown,
EnumVariant has to implement all of the IUnknown methods too (by calling the
correct base class implementation). I'm not sure if there's a way around this -
Microsoft's CUnknown and ATL each require you to do this too, but they abstract
it via macros. The clean solution would be virtual inheritance, but we can't
change IEnumVARIANT to inherit virtually from IUnknown.

I still think it's better to have IUnknownImpl because I'll definitely use it
for more classes.

[M-A Ruel, 2011-11-29 20:28:31]

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant.cc
File base/win/enum_variant.cc (right):

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant.cc#ne...
base/win/enum_variant.cc:14: items_.reset(new VARIANT[count_]);
Why not do it in the member constructor?

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant.cc#ne...
base/win/enum_variant.cc:36: *ppv = this;
In theory, you need to static_cast<IEnumVariant*>(this)

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant.cc#ne...
base/win/enum_variant.cc:47: unsigned long count = requested_count;
unsigned long count = min(requested_count, count_ - current_index_);

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant.cc#ne...
base/win/enum_variant.cc:76: *other->ItemAt(i) = items_[i];
Why is this better than a memcpy or just implementing a private copy
constructor?

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant.h
File base/win/enum_variant.h (right):

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant.h#new...
base/win/enum_variant.h:24: explicit EnumVariant(unsigned long count);
why do you alternate between unsigned long and ULONG? e.g. count_ is ULONG but
the argument is unsigned long.

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant_unitt...
File base/win/enum_variant_unittest.cc (right):

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant_unitt...
base/win/enum_variant_unittest.cc:42: EXPECT_TRUE(ev2 != NULL);
EXPECT_NE(NULL, ev2);

http://codereview.chromium.org/8588036/diff/27037/base/win/iunknown_impl.h
File base/win/iunknown_impl.h (right):

http://codereview.chromium.org/8588036/diff/27037/base/win/iunknown_impl.h#ne...
base/win/iunknown_impl.h:19: // An base implementation of IUnknown, for other
classes to derive from.
// IUnknown implementation for other classes to derive from.

http://codereview.chromium.org/8588036/diff/27037/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/8588036/diff/27037/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:572:
selected->punkVal = static_cast<base::win::IUnknownImpl*>(enum_variant);
I think you forgot to increase the ref count here.

http://codereview.chromium.org/8588036/diff/27037/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.h (right):

http://codereview.chromium.org/8588036/diff/27037/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.h:725: int32
old_ia_state_;
I'd expect bitmaps to be uint32 ?

[dmazzoni, 2011-11-29 21:54:59]

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant.cc
File base/win/enum_variant.cc (right):

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant.cc#ne...
base/win/enum_variant.cc:14: items_.reset(new VARIANT[count_]);
On 2011/11/29 20:28:32, Marc-Antoine Ruel wrote:
> Why not do it in the member constructor?

You're right, done.

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant.cc#ne...
base/win/enum_variant.cc:36: *ppv = this;
On 2011/11/29 20:28:32, Marc-Antoine Ruel wrote:
> In theory, you need to static_cast<IEnumVariant*>(this)

Done.

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant.cc#ne...
base/win/enum_variant.cc:47: unsigned long count = requested_count;
On 2011/11/29 20:28:32, Marc-Antoine Ruel wrote:
> unsigned long count = min(requested_count, count_ - current_index_);

Done.

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant.cc#ne...
base/win/enum_variant.cc:76: *other->ItemAt(i) = items_[i];
On 2011/11/29 20:28:32, Marc-Antoine Ruel wrote:
> Why is this better than a memcpy or just implementing a private copy
> constructor?

Switched to memcpy.

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant.h
File base/win/enum_variant.h (right):

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant.h#new...
base/win/enum_variant.h:24: explicit EnumVariant(unsigned long count);
On 2011/11/29 20:28:32, Marc-Antoine Ruel wrote:
> why do you alternate between unsigned long and ULONG? e.g. count_ is ULONG but
> the argument is unsigned long.

My intent was to use ULONG only in arguments and return values from COM methods.
Non-COM methods use the style that would be used in the rest of Chromium code.

The member variables were a mistake, I changed them to unsigned long.

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant_unitt...
File base/win/enum_variant_unittest.cc (right):

http://codereview.chromium.org/8588036/diff/27037/base/win/enum_variant_unitt...
base/win/enum_variant_unittest.cc:42: EXPECT_TRUE(ev2 != NULL);
On 2011/11/29 20:28:32, Marc-Antoine Ruel wrote:
> EXPECT_NE(NULL, ev2);

Done - I had to cast NULL for it to compile.

http://codereview.chromium.org/8588036/diff/27037/base/win/iunknown_impl.h
File base/win/iunknown_impl.h (right):

http://codereview.chromium.org/8588036/diff/27037/base/win/iunknown_impl.h#ne...
base/win/iunknown_impl.h:19: // An base implementation of IUnknown, for other
classes to derive from.
On 2011/11/29 20:28:32, Marc-Antoine Ruel wrote:
> // IUnknown implementation for other classes to derive from.

Done.

http://codereview.chromium.org/8588036/diff/27037/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/8588036/diff/27037/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:572:
selected->punkVal = static_cast<base::win::IUnknownImpl*>(enum_variant);
On 2011/11/29 20:28:32, Marc-Antoine Ruel wrote:
> I think you forgot to increase the ref count here.

The EnumVariant constructor gives us a ref count of 1, so we're transferring
ownership rather than returning a new reference.

I'm not totally clear on best practices here, would it be more correct for the
EnumVariant constructor to not return a reference?

http://codereview.chromium.org/8588036/diff/27037/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.h (right):

http://codereview.chromium.org/8588036/diff/27037/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.h:725: int32
old_ia_state_;
On 2011/11/29 20:28:32, Marc-Antoine Ruel wrote:
> I'd expect bitmaps to be uint32 ?

This matches the signedness of the return type of the COM method that retrieves
the state.

[M-A Ruel, 2011-11-29 22:12:28]

http://codereview.chromium.org/8588036/diff/27037/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.h (right):

http://codereview.chromium.org/8588036/diff/27037/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.h:725: int32
old_ia_state_;
On 2011/11/29 21:55:00, Dominic Mazzoni wrote:
> On 2011/11/29 20:28:32, Marc-Antoine Ruel wrote:
> > I'd expect bitmaps to be uint32 ?
> 
> This matches the signedness of the return type of the COM method that
retrieves
> the state.

Right, it's to support ole scripting, I had forgot about that.

http://codereview.chromium.org/8588036/diff/33001/base/win/enum_variant_unitt...
File base/win/enum_variant_unittest.cc (right):

http://codereview.chromium.org/8588036/diff/33001/base/win/enum_variant_unitt...
base/win/enum_variant_unittest.cc:42:
EXPECT_NE(static_cast<IEnumVARIANT*>(NULL), ev2);
Oh right, I don't mind much.

http://codereview.chromium.org/8588036/diff/33001/base/win/iunknown_impl.cc
File base/win/iunknown_impl.cc (right):

http://codereview.chromium.org/8588036/diff/33001/base/win/iunknown_impl.cc#n...
base/win/iunknown_impl.cc:11: ref_count_ = 1;
See base/memory/ref_counted.cc. It's highly preferable to initialize it to 0.

[dmazzoni, 2011-11-29 22:21:34]

On 2011/11/29 22:12:28, Marc-Antoine Ruel wrote:
> See base/memory/ref_counted.cc. It's highly preferable to initialize it to 0.

Done. Now the IUnknownImpl constructor does not add a reference, and to balance
that EnumVariant::Clone() now does add a reference (according to the COM rules).
I updated the tests accordingly.

[David Tseng, 2011-11-30 19:07:09]

http://codereview.chromium.org/8588036/diff/33005/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/8588036/diff/33005/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:538: for (size_t i =
0; i < children_.size(); i++) {
nit: ++i and elsewhere below.

http://codereview.chromium.org/8588036/diff/33005/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:574: return S_OK;
Could we integrate these three loops? Perhaps check for the "multiple" attribute
on the select node?

http://codereview.chromium.org/8588036/diff/33005/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:735: *group_level =
0;
Do we need to check for the validity of these pointers?

[dmazzoni, 2011-11-30 19:55:39]

http://codereview.chromium.org/8588036/diff/33005/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/8588036/diff/33005/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:538: for (size_t i =
0; i < children_.size(); i++) {
On 2011/11/30 19:07:09, David Tseng wrote:
> nit: ++i and elsewhere below.

Done.

http://codereview.chromium.org/8588036/diff/33005/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:574: return S_OK;
On 2011/11/30 19:07:09, David Tseng wrote:
> Could we integrate these three loops? Perhaps check for the "multiple"
attribute
> on the select node?

The multiple attribute indicates whether selecting multiple items is possible,
not whether it's currently true.

I think two passes are necessary: one to determine how many items are selected,
which determines what output format is used, and then one loop to generate the
output. Trying to do that in one pass would mean constructing some extra objects
that get thrown away if not used.

http://codereview.chromium.org/8588036/diff/33005/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:735: *group_level =
0;
On 2011/11/30 19:07:09, David Tseng wrote:
> Do we need to check for the validity of these pointers?

Good catch, done.

[David Tseng, 2011-11-30 21:55:10]

lgtm

On 11/30/11, dmazzoni@chromium.org <dmazzoni@chromium.org> wrote:
>
>
http://codereview.chromium.org/8588036/diff/33005/content/browser/accessibili...
> File content/browser/accessibility/browser_accessibility_win.cc (right):
>
>
http://codereview.chromium.org/8588036/diff/33005/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_win.cc:538: for
> (size_t i = 0; i < children_.size(); i++) {
> On 2011/11/30 19:07:09, David Tseng wrote:
>> nit: ++i and elsewhere below.
>
> Done.
>
>
http://codereview.chromium.org/8588036/diff/33005/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_win.cc:574: return
> S_OK;
> On 2011/11/30 19:07:09, David Tseng wrote:
>> Could we integrate these three loops? Perhaps check for the "multiple"
> attribute
>> on the select node?
>
> The multiple attribute indicates whether selecting multiple items is
> possible, not whether it's currently true.
>
> I think two passes are necessary: one to determine how many items are
> selected, which determines what output format is used, and then one loop
> to generate the output. Trying to do that in one pass would mean
> constructing some extra objects that get thrown away if not used.
>
>
http://codereview.chromium.org/8588036/diff/33005/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_win.cc:735:
> *group_level = 0;
> On 2011/11/30 19:07:09, David Tseng wrote:
>> Do we need to check for the validity of these pointers?
>
> Good catch, done.
>
> http://codereview.chromium.org/8588036/
>

[M-A Ruel, 2011-12-01 00:35:13]

lgtm with optional nits

http://codereview.chromium.org/8588036/diff/39001/base/win/enum_variant.h
File base/win/enum_variant.h (right):

http://codereview.chromium.org/8588036/diff/39001/base/win/enum_variant.h#new...
base/win/enum_variant.h:27: VARIANT* ItemAt(unsigned long index);
Why doesn't it return a reference? Just wondering.

http://codereview.chromium.org/8588036/diff/39001/base/win/enum_variant_unitt...
File base/win/enum_variant_unittest.cc (right):

http://codereview.chromium.org/8588036/diff/39001/base/win/enum_variant_unitt...
base/win/enum_variant_unittest.cc:13:
EXPECT_TRUE(SUCCEEDED(::CoInitialize(NULL)));
If you were using ScopedCOMInitializer, you wouldn't need the inner brackets.

http://codereview.chromium.org/8588036/diff/39001/base/win/iunknown_impl.cc
File base/win/iunknown_impl.cc (right):

http://codereview.chromium.org/8588036/diff/39001/base/win/iunknown_impl.cc#n...
base/win/iunknown_impl.cc:11: : ref_count_(0) {}
One reason to keep the brackets on two lines, like you did for the destructor,
is that it makes it easier to put a breakpoint with Visual Studio.

http://codereview.chromium.org/8588036/diff/39001/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/8588036/diff/39001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:553:
children_[i]->toBrowserAccessibilityWin()->NewReference();
That's funny, why not AddRef?

[darin (slow to review), 2011-12-01 17:58:32]

On Tue, Nov 29, 2011 at 11:57 AM, <dmazzoni@chromium.org> wrote:

> On 2011/11/29 00:27:26, darin wrote:
>
>> +1 for IUnknownImpl
>>
>
> Ready for another look. I created IUnknownImpl and made EnumVariant derive
> from
> it, with unit tests for both.
>
> I added new methods to atomic_ref_count because otherwise there'd be a race
> condition where two clients could both get 0 back from Release(), making
> them
> both think they were the ones to release the object. Plus even if we
> didn't care
> about that case, implementing AddRef and Release non-atomically could
> trigger
> warnings in data-race-detection tools, right?
>

I think you should implement IUnknown AddRef and Release like this:

  ULONG STDMETHODCALLTYPE IUnknownImpl::AddRef() {
    base::AtomicRefCountInc(&ref_count_);
    return 1;
  }

  ULONG STDMETHODCALLTYPE IUnknownImpl::Release() {
    if (!base::AtomicRefCountDec(&ref_count_)) {
      delete this;
      return 0;
    }
    return 1;
  }

I think there is probably a very good reason why atomic_ref_count.h hides
the current
ref-count value.

I'm pretty convinced that COM does not require you to return the actual ref
count from
AddRef and Release.  1)  It cannot return an accurate value since other
threads could
be manipulating the ref count, 2)  It can be OK to implement COM interfaces
using
globally allocated objects (not heap allocated), and 3)  Ever reference
I've seen just
says that these return values exist for debug purposes only.

-Darin



>
> One new ugliness is that due to the multiple inheritance of IUnknown,
> EnumVariant has to implement all of the IUnknown methods too (by calling
> the
> correct base class implementation). I'm not sure if there's a way around
> this -
> Microsoft's CUnknown and ATL each require you to do this too, but they
> abstract
> it via macros. The clean solution would be virtual inheritance, but we
> can't
> change IEnumVARIANT to inherit virtually from IUnknown.
>
> I still think it's better to have IUnknownImpl because I'll definitely use
> it
> for more classes.
>
>
>
http://codereview.chromium.**org/8588036/<http://codereview.chromium.org/8588...
>

[dmazzoni, 2011-12-02 00:44:06]

Darin - changed AddRef/Release as suggested. You're probably right...I'm just
paranoid because of how many times I've seen a screen reader crash when we
returned something unexpected from a COM method. 

M-A: addressed your comments too, thanks.

http://codereview.chromium.org/8588036/diff/39001/base/win/enum_variant.h
File base/win/enum_variant.h (right):

http://codereview.chromium.org/8588036/diff/39001/base/win/enum_variant.h#new...
base/win/enum_variant.h:27: VARIANT* ItemAt(unsigned long index);
On 2011/12/01 00:35:13, Marc-Antoine Ruel wrote:
> Why doesn't it return a reference? Just wondering.

I'm used to the Google coding style of pointers for mutable, const refs for
immutable - and in this case, I think a pointer is better since we're using
memcpy in Clone(). But it's just a slight preference.

http://codereview.chromium.org/8588036/diff/39001/base/win/enum_variant_unitt...
File base/win/enum_variant_unittest.cc (right):

http://codereview.chromium.org/8588036/diff/39001/base/win/enum_variant_unitt...
base/win/enum_variant_unittest.cc:13:
EXPECT_TRUE(SUCCEEDED(::CoInitialize(NULL)));
On 2011/12/01 00:35:13, Marc-Antoine Ruel wrote:
> If you were using ScopedCOMInitializer, you wouldn't need the inner brackets.

Good idea, done.

http://codereview.chromium.org/8588036/diff/39001/base/win/iunknown_impl.cc
File base/win/iunknown_impl.cc (right):

http://codereview.chromium.org/8588036/diff/39001/base/win/iunknown_impl.cc#n...
base/win/iunknown_impl.cc:11: : ref_count_(0) {}
On 2011/12/01 00:35:13, Marc-Antoine Ruel wrote:
> One reason to keep the brackets on two lines, like you did for the destructor,
> is that it makes it easier to put a breakpoint with Visual Studio.

Done.

http://codereview.chromium.org/8588036/diff/39001/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/8588036/diff/39001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:553:
children_[i]->toBrowserAccessibilityWin()->NewReference();
On 2011/12/01 00:35:13, Marc-Antoine Ruel wrote:
> That's funny, why not AddRef?

It's a convenience method that AddRefs and returns "this" so it can be used
inside a "return" or assignment, rather than needing two statements. AddRef is
needed so many times in this file that it's worth it.

[darin (slow to review), 2011-12-02 06:49:45]

LGTM

(Please feel free to say "I told you so" if my AddRef/Release suggestion comes
back to bite you!)

http://codereview.chromium.org/8588036/diff/47006/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/8588036/diff/47006/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:553:
children_[i]->toBrowserAccessibilityWin()->NewReference();
shouldn't toBrowserAccessibilityWin() start with an uppercase T?

[M-A Ruel, 2011-12-02 12:52:27]

http://codereview.chromium.org/8588036/diff/47006/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/8588036/diff/47006/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:573:
selected->punkVal = static_cast<base::win::IUnknownImpl*>(enum_variant);
That works? Since you are putting the vtable including the destructor. I
understand you have to disambiguate to get IUnknown (overly verbose) but it
stills looks better to me.

[commit-bot: I haz the power, 2011-12-02 23:49:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/8588036/49005

[commit-bot: I haz the power, 2011-12-03 01:04:45]

Try job failure for 8588036-49005 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-12-03 03:03:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/8588036/49005

[commit-bot: I haz the power, 2011-12-03 04:31:15]

Change committed as 112870