Trevor Saunders (:tbsaunde) Assignee
	Description • 8 years ago

      No description provided.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 1 • 8 years ago

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.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 4 • 8 years ago

We can simplify it some to make better use of AccessibleOrProxy.

	alexander :surkov (:asurkov)
	Comment 18 • 8 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?

	Trevor Saunders (:tbsaunde) Assignee
	Comment 19 • 8 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?

	alexander :surkov (:asurkov)
	Comment 20 • 8 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?

	Trevor Saunders (:tbsaunde) Assignee
	Comment 21 • 8 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.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 22 • 8 years ago

Is it okay if I get to reviews on Monday or do we need this sooner? (I might get to them this afternoon)

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 23 • 8 years ago

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.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 24 • 8 years ago

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)

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 25 • 8 years ago

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.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 26 • 8 years ago

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

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 27 • 8 years ago

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?

	Trevor Saunders (:tbsaunde) Assignee
	Comment 28 • 8 years ago

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.

	Pulsebot
	Comment 29 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f35ea53092d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/028866ac3b66
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c4c77b8e2e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dbc0a23db6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dc79d4dac30
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb2fb56c44aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/f97fa91a4a9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/1acf6d48e34c
https://hg.mozilla.org/integration/mozilla-inbound/rev/70f830b7665f
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd3e8b694f8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4044d673c2b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/909c18f9fd6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fa48455c395
https://hg.mozilla.org/integration/mozilla-inbound/rev/33fd55182ab7
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb4ffc4e5f67
https://hg.mozilla.org/integration/mozilla-inbound/rev/35675c2952cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/100f1565de78

	Carsten Book [:Tomcat]
	Comment 30 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f35ea53092d7
https://hg.mozilla.org/mozilla-central/rev/028866ac3b66
https://hg.mozilla.org/mozilla-central/rev/8c4c77b8e2e3
https://hg.mozilla.org/mozilla-central/rev/8dbc0a23db6b
https://hg.mozilla.org/mozilla-central/rev/1dc79d4dac30
https://hg.mozilla.org/mozilla-central/rev/cb2fb56c44aa
https://hg.mozilla.org/mozilla-central/rev/f97fa91a4a9e
https://hg.mozilla.org/mozilla-central/rev/1acf6d48e34c
https://hg.mozilla.org/mozilla-central/rev/70f830b7665f
https://hg.mozilla.org/mozilla-central/rev/bd3e8b694f8b
https://hg.mozilla.org/mozilla-central/rev/4044d673c2b2
https://hg.mozilla.org/mozilla-central/rev/909c18f9fd6d
https://hg.mozilla.org/mozilla-central/rev/6fa48455c395
https://hg.mozilla.org/mozilla-central/rev/33fd55182ab7
https://hg.mozilla.org/mozilla-central/rev/bb4ffc4e5f67
https://hg.mozilla.org/mozilla-central/rev/35675c2952cd
https://hg.mozilla.org/mozilla-central/rev/100f1565de78