1036803 - SharedThreadPool::GetEventTarget should be made private

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[JW Wang [:jwwang] [:jw_wang]]

Returning |mEventTarget| from GetEventTarget() could lead to a longer life of |mEventTarget| than SharedThreadPool which defeat the purpose of SpinUntilShutdown().

Since SharedThreadPool implements nsIEventTarget, the user can just cast a SharedThreadPool into an nsIEventTarget when an nsIEventTarget is wanted.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yes, GetEventTarget() would be better private.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Per comment 0 and comment 1, SharedThreadPool::GetEventTarget() [1] should be private.
[1] http://hg.mozilla.org/mozilla-central/file/7f9db2379b3f/content/media/SharedThreadPool.h#l54

Comment 3

[vikneshwar]

Attachment: private.patch

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8455518 [details] [diff] [review]
private.patch

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

feedback=jwwang. Looks good to me.

Hi Matthew,
Can you review this patch? Thanks.

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 8455518 [details] [diff] [review]
private.patch

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

Looks fine, but why not remove GetEventTarget() completely and change NS_FORWARD_SAFE_NSIEVENTTARGET(GetEventTarget()) to NS_FORWARD_SAFE_NSIEVENTTARGET(mEventTarget)?  There are no other users that I can find.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Good idea.

Hi vikneshwar,
Please change the patch as suggested by Matthew and put r=kinetik in the commit message. Also please push the patch to Try to ensure the patch doesn't break any builds on all platforms.

Comment 7

[vikneshwar]

Attachment: eventtarget.patch

Got RED in try push :( not sure whats going wrong

Comment 8

[Matthew Gregan [:kinetik]]

(In reply to vikneshwar from comment #7)
> Created attachment 8456253 [details] [diff] [review]
> eventtarget.patch
> 
> Got RED in try push :( not sure whats going wrong

Patch looks fine... Can you post a link to the try push please?

Comment 9

[vikneshwar]

hai Matthew , here  https://tbpl.mozilla.org/?tree=Try&rev=337cfbe61146 :)

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Can you update the repo and try again? It built fine for me.

Comment 11

[vikneshwar]

hai jwwang , when i run the build with patch it fails , here is the log http://pastebin.com/zdf7iJaC . However it builds fine without the patch

Comment 12

[Matthew Gregan [:kinetik]]

Mystery solved: bug 996465 removed the only call to GetEventTarget().  So with a current tree, this patch works fine.

Comment 13

[vikneshwar]

is there anymore changes i should make to the patch , or its fine for checkin now  ??

Comment 14

[Matthew Gregan [:kinetik]]

Good to go!

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

This is a simple change that don't need run Try tests provided it doesn't break builds.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f41eab62f108

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f41eab62f108