Closed Bug 949260 Opened 9 years ago Closed 9 years ago

sync id manager should get .identity dynamically


(Cloud Services :: Server: Firefox Accounts, defect)

Not set


(Not tracked)



(Reporter: warner, Assigned: ckarlof)



(Whiteboard: [qa?])


(1 file, 1 obsolete file)

Attached patch service.diff (obsolete) — Splinter Review
ckarlof wrote this patch, saying:

"Refactored the way that the sync id manager is instantiated to do it dynamically depending on whether we are using fx accounts; fixed a couple of tests"
Whiteboard: [qa-]
Attachment #8346264 - Attachment is obsolete: true
This patch is a simple way to enable us to (safely) dynamically set the (single) Sync identity manager currently stored in Status._authManager. Previously, Service.identity would refer to a statically fetched and cached instance of the _authManager at Sync initialization in Status. This patch introduces a Service.identity getter that dynamically returns Status._authManager. There are probably more elegant solutions to this problem, but this is likely one of the lowest impact ones.
Attachment #8346877 - Flags: review?(rnewman)
The reason we want to do this is so we can dynamically set Status._authManager to either Sync Identity module or the new one that supports FxA after Sync has initialized.
Assignee: warner-bugzilla → ckarlof
Whiteboard: [qa-] → [qa?]
Comment on attachment 8346877 [details] [diff] [review]
Update patch to remove whitespace differences

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

You should probably inline identity() in modules/stages/cluster.js.

But this patch doesn't do what it seems to. If the identity changes at runtime, the server URLs and cached keys won't.

You need to *at least* call service._updateCachedURLs, invalidate CollectionKeys, etc. if the return value of .identity will ever change.

At that point you might as well not do this patch, because you're already having to proactively respond to the change. Is there a reason you chose to go down this path instead of re-initing Sync when there's a new identity?
Attachment #8346877 - Flags: review?(rnewman) → review-
I agree this might be a bigger hammer than we need, and we're aware of the issues you raise. We'll just do Service.identity = Status._authManager = <myNewIdentity> when we swap out the Sync Identity module.
Closed: 9 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.