Move things out of service.js

RESOLVED FIXED in mozilla17

Status

()

defect
RESOLVED FIXED
7 years ago
11 months ago

People

(Reporter: gps, Assigned: gps)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

It's been a while since I've contributed to the slow death of service.js.

The first patch moves a lot of the engine syncing logic out of service.js into its own file. I've created a new "stages" directory for the new file, anticipating the eventual existence of many stages and files for stages.

I basically lifted code from service.js as-is. Although, I did get a little crafty and I added an onComplete callback which gets called during sync(). Service.js creates a spinning callback and waits on that. It's a start.

I concede that the coupling between service.js and the extracted code is so tight it should be XXX rated. This will will get rewritten with the async rewrite and the refactorings of service.js, so I'm comfortable with leaving it as-is for the time-being.
Attachment #651014 - Flags: review?(rnewman)
Same treatment but for cluster management APIs.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #651016 - Flags: review?(rnewman)
AFAICT this function is unused and can be killed. While I was there, I cleaned up the test file.
Attachment #651017 - Flags: review?(rnewman)
Comment on attachment 651014 [details] [diff] [review]
Part 1: Split out engine syncing, v1

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

::: services/sync/Makefile.in
@@ +79,5 @@
>  SYNC_ENGINES_DEST = $(FINAL_TARGET)/modules/services-sync/engines
>  INSTALL_TARGETS += SYNC_ENGINES
>  
> +SYNC_STAGES_FILES := $(addprefix modules/stages/,$(sync_stage_modules))
> +SYNC_STAGES_DEST = $(FINAL_TARGET)/modules/services-sync/stages

What is this DEST define used for? I see them in our makefiles, but it's not clear to me where they're used.

::: services/sync/modules/stages/enginesync.js
@@ +28,5 @@
> +function EngineSynchronizer(service) {
> +  this._log = Log4Moz.repository.getLogger("Sync.Synchronizer");
> +  this._log.level = Log4Moz.Level[Svc.Prefs.get("log.logger.synchronizer")];
> +
> +  this.service = service;

This *vaguely* disquiets me, but I understand that it's used by several methods. I wish it could be scoped to `sync`.

To think about for the future: this should be a `config` delegate.
Attachment #651014 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #3)
> Comment on attachment 651014 [details] [diff] [review]
> Part 1: Split out engine syncing, v1
> 
> Review of attachment 651014 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/sync/Makefile.in
> @@ +79,5 @@
> >  SYNC_ENGINES_DEST = $(FINAL_TARGET)/modules/services-sync/engines
> >  INSTALL_TARGETS += SYNC_ENGINES
> >  
> > +SYNC_STAGES_FILES := $(addprefix modules/stages/,$(sync_stage_modules))
> > +SYNC_STAGES_DEST = $(FINAL_TARGET)/modules/services-sync/stages
> 
> What is this DEST define used for? I see them in our makefiles, but it's not
> clear to me where they're used.

There is Makefile magic in rules.mk. Basically, FOO_FILES and FOO_DEST have special meaning.

> 
> ::: services/sync/modules/stages/enginesync.js
> @@ +28,5 @@
> > +function EngineSynchronizer(service) {
> > +  this._log = Log4Moz.repository.getLogger("Sync.Synchronizer");
> > +  this._log.level = Log4Moz.Level[Svc.Prefs.get("log.logger.synchronizer")];
> > +
> > +  this.service = service;
> 
> This *vaguely* disquiets me, but I understand that it's used by several
> methods. I wish it could be scoped to `sync`.
> 
> To think about for the future: this should be a `config` delegate.

Yes. I agree regarding the config delegate. This is the approach I have in the rewrite-the-world branch. We'll get there eventually. I view this code refactoring as temporary. It sets the stage for more drastic changes later. And, those changes will hopefully now be contained within the same files. That should take some burden off the code review since you don't need to deal with moving content between files *and* changing semantics at the same time.
(In reply to Gregory Szorc [:gps] from comment #4)

> There is Makefile magic in rules.mk. Basically, FOO_FILES and FOO_DEST have
> special meaning.

Gotcha. Just wanted to make sure this wasn't a partial application, and grepping for DEST in m-c is kinda pointless :D


> Yes. I agree regarding the config delegate. This is the approach I have in
> the rewrite-the-world branch. We'll get there eventually. I view this code
> refactoring as temporary. It sets the stage for more drastic changes later.
> And, those changes will hopefully now be contained within the same files.
> That should take some burden off the code review since you don't need to
> deal with moving content between files *and* changing semantics at the same
> time.

Works for me! :D
Comment on attachment 651016 [details] [diff] [review]
Part 2: Split out cluster logic, v1

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

No worse than we've already got. Minor niggles.

::: services/sync/modules/stages/cluster.js
@@ +20,5 @@
> +  this._log = Log4Moz.repository.getLogger("Sync.Service");
> +  this._log.level = Log4Moz.Level[Svc.Prefs.get("log.logger.service.main")];
> +
> +  this.service = service;
> +  this._identity = service._identity;

Let's not do this directly. Set up an accessor to do transparent passthrough, or this is going to bite sometime.

@@ +23,5 @@
> +  this.service = service;
> +  this._identity = service._identity;
> +}
> +ClusterManager.prototype = {
> +  // gets cluster from central LDAP server and returns it, or null on error

Caps and punctuation, as we're here.

@@ +61,5 @@
> +    }
> +    throw fail;
> +  },
> +
> +  // gets cluster from central LDAP server and sets this.clusterURL

No it doesn't. ;)

(Also caps.)

::: services/sync/modules/stages/enginesync.js
@@ +58,5 @@
>        return;
>      }
>  
>      // If we don't have a node, get one. If that fails, retry in 10 minutes.
> +    if (this.service.clusterURL == "" && !this.service._clusterManager.setCluster()) {

I know this pains you as much as it pains me.
Attachment #651016 - Flags: review?(rnewman) → review+
Attachment #651017 - Flags: review?(rnewman) → review+
Apparently this didn't get closed.

https://hg.mozilla.org/mozilla-central/rev/0dfcb4b9a89b
https://hg.mozilla.org/mozilla-central/rev/172d339df585
https://hg.mozilla.org/mozilla-central/rev/4a0f07b83709
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla17
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.