Closed Bug 1168932 Opened 4 years ago Closed 4 years ago

Implement ProxyCreated and ProxyDestroyed to update mozAccessibles

Categories

(Core :: Disability Access APIs, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: lsocks, Assigned: lsocks)

References

Details

Attachments

(1 file, 5 obsolete files)

No description provided.
Assignee: nobody → lorien
Target Milestone: --- → Firefox 41
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
Target Milestone: Firefox 41 → ---
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.
Attachment #8611344 - Flags: review?(tbsaunde+mozbugs)
Attachment #8611344 - Attachment is obsolete: true
Attachment #8611401 - Flags: review?(tbsaunde+mozbugs)
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.
Attachment #8611401 - Flags: review?(tbsaunde+mozbugs) → review+
Attachment #8611401 - Attachment is obsolete: true
Attachment #8612489 - Flags: review?(tbsaunde+mozbugs)
Attached patch ProxyCreated and ProxyDestroyed (obsolete) — Splinter Review
Attachment #8612489 - Attachment is obsolete: true
Attachment #8612489 - Flags: review?(tbsaunde+mozbugs)
Attachment #8612535 - Flags: review?(tbsaunde+mozbugs)
Depends on: 1169408
Attachment #8612535 - Flags: review?(tbsaunde+mozbugs) → review+
Attached patch proxycreated.diff (obsolete) — Splinter Review
Carry r=tbsaunde
Attachment #8612535 - Attachment is obsolete: true
Attachment #8612977 - Flags: review+
Carry r=tbsaunde
Attachment #8612977 - Attachment is obsolete: true
Attachment #8612991 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f83bbd98a88b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.