1000007 - Separate out loop from the social api, and add a loop specific service and API injection

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mark Banner (:standard8)]

LoopService is currently a hacked up version and we need to replace it by something better, now that we've got the basic architecture.

Currently, we might want to replace by a jsm, if we can maintain our startup hooks to register with the server.

Comment 1

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

If this is implemented as a .jsm, the browser will have more control over when the service is initialized.  We'd probably initialize it during browser-delayed-startup-finished (and I'm not sure if the XPCOM mechanics can use this, or is limited to profile-after-change) - but we might choose a few second after and having this control is a good thing.  I'm guessing we'd probably just have browser-loop add the observer notification and perform the import.

That said though, I don't think that much of the implementation would need to change if we move from a service to a .jsm - just the boilerplate at the top of the service code.  Thus, while I think we should try and get this done for MLP, I don't think it should block the work in getting loop fully functional - ie, I think that migrating this to a .jsm should be a relatively low priority.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Got calls working on profiles which don't already have a cookie by pushing <https://github.com/adamroach/gecko-dev/commit/075924824cf812c8f1065094a5d7c0640b58394e> to the -new branch.  As indicated by the comments, we may be able to do better, though just forcing third-party cookies on this channel is probably something we could live with for MLP.

We should fix the error-handling (eg around DNS among other things, as indicated in the other XXX comment) before landing though.  It took me far too long to realize that my loop-server pref was set incorrectly.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

FWIW, I tried a few different principals, and they weren't good enough, but maybe I just didn't stumble on the right one.  Too bad the ThirdPartyUtil code doesn't have logging.

Comment 4

[Mark Banner (:standard8)]

Attachment: Link to Github pull-request: https://github.com/adamroach/gecko-dev/pull/22

This sets up the initial MozLoopService. It has the locale functions that we'll expose from the API, these are unit tested as well. It seemed to make more sense to separate those out from MozLoopAPI, to keep MozLoopAPI clearer as to what it is actually exposing.

Comment 5

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8418190 [details] [review]
Link to Github pull-request: https://github.com/adamroach/gecko-dev/pull/22

This looks OK, although given we can't land via git, I'd prefer to see these patches rolled up and uploaded as a landable patch rather than a pointer to a pull request.  I'm also not sure what advantage there is in landing this incomplete when we know well that additional changes are needed before it is actually useful.

Comment 6

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(oh, and I did add a couple of comments to the pull request, just incase you weren't notified of them)

Comment 7

[Mark Banner (:standard8)]

Attachment: Separate out loop from the social api, and add a loop specific service and API injection.

This is a roll-up patch containing everything from the work on separating out the social API, apart from that contained in bug 976109, bug 1007190, and a new bug that is to be raised for the webrtcUI.jsm prompts issue. As bug 976109 is security, and 1007190 is a core item that can land separately, it seemed best to separate out those parts.

It is the reduced set of 43ed902..906659c on https://github.com/adamroach/gecko-dev/compare/loop-service...privs-landing, and is against the loop-service branch (I'll probably reduce privs-landing down to just this patch later, but wanted to check with dmose if there's any issues first).

If you're testing it, you currently need to run privs-landing with the loop-client/cookies_alt branch.

It has basic xpcshell tests for the service. We need to test the push notification and main service functionality, but we've got some more work on that coming up, and it may need mochitests, so I'm putting that off for now and we'll cover it in either bug 994151 or I'll raise a new one for tests at the mochitest level (or whatever we decide is best).

There are some marionette unit tests for the conversation/panel, these can be run with ./mach marionette-test browser/components/loop/test/manifest.ini. The shared ones may currently fail - they are being fixed in bug 976109.

There's a bunch of documentation been added, and I've also checked that MOZ_LOOP appears to be working correctly.

Comment 8

[Mark Banner (:standard8)]

Attachment: Separate out loop from the social api, and add a loop specific service and API injection. v2

Updated patch to drop some of the cookie handling code that's being reviewed in bug 976109.

Comment 9

[Mark Banner (:standard8)]

Updating bug to reflect what's really happening here now.

Comment 11

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8419465 [details] [diff] [review]
Separate out loop from the social api, and add a loop specific service and API injection. v2

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

This is looking good.  I wouldn't mind another look, but the main reason I'm not giving r+ here is due to the browser/base/content/loop changes - IMO they should be in whatever patch is initially landing that directory, and presumably reviewed by someone familiar with the loop code itself.

::: browser/base/content/browser-loop.js
@@ +52,5 @@
> +    initialize: function() {
> +      var observer = function observer(sbject, topic, data) {
> +        if (topic == "browser-delayed-startup-finished") {
> +          MozLoopService.initialize();
> +          Services.obs.removeObserver(observer, "browser-delayed-startup-finished");

might be worth swapping these 2 lines for the unlikely case that .initialize() fails.

::: browser/components/loop/MozLoopAPI.jsm
@@ +10,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource:///modules/loop/MozLoopService.jsm");
> +
> +// We steal a few things from Social - XXX - do something better here.
> +XPCOMUtils.defineLazyModuleGetter(this, "handleWindowClose", "resource://gre/modules/MozSocialAPI.jsm");

see that other bug which number I've now forgotten :)  Let's stick to importing MozSocialAPI here but rename handleWindowClose, and just remove the 'XXX' part of the comment.

@@ +31,5 @@
> +     *
> +     * @param {String} prefName The preference name
> +     * @return {String} The preference value
> +     */
> +    getCharPref: {

Is this still used only to get the loop.server pref?  If so, I think we should just expose a getter specific for this (eg, named loopServer)

::: browser/components/loop/MozLoopService.jsm
@@ +68,5 @@
> +   * @param {nsISupports} aContext Not used
> +   * @param {nsresult} aStatusCode Reason for stopping (NS_OK = successful)
> +   */
> +  onStop: function(aContext, aStatusCode) {
> +    Cu.reportError("Loop Push server web socket closed! Code: " + aStatusCode);

This and later Cu.reportError() calls should be removed, or wrapped in a test the status code isn't NS_OK.

@@ +129,5 @@
> + * Internal helper methods and state
> + */
> +let MozLoopServiceInternal = {
> +  // The uri of the Loop server.
> +  loopServerUri: Services.prefs.getCharPref("loop.server"),

where does this pref get set?  I expected to see it *added* in firefox.js, but it's not listed there as an addition.

@@ +150,5 @@
> +
> +    this.initialized = true;
> +
> +    // Kick off the push notification service into registering after a timeout
> +    // so that we're not doing everything straight away at startup

is this still relevant given we are now initializing on the delayed-startup observer?

@@ +174,5 @@
> +  onPushRegistered: function(pushUrl) {
> +    this.registerXhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +      .createInstance(Ci.nsIXMLHttpRequest);
> +
> +    // XXX Sync!

it appears to now be async - if so, this comment is stale.

@@ +248,5 @@
> +      map[key][property] = string.value;
> +    }
> +
> +    delete this.localizedStrings;
> +    return this.localizedStrings = map;

did you verify this doesn't cause a warning to be logged in the console now "use strict" is specified?

::: browser/components/loop/content/conversation.html
@@ +24,5 @@
>      <script type="text/javascript" src="shared/js/client.js"></script>
>      <script type="text/javascript" src="shared/js/models.js"></script>
>      <script type="text/javascript" src="shared/js/router.js"></script>
>      <script type="text/javascript" src="shared/js/views.js"></script>
> +    <script type="text/javascript" src="js/desktopRouter.js"></script>

what bug is this loop code landing in, and why aren't these changes in that?

Comment 12

[Mark Banner (:standard8)]

(In reply to Mark Hammond [:markh] from comment #11)
> @@ +31,5 @@
> > +    getCharPref: {
> 
> Is this still used only to get the loop.server pref?  If so, I think we
> should just expose a getter specific for this (eg, named loopServer)

I think you're right it is only for that pref currently. Lets expose it as loopServer and we can always add an extra api for something else if necessary.

> ::: browser/components/loop/MozLoopService.jsm
> @@ +68,5 @@
> > +  onStop: function(aContext, aStatusCode) {
> > +    Cu.reportError("Loop Push server web socket closed! Code: " + aStatusCode);
> 
> This and later Cu.reportError() calls should be removed, or wrapped in a
> test the status code isn't NS_OK.

The reason I'd left these as Cu.reportError was that I was going to handle unexpected closes of sockets in a follow-up bug. In the meantime, if it happened to a developer, I wanted it reasonably obvious what was going on.

> @@ +129,5 @@
> > + * Internal helper methods and state
> > + */
> > +let MozLoopServiceInternal = {
> > +  // The uri of the Loop server.
> > +  loopServerUri: Services.prefs.getCharPref("loop.server"),
> 
> where does this pref get set?  I expected to see it *added* in firefox.js,
> but it's not listed there as an addition.

Its buried in another patch somewhere, but it makes sense to move it here.

> @@ +150,5 @@
> > +
> > +    this.initialized = true;
> > +
> > +    // Kick off the push notification service into registering after a timeout
> > +    // so that we're not doing everything straight away at startup
> 
> is this still relevant given we are now initializing on the delayed-startup
> observer?

Yes, I think we should delay the registration even further as delayed-startup is as soon as the window is opened. I think we can delay by up to a further 5 seconds or so, which I was planning to do in bug 994151. In the meantime, having the short delay already written is super-useful for debugging as we can easily tweak a number to get a delayed startup for registration and have time to hook in the debugger.

I can probably make that comment a bit clearer though.

> ::: browser/components/loop/content/conversation.html
> @@ +24,5 @@
> >      <script type="text/javascript" src="shared/js/client.js"></script>
> >      <script type="text/javascript" src="shared/js/models.js"></script>
> >      <script type="text/javascript" src="shared/js/router.js"></script>
> >      <script type="text/javascript" src="shared/js/views.js"></script>
> > +    <script type="text/javascript" src="js/desktopRouter.js"></script>
> 
> what bug is this loop code landing in, and why aren't these changes in that?

Yep, will move this out.

Comment 13

[Mark Banner (:standard8)]

Attachment: Separate out loop from the social api, and add a loop specific service and API injection. v3

Updated patch for Mark's comments. Separated out the browser/components/loop/content (and relevant tests parts) to a different bug that I'll post in a mo.

Comment 15

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8420025 [details] [diff] [review]
Separate out loop from the social api, and add a loop specific service and API injection. v3

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

Awesome, thanks!  (But be sure to not land this before loop itself!)

Comment 16

[Mark Banner (:standard8)]

In lieu of being able to land on the main repos, landed on our gecko-dev git branch for now:

https://github.com/adamroach/gecko-dev/commit/786db178e68943c4679b4133934734ddb6dc7562

Comment 17

[Mark Banner (:standard8)]

Marking as closed for tracking purposes, this has landed on our current integration branch, and I'll update the bug with the mercurial url when it lands elsewhere.

Comment 18

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/0e71a983d95b

Comment 19

[Mark Banner (:standard8)]

https://hg.mozilla.org/mozilla-central/rev/0e71a983d95b

Comment 20

[u279076]

Marking [qa-], please need-info me to request QA attention.