Closed Bug 731494 Opened 9 years ago Closed 9 years ago

Liberate useful sync modules


(Firefox :: Sync, defect)

Not set





(Reporter: gps, Assigned: gps)



(Whiteboard: [qa-])


(1 file, 3 obsolete files)

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.
Attached patch Work in progress (obsolete) — Splinter Review
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
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)
+ StringBundle
+ Observers
+ Async
Attachment #609888 - Attachment is obsolete: true
Attachment #609913 - Flags: feedback?(rnewman)
Attachment #609888 - Flags: feedback?(rnewman)
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 */
> +
> +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+
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+
Blocks: 727210
Whiteboard: [fixed in services] → [fixed in services][qa-]
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services][qa-] → [qa-]
Target Milestone: --- → mozilla14
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.