If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Liberate useful sync modules

RESOLVED FIXED in mozilla14


Cloud Services
Firefox Sync: Backend
6 years ago
6 years ago


(Reporter: gps, Assigned: gps)


Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [qa-])


(1 attachment, 3 obsolete attachments)

I think it's time to liberate some generic JS modules from services/sync/modules to outside of Sync, as they are applicable to other components. For example, the token server client API, while initially being written for Sync, will eventually have use for other components, like notifications.

For now, the main types that need liberated are RESTRequest and RESTResponse from rest.js. And, the token server client from bug 727210 should also make the journey. Unfortunately, we may have to expand scope to include dependencies of these types, such as log4moz and some of the Utils helpers.

philikon suggested services/common as a good first step. We make things loosely coupled but still have them live inside the services/ tree, so they are owned by us. Alternatively, we could put them in toolkit (likely mozapps/shared), but then they'd be under ownership of the Toolkit people. That's not necessarily a bad things. But, I'm not sure we're ready to make that jump yet. Once we have more consumers of these modules and prove they are multi-consumer friendly, then would be a good time.
I agree with the goal, and I agree with services/common as a staging area.

Lifting tests is probably going to be the biggest PITA; that might entail lifting our HTTP server helpers first.
Created attachment 609829 [details] [diff] [review]
Work in progress

Just submitting a patch so rnewman can assess the scope of this. Things are still horribly broken at this stage.
Assignee: nobody → gps
I'd like to share test code between services modules. Unfortunately, the xpcshell test runner sorts the head and tail list by name (even though it is ordered already in its definition). Hopefully we can change that. If not, things will be painful.
Depends on: 739753
Created attachment 609888 [details] [diff] [review]
Create services-common module, v1

First draft. Will probably be pulling more modules out in the near future.

I've liberated:

* Log4Moz
* RESTRequest
* Utils.{nextTick, namedTimer, exceptionStr, stackTrace, makeURI}
* Preferences
Attachment #609829 - Attachment is obsolete: true
Attachment #609888 - Flags: feedback?(rnewman)
Created attachment 609913 [details] [diff] [review]
Create services-common module, v2

+ StringBundle
+ Observers
+ Async
Attachment #609888 - Attachment is obsolete: true
Attachment #609913 - Flags: feedback?(rnewman)
Attachment #609888 - Flags: feedback?(rnewman)

Comment 6

6 years ago
Try run for 46f82defbce4 is complete.
Detailed breakdown of the results available here:
Results (out of 74 total builds):
    success: 55
    warnings: 16
    failure: 3
Builds (or logs if builds failed) available at:
Comment on attachment 609913 [details] [diff] [review]
Create services-common module, v2

Review of attachment 609913 [details] [diff] [review]:


I'd be pretty happy for this to land, and have a follow-up or a part two to go through the seven exported components and tidy them up.

::: services/sync/modules/log4moz.js
@@ +368,5 @@
>  }
>  BasicFormatter.prototype = {
>    __proto__: Formatter.prototype,
>    format: function BF_format(message) {

There's a lot of code in log4moz that I'd like to see fixed before it becomes a "published" module -- like these function names, code style, and an audit for efficiency and such.

::: services/common/tests/unit/head_helpers.js
@@ +37,5 @@
> + *        Any number of arguments to print out
> + * @usage _("Hello World") -> prints "Hello World"
> + * @usage _(1, 2, 3) -> prints "1 2 3"
> + */
> +let _ = function(some, debug, text, to) print(Array.slice(arguments).join(" "));

And again, in a part 2 of this we'd want to fix the code style for this kind of thing.

::: services/sync/tests/unit/test_Preferences.js
@@ +3,2 @@
> +Cu.import("resource://services-common/preferences.js");

... so we can rename this test_preferences.js.

You might have already done this, but Splinter seems to handle file renames oddly.

::: services/common/utils.js
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +const EXPORTED_SYMBOLS = ["CommonUtils"];

Thank you so much for renaming this to "utils.js" not "util.js".

@@ +16,5 @@
> +  },
> +
> +  stackTrace: function stackTrace(e) {
> +    // Wrapped nsIException
> +    if (e.location){

Space between paren and bracket. (And so on; just another file that would probably benefit from a scan through.)
Attachment #609913 - Flags: feedback?(rnewman) → feedback+
Created attachment 612626 [details] [diff] [review]
Create services-common module, v3

Since v2:

* Registered services-comm in manifest (manifest management probably needs a follow-up bug since it lives in services/sync)
* Created services-common.js file with default JS prefs for logging.
* Updated rest.js to reference proper prefs for default log levels.
* Syntax nit from previous review.

I've verified xpcshell and TPS tests pass.
Attachment #609913 - Attachment is obsolete: true
Attachment #612626 - Flags: review?(rnewman)
Comment on attachment 612626 [details] [diff] [review]
Create services-common module, v3

This is why I always do a thorough review when asked f? :)
Attachment #612626 - Flags: review?(rnewman) → review+
Whiteboard: [fixed in services]
Blocks: 727210


6 years ago
Whiteboard: [fixed in services] → [fixed in services][qa-]
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services][qa-] → [qa-]
Target Milestone: --- → mozilla14
Depends on: 745209
Blocks: 744702
You need to log in before you can comment on or make changes to this bug.