1191433 - cleanup proxy documents some

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: add methods to downcast ProxyAccessible to DocAccessibleParent

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: add ProxyAccessible::Document

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: use ProxyAccessible::AsDoc() in ProxyAccessible::Shutdown()

Comment 4

[Lorien Hu (:lsocks)]

Comment on attachment 8643815 [details] [diff] [review]
add methods to downcast ProxyAccessible to DocAccessibleParent

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

::: accessible/ipc/DocAccessibleParent.h
@@ +169,5 @@
> +ProxyAccessible::AsDoc()
> +{
> +  return IsDoc() ? static_cast<DocAccessibleParent*>(this) : nullptr;
> +}
> +

Shouldn't this be in ProxyAccessible.cpp instead?

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to Lorien Hu (:lsocks) from comment #4)
> Comment on attachment 8643815 [details] [diff] [review]
> add methods to downcast ProxyAccessible to DocAccessibleParent
> 
> Review of attachment 8643815 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/ipc/DocAccessibleParent.h
> @@ +169,5 @@
> > +ProxyAccessible::AsDoc()
> > +{
> > +  return IsDoc() ? static_cast<DocAccessibleParent*>(this) : nullptr;
> > +}
> > +
> 
> Shouldn't this be in ProxyAccessible.cpp instead?

I'd say not so it can be inlined.

Comment 6

[Lorien Hu (:lsocks)]

Comment on attachment 8643817 [details] [diff] [review]
add ProxyAccessible::Document

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

::: accessible/ipc/ProxyAccessible.h
@@ +310,5 @@
> +   * Return the document containing this proxy, or the proxy itself it is a
> +   * document.
> +   */
> +  DocAccessibleParent* Document() const { return mDoc; }
> +

Typo in comment *if it is

Also, could return mDoc in AsDoc() instead as you did here which would allow that to be defined in ProxyAccessible.h instead of DocAccessible.h

Comment 7

[Lorien Hu (:lsocks)]

Comment on attachment 8643818 [details] [diff] [review]
use ProxyAccessible::AsDoc() in ProxyAccessible::Shutdown()

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

I guess I just feel like you could do  mChildren[0]->Document()->Unbind() instead of needing AsDoc

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to Lorien Hu (:lsocks) from comment #6)
> Comment on attachment 8643817 [details] [diff] [review]
> add ProxyAccessible::Document
> 
> Review of attachment 8643817 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/ipc/ProxyAccessible.h
> @@ +310,5 @@
> > +   * Return the document containing this proxy, or the proxy itself it is a
> > +   * document.
> > +   */
> > +  DocAccessibleParent* Document() const { return mDoc; }
> > +
> 
> Typo in comment *if it is
> 
> Also, could return mDoc in AsDoc() instead as you did here which would allow
> that to be defined in ProxyAccessible.h instead of DocAccessible.h

yeah, I just did it the way accessible does it without thinking but that seems to make sense.

It means an extra load one to check its a doc and the next to get the pointer, but meh its almost certainly fine

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to Lorien Hu (:lsocks) from comment #7)
> Comment on attachment 8643818 [details] [diff] [review]
> use ProxyAccessible::AsDoc() in ProxyAccessible::Shutdown()
> 
> Review of attachment 8643818 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess I just feel like you could do  mChildren[0]->Document()->Unbind()
> instead of needing AsDoc

its a fair point, but doing this means we crash instead of doing something wierd if the proxy is marked as being an outer doc and has a child that's not a document.  I can't imagine how that happens, but seems better to not risk it if its easy.

Comment 10

[Lorien Hu (:lsocks)]

Wouldn't you crash anyway as is? It'd be trying to dereference a nullptr if AsDoc returns null which it will if it's not a doc

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to Lorien Hu (:lsocks) from comment #10)
> Wouldn't you crash anyway as is? It'd be trying to dereference a nullptr if
> AsDoc returns null which it will if it's not a doc

but you wouldn't be calling AsDoc() if you called Document()?

Comment 12

[Lorien Hu (:lsocks)]

Okay, fair. I misunderstood you, thought we didn't want the crash.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/640b74a61ade
https://hg.mozilla.org/integration/mozilla-inbound/rev/88ac2b2e9108
https://hg.mozilla.org/integration/mozilla-inbound/rev/a88fd055d901

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/640b74a61ade
https://hg.mozilla.org/mozilla-central/rev/88ac2b2e9108
https://hg.mozilla.org/mozilla-central/rev/a88fd055d901