Closed Bug 1210211 Opened 4 years ago Closed 4 years ago

Relax quotas for visible notifications

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: lina, Assigned: wchen)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 3 obsolete files)

After the initial push event is received, set a timer for a short period (e.g., 3 seconds; it should be less than the default duration for transient notifications). If there are any notifications visible for that origin when the timer fires, don't reduce the quota.

This addresses the case of receiving a burst of push messages, or where a subsequent message updates an existing one with the same tag. It does mean we'll still dock a worker that receives a "show this notification" push message, followed by a "never mind" (say, because the user responded to the notification on another device).

Martin, did I capture that accurately?
(In reply to Kit Cambridge [:kitcambridge] from comment #0)
> After the initial push event is received...

That should be "processed," once the promise passed to `pushEvent.waitUntil()` resolves.
NI to track this task
Flags: needinfo?(wchen)
Assignee: nobody → wchen
Flags: needinfo?(wchen)
We should probably document what we do here.
Keywords: dev-doc-needed
Attachment #8680999 - Flags: review?(amarchesini)
Attachment #8681000 - Flags: review?(kcambridge)
Comment on attachment 8680999 [details] [diff] [review]
Part 2: Notify Push service of visible notifications.

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

::: dom/notification/Notification.cpp
@@ +1189,5 @@
> +      } else {
> +        rv = pushQuotaManager->NotificationForOriginClosed(origin.get());
> +      }
> +
> +      NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed to call quota manager?");

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

::: dom/push/PushNotificationService.js
@@ +80,5 @@
> +  // nsIPushQuotaManager methods
> +
> +  notificationForOriginShown: function(origin) {
> +    if (!isParent) {
> +      Services.cpmm.sendAsyncMessage("Push:NotificationForOriginShown", origin);

is this handled in other patches?
Attachment #8680999 - Flags: review?(amarchesini) → review+
Comment on attachment 8680998 [details] [diff] [review]
Part 1: Delay updating push quota and check for visible notifications before applying quota penalty.

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

Looks great! Please fix up the conditional in `_notificationForOriginShown`. The other three comments are nits.

::: dom/push/PushService.jsm
@@ +855,5 @@
>            notified = this._notifyApp(record, message);
>          }
> +        // Update quota after 3 seconds, at which point
> +        // we check for visible notifications.
> +        setTimeout(() => this._updateQuota(keyID), 3000);

Nit: Could you move this into a top-level constant or pref, please?

@@ +898,5 @@
> +  _notificationForOriginShown(origin) {
> +    debug("_notificationForOriginShown, origin: " + origin);
> +
> +    var count;
> +    if (!this._visibleNotifications.has(origin)) {

if (this._visibleNotifications.has(origin))

@@ +1108,5 @@
>      }
>  
> +    if (aMessage.name === "Push:NotificationForOriginShown") {
> +      debug("Notification shown from child.");
> +      this._notificationForOriginShown(aMessage.data);

Noting for the future: I think we'll want to use the principal on B2G, or at least include the origin attributes.

@@ +1128,5 @@
>          }
>        }
> +
> +      debug("Clearing notifications from child");
> +      this._visibleNotifications.clear();

I'm not sure about this, but we might need to keep track of the listener on B2G...the architecture is one child process per app, right? We can save that for a follow-up bug, since Push doesn't currently work on B2G, anyway.
Attachment #8680998 - Flags: review?(kcambridge) → review+
Comment on attachment 8681000 [details] [diff] [review]
Part 3: Test for push notification quota with web notifications.

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

::: dom/push/test/xpcshell/test_quota_with_notification.js
@@ +62,5 @@
> +  let updateQuotaPromise = new Promise((resolve, reject) => {
> +    let quotaUpdateCount = 0;
> +    PushService._updateQuotaTestCallback = function() {
> +      quotaUpdateCount++;
> +      if (numMessages == 10) {

if (quotaUpdateCount == 10) {
Attachment #8681000 - Flags: review?(kcambridge) → feedback+
:wchen, once you land this please request uplift to aurora.
In speaking with overholt, wchen is out most of this week.  Kit can you try to land this patch for him.
Flags: needinfo?(kcambridge)
Fixed conditional and rebased. Carrying over r+.
Attachment #8680998 - Attachment is obsolete: true
Flags: needinfo?(kcambridge)
Attachment #8688080 - Flags: review+
Blocks: 1212912
Fwiw, this breaks on OpenBSD:

12:43.58 In file included from /usr/obj/m-c/dom/notification/Unified_cpp_dom_notification0.cpp:11:
12:43.58 /home/othersrc/mozilla-central/dom/notification/Notification.cpp:1196:12: error: use of undeclared identifier 'nsIPushQuotaManager'
12:43.59   nsCOMPtr<nsIPushQuotaManager> pushQuotaManager =
12:43.59            ^
12:43.66 /home/othersrc/mozilla-central/dom/notification/Notification.cpp:1198:8: error: use of undeclared identifier 'pushQuotaManager'
12:43.67   if (!pushQuotaManager) {
12:43.67        ^
12:43.70 /home/othersrc/mozilla-central/dom/notification/Notification.cpp:1209:12: error: use of undeclared identifier 'pushQuotaManager'
12:43.70     return pushQuotaManager->NotificationForOriginShown(origin.get());
12:43.71            ^
12:43.76 /home/othersrc/mozilla-central/dom/notification/Notification.cpp:1211:10: error: use of undeclared identifier 'pushQuotaManager'
12:43.77   return pushQuotaManager->NotificationForOriginClosed(origin.get());

Probably some missing #ifdef/#define.
Or maybe CLOBBER should be touched since this adds an interface.. will retry after a local clobber
Landry, did clobbering fix your local build?
Flags: needinfo?(landry)
Yes - wasnt there a policy to touch CLOBBER when adding interfaces?
Flags: needinfo?(landry)
Thanks! I didn't know about that policy. :-)
Wait, I thought we decided to request uplift?
Flags: needinfo?(edwong)
Flags: needinfo?(edwong)
Comment on attachment 8688080 [details] [diff] [review]
0001-Bug-1210211-Part-1-Delay-updating-push-quota.-r-kitc.patch

Requesting uplift for all the patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: This bug
[User impact if declined]: Push API quota will be more restrictive for common use cases leading to poor experience.
[Describe test coverage new/current, TreeHerder]: One of the patches adds tests that run in treeherder.
[Risks and why]: Possibly introduce bugs in the push service. Low risk, changes to the push service are relatively minor and have test coverage.
[String/UUID change made/needed]: None
Attachment #8688080 - Flags: approval-mozilla-aurora?
Comment on attachment 8688080 [details] [diff] [review]
0001-Bug-1210211-Part-1-Delay-updating-push-quota.-r-kitc.patch

These patches have been in Nightly for ~2 weeks, and push notification is planned for FF44. Let's uplift to Aurora44.
Attachment #8688080 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8688081 [details] [diff] [review]
0002-Bug-1210211-Part-2-Notify-Push-service-of-visible-no.patch

Aurora44+
Attachment #8688081 - Flags: approval-mozilla-aurora+
Comment on attachment 8688083 [details] [diff] [review]
0003-Bug-1210211-Part-3-Test-for-push-notification-quota-.patch

Aurora44+
Attachment #8688083 - Flags: approval-mozilla-aurora+
this has problems with the aurora uplift:

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r 7343fdb5d581
grafting 315311:7343fdb5d581 "Bug 1210211 - Part 1: Delay updating push quota. r=kitcambridge"
merging dom/push/PushRecord.jsm
merging dom/push/PushService.jsm
merging dom/push/test/xpcshell/head.js
merging modules/libpref/init/all.js
warning: conflicts while merging dom/push/PushService.jsm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

could you take a look ?
Flags: needinfo?(wchen)
I added a note to try to added this; see the first "Note: ... " box in the following section:

https://developer.mozilla.org/en-US/docs/Web/API/Push_API#Push_concepts_and_usage

Is the wording ok here?
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #38)
> I added a note to try to added this; see the first "Note: ... " box in the
> following section:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/
> Push_API#Push_concepts_and_usage
> 
> Is the wording ok here?

I asked wchen and he suggested mt would be a better person to verify so I asked him and he suggested the following small change which I made (hope that's ok):

https://developer.mozilla.org/en-US/docs/Web/API/Push_API$compare?locale=en-US&to=976179&from=972961
Flags: needinfo?(cmills)
(In reply to Andrew Overholt [:overholt] from comment #39)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #38)
> > I added a note to try to added this; see the first "Note: ... " box in the
> > following section:
> > 
> > https://developer.mozilla.org/en-US/docs/Web/API/
> > Push_API#Push_concepts_and_usage
> > 
> > Is the wording ok here?
> 
> I asked wchen and he suggested mt would be a better person to verify so I
> asked him and he suggested the following small change which I made (hope
> that's ok):
> 
> https://developer.mozilla.org/en-US/docs/Web/API/Push_API$compare?locale=en-
> US&to=976179&from=972961

Looks fine - cheers Andrew.
Flags: needinfo?(cmills)
You need to log in before you can comment on or make changes to this bug.