Closed
Bug 942032
Opened 11 years ago
Closed 10 years ago
[AudioManager] Add Singleton Protection and Prevent been Created from Content Process.
Categories
(Firefox OS Graveyard :: AudioChannel, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mchen, Assigned: kikuo)
Details
(Whiteboard: [u=devices c=media p=0])
Attachments
(1 file, 4 obsolete files)
4.57 KB,
patch
|
kikuo
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•11 years ago
|
||
AudioManager now became a system service in chrome process for controlling audio related configuration (volume control, routing path control, mic mute). So it should be guaranteed to have only one instance in chrome process and can't be created from content process.
Whiteboard: [u=devices c=media p=0]
Updated•11 years ago
|
Component: General → AudioChannel
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → kikuo
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8428554 [details] [diff] [review] Add singleton pattern to AudioManager, and avoid the access from content process. Any feedback would be appreciated.
Attachment #8428554 -
Flags: feedback?(slin)
Comment 4•10 years ago
|
||
Comment on attachment 8428554 [details] [diff] [review] Add singleton pattern to AudioManager, and avoid the access from content process. Review of attachment 8428554 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/AudioManager.cpp @@ +464,5 @@ > +AudioManager* > +AudioManager::Get() > +{ > + if (!sAudioManager) > + { // Avoid createing multiple AudioManager instance inside main process Put the { at the end of the if statement, unless it is the coding style of this current file. Comment should be a sentence, end it with a period. ::: dom/system/gonk/AudioManager.h @@ +43,5 @@ > , public nsIObserver > { > public: > + static already_AddRefed<AudioManager> GetInstance(); > + static AudioManager* Get(); You can move the implementation of Get() to your GetInstance(), and remove this method.
Attachment #8428554 -
Flags: feedback?(slin) → feedback+
Comment 5•10 years ago
|
||
Btw, you don't need to copy the exact bug title as your check-in comment, rephrase what your're doing here into one single line should be good enough.
Assignee | ||
Comment 6•10 years ago
|
||
New patch to fix issue with consistent style and remove unnecessary function.
Attachment #8428554 -
Attachment is obsolete: true
Attachment #8429003 -
Flags: review?(mchen)
Attachment #8429003 -
Flags: feedback?(slin)
Comment 7•10 years ago
|
||
Comment on attachment 8429003 [details] [diff] [review] patch Review of attachment 8429003 [details] [diff] [review]: ----------------------------------------------------------------- "avoid accessing it from content processes." ::: dom/system/gonk/AudioManager.cpp @@ +473,5 @@ > + if (!sAudioManager) { > + sAudioManager = new AudioManager(); > + } > + > + nsRefPtr<AudioManager> audioMgr = (AudioManager*)sAudioManager; Use sAudioManager.get()
Attachment #8429003 -
Flags: feedback?(slin) → feedback+
Assignee | ||
Comment 8•10 years ago
|
||
Instead of casting sAudioManaget to AudioManager* directly, using sAudioManager.get() is a better way.
Attachment #8429003 -
Attachment is obsolete: true
Attachment #8429003 -
Flags: review?(mchen)
Attachment #8429086 -
Flags: review?(mchen)
Attachment #8429086 -
Flags: feedback?(slin)
Updated•10 years ago
|
Attachment #8429086 -
Flags: feedback?(slin) → feedback+
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8429086 [details] [diff] [review] patch Review of attachment 8429086 [details] [diff] [review]: ----------------------------------------------------------------- Thanks to contribute this patch. And there are some comments here. ::: dom/system/gonk/AudioManager.cpp @@ +458,5 @@ > NS_WARNING("Failed to remove mozsettings-changed observer!"); > } > } > > +StaticRefPtr<AudioManager> AudioManager::sAudioManager; Could we just declare this global variable here and outside the class of AudioManager? @@ +465,5 @@ > +AudioManager::GetInstance() > +{ > + // Avoid createing AudioManager from content process. > + if (XRE_GetProcessType() != GeckoProcessType_Default) { > + return nullptr; According to there is no use case of using AudioManager on the content process and there is no IPC client, I suggest to add MOZ_RUNTIMEABORT(). @@ +470,5 @@ > + } > + > + // Avoid createing multiple AudioManager instance inside main process. > + if (!sAudioManager) { > + sAudioManager = new AudioManager(); This global variable should join the "ClearOnShutdown()".
Attachment #8429086 -
Flags: review?(mchen)
Attachment #8429086 -
Flags: feedback?(slin)
Attachment #8429086 -
Flags: feedback+
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8429086 [details] [diff] [review] patch Sorry to modify the feedback+ from Shelly.
Attachment #8429086 -
Flags: feedback?(slin) → feedback+
Assignee | ||
Comment 11•10 years ago
|
||
1) Move singleton object out of class AudioManager. Treat it as a static global variable in AudioManager.cpp scope. 2) It seems that there's no MOZ_RUNTIMEABOUT() anymore, so I choose MOZ_CRASH("Message") instead. 3) Add ClearOnShutdown(&gAudioManager) to release resources on shutdown.
Attachment #8429086 -
Attachment is obsolete: true
Attachment #8429197 -
Flags: review?(mchen)
Attachment #8429197 -
Flags: feedback?(slin)
Comment 12•10 years ago
|
||
Comment on attachment 8429197 [details] [diff] [review] patch.diff Review of attachment 8429197 [details] [diff] [review]: ----------------------------------------------------------------- I think the patch is good enough for mchen to review :)
Attachment #8429197 -
Flags: feedback?(slin)
Reporter | ||
Comment 13•10 years ago
|
||
Comment on attachment 8429197 [details] [diff] [review] patch.diff Review of attachment 8429197 [details] [diff] [review]: ----------------------------------------------------------------- Good work. give review+ after fixing these nits. ::: dom/system/gonk/AudioManager.cpp @@ +459,5 @@ > NS_WARNING("Failed to remove mozsettings-changed observer!"); > } > } > > +static StaticRefPtr<AudioManager> gAudioManager; There are a few cases used g as prefix and most cases used s (ex: sAudioManager) @@ +469,5 @@ > + if (XRE_GetProcessType() != GeckoProcessType_Default) { > + MOZ_CRASH("Non-chrome processes should not get here."); > + } > + > + // Avoid createing multiple AudioManager instance inside main process. nit: indent. ::: dom/system/gonk/AudioManager.h @@ +16,5 @@ > #ifndef mozilla_dom_system_b2g_audiomanager_h__ > #define mozilla_dom_system_b2g_audiomanager_h__ > > #include "mozilla/Observer.h" > +#include "mozilla/StaticPtr.h" This can be moved to AudioManager.cpp.
Attachment #8429197 -
Flags: review?(mchen) → review+
Assignee | ||
Comment 14•10 years ago
|
||
1) Refine variable prefix fromg 'g'(global) to 's'(static). 2) Remove nit indent. 3) Include header("mozilla/StaticPtr.h") in AudioManager.cpp directly.
Attachment #8429197 -
Attachment is obsolete: true
Attachment #8429826 -
Flags: review?(mchen)
Attachment #8429826 -
Flags: feedback?(slin)
Comment 15•10 years ago
|
||
Comment on attachment 8429826 [details] [diff] [review] patch.diff Review of attachment 8429826 [details] [diff] [review]: ----------------------------------------------------------------- Nice work! Once you got an approval from a reviewer, please feel free to r+ yourself, and comment with "carry r+ from _your_reviewer_". And don't forget to run and paste your try server result :)
Attachment #8429826 -
Flags: feedback?(slin)
Assignee | ||
Updated•10 years ago
|
Attachment #8429826 -
Flags: review?(mchen) → review+
Assignee | ||
Comment 16•10 years ago
|
||
carry r+ from mchen. Thanks for slin's feedback and mchen's review.
Assignee | ||
Comment 17•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=63d6382caec6
Keywords: checkin-needed
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/f76aa5cf4e5a
Keywords: checkin-needed
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f76aa5cf4e5a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•