Add an XPCOM component to use the Push API from chrome code

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: lina, Assigned: lina)

Tracking

unspecified
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(4 attachments, 22 obsolete attachments)

7.07 KB, patch
lina
: review+
Details | Diff | Splinter Review
30.84 KB, patch
lina
: review+
Details | Diff | Splinter Review
16.35 KB, patch
lina
: review+
Details | Diff | Splinter Review
78.92 KB, patch
lina
: review+
Details | Diff | Splinter Review
Exposing the API as a component will allow Loop, Sync, and FxA to use push notifications from chrome code, without migrating to service workers.

:dougt suggested using an XPCOM service to handle subscriptions, and observer notifications to broadcast incoming push messages.
(Assignee)

Comment 3

4 years ago
I used the mock WebSocket from bug 899742 for speed, and so the error conditions could be tested. This can be replaced with a real server to match the mochitests, though.
Attachment #8587656 - Flags: feedback?(dougt)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1129256
Blocks: 1153499
(Assignee)

Comment 5

4 years ago
Attachment #8587651 - Attachment is obsolete: true
Attachment #8587651 - Flags: review?(dougt)
Attachment #8591404 - Flags: review?(dougt)
(Assignee)

Comment 6

4 years ago
Attachment #8587656 - Attachment is obsolete: true
Attachment #8587656 - Flags: feedback?(dougt)
Attachment #8591405 - Flags: review?(dougt)
(Assignee)

Updated

4 years ago
Attachment #8591405 - Flags: review?(dougt) → feedback?(dougt)
Comment on attachment 8587648 [details] [diff] [review]
Bug 1150683 - Add XPCOM interfaces for push notifications

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

lgtm

::: dom/interfaces/push/nsIPushObserverNotification.idl
@@ +12,5 @@
> +[scriptable, uuid(66a87970-6dc9-46e0-ac61-adb4a13791de)]
> +interface nsIPushObserverNotification : nsISupports
> +{
> +  /* The URL that receives push messages from an application server. */
> +  attribute string pushEndpoint;

In the spec, we also have subscriptionID.  Not sure which way the spec will go, but I implemented subscriptionID for the stuff exposed to the web.

@@ +18,5 @@
> +  /**
> +   * The notification version sent by the application server. This is a
> +   * monotonically increasing number.
> +   */
> +  attribute long long version;

version is going away, right?
Attachment #8587648 - Flags: review?(dougt) → review+
I tested these patches, and it looks like they broke my mochitests.

Apply the patch from 1153937, rebuild, then:

./mach mochitest --subsuite push dom/push
Comment on attachment 8591405 [details] [diff] [review]
Bug 1150683 - Add xpcshell tests for nsIPushNotificationService

This patch should just be testing.  Pull out the changes to PushService.jsm,
Attachment #8591405 - Flags: feedback?(dougt) → feedback-
Comment on attachment 8591404 [details] [diff] [review]
Bug 1150683 - Implement nsIPushNotificationService

clearing until we understand what the test failure is about.
Attachment #8591404 - Flags: review?(dougt)
(Assignee)

Comment 11

4 years ago
This fixes the mochitest failures.
Attachment #8591404 - Attachment is obsolete: true
Attachment #8591875 - Flags: review?(dougt)
(Assignee)

Comment 12

4 years ago
Attachment #8591875 - Attachment is obsolete: true
Attachment #8591875 - Flags: review?(dougt)
Attachment #8591876 - Flags: review?(dougt)
(Assignee)

Comment 13

4 years ago
Attachment #8591405 - Attachment is obsolete: true
Attachment #8591877 - Flags: feedback?(dougt)
(Assignee)

Comment 15

4 years ago
Fixing up commit message and carrying over r+ from comment #7.
Attachment #8587648 - Attachment is obsolete: true
Attachment #8591884 - Flags: review+
(Assignee)

Comment 16

4 years ago
(In reply to Doug Turner (:dougt) from comment #7)
> In the spec, we also have subscriptionID.  Not sure which way the spec will
> go, but I implemented subscriptionID for the stuff exposed to the web.

Cool. I see you're tracking that in bug 1149271. Would you prefer a `subscriptionId` placeholder for XPCOM, too, or wait until we know what it does?

> version is going away, right?

Yup, though I think Loop relies on it now. Data should remove the need for version eventually.
i think you can leave it as is for now.
Attachment #8591876 - Flags: review?(dougt) → review+
Attachment #8591877 - Flags: feedback?(dougt) → feedback+
Attachment #8591878 - Flags: feedback?(dougt) → feedback+
(Assignee)

Comment 18

4 years ago
Fix Android bustage.
Attachment #8591884 - Attachment is obsolete: true
Attachment #8591989 - Flags: review?(dougt)
Attachment #8591989 - Flags: review?(dougt) → review+
And backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/aa01c3fd458c for failing Android xpcshell like it did on try.
(Assignee)

Comment 22

4 years ago
Updated against `mozilla-inbound`, removed `dom.push.enabled` pref. Carrying over r+ from previous patch, and bug 1153937 for the pref removal.
Attachment #8591876 - Attachment is obsolete: true
Attachment #8592990 - Flags: review+
(Assignee)

Comment 23

4 years ago
Updated against `mozilla-inbound`.
Attachment #8591877 - Attachment is obsolete: true
Attachment #8592991 - Flags: review+
(Assignee)

Comment 24

4 years ago
Updated against `mozilla-inbound`.
Attachment #8591878 - Attachment is obsolete: true
Attachment #8592992 - Flags: review+
(Assignee)

Comment 25

4 years ago
* Disable the wake lock on non-B2G platforms. Works around bug 1154492.
* Include alarms and push in the Android install manifest.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5439476e21e1
Attachment #8592998 - Flags: review?(dougt)
Attachment #8592998 - Flags: review?(dougt) → review+
Duplicate of this bug: 1153937
rebase + a small change to when we startup everything.

See https://treeherder.mozilla.org/#/jobs?repo=try&revision=37b06eded7b5
(Assignee)

Comment 28

4 years ago
Attachment #8591989 - Attachment is obsolete: true
Attachment #8594011 - Flags: review+
(Assignee)

Comment 29

4 years ago
Rebased and switched to `sessionstore-windows-restored`.
Attachment #8592990 - Attachment is obsolete: true
Attachment #8594012 - Flags: review+
(Assignee)

Comment 30

4 years ago
Attachment #8592991 - Attachment is obsolete: true
Attachment #8594013 - Flags: review+
(Assignee)

Comment 31

4 years ago
Attachment #8592992 - Attachment is obsolete: true
Attachment #8594015 - Flags: review+
(Assignee)

Comment 32

4 years ago
Attachment #8592998 - Attachment is obsolete: true
Attachment #8594016 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 33

4 years ago
Attachment #8594011 - Attachment is obsolete: true
Attachment #8594221 - Flags: review+
(Assignee)

Comment 34

4 years ago
Attachment #8594221 - Attachment is obsolete: true
Attachment #8594222 - Flags: review+
(Assignee)

Comment 38

4 years ago
Disabled `nsIPushNotificationService` and the tests on Android. No more IndexedDB errors: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a76e7402e610
Attachment #8594015 - Attachment is obsolete: true
Attachment #8594016 - Attachment is obsolete: true
Attachment #8594235 - Flags: review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0e980ff1e1 for both the same indexedDB crashes in talos @mozilla::dom::indexedDB::::ConnectionPool::Start that it had on try and also possibly more informative talos assertions and crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=9031652&repo=mozilla-inbound
And despite matching up with the existing intermittent bug 1146705, I think the Windows Mn crashes are yours too.
The indexeddb crash is also being looked at in bug 1156063.

Updated

4 years ago
Depends on: 1156063
Blocks: 1156052
Blocks: 1156702
No longer blocks: 1156702
Depends on: 1157649
Blocks: 1131813
You need to log in before you can comment on or make changes to this bug.