Closed Bug 1113407 Opened 5 years ago Closed 5 years ago

clean up remaining proxies when documents are destroyed

Categories

(Core :: Disability Access APIs, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(1 file)

No description provided.
Attachment #8538875 - Flags: review?(surkov.alexander)
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)
> ::: 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?
(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.
(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.
(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
(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.
(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
(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.
(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
(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 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+
> How did you notice this was necessary?

not doing it can cause crashes that I saw locally.
https://hg.mozilla.org/mozilla-central/rev/eeeba6f2f1e1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Assignee: nobody → tbsaunde+mozbugs
You need to log in before you can comment on or make changes to this bug.