make 32 bit unique ids work with proxied accessibles

RESOLVED FIXED in Firefox 43

Status

()

Core
Disability Access APIs
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(8 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8650118 [details] [diff] [review]
add class for wrapping proxies of document accessibles

For now this isn't really different from the class used to wrap
HyperTextAccessibles.  However we will need to store extra data to map IDs to
accessibles when we implement events.
Attachment #8650118 - Flags: review?(surkov.alexander)
(Assignee)

Comment 2

3 years ago
Created attachment 8650119 [details] [diff] [review]
create different proxy wrappers depending on the type of the proxy
Attachment #8650119 - Flags: review?(surkov.alexander)
(Assignee)

Comment 3

3 years ago
Created attachment 8650120 [details] [diff] [review]
make IsDoc() return true for wrappers of proxied documents
Attachment #8650120 - Flags: review?(surkov.alexander)
(Assignee)

Comment 4

3 years ago
Created attachment 8650121 [details] [diff] [review]
add method to get wrapper of proxy for document containing this proxied accessible
Attachment #8650121 - Flags: review?(surkov.alexander)
(Assignee)

Comment 5

3 years ago
Created attachment 8650122 [details] [diff] [review]
provide mapping from id to accessible in DocProxyAccessibleWrap
Attachment #8650122 - Flags: review?(surkov.alexander)
(Assignee)

Comment 6

3 years ago
Created attachment 8650123 [details] [diff] [review]
teach GetChildIDFor() to deal with proxied accessibles
Attachment #8650123 - Flags: review?(surkov.alexander)
(Assignee)

Comment 7

3 years ago
Created attachment 8650133 [details] [diff] [review]
make the ctor of HyperTextProxyAccessiblewrap public
Attachment #8650133 - Flags: review?(surkov.alexander)

Comment 8

3 years ago
Comment on attachment 8650133 [details] [diff] [review]
make the ctor of HyperTextProxyAccessiblewrap public

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

::: accessible/windows/ProxyWrappers.h
@@ +28,5 @@
>      mBits.proxy = nullptr;
>    }
>  };
>  
>  class HyperTextProxyAccessibleWrap : public HyperTextAccessibleWrap

not introduced by this patch but it caught me by surprise, since it inherits all data fields and it blows out memory usage
Attachment #8650133 - Flags: review?(surkov.alexander) → review+

Comment 9

3 years ago
the patches look about right, I didn't noticed RemoveID part though, but because of some pto days, I might not be able to review all these until the end of next week. You may want to redirect requests to David.

Updated

3 years ago
Attachment #8650118 - Flags: review?(surkov.alexander) → review+

Comment 10

3 years ago
Comment on attachment 8650119 [details] [diff] [review]
create different proxy wrappers depending on the type of the proxy

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

::: accessible/windows/msaa/Platform.cpp
@@ +53,5 @@
>  void
>  a11y::ProxyDestroyed(ProxyAccessible* aProxy)
>  {
> +  AccessibleWrap* wrapper =
> +    reinterpret_cast<AccessibleWrap*>(aProxy->GetWrapper());

what's the point to change that?
Attachment #8650119 - Flags: review?(surkov.alexander) → review+

Updated

3 years ago
Attachment #8650120 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 11

3 years ago
(In reply to alexander :surkov from comment #8)
> Comment on attachment 8650133 [details] [diff] [review]
> make the ctor of HyperTextProxyAccessiblewrap public
> 
> Review of attachment 8650133 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/windows/ProxyWrappers.h
> @@ +28,5 @@
> >      mBits.proxy = nullptr;
> >    }
> >  };
> >  
> >  class HyperTextProxyAccessibleWrap : public HyperTextAccessibleWrap
> 
> not introduced by this patch but it caught me by surprise, since it inherits
> all data fields and it blows out memory usage

its... really unfortunate, but using AccessibleWrap is also really unfortunate.  The problem is ia2AccessibleText and friends down cast to HyperTextAccessibleWrap*.  We really need to separate the windows interfaces from Accessible.

(In reply to alexander :surkov from comment #10)
> Comment on attachment 8650119 [details] [diff] [review]
> create different proxy wrappers depending on the type of the proxy
> 
> Review of attachment 8650119 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/windows/msaa/Platform.cpp
> @@ +53,5 @@
> >  void
> >  a11y::ProxyDestroyed(ProxyAccessible* aProxy)
> >  {
> > +  AccessibleWrap* wrapper =
> > +    reinterpret_cast<AccessibleWrap*>(aProxy->GetWrapper());
> 
> what's the point to change that?

if its a HyperTextProxyAccessibleWrap then casting it to a ProxyAccessibleWrap isn't valid.

Comment 12

3 years ago
Comment on attachment 8650121 [details] [diff] [review]
add method to get wrapper of proxy for document containing this proxied accessible

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

::: accessible/windows/msaa/AccessibleWrap.cpp
@@ +1287,5 @@
> +  }
> +
> +  AccessibleWrap* acc = WrapperFor(proxy->Document());
> +
> + return acc->IsDoc() ? static_cast<DocProxyAccessibleWrap*>(acc) : nullptr;

nit: no empty line between these lines and add extra space for the last one.
I'm curious if IsDoc() may fail on proxy->Document()
Attachment #8650121 - Flags: review?(surkov.alexander) → review+

Comment 13

3 years ago
Comment on attachment 8650122 [details] [diff] [review]
provide mapping from id to accessible in DocProxyAccessibleWrap

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

::: accessible/windows/ProxyWrappers.h
@@ +50,5 @@
>    { mGenericTypes |= eDocument; }
> +
> +  void AddID(uint32_t aID, AccessibleWrap* aAcc)
> +    { mIDToAccessibleMap.Put(aID, aAcc); }
> +  void RemoveID(uint32_t aID) { mIDToAccessibleMap.Remove(aID); }

is it in use?
Attachment #8650122 - Flags: review?(surkov.alexander) → review+

Comment 14

3 years ago
Comment on attachment 8650123 [details] [diff] [review]
teach GetChildIDFor() to deal with proxied accessibles

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

::: accessible/windows/msaa/AccessibleWrap.cpp
@@ +1328,5 @@
> +  int32_t id = - reinterpret_cast<intptr_t>(aAccessible);
> +  if (aAccessible->IsProxy()) {
> +    DocProxyAccessibleWrap* doc =
> +      static_cast<AccessibleWrap*>(aAccessible)->DocProxyWrapper();
> +    doc->AddID(id, static_cast<AccessibleWrap*>(aAccessible));

why you cannot deal with raw pointers?
Attachment #8650123 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 15

3 years ago
> I'm curious if IsDoc() may fail on proxy->Document()

I think that would be a bug so changed to an assert


(In reply to alexander :surkov from comment #14)
> Comment on attachment 8650123 [details] [diff] [review]
> teach GetChildIDFor() to deal with proxied accessibles
> 
> Review of attachment 8650123 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/windows/msaa/AccessibleWrap.cpp
> @@ +1328,5 @@
> > +  int32_t id = - reinterpret_cast<intptr_t>(aAccessible);
> > +  if (aAccessible->IsProxy()) {
> > +    DocProxyAccessibleWrap* doc =
> > +      static_cast<AccessibleWrap*>(aAccessible)->DocProxyWrapper();
> > +    doc->AddID(id, static_cast<AccessibleWrap*>(aAccessible));
> 
> why you cannot deal with raw pointers?

what do you mean?

Comment 16

3 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #15)

> > ::: accessible/windows/msaa/AccessibleWrap.cpp
> > @@ +1328,5 @@
> > > +  int32_t id = - reinterpret_cast<intptr_t>(aAccessible);
> > > +  if (aAccessible->IsProxy()) {
> > > +    DocProxyAccessibleWrap* doc =
> > > +      static_cast<AccessibleWrap*>(aAccessible)->DocProxyWrapper();
> > > +    doc->AddID(id, static_cast<AccessibleWrap*>(aAccessible));
> > 
> > why you cannot deal with raw pointers?
> 
> what do you mean?

I meant whether you can use a pointer address (either of proxy or accessible object) as ID (instead of ID map usage) like we do for single process.
(Assignee)

Comment 17

3 years ago
(In reply to alexander :surkov from comment #16)
> (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> 
> > > ::: accessible/windows/msaa/AccessibleWrap.cpp
> > > @@ +1328,5 @@
> > > > +  int32_t id = - reinterpret_cast<intptr_t>(aAccessible);
> > > > +  if (aAccessible->IsProxy()) {
> > > > +    DocProxyAccessibleWrap* doc =
> > > > +      static_cast<AccessibleWrap*>(aAccessible)->DocProxyWrapper();
> > > > +    doc->AddID(id, static_cast<AccessibleWrap*>(aAccessible));
> > > 
> > > why you cannot deal with raw pointers?
> > 
> > what do you mean?
> 
> I meant whether you can use a pointer address (either of proxy or accessible
> object) as ID (instead of ID map usage) like we do for single process.

well, it is just using the address of the object, but if you are trying to ask why the hash table is necessary because there isn't an existing one in this case.  Nothing else tracks all the AccessibleWraps that point at proxies.  There's the table mapping proxy's id to the object, but those can always be 64 bit.
(Assignee)

Comment 18

3 years ago
Created attachment 8659451 [details] [diff] [review]
remove proxy's ids when they are destroyed
Attachment #8659451 - Flags: review?(surkov.alexander)

Updated

3 years ago
Attachment #8659451 - Flags: review?(surkov.alexander) → review+
Assignee: nobody → tbsaunde+mozbugs
You need to log in before you can comment on or make changes to this bug.