Closed
Bug 1043854
Opened 10 years ago
Closed 10 years ago
FFOS [flame] FmRadio is turned on while turning on/off airplay mode.
Categories
(Firefox OS Graveyard :: Gaia::FMRadio, defect)
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 1042478
People
(Reporter: xianmao.meng, Unassigned)
References
Details
(Keywords: regression, Whiteboard: [sprd333713][partner-blocker])
Attachments
(1 file)
698 bytes,
patch
|
pzhang
:
review-
|
Details | Diff | Splinter Review |
steps to reproduce: 1.turn on FmRadio app,then stop FmRadio by click the stop button; 2.show utility tray,and start the airplay mode; 3.turn off the airplay mode; test result: 1.when turn on the the airplay mode,the FmRadio will be turned on for about 2second; 2.when turn off the the airplay mode,FmRadio will be turned.
The main cause is that: the addEventListener of 'statechange' is not been removed successfully in airplay mode helper,when opening the FmRadio at the first time.
Flags: needinfo?(timdream)
Flags: needinfo?(pzhang)
Comment 2•10 years ago
|
||
Just FYI. If there is no SIM card, this issue cannot be reproduced. When there is SIM card inserted, it took longer time to turn off and on airplane mode. Not sure if it's related.
Comment 3•10 years ago
|
||
(In reply to 孟宪茂 from comment #1) > The main cause is that: > > the addEventListener of 'statechange' is not been removed successfully in > airplay mode helper,when opening the FmRadio at the first time. Hi 宪茂, would you mind make a patch and send a PR? You can flag r? on me for review, thanks.
Flags: needinfo?(pzhang)
Updated•10 years ago
|
Flags: needinfo?(xianmao.meng)
Attachment #8463196 -
Flags: review?(pzhang)
Flags: needinfo?(xianmao.meng)
(In reply to Pin Zhang [:pzhang] from comment #3) > (In reply to 孟宪茂 from comment #1) > > The main cause is that: > > > > the addEventListener of 'statechange' is not been removed successfully in > > airplay mode helper,when opening the FmRadio at the first time. > > Hi 宪茂, would you mind make a patch and send a PR? You can flag r? on me for > review, thanks. Hi Pin Zhang, Please help review the patch. Thanks!
Flags: needinfo?(pzhang)
Comment 7•10 years ago
|
||
Comment on attachment 8463196 [details] [diff] [review] airplay_mode.patch Review of attachment 8463196 [details] [diff] [review]: ----------------------------------------------------------------- Hi 宪茂, thanks for your patch. As your patch, the problem here is that the same callback could be registered as event listener multiple times (in |addEventListener|, the event listener is simply cached by appending to an array without checking if it has been registered), but only the first one is removed when calling |removeEventListener|. So the right way to go should be register callback function only once by checking if it's already registered when calling |addEventListener|. ::: shared/js/airplane_mode_helper.js @@ +57,5 @@ > }, > removeEventListener: function(eventName, callback) { > if (eventName === kEventName) { > + for (var i = 0; i < this._callbacks.length; i++) { > + if(this._callbacks[i].name === callback.name){ Nits, s/(/ (/, please leave a space before the opening bracket, and s/)/) /, i.e. leave a space after the closing bracket. I don't think this is the right way to go, you can't rely on the function name, name could be shared by anonymous functions, like: var callbacks = []; // Push an anonymous function with name |a|. callbacks.push(function a() {}); // Push another anonymous function with the same name |a|. callbacks.push(function a() {}); // true console.log(callbacks[0].name == callbacks[1].name); // false console.log(callbacks[0] == callbacks[1]); @@ +60,5 @@ > + for (var i = 0; i < this._callbacks.length; i++) { > + if(this._callbacks[i].name === callback.name){ > + this._callbacks.splice(i, 1); > + } > + }; Nits, wrong indention, and no semicolon. BTW, this kinda nits could be found by running |gjslint --nojsdoc airplane_mode_helper.js|[1]. [1] https://developer.mozilla.org/fr/Firefox_OS/Platform/Gaia/Hacking#Additional_rules
Attachment #8463196 -
Flags: review?(pzhang) → review-
Updated•10 years ago
|
Flags: needinfo?(pzhang)
(In reply to Pin Zhang [:pzhang] from comment #7) > Comment on attachment 8463196 [details] [diff] [review] > airplay_mode.patch > > Review of attachment 8463196 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hi 宪茂, thanks for your patch. As your patch, the problem here is that the > same callback could be registered as event listener multiple times (in > |addEventListener|, the event listener is simply cached by appending to an > array without checking if it has been registered), but only the first one is > removed when calling |removeEventListener|. So the right way to go should be > register callback function only once by checking if it's already registered > when calling |addEventListener|. > > ::: shared/js/airplane_mode_helper.js > @@ +57,5 @@ > > }, > > removeEventListener: function(eventName, callback) { > > if (eventName === kEventName) { > > + for (var i = 0; i < this._callbacks.length; i++) { > > + if(this._callbacks[i].name === callback.name){ > > Nits, s/(/ (/, please leave a space before the opening bracket, and s/)/) /, > i.e. leave a space after the closing bracket. > > I don't think this is the right way to go, you can't rely on the function > name, name could be shared by anonymous functions, like: > > var callbacks = []; > // Push an anonymous function with name |a|. > callbacks.push(function a() {}); > // Push another anonymous function with the same name |a|. > callbacks.push(function a() {}); > // true > console.log(callbacks[0].name == callbacks[1].name); > // false > console.log(callbacks[0] == callbacks[1]); > > @@ +60,5 @@ > > + for (var i = 0; i < this._callbacks.length; i++) { > > + if(this._callbacks[i].name === callback.name){ > > + this._callbacks.splice(i, 1); > > + } > > + }; > > Nits, wrong indention, and no semicolon. > > BTW, this kinda nits could be found by running |gjslint --nojsdoc > airplane_mode_helper.js|[1]. > > [1] > https://developer.mozilla.org/fr/Firefox_OS/Platform/Gaia/ > Hacking#Additional_rules Hi Pin Zhang, Thank you for your advices. 1.It is all right that "only the first one is removed when calling |removeEventListener|"; 2.use the function name to remove the callback may not the good way, but mostly the callbacks being removed are not same,as they all have meaningful names. BUT the current design in airplane_mode_helper.js, when the 'statechange' happen,we can not remove the callback. addEventListener: function(eventName, callback) { if (eventName === kEventName) { this._callbacks.push(callback); console.log('-->' + this._callbacks.indexOf(callback));//the result is 0 } removeEventListener: function(eventName, callback) { if (eventName === kEventName) { var index = this._callbacks.indexOf(callback); console.log(--> "The index now is " + index);//the result is -1 if (index >= 0) { this._callbacks.splice(index, 1); } Just using the way "this._callbacks.splice(index, 1);" to remove the callback does not work,as the callback is in the different action scope,they are not exactly the same. In my opinion,using the function name to remove the callback is fine. Could you have suggestion to fix this issue?
Flags: needinfo?(pzhang)
Comment 9•10 years ago
|
||
Hi 宪茂, my suggestion is modifying |addEventListener| instead of |removeEventListener|, i think checking if the callback already be registered will be enough, so the code looks like: addEventListener: function(eventName, callback) { if (eventName === kEventName && this._callbacks.indexOf(callback) === -1) { this._callbacks.push(callback); } } does it make sense?
Flags: needinfo?(pzhang)
Reporter | ||
Comment 10•10 years ago
|
||
(In reply to Pin Zhang [:pzhang] from comment #9) > Hi 宪茂, my suggestion is modifying |addEventListener| instead of > |removeEventListener|, i think checking if the callback already be > registered will be enough, so the code looks like: > > addEventListener: function(eventName, callback) { > if (eventName === kEventName && this._callbacks.indexOf(callback) === > -1) { > this._callbacks.push(callback); > } > } > > does it make sense? Hi Pin Zhang, It seem that it does make any sense,as the callback has been registered. From my log,the callback is already been registered. console.log('-->' + this._callbacks.indexOf(callback));//the result is 0; When we remove the callback,we can not find the callback.Actually,it is in the first place of _callbacks. My log is bellow: console.log(--> "The index now is " + index);//the result is -1 The main cause is that callback to be pushed and removed is in the different action scope,they are not exactly the same.So we can not remove it. So I think we should modify |removeEventListener|,and change the removing strategy,not the |addEventListener|. Is it right?
Flags: needinfo?(pzhang)
Comment 11•10 years ago
|
||
OK ... so could you give me more detailed information about when is the callback is registered and unregistered? It would be helpful if you give the exactly where the code is, thanks!
Flags: needinfo?(pzhang) → needinfo?(xianmao.meng)
Comment 12•10 years ago
|
||
BTW, could you help to check if bug 1042478 fix this? You can get the patch from here: https://bugzilla.mozilla.org/attachment.cgi?id=8460719
Reporter | ||
Comment 13•10 years ago
|
||
(In reply to Pin Zhang [:pzhang] from comment #11) > OK ... so could you give me more detailed information about when is the > callback is registered and unregistered? It would be helpful if you give the > exactly where the code is, thanks! The process of registering and unregistering is: 1.When opening the Fm app,call AirplaneModeHelper.ready(which is in /gaia/shared/js/airplay_mode_helper.js): window.addEventListener('load', function(e) { AirplaneModeHelper.ready(function() { airplaneModeEnabled = AirplaneModeHelper.getStatus() == 'enabled'; 2.If the AirplaneModeHelper.init() is not ready,it can not get _cachedStatus; so in the ready attribute of AirplaneModeHelper,the status of this._cachedStatus === '' is true; then register the callback (function onChangeEvent()) using the addEventListener attribute of AirplaneModeHelper; 3.At this moment,it has got _cachedStatus,and the status of this._cachedStatus !== '' is true; then unregister the callback (function onChangeEvent()) using the removeEventListener attribute of AirplaneModeHelper; 4.If the callback is removed normally,it can not |addEventListener| the 'statechange',and AirplaneModeHelper.ready executes only once; If the callback can not been removed, when the 'statechange' event happen,AirplaneModeHelper.ready will execute,then the init() of FmRadio executes and then FmRadio is opened. Besides,in [2],if AirplaneModeHelper.init() is ready and get the _cachedStatus,the callback of registering and unregistering will not happen,the Fm function is normal. Actually,if the case in [2] happened,every time you turn on and off airplay mode,the FmRadio will be opened twice,as the 'statechange' has status "enabling" and "enabled". Do you have any suggestion? Thanks!
Flags: needinfo?(xianmao.meng) → needinfo?(pzhang)
Reporter | ||
Comment 14•10 years ago
|
||
Hi peipei, The issue seems more serious.Please help add the owner airplay mode to check the issue. Thanks!
Flags: needinfo?(pcheng)
Reporter | ||
Comment 15•10 years ago
|
||
(In reply to Pin Zhang [:pzhang] from comment #12) > BTW, could you help to check if bug 1042478 fix this? You can get the patch > from here: > https://bugzilla.mozilla.org/attachment.cgi?id=8460719 It seem that it is related to this issue,it may sovle the callback's difference,I will check it. Thanks!
Comment 16•10 years ago
|
||
Pin, is there any risk of landing the fix for 1042478 in V1.4?
Depends on: 1042478
Flags: needinfo?(pcheng)
Updated•10 years ago
|
Whiteboard: [sprd333713][partner-blocker]
Comment 17•10 years ago
|
||
(In reply to Peipei Cheng from comment #16) > Pin, is there any risk of landing the fix for 1042478 in V1.4? No, i don't think there is anything risk to land it in v1.4.
Flags: needinfo?(pzhang)
Comment 18•10 years ago
|
||
I didn't encounter this problem after applying the patch of bug 1042478. So it should be the same reason. duplicate to bug 1042478.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Updated•10 years ago
|
blocking-b2g: 1.4? → ---
Updated•10 years ago
|
Keywords: regression
You need to log in
before you can comment on or make changes to this bug.
Description
•