981401 - [Messages] Notification does not vanish when the SMS is read when going back from sleep

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

STR:
* be on the thread for number A
* press the "power" button to put the phone in sleep mode
* receive a message from number A
* wake up the phone
* unlock the device

=> the notification should be removed

This is a regression from the transition to the new Notification API (bug 855165).

Comment 1

[Jason Smith [:jsmith]]

Might be related to the underlying gecko problem in bug 980132.

Comment 2

[Julien Wajsberg [:julienw]]

Nope, this is not the same code path. We're "just" not closing them in that case :(

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.4+ regression

Comment 4

[Kai-Chih Hu [:khu]]

The notification is handled by the system app?

Comment 5

[David Scravaglieri [:scravag]]

Steve, could you take a look at this please ?

Comment 6

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Francisco, this patch force to close notification after popup when in the same thread. Might need you help to verify if the solution is safe, thanks.

Comment 7

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8393580 [details] [review]
Link to github

Just left a tiny comment on github, also relaunched the marionette test they timed out.

Please land once travis is green.

Thanks Steve!

Comment 8

[sam.hua]

add Siiaceon

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

does it really impact 1.3T?

Comment 10

[Steve Chung [:steveck]]

I think the regression(bug 855165) is not uplift to 1.3.

Comment 11

[Steve Chung [:steveck]]

Comment on attachment 8393580 [details] [review]
Link to github

Hi Francisco, I reset the flag because I consider the visibility and add more related changes for unit test. Thanks for the comments!

Comment 12

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8393580 [details] [review]
Link to github

\o/ amazing job, and I learned new stuff :)

Thanks Steve!

Comment 13

[Francisco Jordano [:arcturus] [:francisco]]

I forgot, merge once travis green, just saw a ui test failing (not related to your PR)

Comment 14

[Steve Chung [:steveck]]

Landed in master: 26a5e2693df45e095bc8f7b5cc04d19b2c64c9d7

I'll left status unchanged until QA confirms it isn't reproducible on v1.3

Comment 15

[Steve Chung [:steveck]]

I've verified it's not reproducible on v1.3, so close it as resolved first.

Comment 16

[James Zhang (Spreadtrum)]

(In reply to Joe Cheng [:jcheng] from comment #9)
> does it really impact 1.3T?

1.3T has this issue.

Comment 17

[Jason Smith [:jsmith]]

(In reply to James Zhang from comment #16)
> (In reply to Joe Cheng [:jcheng] from comment #9)
> > does it really impact 1.3T?
> 
> 1.3T has this issue.

1.3T doesn't have the new notifications API work, so this patch shouldn't be needed on that branch.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

v1.4: 32ddd4e961d9e55749fc3c99e772f41eade8d7dc

Comment 19

[Julien Wajsberg [:julienw]]

(In reply to James Zhang from comment #16)
> (In reply to Joe Cheng [:jcheng] from comment #9)
> > does it really impact 1.3T?
> 
> 1.3T has this issue.

Please file another bug if you have this issue on 1.3T. Maybe we have another 1.3-specific issue.