Closed Bug 1110510 Opened 5 years ago Closed 5 years ago

Create proxies for document accessibles

Categories

(Core :: Disability Access APIs, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(2 files)

I'm not sure if this the cause of bug 1099153, but its certainly a bug.  Because document accessibles are created differently from other proxies we need make sure to create proxies there too.
Attachment #8535288 - Flags: review?(surkov.alexander)
Attachment #8535288 - Attachment is obsolete: true
Attachment #8535288 - Flags: review?(surkov.alexander)
Attachment #8535727 - Flags: review?(surkov.alexander)
Comment on attachment 8535288 [details] [diff] [review]
make sure to create proxies for documents

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

the patch looks reasonable but I do have some lack of knowledge of e10s a11y architecture so it might make sense to ask for another review, it's up to you though

::: accessible/ipc/ProxyAccessible.h
@@ +87,5 @@
>  
>  protected:
> +  ProxyAccessible(DocAccessibleParent* aThisAsDoc) :
> +    mParent(nullptr), mDoc(aThisAsDoc), mWrapper(0), mID(0),
> +    mRole(roles::DOCUMENT)

generic proxy accessible initialized by document role looks weird
Attachment #8535288 - Attachment is obsolete: false
Comment on attachment 8535727 [details] [diff] [review]
make sure to create and destroy proxies for documents

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

::: accessible/ipc/DocAccessibleParent.cpp
@@ +148,5 @@
> +
> +void
> +DocAccessibleParent::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  ProxyDestroyed(this);

don't you need similar for DocManager?
Attachment #8535727 - Flags: review?(surkov.alexander) → review+
> ::: accessible/ipc/ProxyAccessible.h
> @@ +87,5 @@
> >  
> >  protected:
> > +  ProxyAccessible(DocAccessibleParent* aThisAsDoc) :
> > +    mParent(nullptr), mDoc(aThisAsDoc), mWrapper(0), mID(0),
> > +    mRole(roles::DOCUMENT)
> 
> generic proxy accessible initialized by document role looks weird

Its a constructor only used for a subclass which is kind of weird I guess, and its not a fully correct solution, but making documents always have correct role doesn't seem like super urgent problem.
(In reply to alexander :surkov from comment #4)
> Comment on attachment 8535727 [details] [diff] [review]
> make sure to create and destroy proxies for documents
> 
> Review of attachment 8535727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/ipc/DocAccessibleParent.cpp
> @@ +148,5 @@
> > +
> > +void
> > +DocAccessibleParent::ActorDestroy(ActorDestroyReason aWhy)
> > +{
> > +  ProxyDestroyed(this);
> 
> don't you need similar for DocManager?

no, ActorDestroy is called on both root and not root DocAccessible Parents.
yeah, it's just weird, can you pass role as constructor argument?
https://hg.mozilla.org/mozilla-central/rev/a8b8d912bd69
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment on attachment 8535727 [details] [diff] [review]
make sure to create and destroy proxies for documents

>+DocAccessibleParent::ActorDestroy(ActorDestroyReason aWhy)
>+{
>+  ProxyDestroyed(this);
>+  MOZ_ASSERT(mChildDocs.IsEmpty(),
>+      "why wheren't the child docs destroyed already?");

FWIW, I just pushed a trivial typo fix for this assertion-message (s/wheren't/weren't/):
  https://hg.mozilla.org/integration/mozilla-inbound/rev/a170277d1c67

(This assertion flew by in my terminalspew, and I tried to find it in the backscroll with my terminal's "find" functionality, but I got no hits because I was spelling it correctly when searching. :)  Fixing it so that this doesn't bite anyone else.)
Assignee: nobody → tbsaunde+mozbugs
You need to log in before you can comment on or make changes to this bug.