Closed Bug 1266235 Opened 8 years ago Closed 8 years ago

Rename Kinto blocklist clients internal preferences

Categories

(Cloud Services :: Firefox: Common, defect)

defect
Not set
normal

Tracking

(firefox49 fixed)

RESOLVED FIXED
Tracking Status
firefox49 --- fixed

People

(Reporter: leplatrem, Assigned: leplatrem)

References

Details

Attachments

(4 files)

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
Depends on: 1257556
See Also: → 1264914
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.
> 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 :))
Assignee: nobody → mathieu
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 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 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 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/
(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.
Attachment #8744319 - Flags: review?(MattN+bmo)
Attachment #8744320 - Flags: review?(MattN+bmo)
Attachment #8744321 - Flags: review?(MattN+bmo)
Attachment #8747050 - Flags: review?(MattN+bmo)
Comment on attachment 8744319 [details]
MozReview Request: Bug 1266235 - Rename kinto-updater to blocklist-updater,r=mgoodwin

https://reviewboard.mozilla.org/r/48455/#review47019
Attachment #8744319 - Flags: review?(mgoodwin) → review+
Comment on attachment 8744320 [details]
MozReview Request: Bug 1266235 - Rename KintoBlocklist to blocklist-clients,r=mgoodwin

https://reviewboard.mozilla.org/r/48457/#review47021
Attachment #8744320 - Flags: review?(mgoodwin) → review+
Blocks: 1269773
Comment on attachment 8744319 [details]
MozReview Request: Bug 1266235 - Rename kinto-updater to blocklist-updater,r=mgoodwin

https://reviewboard.mozilla.org/r/48455/#review47619
Attachment #8744319 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8744320 [details]
MozReview Request: Bug 1266235 - Rename KintoBlocklist to blocklist-clients,r=mgoodwin

https://reviewboard.mozilla.org/r/48457/#review47621
Attachment #8744320 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8744321 [details]
MozReview Request: Bug 1266235 - Use blocklist prefix in preference names

https://reviewboard.mozilla.org/r/48459/#review47623
Attachment #8744321 - Flags: review?(MattN+bmo) → review+
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
Attachment #8747050 - Flags: review?(MattN+bmo) → review+
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/
(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.
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 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/
Keywords: checkin-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
Flags: needinfo?(mathieu)
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
Flags: needinfo?(wkocher)
Flags: needinfo?(mathieu)
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(wkocher)
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.
Flags: needinfo?(MattN+bmo)
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 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 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 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/
I rebased and submitted a Try build, and test are now ok.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63aa3b6b8590
Keywords: checkin-needed
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 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 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 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/
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...
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: