Closed
Bug 1196460
Opened 9 years ago
Closed 9 years ago
make 32 bit unique ids work with proxied accessibles
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: tbsaunde, Assigned: tbsaunde)
References
Details
Attachments
(8 files)
1.14 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
1.41 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
939 bytes,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
3.10 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
1.24 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
1.64 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
785 bytes,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
2.43 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
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•9 years ago
|
||
Attachment #8650119 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8650120 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8650121 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8650122 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8650123 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8650133 -
Flags: review?(surkov.alexander)
Comment 8•9 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•9 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•9 years ago
|
Attachment #8650118 -
Flags: review?(surkov.alexander) → review+
Comment 10•9 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•9 years ago
|
Attachment #8650120 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 11•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 years ago
|
||
Attachment #8659451 -
Flags: review?(surkov.alexander)
Updated•9 years ago
|
Attachment #8659451 -
Flags: review?(surkov.alexander) → review+
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f874c78a40b2 https://hg.mozilla.org/integration/mozilla-inbound/rev/680f1b8efbdf https://hg.mozilla.org/integration/mozilla-inbound/rev/d9be94b01f90 https://hg.mozilla.org/integration/mozilla-inbound/rev/2da419f8a791 https://hg.mozilla.org/integration/mozilla-inbound/rev/2b75e52881e9 https://hg.mozilla.org/integration/mozilla-inbound/rev/c6212940fbce https://hg.mozilla.org/integration/mozilla-inbound/rev/4ea6b5620ce4 https://hg.mozilla.org/integration/mozilla-inbound/rev/be192df7732e
https://hg.mozilla.org/mozilla-central/rev/f874c78a40b2 https://hg.mozilla.org/mozilla-central/rev/680f1b8efbdf https://hg.mozilla.org/mozilla-central/rev/d9be94b01f90 https://hg.mozilla.org/mozilla-central/rev/2da419f8a791 https://hg.mozilla.org/mozilla-central/rev/2b75e52881e9 https://hg.mozilla.org/mozilla-central/rev/c6212940fbce https://hg.mozilla.org/mozilla-central/rev/4ea6b5620ce4 https://hg.mozilla.org/mozilla-central/rev/be192df7732e
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Updated•7 years ago
|
Assignee: nobody → tbsaunde+mozbugs
You need to log in
before you can comment on or make changes to this bug.
Description
•