cleanup proxy documents some

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments)

No description provided.
Attachment #8643817 - Flags: review?(lorien)
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?
Attachment #8643815 - Flags: review?(lorien) → review+
(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 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
Attachment #8643817 - Flags: review?(lorien) → review+
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
Attachment #8643818 - Flags: review?(lorien) → review+
(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
(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.
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
(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()?
Okay, fair. I misunderstood you, thought we didn't want the crash.
Assignee: nobody → tbsaunde+mozbugs
You need to log in before you can comment on or make changes to this bug.