Closed
Bug 956276
Opened 11 years ago
Closed 11 years ago
[ZTE]-Two or more instances of open activity will be launched after received files via bluetooth
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Tracking
(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)
People
(Reporter: amitav.anand, Assigned: julienw)
References
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
(deleted),
audio/mpeg
|
Details | |
(deleted),
text/x-github-pull-request
|
etienne
:
review+
fabrice
:
approval-gaia-v1.3+
|
Details |
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131206152142
Steps to reproduce:
Test Environment:
Device: ZTE
Hardware revision : roamer2
OS version : 1.4.0.0-prerelease
BuildID: 20131227113200
Git Commit Info:2013-12-18 21:39:18 8bbb5170
Steps to reproduce:
1.Pair any phone with the DUT (ZTE).
2.Transfer an MP3 file via blue-tooth.
3.After the file transfer is complete, tap on the notification.
4.The Music app launches and plays the music file just received.
Actual results:
Actual Results:
Carefully listen to the song. More than 1 instance of the mp3 files can be heard overlapped with one another.Pause the song from UI .After Pause the playback of the music file can still be heard.
Expected results:
Music app should play only 1 instance of the file received via blue-tooth.
Reporter | ||
Comment 1•11 years ago
|
||
Seems like this issue is similar to Bug 838927.
OS: All → Linux
Hardware: All → ARM
Comment 2•11 years ago
|
||
According to the symptom, this is a duplicated issue with 838927. That's track it via bug 838927.
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Component: Gaia::Bluetooth File Transfer → Gaia::Music
Resolution: --- → DUPLICATE
Comment 3•11 years ago
|
||
amitav.anand@accenture.com, does the reproduce steps launched two instances of music app at the same time? or one is already launched in the background and another is launched by the notification? and I think this is not the same as bug 838927 because the steps are different so I will first reopen it, thanks.
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(amitav.anand)
Resolution: DUPLICATE → ---
Reporter | ||
Comment 4•11 years ago
|
||
Yes,It launches 2 instance at the same time. On pausing the song from UI the song still plays and on resuming the song both the instance plays together.
Before trying this test I had made sure that the music player is not running.
Flags: needinfo?(amitav.anand)
Comment 5•11 years ago
|
||
Thanks amitav.anand@accenture.com, I followed the reproduce steps and also tested on transferring images, then found the notification launches more than one instance of the gallery open activity as well(I believe it's the same for videos), so this should be a general issue of launching apps from the notification, I am changing the component back to Gaia::Bluetooth File Transfer and modifying the title to fit the description.
Ian, as I described above, I found the open activity will be triggered more than once(my test result is 4 times) when users tap on the complete notification from bluetooth, please re-visit this issue and investigate it, thanks.
Component: Gaia::Music → Gaia::Bluetooth File Transfer
Flags: needinfo?(iliu)
Summary: [ZTE]-Two or more instance of Music App is running when an MP3 file received via BT → [ZTE]-Two or more instances of open activity will be launched after received files via bluetooth
Reporter | ||
Comment 6•11 years ago
|
||
I had traced out the same.
Till onTransferComplete: function bt_onTransferComplete in bluetooth_transfer.js the control flow seems to be correct, as it is called only once. On successfully receiving the file below notification is called:-
NotificationHelper.send(_('transferFinished-receivedSuccessful-title'),fileName,icon,
this.openReceivedFile.bind(this, transferInfo));
This calls gaia/shared/js/notification_helper.js, here notification.onclick = (function() is getting called twice.
Reporter | ||
Comment 7•11 years ago
|
||
in gaia/apps/system/js/notification.js the "tap" method has window.dispatchEvent(event) has been called twice even though there is no change in event paramenter, so this caused music app to play the song twice.
event.initCustomEvent('mozContentEvent', true, true, {
type: 'desktop-notification-click',
id: notificationId
});
window.dispatchEvent(event);
window.dispatchEvent(new CustomEvent('notification-clicked', {
detail: {
id: notificationId
}
}));
//window.dispatchEvent(event);
Commenting out the window.dispatchEvent(event) solves this issue.
kindly comment.
Comment 8•11 years ago
|
||
Thanks amitav.anand@accenture.com, that explained why and make sense to cause this issue, since this is a regression so I am nominating this 1.3? and finding which bug has caused this issue.
blocking-b2g: --- → 1.3?
Keywords: regression
Reporter | ||
Comment 9•11 years ago
|
||
second dispatch of the event has been removed. kindly review.
Attachment #8357640 -
Flags: review?(dkuo)
Comment 10•11 years ago
|
||
(In reply to Dominic Kuo [:dkuo] from comment #8)
> Thanks amitav.anand@accenture.com, that explained why and make sense to
> cause this issue, since this is a regression so I am nominating this 1.3?
> and finding which bug has caused this issue.
This bug is caused by the patch of bug 935094, setting dependency and changing the component to Gaia::System.
Comment 11•11 years ago
|
||
Comment on attachment 8357640 [details]
Pull Request for bug 956276
amitav-anand, thanks for contributing this patch, although it's an one line patch but since I am not the peer of system app, I am redirecting the review to Julien.
Julien, would you please review the patch, thanks.
Attachment #8357640 -
Flags: review?(dkuo) → review?(felash)
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 8357640 [details]
Pull Request for bug 956276
I'm not a peer either but I can review this :)
r=me, but I'd like that you add a simple unit test to system/test/unit/notifications_test.js before merging. Something like:
* add a suite
* in setup:
+ call addNotification, save the returned node in a var
+ add event listeners on window for mozContentEvent and notification-clicked, use "sinon.stub()" functions to do this, that you save in variables.
* in test:
+ dispatch a "tap" event to the notification node
+ test that the stubs were called only once (sinon.assert.calledOnce(stub)).
* in teardown:
+ remove the event listeners
You can ask another feedback from me when you do the test, if you don't feel confident enough ;)
Attachment #8357640 -
Flags: review?(felash) → review+
Comment 13•11 years ago
|
||
FWIW if you guys want to land the patch as is you have my blessing :)
It's fixing what was probably a copy/paste mistake in bug 935094, not adding/modifying logic.
I'm all for a follow up bug to cover notification event dispatch with unit tests, but we should fix the regression from bug 935094 ASAP.
Comment 15•11 years ago
|
||
Probably 1.3+ since a 1.3+ bug has just been marked as duplicate.
Assignee | ||
Comment 16•11 years ago
|
||
mmm AFAIK Download Manager was backed out from 1.3 so I doubt this regression is on 1.3.
QAWanted to find out.
Keywords: qawanted
Assignee | ||
Comment 17•11 years ago
|
||
Francisco might provide some insight as well.
Assignee | ||
Comment 19•11 years ago
|
||
Re: comment 16: maybe this part was not backed out. Etienne is having a look :)
Comment 20•11 years ago
|
||
Looks like it was properly backed out.
So hopefully the qawanted will yieled good news :)
(that the bug only happen on master)
Comment 21•11 years ago
|
||
I was unable to reproduce this issue using the current 1.3 build.
Environmental Variables
Device: Buri v1.3 Mozilla RIL
Build ID: 20140109004002
Gecko: http://hg.mozilla.org/releases/mozilla-aurora/rev/2c8f8683bd0d
Gaia: 22bc6be5b76cdc6d4e9667ff070979041a20ce2f
Platform Version: 28.0a2
Firmware Version: 20131115
Keywords: qawanted
QA Contact: pbylenga
Comment 23•11 years ago
|
||
Hi folks, we did rollback of all the commits for DM. We didn't experience this bug during our development.
Cheers,
F.
Assignee | ||
Comment 24•11 years ago
|
||
Francisco, you surely meant to add "from 1.3", right ?
Comment 25•11 years ago
|
||
Oh! Julien, you are totally right, we removed it 'from 1.3' branch :)
Updated•11 years ago
|
Assignee | ||
Comment 26•11 years ago
|
||
Amitav, do you think you can do the unit tests for your patch? See comment 12 for more information.
Assignee: nobody → amitav.anand
Flags: needinfo?(amitav.anand)
Assignee | ||
Comment 28•11 years ago
|
||
I also took the opportunity to fix jshint warnings.
Attachment #8357640 -
Attachment is obsolete: true
Attachment #8361727 -
Flags: review?(etienne)
Comment 29•11 years ago
|
||
Comment on attachment 8361727 [details]
pull request with unit tests
r=me
With all the boilerplate work that you did I think it's worth it to add 2 tests checking the content of the events (that the right notificationId is passed). But it's up to you, we can land without it.
Attachment #8361727 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 30•11 years ago
|
||
Good idea, I added it, and I changed also some other tests that were using an integer id for a notification whereas in the real life it's strings.
Assignee | ||
Comment 31•11 years ago
|
||
(this is important because the notification id is used in a dataset and as a result is converted to a string, so if it was previously an integer, a comparison would fail)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(amitav.anand)
Assignee | ||
Comment 32•11 years ago
|
||
master: 73898bf32b3da33a0de96438302feb21635285aa
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
blocking-b2g: 1.4? → 1.4+
Comment 33•11 years ago
|
||
This needs to be uplifted to 1.3, since we are in the process of uplifting bug 890440 to 1.3, which will cause this same problem there.
Blocks: 890440
blocking-b2g: 1.4+ → 1.3?
Comment 34•11 years ago
|
||
Comment on attachment 8361727 [details]
pull request with unit tests
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.
We have to uplift the bug that caused this regression.
[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):890440
[User impact] if declined: Notifications show up twice.
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:
Attachment #8361727 -
Flags: approval-gaia-v1.3?(fabrice)
Updated•11 years ago
|
blocking-b2g: 1.3? → 1.3+
Updated•11 years ago
|
Attachment #8361727 -
Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3+
Comment 35•11 years ago
|
||
uplifted to 1.3: https://github.com/mozilla-b2g/gaia/commit/932789749406a79c5630c27c724f1856bd3c3f10
status-b2g-v1.3:
--- → fixed
Updated•11 years ago
|
status-b2g-v1.3T:
--- → fixed
status-b2g-v1.4:
--- → fixed
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
You need to log in
before you can comment on or make changes to this bug.
Description
•