Closed
Bug 1113407
Opened 10 years ago
Closed 10 years ago
clean up remaining proxies when documents are destroyed
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: tbsaunde, Assigned: tbsaunde)
References
Details
Attachments
(1 file)
2.69 KB,
patch
|
davidb
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8538875 -
Flags: review?(surkov.alexander)
Comment 2•10 years ago
|
||
Comment on attachment 8538875 [details] [diff] [review] cleanup wrappers on doc shutdown Review of attachment 8538875 [details] [diff] [review]: ----------------------------------------------------------------- it'd be good to get David to take a look at ordering ::: accessible/ipc/DocAccessibleParent.h @@ +104,5 @@ > > uint32_t AddSubtree(ProxyAccessible* aParent, > const nsTArray<AccessibleData>& aNewTree, uint32_t aIdx, > uint32_t aIdxInParent); > + static PLDHashOperator ShutdownAccessibles(ProxyEntry* e, void*); please rename 'e', it's good to specify the second argument is not used, either by name or by comment btw, cannot it be inline? ::: accessible/ipc/ProxyAccessible.h @@ +29,5 @@ > } > + ~ProxyAccessible() > + { > + MOZ_COUNT_DTOR(ProxyAccessible); > + MOZ_ASSERT(!mWrapper); indent
Attachment #8538875 -
Flags: review?(surkov.alexander) → review?(dbolter)
Assignee | ||
Comment 3•10 years ago
|
||
> ::: accessible/ipc/DocAccessibleParent.h > @@ +104,5 @@ > > > > uint32_t AddSubtree(ProxyAccessible* aParent, > > const nsTArray<AccessibleData>& aNewTree, uint32_t aIdx, > > uint32_t aIdxInParent); > > + static PLDHashOperator ShutdownAccessibles(ProxyEntry* e, void*); > > please rename 'e', it's good to specify the second argument is not used, > either by name or by comment how is it not obvious from not having a name? > btw, cannot it be inline? its called indirectly so what would be the point?
Comment 4•10 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #3) > > > + static PLDHashOperator ShutdownAccessibles(ProxyEntry* e, void*); > > > > please rename 'e', it's good to specify the second argument is not used, > > either by name or by comment > > how is it not obvious from not having a name? for me it may look like a bad or inaccurate style, like if somebody didn't care to give it a name > > btw, cannot it be inline? > > its called indirectly so what would be the point? well, I don't much about compiler magic that could happen, so if I see something small then I prefer to see it inlined.
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to alexander :surkov from comment #4) > (In reply to Trevor Saunders (:tbsaunde) from comment #3) > > > > + static PLDHashOperator ShutdownAccessibles(ProxyEntry* e, void*); > > > > > > please rename 'e', it's good to specify the second argument is not used, > > > either by name or by comment > > > > how is it not obvious from not having a name? > > for me it may look like a bad or inaccurate style, like if somebody didn't > care to give it a name if they don't name it how can they use it? (hint they can't) > > > btw, cannot it be inline? > > > > its called indirectly so what would be the point? > > well, I don't much about compiler magic that could happen, so if I see > something small then I prefer to see it inlined. it won't be inlined, you'd just add a useless keyword. And if your building with LTO and the compiler manages to do the constant propigation / function cloning to get rid of the indirect call then it should be smart enough to make its own call on the wizdom of inlining.
Comment 6•10 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #5) > > > how is it not obvious from not having a name? > > > > for me it may look like a bad or inaccurate style, like if somebody didn't > > care to give it a name > > if they don't name it how can they use it? (hint they can't) name is not necessary in header
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to alexander :surkov from comment #6) > (In reply to Trevor Saunders (:tbsaunde) from comment #5) > > > > > how is it not obvious from not having a name? > > > > > > for me it may look like a bad or inaccurate style, like if somebody didn't > > > care to give it a name > > > > if they don't name it how can they use it? (hint they can't) > > name is not necessary in header and this is a header. /* unused */ seems rather ugly, and naming something seems dumb because then people can refer to it.
Comment 8•10 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #7) > (In reply to alexander :surkov from comment #6) > > (In reply to Trevor Saunders (:tbsaunde) from comment #5) > > > > > > > how is it not obvious from not having a name? > > > > > > > > for me it may look like a bad or inaccurate style, like if somebody didn't > > > > care to give it a name > > > > > > if they don't name it how can they use it? (hint they can't) > > > > name is not necessary in header > > and this is a header. exactly > /* unused */ seems rather ugly, and naming something seems dumb because then > people can refer to it. 'unused' at least is explicit and you cannot be blamed being inaccurate
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to alexander :surkov from comment #8) > (In reply to Trevor Saunders (:tbsaunde) from comment #7) > > (In reply to alexander :surkov from comment #6) > > > (In reply to Trevor Saunders (:tbsaunde) from comment #5) > > > > > > > > > how is it not obvious from not having a name? > > > > > > > > > > for me it may look like a bad or inaccurate style, like if somebody didn't > > > > > care to give it a name > > > > > > > > if they don't name it how can they use it? (hint they can't) > > > > > > name is not necessary in header > > > > and this is a header. > > exactly err, it is not a header and it is a definition not a prototype. > > /* unused */ seems rather ugly, and naming something seems dumb because then > > people can refer to it. > > 'unused' at least is explicit and you cannot be blamed being inaccurate you can certainly blaimed for allowing people to do something silly when you don't have to.
Comment 10•10 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #9) > > exactly > > err, it is not a header and it is a definition not a prototype. I commented about prototype > you can certainly blaimed for allowing people to do something silly when you > don't have to. it's not the case
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to alexander :surkov from comment #10) > (In reply to Trevor Saunders (:tbsaunde) from comment #9) > > > > exactly > > > > err, it is not a header and it is a definition not a prototype. > > I commented about prototype er ok, I don't really care about the prototype.
Comment 12•10 years ago
|
||
Comment on attachment 8538875 [details] [diff] [review] cleanup wrappers on doc shutdown Review of attachment 8538875 [details] [diff] [review]: ----------------------------------------------------------------- OK. Please add the harmless /* unused */ comment to the second arg in the prototype and fix the indent as per surkov. One question inline: ::: accessible/ipc/DocAccessibleParent.cpp @@ +159,4 @@ > MOZ_ASSERT(mChildDocs.IsEmpty(), > "why wheren't the child docs destroyed already?"); > + > + mAccessibles.EnumerateEntries(ShutdownAccessibles, nullptr); How did you notice this was necessary?
Attachment #8538875 -
Flags: review?(dbolter) → review+
Assignee | ||
Comment 13•10 years ago
|
||
> How did you notice this was necessary?
not doing it can cause crashes that I saw locally.
Assignee | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeeba6f2f1e1
https://hg.mozilla.org/mozilla-central/rev/eeeba6f2f1e1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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
•