Trevor Saunders (:tbsaunde) Assignee
	Description • 9 years ago

      No description provided.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 1 • 9 years ago

Unfortunately on windows there's no separate object implementing the
native interfaces.  That means we need to have a type of accessible that
just wraps a proxy.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 2 • 9 years ago

Comment on attachment 8552568 [details] [diff] [review]
setup proxies on windows

Review of attachment 8552568 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/base/AccTypes.h
@@ +51,5 @@
>    eApplicationType,
>    eHTMLOptGroupType,
>    eImageMapType,
>    eMenuPopupType,
> +  eProxyType,

(I wondered if maybe we could just check mBits.proxy in IsProxy() but this is fine)

::: accessible/windows/msaa/AccessibleWrap.cpp
@@ +80,5 @@
>  
>    *ppv = nullptr;
>  
> +  if (IID_IUnknown == iid)
> +    *ppv = static_cast<IAccessible*>(this);

Why do we need to separate this check and have it before the IsProxy check?

@@ +85,5 @@
> +
> +  if (!*ppv && IsProxy())
> +    return E_NOINTERFACE;
> +
> +    if (IID_IDispatch == iid || IID_IAccessible == iid)

nit: remove two spaces off this indent

	Trevor Saunders (:tbsaunde) Assignee
	Comment 3 • 9 years ago

> ::: accessible/base/AccTypes.h
> @@ +51,5 @@
> >    eApplicationType,
> >    eHTMLOptGroupType,
> >    eImageMapType,
> >    eMenuPopupType,
> > +  eProxyType,
> 
> (I wondered if maybe we could just check mBits.proxy in IsProxy() but this
> is fine)

no, because if the union is storing a group info you'll just read out a garbage pointer.

> ::: accessible/windows/msaa/AccessibleWrap.cpp
> @@ +80,5 @@
> >  
> >    *ppv = nullptr;
> >  
> > +  if (IID_IUnknown == iid)
> > +    *ppv = static_cast<IAccessible*>(this);
> 
> Why do we need to separate this check and have it before the IsProxy check?

I'm just trying to have a setup were we can enable QIing proxy wrappers to some interfaces we know can deal with that case, but not the others.  Currently if you call IAccessibleHypertext::get_text() say on a accessible holding a proxy you'll just get a strnage crash.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 4 • 9 years ago

Comment on attachment 8552568 [details] [diff] [review]
setup proxies on windows

Review of attachment 8552568 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, just the format nit left.

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > ::: accessible/base/AccTypes.h
> > @@ +51,5 @@
> > >    eApplicationType,
> > >    eHTMLOptGroupType,
> > >    eImageMapType,
> > >    eMenuPopupType,
> > > +  eProxyType,
> > 
> > (I wondered if maybe we could just check mBits.proxy in IsProxy() but this
> > is fine)
> 
> no, because if the union is storing a group info you'll just read out a
> garbage pointer.

Ah right union not struct ok.

> > ::: accessible/windows/msaa/AccessibleWrap.cpp
> > @@ +80,5 @@
> > >  
> > >    *ppv = nullptr;
> > >  
> > > +  if (IID_IUnknown == iid)
> > > +    *ppv = static_cast<IAccessible*>(this);
> > 
> > Why do we need to separate this check and have it before the IsProxy check?
> 
> I'm just trying to have a setup were we can enable QIing proxy wrappers to
> some interfaces we know can deal with that case, but not the others. 
> Currently if you call IAccessibleHypertext::get_text() say on a accessible
> holding a proxy you'll just get a strnage crash.

OK thanks.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 5 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1237502c5e2

	Carsten Book [:Tomcat]
	Comment 6 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/c1237502c5e2