Closed
Bug 1191433
Opened 9 years ago
Closed 9 years ago
cleanup proxy documents some
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: tbsaunde, Assigned: tbsaunde)
Details
Attachments
(3 files)
2.39 KB,
patch
|
lsocks
:
review+
|
Details | Diff | Splinter Review |
945 bytes,
patch
|
lsocks
:
review+
|
Details | Diff | Splinter Review |
829 bytes,
patch
|
lsocks
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8643815 -
Flags: review?(lorien)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8643817 -
Flags: review?(lorien)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8643818 -
Flags: review?(lorien)
Comment 4•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8643815 -
Flags: review?(lorien) → review+
Assignee | ||
Comment 5•9 years ago
|
||
(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•9 years ago
|
||
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 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
(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
Assignee | ||
Comment 9•9 years ago
|
||
(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•9 years ago
|
||
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
Assignee | ||
Comment 11•9 years ago
|
||
(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•9 years ago
|
||
Okay, fair. I misunderstood you, thought we didn't want the crash.
Comment 13•9 years ago
|
||
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•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/640b74a61ade https://hg.mozilla.org/mozilla-central/rev/88ac2b2e9108 https://hg.mozilla.org/mozilla-central/rev/a88fd055d901
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Updated•7 years ago
|
Assignee: nobody → tbsaunde+mozbugs
You need to log in
before you can comment on or make changes to this bug.
Description
•