Closed Bug 1286918 Opened 3 years ago Closed 3 years ago

Implement a validator for the sync password engine

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: tcsc, Assigned: tcsc)

References

Details

(Whiteboard: [data-integrity][measure-integrity][sprint-3])

Attachments

(2 files)

Similar to the bookmark validator from bug 1265419. This should integrate with telemetry in the same way as the bookmark validator does in bug 1267917.
Assignee: nobody → tchiovoloni
Status: NEW → ASSIGNED
Depends on: 1267917
Whiteboard: [data-integrity]
Priority: -- → P2
This applies on top of bug 1286915's patch for addon validator, which hasn't landed yet so I can't use mozreview (Maybe I should have waited longer on uploading this?).

Tests for this are not obviously useful to me since they'd be testing the same things as the addon validator (and the same collection_validator.js code).
Attachment #8776686 - Flags: feedback?(markh)
Whiteboard: [data-integrity] → [data-integrity][measure-integrity]
Whiteboard: [data-integrity][measure-integrity] → [data-integrity][measure-integrity][sprint-3]
Rank: 1
Comment on attachment 8776686 [details] [diff] [review]
0001-Bug-1286918-Implement-sync-validator-for-passwords-f.patch

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

LGTM, but needs tests.
Attachment #8776686 - Flags: feedback?(markh) → feedback+
Priority: P2 → P1
Rank: 1 → 0
Took your advice on moving the common code out of addons and put it here, which seems to be the least problematic of the engines. I also added tests, and TPS integration. Let me know if you think I should abstract the TPS integration more, I'm on the fence about it.
Comment on attachment 8788570 [details]
Bug 1286918 - Implement sync validator for passwords engine and integrate with TPS

https://reviewboard.mozilla.org/r/77012/#review75308

I think that level of abstraction looks about right - thanks!

::: services/sync/modules/collection_validator.js:86
(Diff revision 1)
> +  getClientItems() {
> +    return Promise.reject("Must implement");
> +  }
> +
> +  // Turn the client item into something that can be compared with the server item,
> +  // and is also safe to mutate.

The "safe to mutate" comment here seems wrong WRT returning the item that was passed in - should we be cloning it by default?
Attachment #8788570 - Flags: review?(markh) → review+
Comment on attachment 8788570 [details]
Bug 1286918 - Implement sync validator for passwords engine and integrate with TPS

https://reviewboard.mozilla.org/r/77012/#review75308

> The "safe to mutate" comment here seems wrong WRT returning the item that was passed in - should we be cloning it by default?

Seems unlikely to matter given that every engine will probably reimplement this method, but yeah, cloning by default is probably better.
https://hg.mozilla.org/mozilla-central/rev/2c1d5f860259
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.