Closed
Bug 1296767
Opened 8 years ago
Closed 8 years ago
Remove legacy Sync code from Sync
Categories
(Firefox :: Sync, defect, P1)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: lina, Assigned: eoger)
References
Details
(Whiteboard: [sync-quality])
Attachments
(13 files)
59 bytes,
text/x-review-board-request
|
markh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
markh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
markh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
markh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
markh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
markh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
markh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
markh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
markh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
markh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
markh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
markh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
markh
:
review+
|
Details |
IIUC, the entire J-PAKE flow is obsolete in the FxA world, and Sync's jpakeclient appears to be the only consumer of http://searchfox.org/mozilla-central/source/services/crypto/component/nsISyncJPAKE.idl. We can probably ditch that, jpakeclient, and the old IdentityManager for good measure.
Comment 1•8 years ago
|
||
I agree we should do this and I believe tests are where the pain will lie. I'm not sure this should be only P5 though - a case could well be made that we should do this in the interests of general quality.
Priority: P5 → --
Reporter | ||
Comment 2•8 years ago
|
||
Triage meeting: The pain factor will increase until we do this. But, until such time...P3!
Priority: -- → P3
Whiteboard: [sync-quality]
Comment 3•8 years ago
|
||
And <services/sync/services-sync.js> has a bunch of J-PAKE prefs (including references to servers that are no longer live!).
Comment 4•8 years ago
|
||
See also Bug 1312157. This code is a maintenance burden.
See Also: → 1312157
Updated•8 years ago
|
Priority: P3 → P2
Reporter | ||
Updated•8 years ago
|
Priority: P2 → P3
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•8 years ago
|
||
So here's a WIP of what I've been working on. This patch removes J-PAKE and also removes the old identity manager (and the UserAPI too). I updated the unit tests and they all seem to pass. I also found out that the Weave service methods |getStorageInfo()| and |getStorageRequest()| are broken with the FxA identity manager, hence disabled some tests. We should probably go all the way and remove the old pref screens and the migration assistants.
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8827231 [details] Bug 1296767 part 1 - Remove J-PAKE from Sync. https://reviewboard.mozilla.org/r/104978/#review105800 A drive-by, but I think this is worthwhile - thanks! :) ::: services/sync/tests/unit/xpcshell.ini:45 (Diff revision 1) > [test_addon_utils.js] > run-sequentially = Restarts server, can't change pref. > tags = addons > [test_httpd_sync_server.js] > -[test_jpakeclient.js] > # Bug 618233: this test produces random failures on Windows 7. I think these lines need to be deleted too - now they apply to the previous test.
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8827232 [details] Bug 1296767 part 3 - Make BrowserID the only Sync identity manager. https://reviewboard.mozilla.org/r/104980/#review105842 I *love* seeing code die :) ::: services/sync/modules-testing/utils.js:173 (Diff revision 1) > key: "key", > hashed_fxa_uid: "f".repeat(32), // used during telemetry validation > // uid will be set to the username. > } > }, > sync: { I think we can kill "sync" here? ::: services/sync/modules-testing/utils.js:185 (Diff revision 1) > // Now handle any specified overrides. > if (overrides) { > if (overrides.username) { > result.username = overrides.username; > } > if (overrides.sync) { and here. ::: services/sync/modules-testing/utils.js:262 (Diff revision 1) > > if (server) { > ns.Service.serverURL = server.baseURI; > } > > ns.Service._clusterManager = ns.Service.identity.createClusterManager(ns.Service); I suspect this line can die too ::: services/sync/modules-testing/utils.js:283 (Diff revision 1) > - ns.Service.identity.username = config.username; > - ns.Service._updateCachedURLs(); > - setBasicCredentials(config.username, config.sync.password, config.sync.syncKey); > } > > this.SyncTestingInfrastructure = async function(server, username, password) { We should try and kill |password| here too ::: services/sync/modules-testing/utils.js:322 (Diff revision 1) > // * The test itself should be passed as 'test' - ie, test code will generally > // pass |this|. > // * The test function is a regular test function - although note that it must > // be a generator - async operations should yield them, and run_next_test > // mustn't be called. > this.add_identity_test = function(test, testFunction) { TBH I'd be inclined to kill this too and replace existing use with a simple add_task (but probably the removal of this would make sense as its own patch in this bug) (I'm happy to keep it if you disagree though) ::: services/sync/modules/browserid_identity.js:400 (Diff revision 1) > + get username() { > + return Svc.Prefs.get("username", null); > }, > > /** > - * Obtain the Sync Key. > + * Set the username value. I wonder if we can remove .username completely? Or maybe just the setter (ie, have the .account setter roll that code up) ::: services/sync/modules/browserid_identity.js:442 (Diff revision 1) > }, > > /** > - * Resets/Drops the sync key we hold for the current user. > + * Resets/Drops the sync key bundle we hold for the current user. > */ > resetSyncKey() { did you mean to rename this to resetSyncKeyBundle? ::: services/sync/modules/browserid_identity.js:467 (Diff revision 1) > }, > > /** > + * Deletes Sync credentials from the password manager. > + */ > + deleteSyncCredentials: function deleteSyncCredentials() { use new-style function del here. ::: services/sync/modules/service.js:323 (Diff revision 1) > * Prepare to initialize the rest of Weave after waiting a little bit > */ > onStartup: function onStartup() { > this._migratePrefs(); > > // Status is instantiated before us and is the first to grab an instance of I think we can probably kill this check and comment now - we've since refactored such that it should be impossible to hit ::: services/sync/modules/status.js:27 (Diff revision 1) > > get _authManager() { > if (this.__authManager) { > return this.__authManager; > } > let service = Components.classes["@mozilla.org/weave/service;1"] I think we can kill |service| here too
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8827233 [details] Bug 1296767 part 2 - Remove Sync UserApi. https://reviewboard.mozilla.org/r/104982/#review105848 f+
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8827234 [details] Bug 1296767 part 4 - Update tests to work with the BrowserID identity manager. https://reviewboard.mozilla.org/r/104984/#review105850 Thanks for doing this! I only scanned this quickly, but it LGTM! Other things to consider doing in one of these patches: * have Service.login drop all params - IIRC, it is now never called with any args by sync itself, so we could nuke the code that handles them and nuke the params from all tests which call it. * and even more generally, kill anything that references a "password" at all :) * Look into killing FxaMigrator.jsm completely (in this bug or a different bug) and nuke any references to the migrator. * Look at Service.startOver() and see if we can kill the |services.sync-testing.startOverKeepIdentity| pref - that was added simply to tests could arrange for .startOver() to be called, but still be left with the legacy identity when it completes (as it switches to the bid identity by default). Killing the |weave:service:start-over:init-identity| notification will also be possible if the migrator is completely killed. ::: services/sync/tests/unit/test_keys.js:177 (Diff revision 1) > - let identity = new IdentityManager(); > + var identityConfig = makeIdentityConfig(); > + var browseridManager = new BrowserIDManager(); > + configureFxAccountIdentity(browseridManager, identityConfig); > + await browseridManager.initializeWithCurrentIdentity(); > + await browseridManager.whenReadyToAuthenticate.promise; > + await browseridManager.ensureLoggedIn(); IIUC, this line is the only one necessary here - the 2 lines above should be done by ensureLoggedIn. Ideally initWithCurrentID and whenReadyToAuthenticate will eventually die - these patched should make it far easier to refactor bid_identity in the future :) ::: services/sync/tests/unit/test_node_reassignment.js:93 (Diff revision 1) > * setup, detach observers, etc. > */ > async function syncAndExpectNodeReassignment(server, firstNotification, between, > secondNotification, url) { > let deferred = PromiseUtils.defer(); > + let oldGetAssertion = Service.identity._fxaService.internal._getAssertion; So IIUC, these tests are basically just "ensure we've re-fetched a token after a 401", right? If so, I wonder if we could, say, have util.js's mock of getTokenFromBrowserIDAssertion() increment a counter somewhere to indicate how many token fetches have happened, and most of these tests could just check that counter is exactly 1 (or maybe 2, assuming there is an initial token fetch before the 401) I'm totally making that up, but it seems there might be an opportunity to have these checks made without repeating the same mock/undo-mock dance ::: services/sync/tests/unit/test_resource.js:243 (Diff revision 1) > do_check_false(content.success); > > _("GET a password protected resource"); > let res3 = new Resource(server.baseURI + "/protected"); > - let identity = new IdentityManager(); > - let auth = identity.getBasicResourceAuthenticator("guest", "guest"); > + let identityConfig = makeIdentityConfig(); > + let browseridManager = new BrowserIDManager(); I'm surprised we need to make a new BrowserIDManager() now - isn't the existing one OK to reconfigure?
Updated•8 years ago
|
Assignee: nobody → eoger
Priority: P3 → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 26•8 years ago
|
||
I increased the scope of this patch: all the legacy sync code is removed (+migration) and we also take care of bug 1319457 and bug 1328458.
Comment 27•8 years ago
|
||
mozreview-review |
Comment on attachment 8827231 [details] Bug 1296767 part 1 - Remove J-PAKE from Sync. https://reviewboard.mozilla.org/r/104978/#review107316
Attachment #8827231 -
Flags: review?(markh) → review+
Comment 28•8 years ago
|
||
mozreview-review |
Comment on attachment 8827233 [details] Bug 1296767 part 2 - Remove Sync UserApi. https://reviewboard.mozilla.org/r/104982/#review107320 ::: services/sync/modules/service.js:108 (Diff revision 2) > - } > - > - return url + USER_API_VERSION + "/"; > - }, > - > get pwResetURL() { kill this too (including a reference in test_service_attributes.js) ::: services/sync/modules/service.js:812 (Diff revision 2) > - this.identity.basicPassword = newPassword; > - this.persistLogin(); > - return true; > - }, > - > changePassphrase: function changePassphrase(newphrase) { I think this can die too and test ref in test_service_sync_remoteSetup.js (obviously ignore these if they are already removed in a later patch :)
Attachment #8827233 -
Flags: review?(markh) → review+
Comment 29•8 years ago
|
||
mozreview-review |
Comment on attachment 8827232 [details] Bug 1296767 part 3 - Make BrowserID the only Sync identity manager. https://reviewboard.mozilla.org/r/104980/#review107322 This looks good in general, but I think the removal of the .username pref and spinning in getters is taking things a bit too far :) ::: browser/components/preferences/in-content/sync.js:89 (Diff revision 2) > > xps.ensureLoaded(); > }, > > _showLoadPage(xps) { > - let username; > + fxAccounts.getSignedInUser().then(user => { I'm a little worried about this change - how will the page look between the page being loaded and getSignedInUser resolving? I think I mis-led you in my previous comments - IMO it probably makes sense to keep that pref - addons etc might use it and it's a reasonable synchronous approximation of both "is sync enabled" and "what's the user's email address.) That said though, I think it *does* make sense to move away from that pref where we can - eg, the change above in browser-syncui seems fine as it's already promise based, but here and below I think the pref should stay. ::: services/sync/Weave.js:123 (Diff revision 2) > * It does *not* perform a robust check to see if the client is working. > * For that, you'll want to check Weave.Status.checkSetup(). > */ > get enabled() { > - let prefs = Services.prefs.getBranch(SYNC_PREFS_BRANCH); > - return prefs.prefHasUserValue("username"); > + let fxaService = this.fxaServiceMock || fxAccounts; > + return Async.promiseSpinningly(fxaService.getSignedInUser()) != null; ditto here - using promiseSpinningly seems painful as there's no path forward to removing it later (eg, even if sync becomes fully promise-based, there's still no sane way to make this promise based. ::: services/sync/modules/browserid_identity.js (Diff revision 2) > > initialize() { > for (let topic of OBSERVER_TOPICS) { > Services.obs.addObserver(this, topic, false); > } > - // and a background fetch of account data just so we can set this.account, yay to killing test-only hacks :) ::: services/sync/modules/browserid_identity.js:386 (Diff revision 2) > }, > > /** > * Return credentials hosts for this identity only. > */ > _getSyncCredentialsHosts() { there's probably an opportunity to clean some of this up (ie, remove the functions from util.js and have them live just here) ::: services/sync/modules/browserid_identity.js:428 (Diff revision 2) > > return STATUS_OK; > }, > > + // Do we have a configured FxAccount associated with that identity? > + get configured() { similarly here - I'm not too keen on a getter that spins as it will be difficult to remove without further refactoring. ::: services/sync/modules/browserid_identity.js:752 (Diff revision 2) > > /* An implementation of the ClusterManager for this identity > */ > > function BrowserIDClusterManager(service) { > - ClusterManager.call(this, service); > + this._log = Log.repository.getLogger("Sync.Service"); I'd be inclined to just use the existing "log" global here (although it also looks like that global uses a pref |log.logger.identity|, which should maybe change? I'm not too bothered by that though)
Attachment #8827232 -
Flags: review?(markh)
Comment 30•8 years ago
|
||
mozreview-review |
Comment on attachment 8827234 [details] Bug 1296767 part 4 - Update tests to work with the BrowserID identity manager. https://reviewboard.mozilla.org/r/104984/#review107328 ::: services/sync/tests/unit/head_http_server.js:28 (Diff revision 2) > return timestamp; > } > > +function has_hawk_header(req) { > + return req.hasHeader("Authorization") && > + req.getHeader("Authorization").startsWith("Hawk"); this indentation looks odd. ::: services/sync/tests/unit/test_keys.js:172 (Diff revision 2) > > -add_test(function test_collections_manager() { > +add_task(async function test_ensureLoggedIn() { > let log = Log.repository.getLogger("Test"); > Log.repository.rootLogger.addAppender(new Log.DumpAppender()); > > - let identity = new IdentityManager(); > + var identityConfig = makeIdentityConfig(); any reason not to use |let| here? ::: services/sync/tests/unit/test_status_checkSetup.js:22 (Diff revision 2) > do_check_eq(Status.checkSetup(), CLIENT_NOT_CONFIGURED); > do_check_eq(Status.login, LOGIN_FAILED_NO_USERNAME); > Status.resetSync(); > > - _("Let's provide a username."); > - Status._authManager.username = "johndoe"; > + _("Let's provide the syncKeyBundle"); > + Svc.Prefs.set("username", "carotsalad"); I don't understand why this pref needs to be set here (although it *would* make more sense if the previous patch didn't seem to nuke it entirely?) ::: services/sync/tests/unit/test_syncstoragerequest.js:60 (Diff revision 2) > Svc.Prefs.resetBranch(""); > server.stop(run_next_test); > }); > }); > > -add_test(function test_auth() { > +// XXX - DISABLED BECAUSE getStorageRequest broken with browserid_manager Is this removed in a later patch? ::: services/sync/tests/unit/xpcshell.ini:79 (Diff revision 2) > skip-if = os == "mac" || os == "linux" > -[test_service_checkAccount.js] > [test_service_cluster.js] > [test_service_detect_upgrade.js] > -[test_service_getStorageInfo.js] > +# XXX - Disabled because getStorageInfo is broken > +# [test_service_getStorageInfo.js] ditto here - is this removed later?
Attachment #8827234 -
Flags: review?(markh) → review+
Comment 31•8 years ago
|
||
mozreview-review |
Comment on attachment 8828496 [details] Bug 1296767 part 5 - Remove add_identity_test helper. https://reviewboard.mozilla.org/r/105872/#review107330
Attachment #8828496 -
Flags: review?(markh) → review+
Comment 32•8 years ago
|
||
mozreview-review |
Comment on attachment 8828497 [details] Bug 1296767 part 6 - Remove startOverKeepIdentity pref. https://reviewboard.mozilla.org/r/105874/#review107332 ::: services/fxaccounts/tests/xpcshell/head.js:8 (Diff revision 1) > > var {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components; > > "use strict"; > > +Cu.import("resource://gre/modules/Services.jsm"); this change doesn't seem necessary?
Attachment #8828497 -
Flags: review?(markh) → review+
Comment 33•8 years ago
|
||
mozreview-review |
Comment on attachment 8828498 [details] Bug 1296767 part 7 - Remove non-in-content legacy Sync pref screens. https://reviewboard.mozilla.org/r/105876/#review107334 ::: browser/base/content/aboutaccounts/aboutaccounts.js:199 (Diff revision 1) > * @param accountData the user's account data and credentials > */ > onLogin(accountData) { > log("Received: 'login'. Data:" + JSON.stringify(accountData)); > > - if (accountData.customizeSync) { > + // We don't act on customizeSync anymore, it used to open a dialog inside Can you please check if customizeSync is sent here any more? If not, just delete the delete :) and the new comment. The comments and further delete below can probably also be removed now given that content-server issue has been resolved. ::: browser/base/content/sync/utils.js:38 (Diff revision 1) > - if (Weave.Utils.ensureMPUnlocked()) > - this.openChange("UpdatePassphrase"); > - }, > - > resetPassword() { > this._openLink(Weave.Service.pwResetURL); I think we can kill this too (I think I suggested pwResetURL be removed in an earlier patch - removing the impl in this patch would be fine too) ::: browser/base/content/sync/utils.js:41 (Diff revision 1) > - > resetPassword() { > this._openLink(Weave.Service.pwResetURL); > }, > > get tosURL() { are tosURL and privacyPolicyURL still used now that we've removed the `open*` functions? ::: services/fxaccounts/FxAccountsWebChannel.jsm:262 (Diff revision 1) > */ > login(accountData) { > - if (accountData.customizeSync) { > - this.setShowCustomizeSyncPref(true); > + > + // We don't act on customizeSync anymore, it used to open a dialog inside > + // the browser to selecte the engines to sync but we do it on the web now. > - delete accountData.customizeSync; > + delete accountData.customizeSync; similarly here, can you please check if this is even sent, and if not, delete the delete and comment?
Attachment #8828498 -
Flags: review?(markh) → review+
Comment 34•8 years ago
|
||
mozreview-review |
Comment on attachment 8828499 [details] Bug 1296767 part 8 - Remove legacy Sync in-content pref screens. https://reviewboard.mozilla.org/r/105878/#review107336 ::: browser/components/preferences/in-content/sync.js:367 (Diff revision 1) > }, > > openContentInBrowser(url, options) { > let win = Services.wm.getMostRecentWindow("navigator:browser"); > if (!win) { > // no window to use, so use _openLink to create a new one. We don't this comment is now stale (and it seems a shame that if I'm reading openUILinkIn correctly it doesn't support the switchToTabHavingURI capability, but there you have it!)
Attachment #8828499 -
Flags: review?(markh) → review+
Comment 35•8 years ago
|
||
mozreview-review |
Comment on attachment 8828500 [details] Bug 1296767 part 9 - Remove Old Sync link in about:accounts. https://reviewboard.mozilla.org/r/105880/#review107340
Attachment #8828500 -
Flags: review?(markh) → review+
Comment 36•8 years ago
|
||
mozreview-review |
Comment on attachment 8828501 [details] Bug 1296767 part 10 - Remove FxA Migration code. https://reviewboard.mozilla.org/r/105882/#review107342 ::: browser/components/uitour/test/browser.ini:11 (Diff revision 1) > ../UITour-lib.js > > [browser_backgroundTab.js] > [browser_closeTab.js] > [browser_fxa.js] > -skip-if = debug || asan # updateAppMenuItem leaks > +skip-if = debug || asan # updateUI leaks I think we should get a bug on file for looking further at this - if the leaks are real it would be bad! Can you please open a bug to investigate and add the bug number here? ::: browser/components/uitour/test/browser_fxa.js:38 (Diff revision 1) > > taskify(function* test_highlight_accountStatus_loggedIn() { > yield setSignedInUser(); > let userData = yield fxAccounts.getSignedInUser(); > isnot(userData, null, "Logged in now"); > - gFxAccounts.updateAppMenuItem(); // Causes a leak > + gFxAccounts.updateUI(); // Causes a leak (And I guess add the bug# here too)
Attachment #8828501 -
Flags: review?(markh) → review+
Comment 37•8 years ago
|
||
mozreview-review |
Comment on attachment 8828502 [details] Bug 1296767 part 11 - Remove Weave.Service.serverURL and friends. https://reviewboard.mozilla.org/r/105884/#review107344 ::: services/sync/modules/service.js (Diff revision 1) > - if (misc.indexOf(":") == -1) > - misc = this.serverURL + misc; > - return misc + MISC_API_VERSION + "/"; > - }, > - > - get pwResetURL() { ah, here's the removal I mentioned earlier :)
Attachment #8828502 -
Flags: review?(markh) → review+
Comment 38•8 years ago
|
||
mozreview-review |
Comment on attachment 8828503 [details] Bug 1296767 part 12 - Remove Weave.fxAccountsEnabled. https://reviewboard.mozilla.org/r/105886/#review107348 This looks good, but please check TPS passes with this (and the other) changes. There are docs somewhere and tcsc can help if you have problems. Thanks for all these patches, it's very nice to see this killed! We should also consider seeing if we can get softvision to perform some testing for us here to make sure we haven't broken the UI in subtle ways.
Attachment #8828503 -
Flags: review?(markh) → review+
(In reply to Mark Hammond [:markh] from comment #36) > > [browser_fxa.js] > > -skip-if = debug || asan # updateAppMenuItem leaks > > +skip-if = debug || asan # updateUI leaks > > I think we should get a bug on file for looking further at this - if the > leaks are real it would be bad! Can you please open a bug to investigate and > add the bug number here? Ah, I just found that this came from bug 1185579 comment 21 and *I* was asked to open a bug ;) I opened bug 1332985
Assignee | ||
Comment 40•8 years ago
|
||
mozreview-review |
Comment on attachment 8828497 [details] Bug 1296767 part 6 - Remove startOverKeepIdentity pref. https://reviewboard.mozilla.org/r/105874/#review107608 ::: services/fxaccounts/tests/xpcshell/head.js:8 (Diff revision 1) > > var {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components; > > "use strict"; > > +Cu.import("resource://gre/modules/Services.jsm"); It actually is, because in a previous patch I remove utils.js, which pulled Services.jsm for us in these tests.
Assignee | ||
Comment 41•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8828498 [details] Bug 1296767 part 7 - Remove non-in-content legacy Sync pref screens. https://reviewboard.mozilla.org/r/105876/#review107334 > are tosURL and privacyPolicyURL still used now that we've removed the `open*` functions? We actually remove the whole file in the next commits of this patch. > similarly here, can you please check if this is even sent, and if not, delete the delete and comment? It is still sent, but declinedSyncEngines overrides it anyway.
Assignee | ||
Updated•8 years ago
|
Summary: Remove J-PAKE from Sync → Remove legacy Sync code from Sync
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 55•8 years ago
|
||
I made adjustments to my patch, and put back the username getter/setter.
Comment 56•8 years ago
|
||
mozreview-review |
Comment on attachment 8827232 [details] Bug 1296767 part 3 - Make BrowserID the only Sync identity manager. https://reviewboard.mozilla.org/r/104980/#review108078 \o/
Attachment #8827232 -
Flags: review?(markh) → review+
Try looks bad, but I'm sure they will be easy to resolve :)
Comment 58•8 years ago
|
||
mozreview-review |
Comment on attachment 8829958 [details] Bug 1296767 part 13 - Repair Weave Service getStorageInfo. https://reviewboard.mozilla.org/r/106622/#review108080 LGTM and you should land this, but I *think* we can probably kill getStorageInfo() and that part of rest.js too at some point?
Attachment #8829958 -
Flags: review?(markh) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 83•8 years ago
|
||
Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9f59a0b75c1f part 1 - Remove J-PAKE from Sync. r=markh https://hg.mozilla.org/integration/autoland/rev/0a989b0cea67 part 2 - Remove Sync UserApi. r=markh https://hg.mozilla.org/integration/autoland/rev/8219686be6a2 part 3 - Make BrowserID the only Sync identity manager. r=markh https://hg.mozilla.org/integration/autoland/rev/5d70d87d2a8f part 4 - Update tests to work with the BrowserID identity manager. r=markh https://hg.mozilla.org/integration/autoland/rev/46a52b10a868 part 5 - Remove add_identity_test helper. r=markh https://hg.mozilla.org/integration/autoland/rev/6c9b792cc296 part 6 - Remove startOverKeepIdentity pref. r=markh https://hg.mozilla.org/integration/autoland/rev/f6f59447d22a part 7 - Remove non-in-content legacy Sync pref screens. r=markh https://hg.mozilla.org/integration/autoland/rev/4e71cf94e4ee part 8 - Remove legacy Sync in-content pref screens. r=markh https://hg.mozilla.org/integration/autoland/rev/0362a78d6978 part 9 - Remove Old Sync link in about:accounts. r=markh https://hg.mozilla.org/integration/autoland/rev/26c065f79c54 part 10 - Remove FxA Migration code. r=markh https://hg.mozilla.org/integration/autoland/rev/50294db1d871 part 11 - Remove Weave.Service.serverURL and friends. r=markh https://hg.mozilla.org/integration/autoland/rev/1c0c9289b532 part 12 - Remove Weave.fxAccountsEnabled. r=markh https://hg.mozilla.org/integration/autoland/rev/41ed77788333 part 13 - Repair Weave Service getStorageInfo. r=markh
I had to back these out for xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=72297622&repo=autoland https://hg.mozilla.org/integration/autoland/rev/9eae58c7414b9830f2353315554c46bc5a9aa213
Flags: needinfo?(eoger)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 98•8 years ago
|
||
Looks like I got caught up in the middle of bug 1313045 landing. Rebase fixed.
Flags: needinfo?(eoger)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 125•8 years ago
|
||
Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a0022f621594 part 1 - Remove J-PAKE from Sync. r=markh https://hg.mozilla.org/integration/autoland/rev/b2f0066f5949 part 2 - Remove Sync UserApi. r=markh https://hg.mozilla.org/integration/autoland/rev/e32665726c60 part 3 - Make BrowserID the only Sync identity manager. r=markh https://hg.mozilla.org/integration/autoland/rev/34e62bf16dd0 part 4 - Update tests to work with the BrowserID identity manager. r=markh https://hg.mozilla.org/integration/autoland/rev/5f82f1fc554a part 5 - Remove add_identity_test helper. r=markh https://hg.mozilla.org/integration/autoland/rev/438324d172d7 part 6 - Remove startOverKeepIdentity pref. r=markh https://hg.mozilla.org/integration/autoland/rev/a30baab71ce2 part 7 - Remove non-in-content legacy Sync pref screens. r=markh https://hg.mozilla.org/integration/autoland/rev/ff779cc6775b part 8 - Remove legacy Sync in-content pref screens. r=markh https://hg.mozilla.org/integration/autoland/rev/5d2d98c37b0e part 9 - Remove Old Sync link in about:accounts. r=markh https://hg.mozilla.org/integration/autoland/rev/f08ca0b9ec72 part 10 - Remove FxA Migration code. r=markh https://hg.mozilla.org/integration/autoland/rev/31eb6865049c part 11 - Remove Weave.Service.serverURL and friends. r=markh https://hg.mozilla.org/integration/autoland/rev/026b733245ee part 12 - Remove Weave.fxAccountsEnabled. r=markh https://hg.mozilla.org/integration/autoland/rev/da1c5b3e73f9 part 13 - Repair Weave Service getStorageInfo. r=markh
Comment 126•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a0022f621594 https://hg.mozilla.org/mozilla-central/rev/b2f0066f5949 https://hg.mozilla.org/mozilla-central/rev/e32665726c60 https://hg.mozilla.org/mozilla-central/rev/34e62bf16dd0 https://hg.mozilla.org/mozilla-central/rev/5f82f1fc554a https://hg.mozilla.org/mozilla-central/rev/438324d172d7 https://hg.mozilla.org/mozilla-central/rev/a30baab71ce2 https://hg.mozilla.org/mozilla-central/rev/ff779cc6775b https://hg.mozilla.org/mozilla-central/rev/5d2d98c37b0e https://hg.mozilla.org/mozilla-central/rev/f08ca0b9ec72 https://hg.mozilla.org/mozilla-central/rev/31eb6865049c https://hg.mozilla.org/mozilla-central/rev/026b733245ee https://hg.mozilla.org/mozilla-central/rev/da1c5b3e73f9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in
before you can comment on or make changes to this bug.
Description
•