1286918 - Implement a validator for the sync password engine

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Thom Chiovoloni [:tcsc] (ex-moco)]

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.

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: 0001-Bug-1286918-Implement-sync-validator-for-passwords-f.patch

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).

Comment 2

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1286918 - Implement sync validator for passwords engine and integrate with TPS

Review commit: https://reviewboard.mozilla.org/r/77012/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77012/

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

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 5

[Mark Hammond [:markh] [:mhammond]]

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?

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8788570 [details]
Bug 1286918 - Implement sync validator for passwords engine and integrate with TPS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77012/diff/1-2/

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

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.

Comment 8

[Carsten Book [:Tomcat]]

on autoland https://hg.mozilla.org/integration/autoland/rev/2c1d5f860259

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2c1d5f860259