1214366 - Make PushRecord work with MOZ_ANDROID_HISTORY provider

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Nick Alexander :nalexander [he/him]]

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.

Comment 1

[Nick Alexander :nalexander [he/him]]

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.

Comment 2

[Nick Alexander :nalexander [he/him]]

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?

Comment 3

[Dragana Damjanovic [:dragana]]

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

Comment 4

[Dragana Damjanovic [:dragana]]

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

Comment 5

[Lina Butler [:lina]]

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.

Comment 6

[Martin Thomson [:mt:]]

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.

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1214366 - Part 1: Don't preprocess PushServiceWebSocket.jsm. r?dragana

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.

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1214366 - Part 2: Add getLastVisited equivalent to Fennec. r?rnewman

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

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1214366 - Part 3: Use getLastVisited equivalent in PushService.jsm. r?dragana,rnewman

Bug 1214366 - Part 3: Use getLastVisited equivalent in PushService.jsm. r?dragana,rnewman

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1214366 - Part 4: Add mochitest-chrome test. r?rnewman

Bug 1214366 - Part 4: Add mochitest-chrome test. r?rnewman

Comment 11

[Nick Alexander :nalexander [he/him]]

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.

Comment 12

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=305cb042e87b

Comment 13

[Richard Newman [:rnewman]]

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;

Comment 14

[Richard Newman [:rnewman]]

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

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 8678237 [details]
MozReview Request: Bug 1214366 - Part 4: Add mochitest-chrome test. r?rnewman

https://reviewboard.mozilla.org/r/23141/#review20623

Comment 16

[Richard Newman [:rnewman]]

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

[Lina Butler [:lina]]

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

Comment 18

[Lina Butler [:lina]]

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

Comment 19

[Nick Alexander :nalexander [he/him]]

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 20

[Dragana Damjanovic [:dragana]]

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

Comment 21

[Dragana Damjanovic [:dragana]]

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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/07bf8588dda5
https://hg.mozilla.org/mozilla-central/rev/662e4aff03d4
https://hg.mozilla.org/mozilla-central/rev/243bc49d112b
https://hg.mozilla.org/mozilla-central/rev/8040892f41a8