Closed Bug 938540 Opened 11 years ago Closed 11 years ago

[WAPPush] Make use of the new Notification API

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::System
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

Attachments

(1 file)

WIP patch
46 bytes, text/x-github-pull-request
gsvelto
: review+
julienw
: feedback+
Details | Review
Follow up of bug 929895
Blocks: 890440
Make use of the new Notification API and remove hack that was introduced in bug 929895
Blocks: 942064
Blocks: 942080
Attached file WIP patch —
Assignee: nobody → lissyx+mozillians
Status: NEW → ASSIGNED
Attachment #8356068 - Flags: review?(gsvelto)
Attachment #8356068 - Flags: feedback?(felash)
Comment on attachment 8356068 [details] [review] WIP patch Looks good to me and thanks for expanding the existing test!
Attachment #8356068 - Flags: review?(gsvelto) → review+
(In reply to Gabriele Svelto [:gsvelto] from comment #5) > Comment on attachment 8356068 [details] [review] > WIP patch > > Looks good to me and thanks for expanding the existing test! Thanks, I'll wait for Julien's feedback on the test themselves.
Comment on attachment 8356068 [details] [review] WIP patch I've tested this in the emulator and unfortunately it doesn't seem to work so I'm clearing the review flag for now. Here'a few bits of my analysis: - When the notification is clicked it seems that the click notification handler is invoked thrice: twice with the message.clicked attribute set to true and one with it set to false. This last event used to happen before too but I'm not sure about the first two calls, it should have been only one. - The close() method is called correctly on the notification but it doesn't seem to do anything leaving the notification in the tray. The only thing I see after it in the logcat is the following line: I/GeckoDump( 45): XXX FIXME : Got a mozContentEvent: desktop-notification-close Sounds like we're missing something... - Finally I've noticed that the new notifications API supports adding a tag to the notifications to identify them. I would like if you could remove the timestamp-in-the-icon-URL hack and use the tag instead. See the 'options' parameter in the Notification constructor on how to specify the tag: https://developer.mozilla.org/en-US/docs/Web/API/notification#Method_overview
Attachment #8356068 - Flags: review+
Comment on attachment 8356068 [details] [review] WIP patch feedback- for the test, as I think we need to completely replace the Notification API with a mock.
Attachment #8356068 - Flags: feedback?(felash) → feedback-
(In reply to Julien Wajsberg [:julienw] from comment #8) > Comment on attachment 8356068 [details] [review] > WIP patch > > feedback- for the test, as I think we need to completely replace the > Notification API with a mock. Done. I also tested the code on Inari and Buri, and it works. I can't tell what is happening with the Emulator ...
(In reply to Alexandre LISSY :gerard-majax from comment #9) > Done. I also tested the code on Inari and Buri, and it works. I can't tell > what is happening with the Emulator ... After discussing on IRC let's disregard my emulator result, there must have been something wrong there because the on-device tests pass. Let's just needinfo :echu before landing as she can test with real messages on real devices and confirm everything works. As it turns out we cannot use the 'tag' field to identify the notification because of bug 951150 so let's keep the code to identify notifications as-is and open a follow-up for that.
Comment on attachment 8356068 [details] [review] WIP patch All comments regarding the test should be addressed now.
Attachment #8356068 - Flags: review?(gsvelto)
Attachment #8356068 - Flags: feedback?(felash)
Attachment #8356068 - Flags: feedback-
Comment on attachment 8356068 [details] [review] WIP patch LGTM :) If Julien's OK with it land at will and don't forget to open a follow up for the notification tag
Attachment #8356068 - Flags: review?(gsvelto) → review+
Comment on attachment 8356068 [details] [review] WIP patch feedback+, please remove the parts that MocksHelper does for you and fix the various small nits and it's good for me !
Attachment #8356068 - Flags: feedback?(felash) → feedback+
(In reply to Julien Wajsberg [:julienw] from comment #13) > Comment on attachment 8356068 [details] [review] > WIP patch > > feedback+, please remove the parts that MocksHelper does for you and fix the > various small nits and it's good for me ! Cool ! PR updated, nits addressed, waiting for green travis now: https://travis-ci.org/mozilla-b2g/gaia/builds/16552228
Sorry, I forgot to add the needinfo earlier. We would need some live testing that this patch does not break WapPush. As far as we could test by emulating a wappush, it was okay.
Flags: needinfo?(echou)
(In reply to Alexandre LISSY :gerard-majax from comment #16) > Sorry, I forgot to add the needinfo earlier. We would need some live testing > that this patch does not break WapPush. As far as we could test by emulating > a wappush, it was okay. Hi Alexandre, Although I'd be very happy to help here, I'm not sure I would be the best person to reach out. Would you mind checking again? :)
Flags: needinfo?(echou)
Sorry, mispelled when adding the needinfo? :) Enpei, could you help us and make sure that this is working correctly on a live setup ? Thanks !
Flags: needinfo?(echu)
(In reply to Alexandre LISSY :gerard-majax from comment #18) > Sorry, mispelled when adding the needinfo? :) > > Enpei, could you help us and make sure that this is working correctly on a > live setup ? Thanks ! Hi Alexandre, Has it landed on master branch that I can verify wap push on Buri? I will arrange test next week since I am working on DSDS testing now.
Flags: needinfo?(lissyx+mozillians)
(In reply to Enpei from comment #19) > (In reply to Alexandre LISSY :gerard-majax from comment #18) > > Sorry, mispelled when adding the needinfo? :) > > > > Enpei, could you help us and make sure that this is working correctly on a > > live setup ? Thanks ! > > Hi Alexandre, > > Has it landed on master branch that I can verify wap push on Buri? I will > arrange test next week since I am working on DSDS testing now. No we are waiting for your test to merge it.
Flags: needinfo?(lissyx+mozillians)
Any can show me how to setup the build? 1. Which branch I should use? 2. Either Gaia or Gecko folder I should install attached patch first before build the code?
(In reply to Enpei from comment #21) > Any can show me how to setup the build? > 1. Which branch I should use? > 2. Either Gaia or Gecko folder I should install attached patch first before > build the code? Gecko and gaia master up to date, and just apply the pull request attached to this patch on gaia :).
Flags: needinfo?(echu)
Enpei, you can apply the patch from the pull request https://github.com/mozilla-b2g/gaia/pull/15039 on gaia master. The easiest way to do this is to run: curl https://github.com/mozilla-b2g/gaia/pull/15039.patch | git am And then you can run "make production" from it.
Thank you, Alexandre and Julien! I will arrange test next week.
Flags: needinfo?(echu)
Okay. Thanks. I'm adding bug 855165 as a blocker so that we wait also for this one to land and introduce a shared mock notification that we will use.
Depends on: 855165
(In reply to Alexandre LISSY :gerard-majax from comment #25) > Okay. Thanks. I'm adding bug 855165 as a blocker so that we wait also for > this one to land and introduce a shared mock notification that we will use. Okay, I will wait for bug 855165 before starting evaluation test.
Flags: needinfo?(echu)
Bug 855165 will not affect the functionnality, only tests :)
Comment on attachment 8356068 [details] [review] WIP patch Resetting review since I switched to a new shared mock for notification and added some tests.
Attachment #8356068 - Flags: review+ → review?(gsvelto)
Comment on attachment 8356068 [details] [review] WIP patch r+ with a minor naming nit I've commented in the GitHub comments. I'd be cool to land this now but we still have to fix the problem with the notification handler being invoked twice. I'd suggest to do two things: first of all try to verify if the object with which the notification is called is the same between the two calls. You can check it out by adding a property to it and see if it sticks. If it does that could be used as a workaround (skip the second notification if the message it delivers has the property already set). Second thing let's ask the others if they've seen this problem already or if there's an open bug for it. I have the feeling that others already run into this. If it's not too hard of a problem to identify let's fix that first and then land this right afterwards. Otherwise if it's too complicated then let's go for a simple workaround and then let's open a follow-up.
Attachment #8356068 - Flags: review?(gsvelto) → review+
(In reply to Gabriele Svelto [:gsvelto] from comment #29) > Comment on attachment 8356068 [details] [review] > WIP patch > > r+ with a minor naming nit I've commented in the GitHub comments. I'd be > cool to land this now but we still have to fix the problem with the > notification handler being invoked twice. > > I'd suggest to do two things: first of all try to verify if the object with > which the notification is called is the same between the two calls. You can > check it out by adding a property to it and see if it sticks. If it does > that could be used as a workaround (skip the second notification if the > message it delivers has the property already set). Second thing let's ask > the others if they've seen this problem already or if there's an open bug > for it. I have the feeling that others already run into this. If it's not > too hard of a problem to identify let's fix that first and then land this > right afterwards. Otherwise if it's too complicated then let's go for a > simple workaround and then let's open a follow-up. Nits fixed, I'll check your workaround solution.
(In reply to Alexandre LISSY :gerard-majax from comment #30) > Nits fixed, I'll check your workaround solution. Quick update: turns out that this was a bug in the system app and the fix is ready in bug 956276 and should land soon so I'm adding it as a dependency. You can skip the workaround and land at will once bug 956276 lands, thanks!
Depends on: 956276
Excellent ! Thanks !
I think the issue from bug 956276 is happening whether this patch is applied or not, so there is no need to wait for it. What do you think Gabriele ? :)
I do confirm that bug 956276 fixes this. Gabriele, do we really need to wait bug 956276 for this to land?
Flags: needinfo?(gsvelto)
Let's land this, just put on a note for Enpei that she should not check this until bug 956276 lands (she's done a great job routinely verifying the fixes for WAP Push bugs and I'd hate to waste her time before that dependency is solved).
Flags: needinfo?(gsvelto)
https://github.com/mozilla-b2g/gaia/commit/7517bd042baf2c8e8bf04e8dbc2692263f3c8cb2 Enpei, this is merged. Please wait for bug 956276 to land until testing this :). Thanks !
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: needinfo?(echu)
Resolution: --- → FIXED
(In reply to Alexandre LISSY :gerard-majax from comment #36) > https://github.com/mozilla-b2g/gaia/commit/ > 7517bd042baf2c8e8bf04e8dbc2692263f3c8cb2 > > Enpei, this is merged. Please wait for bug 956276 to land until testing this > :). Thanks ! Hi Alexandre and all, I've completed my test basically running our wap push test cases on moztrap. 2 bugs are found. 1. bug 962974 - this is the major one, unable to remove read wap/cp messages. It's not what's behaved on 1.2 and 1.3 at all. 2. bug 962977 - general problems with SMS and WAP push message that notifications can only be removed by "clear all" button or reboot device.
Flags: needinfo?(echu)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: