Closed Bug 1214366 Opened 4 years ago Closed 4 years ago

Make PushRecord work with MOZ_ANDROID_HISTORY provider

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Right now, PushRecord includes some code that I believe is Desktop-specific: see

https://dxr.mozilla.org/mozilla-central/source/dom/push/PushRecord.jsm#16

and the implementation of getLastVisit() at

https://dxr.mozilla.org/mozilla-central/source/dom/push/PushRecord.jsm#112

We should remove this entirely, or extract this platform-specific code to a pluggable module, as appropriate.
dragana: kitcambridge: can one of you comment on what is appropriate here?  Context: Places does not exist in Fennec.  We can probably find a way to get the same information, but it would help to know if it can be removed entirely, or we really need this and should find a Fennec-friendly approach.
Flags: needinfo?(kcambridge)
Flags: needinfo?(dd.mozilla)
But wait, there's more!  PushService also exposes AlarmManager alarms to protocols.  AlarmManager isn't available in Fennec.  Can we extract some helper, shared by the protocols which need alarms?
(In reply to Nick Alexander :nalexander from comment #1)
> dragana: kitcambridge: can one of you comment on what is appropriate here? 
> Context: Places does not exist in Fennec.  We can probably find a way to get
> the same information, but it would help to know if it can be removed
> entirely, or we really need this and should find a Fennec-friendly approach.

lastVisit is the last time a user visited certain page. On desktop we have implemented quota: "show only a certain number of messages from a site to the user before he visits the site again" So if user is not visiting certain sites at all do not push to many massages from thete sites.
Flags: needinfo?(dd.mozilla)
(In reply to Nick Alexander :nalexander from comment #2)
> But wait, there's more!  PushService also exposes AlarmManager alarms to
> protocols.  AlarmManager isn't available in Fennec.  Can we extract some
> helper, shared by the protocols which need alarms?

We could move it to WebSocket implementation. It is not really needed by http2 implementation (we can remove it from there easily).
Martin's updated proposal uses a time budget that renders the entire service worker dormant for excessive background activations: https://docs.google.com/document/d/1yYUB4nn9Hu6vPHKp_eN_kXOG47gjteEv8QIf_l18q4w IIUC, that means we'll want to move the quota code into the workers module.

That said, the budget is restored when the user visits the origin, so we'll still need something like `getLastVisit`. Alternatively, if we're generalizing quotas, anyway...maybe we can listen for navigation events, and record the last visit time directly in `serviceworker.txt`. Any thoughts, Martin? Have you discussed your proposal with the platform folks? They'll probably have some ideas about how to make it work.

As for alarms: the only other place I see them used is https://dxr.mozilla.org/mozilla-central/source/dom/requestsync/RequestSyncService.jsm, which is B2G-only. I wonder if alarms are specific to B2G, and if we can get away with using `nsITimer` on Desktop and Fennec.
Flags: needinfo?(martin.thomson)
Flags: needinfo?(kcambridge)
I don't think that we need to make it dormant for fetch.  Just dormant in the sense that it can no longer receive events that were not directly initiated.

I haven't had any design-level discussions.

I tend to think that nsITimer is sufficient.  I haven't looked, but Alarms might persist past restarts.
Flags: needinfo?(martin.thomson)
Bug 1214366 - Part 1: Don't preprocess PushServiceWebSocket.jsm. r?dragana

Just a little build system clean-up.  There's no reason for this to be
special, and it's easier to use in the debugger with correct line
numbers.
Attachment #8678234 - Flags: review?(dd.mozilla)
Bug 1214366 - Part 2: Add getLastVisited equivalent to Fennec. r?rnewman

The approach is the same as that used on Desktop.  The function name
calls out the "prePath" and "milliseconds" since the first is likely
to be overlooked by Fennec consumers, and the second is notably
different than Desktop (where the database timestamps are microseconds).
Attachment #8678235 - Flags: review?(rnewman)
Bug 1214366 - Part 3: Use getLastVisited equivalent in PushService.jsm. r?dragana,rnewman
Attachment #8678236 - Flags: review?(rnewman)
Attachment #8678236 - Flags: review?(dd.mozilla)
Bug 1214366 - Part 4: Add mochitest-chrome test. r?rnewman
Attachment #8678237 - Flags: review?(rnewman)
dragana: rnewman: I think this are of the codebase needs to have the "Push Notifications policy" extracted from the Push Service implementation, but I'm not willing to commit to that work right now.  These patches are strictly to unblock Fennec using the PushService.jsm implementation mostly as it is right now.

More context will be coming shortly in the form of additional patches.

rnewman: there's nothing here that really needs your attention, but you have a handle on the Android history data layer.  Redirect as you see fit.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Summary: Make PushRecord platform agnostic → Make PushRecord work with MOZ_ANDROID_HISTORY provider
Comment on attachment 8678235 [details]
MozReview Request: Bug 1214366 - Part 2: Add getLastVisited equivalent to Fennec. r?rnewman

https://reviewboard.mozilla.org/r/23137/#review20619

::: mobile/android/base/db/LocalBrowserDB.java:749
(Diff revision 1)
> +            cursor.moveToFirst();

if (cursor.moveToFirst()) {
    return cursor.getLong(0);
}
return 0;
Attachment #8678235 - Flags: review?(rnewman) → review+
Comment on attachment 8678236 [details]
MozReview Request: Bug 1214366 - Part 3: Use getLastVisited equivalent in PushService.jsm. r?dragana,rnewman

https://reviewboard.mozilla.org/r/23139/#review20621
Attachment #8678236 - Flags: review?(rnewman) → review+
Attachment #8678237 - Flags: review?(rnewman) → review+
Comment on attachment 8678237 [details]
MozReview Request: Bug 1214366 - Part 4: Add mochitest-chrome test. r?rnewman

https://reviewboard.mozilla.org/r/23141/#review20623
(In reply to Nick Alexander :nalexander from comment #11)
> dragana: rnewman: I think this are of the codebase needs to have the "Push
> Notifications policy" extracted from the Push Service implementation, but
> I'm not willing to commit to that work right now.

Could you file a follow-up?
Comment on attachment 8678234 [details]
MozReview Request: Bug 1214366 - Part 1: Don't preprocess PushServiceWebSocket.jsm. r?dragana

https://reviewboard.mozilla.org/r/23135/#review20765

Looks good! Bonus points if you remove `EXTRA_PP_JS_MODULES` in `moz.build`.
Attachment #8678234 - Flags: review+
Attachment #8678236 - Flags: review+
Comment on attachment 8678236 [details]
MozReview Request: Bug 1214366 - Part 3: Use getLastVisited equivalent in PushService.jsm. r?dragana,rnewman

https://reviewboard.mozilla.org/r/23139/#review20767
https://hg.mozilla.org/integration/fx-team/rev/07bf8588dda5af0b075918f578ad504fbf2a8757
Bug 1214366 - Part 1: Don't preprocess PushServiceWebSocket.jsm. r=kitcambridge

https://hg.mozilla.org/integration/fx-team/rev/662e4aff03d4b102da29c5d500a072346be44bda
Bug 1214366 - Part 2: Add getLastVisited equivalent to Fennec. r=rnewman

https://hg.mozilla.org/integration/fx-team/rev/243bc49d112b253bb081308e97043623cb6bbabd
Bug 1214366 - Part 3: Use getLastVisited equivalent in PushService.jsm. r=kitcambridge,rnewman

https://hg.mozilla.org/integration/fx-team/rev/8040892f41a854e50e0939101c7301925934f2f3
Bug 1214366 - Part 4: Add mochitest-chrome test. r=rnewman
Comment on attachment 8678234 [details]
MozReview Request: Bug 1214366 - Part 1: Don't preprocess PushServiceWebSocket.jsm. r?dragana

https://reviewboard.mozilla.org/r/23135/#review20875

LGTM
Attachment #8678234 - Flags: review?(dd.mozilla) → review+
Attachment #8678236 - Flags: review?(dd.mozilla) → review+
Comment on attachment 8678236 [details]
MozReview Request: Bug 1214366 - Part 3: Use getLastVisited equivalent in PushService.jsm. r?dragana,rnewman

https://reviewboard.mozilla.org/r/23139/#review20883
Blocks: 1219963
You need to log in before you can comment on or make changes to this bug.