Closed Bug 754062 Opened 8 years ago Closed 8 years ago

Add services/notifications to services-central

Categories

(Cloud Services :: WebPush, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla16

People

(Reporter: jbalogh, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

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.
Attachment #622930 - Flags: review?(mconnor)
Attachment #622930 - Flags: feedback?(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.
Attachment #622930 - Flags: review?(mconnor)
Attachment #622930 - Flags: feedback?(gps)
Attachment #622930 - Flags: feedback+
You may also want to create a new Bugzilla component for notifications. See bug 743333 for inspiration.
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.
Attachment #622930 - Attachment is obsolete: true
Attachment #622946 - Flags: review?(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.
Attachment #622946 - Flags: review?(gps) → review+
(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/");
Whiteboard: [autoland-try]
Whiteboard: [autoland-try] → [autoland-in-queue]
(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.
Blocks: 756657
Blocks: 747907
Less bitrot, more fun!
Attachment #622946 - Attachment is obsolete: true
https://hg.mozilla.org/services/services-central/rev/c53f474c502b
Status: NEW → ASSIGNED
Whiteboard: [autoland-in-queue] → [fixed in services]
Target Milestone: --- → mozilla16
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...
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.
I decided to combine the packaging fixes with AITC changes to avoid bit rot. See bug 765294.
Backed out because of xpcshell breakage.

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

Will reland once bug 765294 is reviewed.
Depends on: 765294
Whiteboard: [fixed in services]
Target Milestone: mozilla16 → ---
Assignee: jbalogh → nobody
Component: Firefox: Common → Notifications
QA Contact: firefox-common → notifications
https://hg.mozilla.org/services/services-central/rev/b4b7fb624242
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/b4b7fb624242
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Blocks: 791869
Blocks: 827683
You need to log in before you can comment on or make changes to this bug.