754062 - Add services/notifications to services-central

Status: RESOLVED FIXED

(Cloud Services Graveyard :: Server: WebPush, defect)

Description

[Jeff Balogh (:jbalogh)]

Attachment: [PATCH] create services/notifications

This patch starts up services/notifications. I don't know what I'm doing inside Firefox, so the structure and style are probably not great. Some parts I copy/pasted. Please be harsh.

I think the NotificationSvc singleton should eventually become something like the app-startup WeaveService.

Here's the server API: http://push.rtfd.org#client-http-api

From the README:

Here lies most of the moving parts for push notifcations in the browser. DOM
and UI bindings will live elsewhere; these files deal with talking to the API,
storing messages, and creating persistent connections to the notification
server.

Structure:

services.js::Service
This is a singleton that manages API calls and message storage. It's an
instance of the NotificationSvc class. Messages and state are persisted to a
JSON file on disk.

Comment 1

[Gregory Szorc [:gps]]

Comment on attachment 622930 [details] [diff] [review]
[PATCH] create services/notifications

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

Looking pretty good! The main piece lacking is the manifest registration magic. It's confusing, especially for the uninitiated. I can help you piece this together.

Also, cancelling review from mconnor. I can handle reviews for you (unless mconnor really wants to review this).

::: services/notifications/Makefile.in
@@ +18,5 @@
> +source_modules = $(foreach module,$(modules),$(srcdir)/$(module))
> +module_dir = $(FINAL_TARGET)/modules/services-notifications
> +
> +libs::
> +	$(NSINSTALL) -D $(module_dir)

The new convention (as of about 2 weeks ago) is:

GENERATED_DIRS += $(module_dir)

libs::
    $(NSINSTALL) $(source_modules) $(module_dir)

::: services/notifications/service.js
@@ +1,1 @@
> +const EXPORTED_SYMBOLS = ["Service"];

MPL header.

::: services/notifications/tests/unit/head_helpers.js
@@ +1,1 @@
> +const {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;

I don't believe you need this. I could be wrong.

::: services/notifications/tests/unit/xpcshell.ini
@@ +1,2 @@
> +[DEFAULT]
> +head = head_helpers.js

You probably want to pull in head_global.js from services/common. See sync's xpcshell.ini for an example. Yes, it is ugly. What that file does is set up some magical XUL registration voodoo. More advanced xpcshell tests won't work without it.

::: services/sync/SyncComponents.manifest
@@ +21,4 @@
>  resource services-sync resource:///modules/services-sync/
>  resource services-common resource:///modules/services-common/
>  resource services-crypto resource:///modules/services-crypto/
> +resource services-notifications resource:///modules/services-notifications/

You'll want to write your own .manifest file. I can help you with this.

Comment 2

[Gregory Szorc [:gps]]

You may also want to create a new Bugzilla component for notifications. See bug 743333 for inspiration.

Comment 3

[Jeff Balogh (:jbalogh)]

Attachment: [PATCH] create services/notifications v2

Now with my own manifest, GENERATED_DIRS, and MPL header.

> ::: services/notifications/tests/unit/head_helpers.js
> @@ +1,1 @@
> > +const {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
> 
> I don't believe you need this. I could be wrong.

I need it until I pull in do_load_httpd_js() in the next patch.
 
> ::: services/notifications/tests/unit/xpcshell.ini
> @@ +1,2 @@
> > +[DEFAULT]
> > +head = head_helpers.js
> 
> You probably want to pull in head_global.js from services/common. See sync's
> xpcshell.ini for an example. Yes, it is ugly. What that file does is set up
> some magical XUL registration voodoo. More advanced xpcshell tests won't
> work without it.

I don't understand it so so I'm leaving it out for now, until I know why I want it. Trying to keep copy/paste to a minimum.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 622946 [details] [diff] [review]
[PATCH] create services/notifications v2

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

You are still missing a lot of xpcom glue around service registration and loading. The patch in https://bugzilla.mozilla.org/attachment.cgi?id=623924&action=diff is basically what you need to do. Be sure to generate new UUIDs!

If you want to land things as-is just to get some code landed, I'm cool with that. For the record, I'm only acceptable of it because it isn't registered as a service so it can't break the browser or incur a start-up penalty.

I would also like to see a try build of this work before landing. You may want to use https://wiki.mozilla.org/ReleaseEngineering:Autoland.

::: services/notifications/NotificationsComponents.manifest
@@ +1,2 @@
> +# Register resource aliases
> +resource services-notifications resource:///modules/services-notifications/

This file is missing xpcom registration. Not sure if you'll want that some day.

::: services/notifications/tests/unit/test_service_start.js
@@ +2,5 @@
> +function run_test() {
> +  _("When imported, Service.onStartup is called.");
> +  Cu.import("resource://services-notifications/service.js");
> +
> +  do_check_eq(Service.serverURL, "https://notifications.mozilla.org/");

Did you actually test this? services-notifications.js (the prefs file) seems to be empty. So, I'm not sure where this value is coming from.

Comment 5

[Jeff Balogh (:jbalogh)]

(In reply to Gregory Szorc [:gps] from comment #4) 
> Did you actually test this? services-notifications.js (the prefs file) seems
> to be empty. So, I'm not sure where this value is coming from.

It's in there:

+pref("services.notifications.serverURL", "https://notifications.mozilla.org/");

Comment 6

[Gregory Szorc [:gps]]

(In reply to Jeff Balogh (:jbalogh) from comment #5)
> (In reply to Gregory Szorc [:gps] from comment #4) 
> > Did you actually test this? services-notifications.js (the prefs file) seems
> > to be empty. So, I'm not sure where this value is coming from.
> 
> It's in there:
> 
> +pref("services.notifications.serverURL",
> "https://notifications.mozilla.org/");

I blame splinter.

Comment 7

[Jeff Balogh (:jbalogh)]

Attachment: [PATCH] Bug 754062 - Create services/notifications v3

Less bitrot, more fun!

Comment 8

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/c53f474c502b

Comment 9

[Gregory Szorc [:gps]]

This broke s-c because of build system foo.

The patch introduced a new build system variable, MOZ_SERVICES_NOTIFICATIONS, which gated having things loaded. However, the patch missed some plumbing necessary to hook things up.

I have a Try build at http://tbpl.mozilla.org/?tree=Try&rev=fbce2f402d81. It may take a few iterations to get working...

Comment 10

[Gregory Szorc [:gps]]

New Try: http://tbpl.mozilla.org/?tree=Try&rev=0b40306c8106

I'm fairly confident this one will work. It looks like we were missing the preferences file in package-manifest.in.

Comment 11

[Gregory Szorc [:gps]]

Typo in my last push. New Try: http://tbpl.mozilla.org/?tree=Try&rev=d9ea8718680b

Comment 12

[Gregory Szorc [:gps]]

I decided to combine the packaging fixes with AITC changes to avoid bit rot. See bug 765294.

Comment 13

[Gregory Szorc [:gps]]

Backed out because of xpcshell breakage.

https://hg.mozilla.org/services/services-central/rev/591fd1c696f0

Will reland once bug 765294 is reviewed.

Comment 14

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/b4b7fb624242

Comment 15

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/b4b7fb624242