1266235 - Rename Kinto blocklist clients internal preferences

Status: RESOLVED FIXED

(Cloud Services :: Firefox: Common, defect)

Description

[Mathieu Leplatre [:leplatrem]]

From Bug 1264914 https://bugzilla.mozilla.org/show_bug.cgi?id=1264914#c5

The preferences names used by the Kinto blocklist client can be improved:

* Add a `blocklist` prefix since we may have several places in firefox relying on kinto (eg. url, bucket and collection names)
* Replace the word `kinto` to something generic like `storage`, in case the product gets rebranded or replaced

Comment 1

[Matthew N. [:MattN]]

Do we think that we will only use Kinto for blocklist features? The use case I had in mind was password manager recipes which isn't really a blocklist.

Comment 2

[Mathieu Leplatre [:leplatrem]]

> Do we think that we will only use Kinto for blocklist features?

No, quite the opposite!

I must have formulated it badly: we shall add a prefix to the current preferences since they apply only for the blocklist. 

For example, in `services/common/KintoBlocklist.js`, we currently have:

    "services.kinto.base"
    "services.kinto.bucket"

They should probably become something like:


    "services.storage.blocklist.base"
    "services.storage.blocklist.bucket"

So that we can have another dedicated pref for the base url and bucket used for other use-cases like the password manager recipes (cool by the way! I'm looking forward to knowing more some day :))

Comment 3

[Mathieu Leplatre [:leplatrem]]

Attachment: MozReview Request: Bug 1266235 - Rename kinto-updater to blocklist-updater,r=mgoodwin

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

Comment 4

[Mathieu Leplatre [:leplatrem]]

Attachment: MozReview Request: Bug 1266235 - Rename KintoBlocklist to blocklist-clients,r=mgoodwin

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

Comment 5

[Mathieu Leplatre [:leplatrem]]

Attachment: MozReview Request: Bug 1266235 - Use blocklist prefix in preference names

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

Comment 6

[Mathieu Leplatre [:leplatrem]]

https://reviewboard.mozilla.org/r/48459/#review45165

::: addon-sdk/source/test/preferences/no-connections.json:31
(Diff revision 1)
>    "extensions.update.url": "http://localhost/extensions-dummy/updateURL",
>    "extensions.update.background.url": "http://localhost/extensions-dummy/updateBackgroundURL",
>    "extensions.blocklist.url": "http://localhost/extensions-dummy/blocklistURL",
>    "extensions.webservice.discoverURL": "http://localhost/extensions-dummy/discoveryURL",
>    "extensions.getAddons.maxResults": 0,
> -  "services.kinto.base": "http://localhost/dummy-kinto/v1",
> +  "services.blocklist.base": "http://localhost/dummy-kinto/v1",

This could be `services.settings.server` IMO

Comment 7

[Mathieu Leplatre [:leplatrem]]

Attachment: MozReview Request: Bug 1266235 - Move blocklist preferences to all.js

Related:

* https://bugzilla.mozilla.org/show_bug.cgi?id=1248557
* https://bugzilla.mozilla.org/show_bug.cgi?id=1259947

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

Comment 8

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8744319 [details]
MozReview Request: Bug 1266235 - Rename kinto-updater to blocklist-updater,r=mgoodwin

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

Comment 9

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8744320 [details]
MozReview Request: Bug 1266235 - Rename KintoBlocklist to blocklist-clients,r=mgoodwin

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

Comment 10

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8744321 [details]
MozReview Request: Bug 1266235 - Use blocklist prefix in preference names

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

Comment 11

[rsx11m]

(In reply to Mathieu Leplatre (:leplatrem) from comment #7)
> Created attachment 8747050 [details]
> MozReview Request: Bug 1266235 - Move blocklist preferences to all.js

Shouldn't security.onecrl.maximum_staleness_in_seconds be moved to all.js as well?
I can't find it in your patch, only its reference for extensions.blocklist.interval.

Comment 12

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8744319 [details]
MozReview Request: Bug 1266235 - Rename kinto-updater to blocklist-updater,r=mgoodwin

https://reviewboard.mozilla.org/r/48455/#review47019

Comment 13

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8744320 [details]
MozReview Request: Bug 1266235 - Rename KintoBlocklist to blocklist-clients,r=mgoodwin

https://reviewboard.mozilla.org/r/48457/#review47021

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8744319 [details]
MozReview Request: Bug 1266235 - Rename kinto-updater to blocklist-updater,r=mgoodwin

https://reviewboard.mozilla.org/r/48455/#review47619

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8744320 [details]
MozReview Request: Bug 1266235 - Rename KintoBlocklist to blocklist-clients,r=mgoodwin

https://reviewboard.mozilla.org/r/48457/#review47621

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8744321 [details]
MozReview Request: Bug 1266235 - Use blocklist prefix in preference names

https://reviewboard.mozilla.org/r/48459/#review47623

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8747050 [details]
MozReview Request: Bug 1266235 - Move blocklist preferences to all.js

https://reviewboard.mozilla.org/r/49705/#review47627

::: modules/libpref/init/all.js:2089
(Diff revision 1)
> +
> +// For now, let's keep settings server update out of the release channel
>  #ifdef RELEASE_BUILD
> +pref("services.blocklist.update_enabled", false);

"out of release builds"

since release builds include more than the "release channel" e.g. betas

Comment 18

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8747050 [details]
MozReview Request: Bug 1266235 - Move blocklist preferences to all.js

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

Comment 19

[Mathieu Leplatre [:leplatrem]]

(In reply to rsx11m from comment #11)
> (In reply to Mathieu Leplatre (:leplatrem) from comment #7)
> > Created attachment 8747050 [details]
> > MozReview Request: Bug 1266235 - Move blocklist preferences to all.js
> 
> Shouldn't security.onecrl.maximum_staleness_in_seconds be moved to all.js as
> well?
> I can't find it in your patch, only its reference for
> extensions.blocklist.interval.

Yes, I think so. Let's move it too.

Comment 20

[rsx11m]

Ok, I'm holding back on bug 1269773 then until this checks in.

(In reply to Mathieu Leplatre (:leplatrem) from comment #18)
> https://reviewboard.mozilla.org/r/49705/diff/1-2/

This reads now "out of the release build" which should grammatically either be "out of [the] release builds" (plural, where I'm not sure if a "the" is needed here) or "out of a release build" (singular).

Comment 21

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8747050 [details]
MozReview Request: Bug 1266235 - Move blocklist preferences to all.js

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

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/93deaa5641bc
https://hg.mozilla.org/integration/fx-team/rev/5f321f10da1e
https://hg.mozilla.org/integration/fx-team/rev/f560cba61749
https://hg.mozilla.org/integration/fx-team/rev/f684fac95bd9

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for xpcshell failures like http://archive.mozilla.org/pub/firefox/tinderbox-builds/fx-team-win32-debug/1462880591/fx-team_xp_ix-debug_test-xpcshell-bm119-tests1-windows-build25.txt.gz (Search the page for `<<<<` and scroll up a bit to find the actual failure, `>>>>` and scroll up a bit to see what Treeherder says the failure is.)

https://hg.mozilla.org/integration/fx-team/rev/ee562525573f

Comment 24

[Mathieu Leplatre [:leplatrem]]

I am truely sorry for this.

I've got a patch[0], what is the process now to submit it? Should backout your backout commit?

Thanks for your patience :/

[0] https://pastebin.mozilla.org/8870913

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

If you get the new patch reviewed, set another needinfo for me, I can reland the original ones and then land your new one on top of it.

Comment 26

[Matthew N. [:MattN]]

Yeah, just push to mozreview again with the fixes in the appropriate commits (part 3 I think) then add checkin-needed as I'm fine with the changes.

Comment 27

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8744319 [details]
MozReview Request: Bug 1266235 - Rename kinto-updater to blocklist-updater,r=mgoodwin

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

Comment 28

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8744320 [details]
MozReview Request: Bug 1266235 - Rename KintoBlocklist to blocklist-clients,r=mgoodwin

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

Comment 29

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8744321 [details]
MozReview Request: Bug 1266235 - Use blocklist prefix in preference names

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

Comment 30

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8747050 [details]
MozReview Request: Bug 1266235 - Move blocklist preferences to all.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49705/diff/3-4/

Comment 31

[Mathieu Leplatre [:leplatrem]]

I rebased and submitted a Try build, and test are now ok.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63aa3b6b8590

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c25b2f3a7ce9
https://hg.mozilla.org/integration/fx-team/rev/04cd17b0025c
https://hg.mozilla.org/integration/fx-team/rev/90ef5dbbb7c4
https://hg.mozilla.org/integration/fx-team/rev/085a53f65ff3

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_aboutCertError.js failures.
https://hg.mozilla.org/integration/fx-team/rev/aa14a4a7c5df

https://treeherder.mozilla.org/logviewer.html#?job_id=9331350&repo=fx-team#L1669

Comment 34

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8744319 [details]
MozReview Request: Bug 1266235 - Rename kinto-updater to blocklist-updater,r=mgoodwin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48455/diff/3-4/

Comment 35

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8744320 [details]
MozReview Request: Bug 1266235 - Rename KintoBlocklist to blocklist-clients,r=mgoodwin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48457/diff/3-4/

Comment 36

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8744321 [details]
MozReview Request: Bug 1266235 - Use blocklist prefix in preference names

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48459/diff/3-4/

Comment 37

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8747050 [details]
MozReview Request: Bug 1266235 - Move blocklist preferences to all.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49705/diff/4-5/

Comment 38

[Mathieu Leplatre [:leplatrem]]

I rebased again and submitted a Try build. A very few tests are failing for weird reasons. But :arai said they are known to be intermittent.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2b68d58de1e

(I could not retrigger the failing tests, I was prompted for credentials and mine are rejected)

I'd take the risk and ask for check-in again...

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3ffcd4e5783a
https://hg.mozilla.org/integration/fx-team/rev/dc96a1440e3c
https://hg.mozilla.org/integration/fx-team/rev/c6c57d394549
https://hg.mozilla.org/integration/fx-team/rev/a5aad78f70ea

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ffcd4e5783a
https://hg.mozilla.org/mozilla-central/rev/dc96a1440e3c
https://hg.mozilla.org/mozilla-central/rev/c6c57d394549
https://hg.mozilla.org/mozilla-central/rev/a5aad78f70ea