start exposing proxied accessibles to js

RESOLVED FIXED in Firefox 47

Status

()

Core
Disability Access APIs
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(18 attachments)

2.21 KB, patch
davidb
: review+
Details | Diff | Splinter Review
4.12 KB, patch
davidb
: review+
Details | Diff | Splinter Review
3.08 KB, patch
davidb
: review+
Details | Diff | Splinter Review
1.33 KB, patch
davidb
: review+
Details | Diff | Splinter Review
8.81 KB, patch
davidb
: review+
Details | Diff | Splinter Review
5.86 KB, patch
davidb
: review+
Details | Diff | Splinter Review
865 bytes, patch
davidb
: review+
Details | Diff | Splinter Review
4.40 KB, patch
davidb
: review+
Details | Diff | Splinter Review
4.70 KB, patch
davidb
: review+
Details | Diff | Splinter Review
2.81 KB, patch
davidb
: review+
Details | Diff | Splinter Review
5.49 KB, patch
davidb
: review+
Details | Diff | Splinter Review
2.43 KB, patch
davidb
: review+
Details | Diff | Splinter Review
3.93 KB, patch
davidb
: review+
Details | Diff | Splinter Review
1.93 KB, patch
davidb
: review+
Details | Diff | Splinter Review
1.38 KB, patch
davidb
: review+
Details | Diff | Splinter Review
1.26 KB, patch
davidb
: review+
Details | Diff | Splinter Review
1.37 KB, patch
davidb
: review+
Details | Diff | Splinter Review
5.11 KB, patch
davidb
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8710364 [details] [diff] [review]
add an AccessibleOrProxy class

We have several places that store either a ProxyAccessible* or an Accessible*
 in the same member using a uintptr_t and stealing the lowest bit of the
 pointer.  The goal of the AccessibleOrProxy class is to make this simpler and
 consolidate the logic involved in doing it.
Attachment #8710364 - Flags: review?(dbolter)
(Assignee)

Comment 2

3 years ago
Created attachment 8710365 [details] [diff] [review]
switch MaiAtkObject to use AccessibleOrProxy
Attachment #8710365 - Flags: review?(dbolter)
(Assignee)

Comment 3

3 years ago
Created attachment 8710366 [details] [diff] [review]
switch MaiHyperLink to use AccessibleOrProxy
Attachment #8710366 - Flags: review?(dbolter)
(Assignee)

Comment 4

3 years ago
Created attachment 8710367 [details] [diff] [review]
clean up GetAccessibleWrap()

We can simplify it some to make better use of AccessibleOrProxy.
Attachment #8710367 - Flags: review?(dbolter)
(Assignee)

Comment 5

3 years ago
Created attachment 8710368 [details] [diff] [review]
allow storing proxies in xpcAccessibleGeneric::mIntl
Attachment #8710368 - Flags: review?(dbolter)
(Assignee)

Comment 6

3 years ago
Created attachment 8710369 [details] [diff] [review]
allow constructing xpcAccessibles with proxies
Attachment #8710369 - Flags: review?(dbolter)
(Assignee)

Comment 7

3 years ago
Created attachment 8710370 [details] [diff] [review]
allow xpcAccessibleDocument::mCache to use proxies as keys
Attachment #8710370 - Flags: review?(dbolter)
(Assignee)

Comment 8

3 years ago
Created attachment 8710371 [details] [diff] [review]
fixup xpcAccessible Intl() methods to not assume mIntl is always an Accessible
Attachment #8710371 - Flags: review?(dbolter)
(Assignee)

Comment 9

3 years ago
Created attachment 8710374 [details] [diff] [review]
allow caching xpc documents for remote documents
Attachment #8710374 - Flags: review?(dbolter)
(Assignee)

Comment 10

3 years ago
Created attachment 8710375 [details] [diff] [review]
assert accessibles are only added to non remote xpcAccessibleDocuments
Attachment #8710375 - Flags: review?(dbolter)
(Assignee)

Comment 11

3 years ago
Created attachment 8710377 [details] [diff] [review]
allow caching proxies in xpcAccessibleDocuments
Attachment #8710377 - Flags: review?(dbolter)
(Assignee)

Comment 12

3 years ago
Created attachment 8710378 [details] [diff] [review]
add DocAccessibleParent::GetXPCAccessible()
Attachment #8710378 - Flags: review?(dbolter)
(Assignee)

Comment 13

3 years ago
Created attachment 8710379 [details] [diff] [review]
factor dispatching nsIAccessibleEvents out of HandleAccEvent()
Attachment #8710379 - Flags: review?(dbolter)
(Assignee)

Comment 14

3 years ago
Created attachment 8710380 [details] [diff] [review]
fire nsIAccessibleStateChangeEvents for proxied accessibles
Attachment #8710380 - Flags: review?(dbolter)
(Assignee)

Comment 15

3 years ago
Created attachment 8710381 [details] [diff] [review]
fire nsIAccessibleTextChangeEvents for proxies
Attachment #8710381 - Flags: review?(dbolter)
(Assignee)

Comment 16

3 years ago
Created attachment 8710382 [details] [diff] [review]
fire nsIAccessibleEvents for proxied accessibles
Attachment #8710382 - Flags: review?(dbolter)
(Assignee)

Comment 17

3 years ago
Created attachment 8710383 [details] [diff] [review]
fire nsIAccessibleCaretMoveEvents for proxies
Attachment #8710383 - Flags: review?(dbolter)

Updated

3 years ago
Assignee: nobody → tbsaunde+mozbugs
Status: NEW → ASSIGNED

Comment 18

3 years ago
Comment on attachment 8710369 [details] [diff] [review]
allow constructing xpcAccessibles with proxies

Review of attachment 8710369 [details] [diff] [review]:
-----------------------------------------------------------------

why do you need this?
(Assignee)

Comment 19

3 years ago
(In reply to alexander :surkov from comment #18)
> Comment on attachment 8710369 [details] [diff] [review]
> allow constructing xpcAccessibles with proxies
> 
> Review of attachment 8710369 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> why do you need this?

for the later patches in the series?

Comment 20

3 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #19)
> (In reply to alexander :surkov from comment #18)
> > Comment on attachment 8710369 [details] [diff] [review]
> > allow constructing xpcAccessibles with proxies
> > 
> > Review of attachment 8710369 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > why do you need this?
> 
> for the later patches in the series?

I was rather asking about the bug in whole. I see you may be using that for e10s layer testing, are there other consumers/reasons?
(Assignee)

Comment 21

3 years ago
(In reply to alexander :surkov from comment #20)
> (In reply to Trevor Saunders (:tbsaunde) from comment #19)
> > (In reply to alexander :surkov from comment #18)
> > > Comment on attachment 8710369 [details] [diff] [review]
> > > allow constructing xpcAccessibles with proxies
> > > 
> > > Review of attachment 8710369 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > why do you need this?
> > 
> > for the later patches in the series?
> 
> I was rather asking about the bug in whole. I see you may be using that for
> e10s layer testing, are there other consumers/reasons?

that wasn't really clear since you commented on a random patch.

I think testing is the only thing currently needing this.
Is it okay if I get to reviews on Monday or do we need this sooner? (I might get to them this afternoon)
Comment on attachment 8710364 [details] [diff] [review]
add an AccessibleOrProxy class

Review of attachment 8710364 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/base/AccessibleOrProxy.h
@@ +18,5 @@
> +  /**
> +   * This class stores an Accessible* or a ProxyAccessible* in a safe manner
> +   * with size sizeof(void*).
> +   */
> +class AccessibleOrProxy

Should probably remove the comment indent.
Attachment #8710364 - Flags: review?(dbolter) → review+
Attachment #8710365 - Flags: review?(dbolter) → review+
Attachment #8710366 - Flags: review?(dbolter) → review+
Attachment #8710367 - Flags: review?(dbolter) → review+
Attachment #8710368 - Flags: review?(dbolter) → review+
Attachment #8710369 - Flags: review?(dbolter) → review+
Comment on attachment 8710370 [details] [diff] [review]
allow xpcAccessibleDocument::mCache to use proxies as keys

Review of attachment 8710370 [details] [diff] [review]:
-----------------------------------------------------------------

(Aside: I'm just reviewing for what you've done here, not reviewing for anything you might have missed)
Attachment #8710370 - Flags: review?(dbolter) → review+
Comment on attachment 8710371 [details] [diff] [review]
fixup xpcAccessible Intl() methods to not assume mIntl is always an Accessible

Review of attachment 8710371 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/xpcom/xpcAccessibleHyperText.h
@@ +45,5 @@
>  private:
> +  HyperTextAccessible* Intl()
> +  {
> +    if (Accessible* acc = mIntl.AsAccessible()) {
> +    return acc->AsHyperText();

nit: need more indent on return.
Attachment #8710371 - Flags: review?(dbolter) → review+
Attachment #8710374 - Flags: review?(dbolter) → review+
Comment on attachment 8710375 [details] [diff] [review]
assert accessibles are only added to non remote xpcAccessibleDocuments

Review of attachment 8710375 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/xpcom/xpcAccessibleDocument.h
@@ +29,4 @@
>  
>    xpcAccessibleDocument(ProxyAccessible* aProxy, uint32_t aInterfaces) :
> +    xpcAccessibleHyperText(aProxy, aInterfaces), mCache(kDefaultCacheLength),
> + mRemote(true) {}

nit: more (double) indent for mRemote initializer
Attachment #8710375 - Flags: review?(dbolter) → review+
Attachment #8710377 - Flags: review?(dbolter) → review+
Comment on attachment 8710378 [details] [diff] [review]
add DocAccessibleParent::GetXPCAccessible()

Review of attachment 8710378 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/ipc/DocAccessibleParent.cpp
@@ +294,5 @@
>  }
>  
> +xpcAccessibleGeneric*
> +DocAccessibleParent::GetXPCAccessible(ProxyAccessible* aProxy)
> +{

Should we assert mRemote here?
Attachment #8710379 - Flags: review?(dbolter) → review+
Attachment #8710380 - Flags: review?(dbolter) → review+
Attachment #8710378 - Flags: review?(dbolter) → review+
Attachment #8710381 - Flags: review?(dbolter) → review+
Attachment #8710382 - Flags: review?(dbolter) → review+
Attachment #8710383 - Flags: review?(dbolter) → review+
(Assignee)

Comment 28

3 years ago
Created attachment 8711636 [details] [diff] [review]
allow caching xpc documents for remote documents

We need to handle the case that accessibility is enabled in a child process, but not the parent.  The easiest way to do that is to change from using a member to using a static pointer to a hash table.
Attachment #8711636 - Flags: review?(dbolter)
Attachment #8711636 - Flags: review?(dbolter) → review+
You need to log in before you can comment on or make changes to this bug.