See bug 555755 comment #20.

/r/4519 - Bug 1134881 - Sync password timeCreated & timePasswordChanged fields. r=rnewman Pull down this commit: hg pull review -r d4b8c39090abbb1bab94cb0752f6f5158fbfeb82

Chopped up rnewman's patch and going to see if it needs anything else. Mobile already syncs these values actually, and I don't see why we would start overwriting the password records if we start syncing timeCreated and timeChanged (which isn't a common event and accompanies a password sync anyways), but I'll doublecheck.

> Mobile already syncs these values actually, and I don't see why we would start overwriting the password records if we start syncing timeCreated and timeChanged (which isn't a common event and accompanies a password sync anyways), but I'll doublecheck. Really? Does it sync any other properties of password records? If so, we should add those as well for parity.

On Fennec we include creation time, time last used, times used, and time changed. Chenxia included these in Bug 709385. This is only safe because we only treat a record as modified when it's deleted or the password changes. This is basically the safe-ish minimal approach that we discussed in Bug 555755 -- only send updated values when something substantial changes. Note that we don't do anything smart about merging these -- if another client changes a password, we replace all of our timestamp/count fields with theirs. Notably, this code is wrong if the remote record doesn't include these values. We'll set them to zero locally. I would be happy to see us stop syncing time last used and times used until we can do something smarter there, and we should certainly stop blanking the local fields if the server record omits them.

> I would be happy to see us stop syncing time last used and times used until we can do something smarter there, and we should certainly stop blanking the local fields if the server record omits them. I agree.

Comment on attachment 8619544 [details] MozReview Request: Bug 1134881 - Sync password timeCreated & timePasswordChanged fields. r=rnewman Hey Richard, is there more to do here for the desktop side? What's a good way to test this?

https://reviewboard.mozilla.org/r/4517/#review10711 This looks (unsurprisingly) sane to me. To test this, you should add to services/sync/tests/unit/test_password_store.js. You should also try syncing between two desktops; you should see the new timestamps appear in the new profile's password store, and changes move back and forth. You can verify this by looking at the storage JSON in the profile dir for nsILMS. The next stage of testing is to make sure it doesn't go crazy when connected to (a) an Android device that syncs these fields, and (b) a desktop that doesn't (e.g., current release).

Richard, our new passwords intern, Riadh, is going to work on finalizing this.

Comment on attachment 8628466 [details] [diff] [review] Bug 1134881 - Test sync password timeCreated & timePasswordChanged fields Review of attachment 8628466 [details] [diff] [review]: ----------------------------------------------------------------- You need to add some kind of test that applying a record with different times will change the times in the DB. All you're testing here is that nothing throws when applying a record -- not that it does the right thing. ::: services/sync/tests/unit/test_password_store.js @@ +5,5 @@ > Cu.import("resource://services-sync/service.js"); > Cu.import("resource://services-sync/util.js"); > > +function checkRecord(hostname, formSubmitURL, httpRealm, name, record_count, > + BOGUS_GUID) { s/BOGUS_GUID/guid @@ +18,5 @@ > + _("Count" + name + ":" + count.value); > + > + if (record_count > 0) > + do_check_true(!!store.getAllIDs()[BOGUS_GUID]); > + else Braces on every conditional. @@ +23,5 @@ > + do_check_true(!store.getAllIDs()[BOGUS_GUID]); > + > +} > + > +function time_test(timeCreated, timePasswordChanged) { test_apply_records_with_times @@ +38,5 @@ > + usernameField: "username", > + passwordField: "password"}; > + > + if (timeCreated !== null) > + record.timeCreated = timeCreated; Braces on every conditional. @@ +54,5 @@ > + try { > + applyEnsureNoFailures([record]); > + > + // The following record has to be found in the store. > + checkRecord(record.hostname, record.formSubmitURL, null, "", 1, BOGUS_GUID); Check the timestamp! @@ +61,5 @@ > + store.wipe(); > + } > +} > + > +function time_many_records_test() { test_apply_multiple_records_with_times @@ +167,5 @@ > + BOGUS_GUID_B); > + > + checkRecord(recordC.hostname, recordC.formSubmitURL, null, "C", 1, > + BOGUS_GUID_C); > + Shouldn't you be checking the timestamps? @@ +244,5 @@ > store.wipe(); > } > + > + time_test(null, null); > + time_test(1000, null); These should be inside the try block that ends on 243.

latest patch https://reviewboard.mozilla.org/r/12451/diff/1#index_header

Bug 1134881 - Sync password timeCreated & timePasswordChanged fields. r=rnewman

Bug 1134881 - Test sync password timeCreated & timePasswordChanged fields

Comment on attachment 8630736 [details] MozReview Request: Bug 1134881 - Test sync password timeCreated & timePasswordChanged fields https://reviewboard.mozilla.org/r/12451/#review11787 Ship It! ::: services/sync/tests/unit/test_password_store.js:9 (Diff revision 1) > +function checkRecord(name, record, record_count, timeCreated, Be consistent. "record_count" right next to "timeCreated". "recordCount" is probably best. ::: services/sync/tests/unit/test_password_store.js:40 (Diff revision 1) > + else { Nit: cuddle } else {. ::: services/sync/tests/unit/test_password_store.js:41 (Diff revision 1) > + do_check_true(!store.getAllIDs()[record.id]); So if a record is missing entirely (and thus is omitted from getAllIDs), this test function just passes? I'd expect this to fail hard if the record doesn't exist. ::: services/sync/tests/unit/test_password_store.js:22 (Diff revision 1) > + do_check_eq(count.value, record_count); expectedCount? ::: services/sync/tests/unit/test_password_store.js:31 (Diff revision 1) > + if (timePasswordChanged !== undefined) { Newline before. ::: services/sync/tests/unit/test_password_store.js:46 (Diff revision 1) > +function change_password(name, hostname, password, record_count, timeCreated, Again, naming consistency for both function name and params. ::: services/sync/tests/unit/test_password_store.js:11 (Diff revision 1) > + expectedTimePasswordChanged, update) { isUpdate. But even better is to make the param say what it really is. ::: services/sync/tests/unit/test_password_store.js:71 (Diff revision 1) > + function applyEnsureNoFailures(records) { Inline this. It's used once.

https://reviewboard.mozilla.org/r/12451/#review11787 > So if a record is missing entirely (and thus is omitted from getAllIDs), this test function just passes? > > I'd expect this to fail hard if the record doesn't exist. it's possible to use the function to test if a record that is not in the database is missing, by passing zero as the recordCount. This is why if the recordCount is 0, we just check if the record is not in the database.

Comment on attachment 8630735 [details] MozReview Request: Bug 1134881 - Sync password timeCreated & timePasswordChanged fields. Bug 1134881 - Sync password timeCreated & timePasswordChanged fields. r=rnewman

Comment on attachment 8630736 [details] MozReview Request: Bug 1134881 - Test sync password timeCreated & timePasswordChanged fields Bug 1134881 - Test sync password timeCreated & timePasswordChanged fields

Comment on attachment 8630735 [details] MozReview Request: Bug 1134881 - Sync password timeCreated & timePasswordChanged fields. Assuming I can trust reviewboard, this hasn't changed since Chenxia chopped up and finished my patch. If so, it's a little odd for me to review it. Please make sure it lands with Chenxia's name on it; credit where credit is due. If there are changes I'm not seeing, please let me know!

url: https://hg.mozilla.org/integration/fx-team/rev/d05da1d7e56c586e09faa88e541bdffd313cb1c3 changeset: d05da1d7e56c586e09faa88e541bdffd313cb1c3 user: Richard Newman <rnewman@mozilla.com> date: Wed Oct 15 18:56:29 2014 -0700 description: Bug 1134881 - Sync password timeCreated & timePasswordChanged fields. r=rnewman url: https://hg.mozilla.org/integration/fx-team/rev/7040a1d6ce2e9457560b38634106922bed46154b changeset: 7040a1d6ce2e9457560b38634106922bed46154b user: Riadh Chtara <rchtara@mozilla.com> date: Mon Jul 13 23:02:07 2015 -0700 description: Bug 1134881 - Test sync password timeCreated & timePasswordChanged fields. r=rnewman

https://hg.mozilla.org/integration/mozilla-inbound/rev/d05da1d7e56c https://hg.mozilla.org/integration/mozilla-inbound/rev/7040a1d6ce2e

https://hg.mozilla.org/mozilla-central/rev/d05da1d7e56c https://hg.mozilla.org/mozilla-central/rev/7040a1d6ce2e

It seems that we have a test covering this change, so removing the qe-verify+ flag. Please feel free to add it back if you think manual testing is needed here.