Closed Bug 1100876 Opened 10 years ago Closed 9 years ago

Do not resend notifications that we should not resend

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S1 (5dec)

People

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

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 3 obsolete files)

Let's leverage the mozbehavior dict and implement this properly. We will be able to cleanup some code (Voicemail for instance).
There are some notifications that we know we don't want to resend. For
instance, Voicemail notifications are some, since everytime the network
will be ready it will notify us of the status, so we don't have to
resend it on reboot. We add a 'resend' key in the MozBehavior dict for
this usecase.
Without the fix:
> 60 INFO ::Notification Tests:: | Trying to resend non-resendable notification
> 61 INFO TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/notification/test_notification_resend.html | One notification not resent - got 1, expected 0

With the fix:
> 60 INFO ::Notification Tests:: | Trying to resend non-resendable notification
> 61 INFO TEST-PASS | /tests/dom/tests/mochitest/notification/test_notification_resend.html | One notification not resent
Attachment #8525327 - Flags: review?(mhenretty)
Whiteboard: [systemsfe]
Target Milestone: --- → 2.2 S1 (5dec)
Comment on attachment 8525327 [details] [diff] [review]
Add noresend to MozBehavior dict r=mhenretty

Review of attachment 8525327 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome! I flashed this on a phone, and updated apps/system/js/voicemail.js to specify noresend for its notifications, and bug 1088224 was indeed fixed!
Attachment #8525327 - Flags: review?(mhenretty) → review+
Keywords: checkin-needed
Blocks: 1103559
need dom peer review:
remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...
Flags: needinfo?(lissyx+mozillians)
Keywords: checkin-needed
Comment on attachment 8525327 [details] [diff] [review]
Add noresend to MozBehavior dict r=mhenretty

Olli, if you can check the webidl part :)
Flags: needinfo?(lissyx+mozillians)
Attachment #8525327 - Flags: review?(bugs)
There are some notifications that we know we don't want to resend. For
instance, Voicemail notifications are some, since everytime the network
will be ready it will notify us of the status, so we don't have to
resend it on reboot. We add a 'resend' key in the MozBehavior dict for
this usecase.
Attachment #8525327 - Attachment is obsolete: true
Attachment #8525327 - Flags: review?(bugs)
Comment on attachment 8528286 [details] [diff] [review]
Add noresend to MozBehavior dict r=mhenretty,smaug

Carrying r+
Attachment #8528286 - Flags: review+
Comment on attachment 8528286 [details] [diff] [review]
Add noresend to MozBehavior dict r=mhenretty,smaug

Asking review for the WebIDL change
Attachment #8528286 - Flags: review?(bugs)
Comment on attachment 8528286 [details] [diff] [review]
Add noresend to MozBehavior dict r=mhenretty,smaug

Curious, do we have a bug open to move vibrationPattern to NotificationOptions
and rename it to vibrate (to follow the spec).


What does noresend mean in this context? Notifications aren't "sent" to anywhere.
(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 8528286 [details] [diff] [review]
> Add noresend to MozBehavior dict r=mhenretty,smaug
> 
> Curious, do we have a bug open to move vibrationPattern to
> NotificationOptions
> and rename it to vibrate (to follow the spec).
> 
> 
> What does noresend mean in this context? Notifications aren't "sent" to
> anywhere.

The noresend here controls the behavior of the mozChromeNotifications.mozResendAllNotifications() behavior. This is used in the Gaia, in the System application, to be able to redisplay stored notifications.
Those sound like implementation details.
What functionality does noresend represent?
Is it like "don't re-show the notification" (though, the "re-" part is not clear to me.)

Or is "notifyOnlyOnce" more correct?

Or is the flag about persistency somehow?
(In reply to Olli Pettay [:smaug] from comment #12)
> Those sound like implementation details.
> What functionality does noresend represent?
> Is it like "don't re-show the notification" (though, the "re-" part is not
> clear to me.)

That's it. What is not clear with the "re-" part?
Comment on attachment 8528286 [details] [diff] [review]
Add noresend to MozBehavior dict r=mhenretty,smaug

so, noresend doesn't really mean anything to me in this context.
Notifications aren't sent anywhere. They are shown.

So, showOnlyOnce ?
Attachment #8528286 - Flags: review?(bugs) → review-
There are some notifications that we know we don't want to resend. For
instance, Voicemail notifications are some, since everytime the network
will be ready it will notify us of the status, so we don't have to
resend it on reboot. We add a 'resend' key in the MozBehavior dict for
this usecase.
There are some notifications that we know we don't want to resend. For
instance, Voicemail notifications are some, since everytime the network
will be ready it will notify us of the status, so we don't have to
resend it on reboot. We add a 'showOnlyOnce' key in the MozBehavior dict
for this usecase.
Attachment #8528286 - Attachment is obsolete: true
Attachment #8528414 - Attachment is obsolete: true
Comment on attachment 8528415 [details] [diff] [review]
Add showOnlyOnce to MozBehavior dict r=mhenretty,smaug

Carrying r+
Attachment #8528415 - Flags: review+
Comment on attachment 8528415 [details] [diff] [review]
Add showOnlyOnce to MozBehavior dict r=mhenretty,smaug

Changed from noresend to showOnlyOnce
Attachment #8528415 - Flags: review?(bugs)
Attachment #8528415 - Flags: review?(bugs) → review+
(In reply to Alexandre LISSY :gerard-majax from comment #19)
> New try: https://tbpl.mozilla.org/?tree=Try&rev=7519c6ed7e4a

Need to check that this one is green instead. https://tbpl.mozilla.org/?tree=Try&rev=3d6d3f4f8c23
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9155e0c35233
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.