1168932 - Implement ProxyCreated and ProxyDestroyed to update mozAccessibles

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Lorien Hu (:lsocks)]

Attachment: Refactoring to implement ProxyCreated and ProxyDestroyed. Pulled role to type switch statement out of GetNativeType into GetTypeFromRole.

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8611344 [details] [diff] [review]
Refactoring to implement ProxyCreated and ProxyDestroyed. Pulled role to type switch statement out of GetNativeType into GetTypeFromRole.

its nice if patch titles aren't a long rambling sentence ;)

>diff --git a/accessible/mac/AccessibleWrap.h b/accessible/mac/AccessibleWrap.h
>--- a/accessible/mac/AccessibleWrap.h
>+++ b/accessible/mac/AccessibleWrap.h
>@@ -103,12 +103,14 @@ private:
>   /**
>    * We have created our native. This does not mean there is one.
>    * This can never go back to false.
>    * We need it because checking whether we need a native object cost time.
>    */
>   bool mNativeInited;  
> };
> 
>+Class GetTypeFromRole(roles::Role role, uint64_t state);

aRole, aState

>   
>   if (!mNativeInited && !mNativeObject && !IsDefunct() && !AncestorIsFlat())
>-    mNativeObject = [[GetNativeType() alloc] initWithAccessible:this];
>+    mNativeObject = [[GetNativeType() alloc]
>+     initWithAccessible:reinterpret_cast<uintptr_t>(this)];

might be simpler to make it separate statements.

>+mozilla::a11y::GetTypeFromRole(roles::Role role, uint64_t state) 

aRole, aState

I think you should be able to skip mozilla:: (maybe you need a using namespace mozilla)

>+        // if this button may show a popup, let's make it of the popupbutton type.
>+        return (state & NS_STATE_MENU_HAS_POPUP_LIST)
you need states::HASPOPUP instead of the NS_ thing.

> void
>-ProxyCreated(ProxyAccessible*, uint32_t)
>+ProxyCreated(ProxyAccessible* aProxy, uint32_t)
> {
>+  // Proxies currently have no states, so they cannot have popups.

its not true they don't have states, its just not safe to get them here.  btw it might be good to have an XXX comment about merging the button class in the GetTypeFromRole function.

>+  Class type = GetTypeFromRole(aProxy->Role(), 0);
>+  mozAccessible* wrapper =
>+    [[type alloc] initWithAccessible:reinterpret_cast<uintptr_t>(aProxy) | IS_PROXY];

multiple statemenets might be easier to read

>-- (id)initWithAccessible:(mozilla::a11y::AccessibleWrap*)geckoParent;
>+- (id)initWithAccessible:(uintptr_t)geckoParent;

its not the parent of anything so lets rename it to aGeckoObj maybe

>-- (id)initWithAccessible:(AccessibleWrap*)geckoAccessible
>+- (id)initWithAccessible:(uintptr_t)geckoAccessible

aGeckoAccessible

>+    if (geckoAccessible & IS_PROXY)
>+      mRole = reinterpret_cast<ProxyAccessible*>(geckoAccessible & ~IS_PROXY)->Role();
>+    else
>+      mRole = reinterpret_cast<AccessibleWrap*>(geckoAccessible)->Role();

seems like it would be nicer to either use accessors on mGeckoAccessible or have the role passed in.

Comment 3

[Lorien Hu (:lsocks)]

Attachment: implementing ProxyCreated, ProxyDestroyed. pulled GetNativeType into GetTypeFromRole.

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8611401 [details] [diff] [review]
implementing ProxyCreated, ProxyDestroyed. pulled GetNativeType into GetTypeFromRole.

>-    mRole = geckoAccessible->Role();
>+    mGeckoAccessible = aGeckoAccessible;
>+    if (aGeckoAccessible & IS_PROXY)
>+      mRole = [self getProxyAccessible]->Role();
>+    else
>+      mRole = [self getGeckoAccessible]->Role();
>   }

I'd rather we didn't look at the internals of IS_PROXY here, but whatever we can worry about it later and this is kind of a reasonable place to poke at it.

Comment 5

[Lorien Hu (:lsocks)]

Attachment: merged mozPopupButtonAccessible and mozButtonAccessible

Comment 6

[Lorien Hu (:lsocks)]

Attachment: ProxyCreated and ProxyDestroyed

Comment 7

[Lorien Hu (:lsocks)]

Attachment: proxycreated.diff

Carry r=tbsaunde

Comment 8

[Lorien Hu (:lsocks)]

Attachment: implementing ProxyCreated, ProxyDestroyed. pulled GetNativeType into GetTypeFromRole.

Carry r=tbsaunde

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

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