* Rewrite the LoopService + its unit tests * Get hacked code ready for landing with any appropriate unit tests + get reviewed
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.
User Story: (updated)
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.
backlog: --- → Fx32+
Priority: -- → P1
Whiteboard: [p=3, est:3d, 1.5:p2, ft:webrtc]
Target Milestone: --- → mozilla32
Assignee: nobody → standard8
Status: NEW → ASSIGNED
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.
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.
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.
Attachment #8418190 - Flags: review?(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.
Attachment #8418190 - Flags: review?(mhammond) → feedback+
(oh, and I did add a couple of comments to the pull request, just incase you weren't notified of them)
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.
Attachment #8418190 - Attachment is obsolete: true
Updated patch to drop some of the cookie handling code that's being reviewed in bug 976109.
Updating bug to reflect what's really happening here now.
Summary: Write the LoopService properly and add unit tests → Separate out loop from the social api, and add a loop specific service and API injection
Duplicate of this bug: 1000012
Attachment #8419465 - Flags: review?(mhammond) → feedback+
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.
Duplicate of this bug: 1007190
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!)
Attachment #8420025 - Flags: review?(mhammond) → review+
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
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.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla32 → mozilla33
Marking [qa-], please need-info me to request QA attention.
Whiteboard: [p=3, est:3d, 1.5:p2, ft:webrtc] → [p=3, est:3d, 1.5:p2, ft:webrtc][qa-]
You need to log in before you can comment on or make changes to this bug.