938540 - [WAPPush] Make use of the new Notification API

Status: RESOLVED FIXED

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

Description

[:gerard-majax]

Follow up of bug 929895

Comment 1

[:gerard-majax]

Make use of the new Notification API and remove hack that was introduced in bug 929895

Comment 3

[:gerard-majax]

Attachment: WIP patch

Comment 4

[:gerard-majax]

Travis is green https://travis-ci.org/mozilla-b2g/gaia/builds/16468824

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8356068 [details] [review]
WIP patch

Looks good to me and thanks for expanding the existing test!

Comment 6

[:gerard-majax]

(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 7

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 8

[Julien Wajsberg [:julienw]]

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.

Comment 9

[:gerard-majax]

(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 ...

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

(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 11

[:gerard-majax]

Comment on attachment 8356068 [details] [review]
WIP patch

All comments regarding the test should be addressed now.

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 13

[Julien Wajsberg [:julienw]]

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 !

Comment 14

[:gerard-majax]

(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

Comment 15

[:gerard-majax]

Travis is green: https://travis-ci.org/mozilla-b2g/gaia/builds/16552228

Comment 16

[:gerard-majax]

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.

Comment 17

[Eric Chou [:ericchou] [: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? :)

Comment 18

[:gerard-majax]

Sorry, mispelled when adding the needinfo? :)

Enpei, could you help us and make sure that this is working correctly on a live setup ? Thanks !

Comment 19

[Enpei]

(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.

Comment 20

[:gerard-majax]

(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.

Comment 21

[Enpei]

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?

Comment 22

[:gerard-majax]

(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 :).

Comment 23

[Julien Wajsberg [:julienw]]

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.

Comment 24

[Enpei]

Thank you, Alexandre and Julien! I will arrange test next week.

Comment 25

[:gerard-majax]

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.

Comment 26

[Enpei]

(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.

Comment 27

[:gerard-majax]

Bug 855165 will not affect the functionnality, only tests :)

Comment 28

[:gerard-majax]

Comment on attachment 8356068 [details] [review]
WIP patch

Resetting review since I switched to a new shared mock for notification and added some tests.

Comment 29

[Gabriele Svelto [:gsvelto] (PTO)]

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.

Comment 30

[:gerard-majax]

(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.

Comment 31

[Gabriele Svelto [:gsvelto] (PTO)]

(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!

Comment 32

[:gerard-majax]

Excellent ! Thanks !

Comment 33

[Julien Wajsberg [:julienw]]

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 ? :)

Comment 34

[:gerard-majax]

I do confirm that bug 956276 fixes this. Gabriele, do we really need to wait bug 956276 for this to land?

Comment 35

[Gabriele Svelto [:gsvelto] (PTO)]

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).

Comment 36

[:gerard-majax]

https://github.com/mozilla-b2g/gaia/commit/7517bd042baf2c8e8bf04e8dbc2692263f3c8cb2

Enpei, this is merged. Please wait for bug 956276 to land until testing this :). Thanks !

Comment 37

[Enpei]

(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.