Closed Bug 1196460 Opened 4 years ago Closed 4 years ago

make 32 bit unique ids work with proxied accessibles

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(8 files)

No description provided.
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)
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+
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.
Attachment #8650118 - Flags: review?(surkov.alexander) → review+
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+
Attachment #8650120 - Flags: review?(surkov.alexander) → review+
(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 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 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 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+
> 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?
(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.
(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.
Attachment #8659451 - Flags: review?(surkov.alexander)
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.