Closed
Bug 1168932
Opened 8 years ago
Closed 8 years ago
Implement ProxyCreated and ProxyDestroyed to update mozAccessibles
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: lsocks, Assigned: lsocks)
References
Details
Attachments
(1 file, 5 obsolete files)
9.24 KB,
patch
|
lsocks
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → lorien
Target Milestone: --- → Firefox 41
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8611344 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Updated•8 years ago
|
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
Target Milestone: Firefox 41 → ---
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8611344 -
Attachment is obsolete: true
Attachment #8611401 -
Flags: review?(tbsaunde+mozbugs)
Comment 4•8 years ago
|
||
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+
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8611401 -
Attachment is obsolete: true
Attachment #8612489 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8612489 -
Attachment is obsolete: true
Attachment #8612489 -
Flags: review?(tbsaunde+mozbugs)
Attachment #8612535 -
Flags: review?(tbsaunde+mozbugs)
Updated•8 years ago
|
Attachment #8612535 -
Flags: review?(tbsaunde+mozbugs) → review+
Assignee | ||
Comment 7•8 years ago
|
||
Carry r=tbsaunde
Attachment #8612535 -
Attachment is obsolete: true
Attachment #8612977 -
Flags: review+
Assignee | ||
Comment 8•8 years ago
|
||
Carry r=tbsaunde
Attachment #8612977 -
Attachment is obsolete: true
Attachment #8612991 -
Flags: review+
Comment 10•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f83bbd98a88b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•