Closed
Bug 731494
Opened 13 years ago
Closed 13 years ago
Liberate useful sync modules
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: gps, Assigned: gps)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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.
Assignee | ||
Comment 2•13 years ago
|
||
Just submitting a patch so rnewman can assess the scope of this. Things are still horribly broken at this stage.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•13 years ago
|
||
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
Assignee | ||
Comment 4•13 years ago
|
||
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)
Assignee | ||
Comment 5•13 years ago
|
||
+ StringBundle
+ Observers
+ Async
Attachment #609888 -
Attachment is obsolete: true
Attachment #609913 -
Flags: feedback?(rnewman)
Attachment #609888 -
Flags: feedback?(rnewman)
Comment 6•13 years ago
|
||
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/gszorc@mozilla.com-46f82defbce4
Comment 7•13 years ago
|
||
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.)
Attachment #609913 -
Flags: feedback?(rnewman) → feedback+
Assignee | ||
Comment 8•13 years ago
|
||
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 9•13 years ago
|
||
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+
Assignee | ||
Comment 10•13 years ago
|
||
Whiteboard: [fixed in services]
Updated•13 years ago
|
Whiteboard: [fixed in services] → [fixed in services][qa-]
Assignee | ||
Comment 11•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services][qa-] → [qa-]
Target Milestone: --- → mozilla14
Comment 12•13 years ago
|
||
Updated•6 years ago
|
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.
Description
•