731494 - Liberate useful sync modules

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[Richard Newman [:rnewman]]

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.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Work in progress

Just submitting a patch so rnewman can assess the scope of this. Things are still horribly broken at this stage.

Comment 3

[Gregory Szorc [: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.

Comment 4

[Gregory Szorc [:gps]]

Attachment: 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

Comment 5

[Gregory Szorc [:gps]]

Attachment: Create services-common module, v2

+ StringBundle
+ Observers
+ Async

Comment 6

[Mozilla RelEng Bot]

Try run for 46f82defbce4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=46f82defbce4
Results (out of 74 total builds):
    success: 55
    warnings: 16
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-46f82defbce4

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 609913 [details] [diff] [review]
Create services-common module, v2

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

Beautiful.

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.)

Comment 8

[Gregory Szorc [:gps]]

Attachment: 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.

Comment 9

[Richard Newman [: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? :)

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/579f1d93491c

Comment 11

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/579f1d93491c

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/018cac438918