Comment on attachment 8731726 [details] [diff] [review] Generalize Kinto blocklist client to addons/plugins/gfx Review of attachment 8731726 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. You'll also need to get a services peer to review (rnewman, perhaps?). ::: browser/app/profile/firefox.js @@ +67,5 @@ > pref("services.kinto.changes.path", "/buckets/monitor/collections/changes/records"); > pref("services.kinto.bucket", "blocklists"); > pref("services.kinto.onecrl.collection", "certificates"); > pref("services.kinto.onecrl.checked", 0); > +pref("services.kinto.addons.collection", "addons"); Maybe now is not the time to address this but, perhaps, once we're happy this works for Firefox and mobile, we could move these to all.js

Comment on attachment 8731726 [details] [diff] [review] Generalize Kinto blocklist client to addons/plugins/gfx Matt, dolske suggested that you'd be a good person to get acquainted with this stuff.

Comment on attachment 8731726 [details] [diff] [review] Generalize Kinto blocklist client to addons/plugins/gfx Review of attachment 8731726 [details] [diff] [review]: ----------------------------------------------------------------- Pleas use a Git/HG move from services/common/KintoCertificateBlocklist.js to services/common/KintoBlocklist.js so it's easier to review what actually changed.

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

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

Comment on attachment 8738085 [details] MozReview Request: Bug 1257556 - Rename KintoBlocklist client,r=MattN Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44295/diff/1-2/

Comment on attachment 8738085 [details] MozReview Request: Bug 1257556 - Rename KintoBlocklist client,r=MattN https://reviewboard.mozilla.org/r/44295/#review42849

Comment on attachment 8738091 [details] MozReview Request: Bug 1257556 - Generalize Kinto blocklist client to addons/plugins/gfx,r=MattN https://reviewboard.mozilla.org/r/44297/#review42853 Apologies again for the delay. r=me with some changes and some discussion points below. ::: services/common/KintoBlocklist.js:7 (Diff revision 1) > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > -this.EXPORTED_SYMBOLS = ["OneCRLClient"]; > +this.EXPORTED_SYMBOLS = ["AddonClient", "GfxClient", "OneCRLClient", "PluginClient"]; Some of these symbols aren't descriptive enough. I think they should have blocklist or somethng like that in the name. If I saw code using `AddonClient` I wouldn't suspect it has anything to do with the blocklist. ::: services/common/KintoBlocklist.js:63 (Diff revision 1) > - let updateLastCheck = function() { > - let checkedServerTimeInSeconds = Math.round(serverTime / 1000); > - Services.prefs.setIntPref(PREF_KINTO_ONECRL_CHECKED_SECONDS, > - checkedServerTimeInSeconds); > + > +class BlocklistClient { > + > + constructor(collectionName, checkPrefName, processCallback) { > + this.collectionName = collectionName; > + this.checkPrefName = checkPrefName; I find checkPrefName to be a bit vague for the property name. (I actually don't like the "checked" naming anywhere as it's also vague and I believe "updated" would be more descriptive and accurate IIUC. I'm not sure if it's too late to fix that now?) Since there is no such thing as a reference to a preference I think the "Name" suffix can be dropped so it's not too long. How about "updateTimePref" or "lastUpdatePref"? Or if you're using "check" instead of "update" since there may be a check with no actual change then perhaps "lastCheckTimePref"? ::: services/common/KintoBlocklist.js:71 (Diff revision 1) > + the remote collection. > + * @param {Date} serverTime the current date return by server. Nit: "returned by the server" ::: services/common/KintoBlocklist.js:78 (Diff revision 1) > + > + return Task.spawn((function* () { > try { Nit: Adding a function name here can be useful for stack traces ::: services/common/KintoBlocklist.js:94 (Diff revision 1) > + > + yield this.processCallback(list.data); > + > + // Track last update. > + this.updateLastCheck(serverTime); Is there ever a case where we would want an exception in the callback to prevent the updateLastCheck call? If not, I suggest that the try…catch move from updateJSONBlocklist to here. ::: services/common/KintoBlocklist.js:148 (Diff revision 1) > + try { > + yield OS.File.writeAtomic(path, serialized, {tmpPath: path + ".tmp"}); > } > + catch(e) { > + Cu.reportError(e); > + } > + > + // Notify change to `nsBlocklistService` > + const eventData = {filename: filename}; > + Services.cpmm.sendAsyncMessage("Blocklist:reload-from-disk", eventData); If we failed writing the new file, I don't think we would want to notify about a change (which didn't happen). ::: services/common/KintoBlocklist.js:150 (Diff revision 1) > } > + catch(e) { Nit: cuddle the catch ::: services/common/kinto-updater.js:83 (Diff revision 1) > -kintoClients.certificates = > - Cu.import("resource://services-common/KintoCertificateBlocklist.js", {}) > - .OneCRLClient; > +const KintoBlocklist = Cu.import("resource://services-common/KintoBlocklist.js", {}); > +gKintoClients[Services.prefs.getCharPref(PREF_KINTO_ONECRL_COLLECTION)] = KintoBlocklist.OneCRLClient; > +gKintoClients[Services.prefs.getCharPref(PREF_KINTO_ADDONS_COLLECTION)] = KintoBlocklist.AddonClient; > +gKintoClients[Services.prefs.getCharPref(PREF_KINTO_GFX_COLLECTION)] = KintoBlocklist.GfxClient; > +gKintoClients[Services.prefs.getCharPref(PREF_KINTO_PLUGINS_COLLECTION)] = KintoBlocklist.PluginClient; Nit: You could use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#Computed_property_names for the gKintoClients properties. That would at least keep the const defintion closer to the properties and make these lines less verbose. ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:1 (Diff revision 1) > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + Nit: Tests are public domain by default nowadays so this isn't necessary. ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:7 (Diff revision 1) > +const FILE_BLOCKLIST_ADDONS = "blocklist-addons.json"; > +const FILE_BLOCKLIST_GFX = "blocklist-gfx.json"; > +const FILE_BLOCKLIST_PLUGINS = "blocklist-plugins.json"; ```const {FILENAME_ADDONS_JSON, FILENAME_GFX_JSON, …} = Cu.import("resource://services-common/KintoBlocklist.js", {});``` to avoid duplication. ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:10 (Diff revision 1) > +const PREF_KINTO_ADDONS_CHECKED_SECONDS = "services.kinto.addons.checked"; > +const PREF_KINTO_GFX_CHECKED_SECONDS = "services.kinto.gfx.checked"; > +const PREF_KINTO_PLUGINS_CHECKED_SECONDS = "services.kinto.plugins.checked"; Nit: You could avoid duplicating this by getting it from the exported clients' `checkPrefName` property. ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:25 (Diff revision 1) > +XPCOMUtils.defineLazyModuleGetter(this, "FileUtils", > + "resource://gre/modules/FileUtils.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "OS", > + "resource://gre/modules/osfile.jsm"); Nit: No need to lazy-load stuff in tests or in-general when you know it's going to get loaded right away anyways. ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:30 (Diff revision 1) > +var server; > +var kintoClient; Don't use `var` in new code (regardless of scope), only `const` or `let`. ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:47 (Diff revision 1) > +} > + > + > +function* readJSON(filepath) { Nit: extra new line ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:54 (Diff revision 1) > +} > + > + > +function* clear_state() { Nit: extra newline ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:65 (Diff revision 1) > + if (blocklist.exists()) > + blocklist.remove(true); It's prefered to always brace if/else in new code. ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:75 (Diff revision 1) > + blocklist = FileUtils.getFile(KEY_PROFILEDIR, [FILE_BLOCKLIST_GFX]); > + if (blocklist.exists()) > + blocklist.remove(true); > + > + // Clear local DBs. > + for(let name of ["addons", "gfx", "plugins"]) { Nit: missing space after `for` ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:92 (Diff revision 1) > +function run_test() { > + // Set up an HTTP Server > + server = new HttpServer(); > + server.start(-1); > + > + // Point the KintoAddonPlugin client to use this local HTTP server. I think `KintoAddonPlugin` is a leftover name? I guess it's also used by this test file to describe what it's testing as I guess it doesn't test oneCRL stuff? ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:97 (Diff revision 1) > + // Point the KintoAddonPlugin client to use this local HTTP server. > + Services.prefs.setCharPref("services.kinto.base", > + `http://localhost:${server.identity.primaryPort}/v1`); > + > + // Setup server fake responses. > + function handleResponse (request, response) { Nit: remove the space before `(` ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:99 (Diff revision 1) > + const sampled = getSampleResponse(request, server.identity.primaryPort); > + if (!sampled) { Nit: `sample`? ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:113 (Diff revision 1) > + response.write(sampled.responseBody); > + } catch (e) { I think you're missing response.finish() ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:115 (Diff revision 1) > + } > + response.setHeader("Date", (new Date()).toUTCString()); > + > + response.write(sampled.responseBody); > + } catch (e) { > + dump(`${e}\n`); I think do_print is preferred and it includes the \n for you ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:130 (Diff revision 1) > + do_register_cleanup(function() { > + server.stop(function() { }); Nit: if you're creating an anonymous function you can use fat arrow functions: => ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:174 (Diff revision 1) > +}); > + > +add_task(clear_state); > + > + > +add_task(function* test_plugins_list_is_written_to_file_in_profile(){ Nit: I wouldn't put a newline before the clear_state lines and only have one after ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:197 (Diff revision 1) > + > +add_task(function* test_current_server_time_is_saved_in_addons_pref(){ > + const before = Services.prefs.getIntPref(PREF_KINTO_ADDONS_CHECKED_SECONDS); > + yield AddonClient.maybeSync(2000, Date.now()); > + const after = Services.prefs.getIntPref(PREF_KINTO_ADDONS_CHECKED_SECONDS); > + notEqual(before, after); I think you should do a sanity check on the value e.g. check that it's a date from the last 5 minutes. Same as the others below ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:288 (Diff revision 1) > + received = aMsg; > + resolve(); Why not just resolve(aMsg) to avoid a variable and some lines: let received = yield new Promise((resolve, reject) => { ::: services/common/tests/unit/test_kintoAddonPluginBlocklist.js:303 (Diff revision 1) > +}); > + > +add_task(clear_state); > + > + > +add_task(function* test_sends_reload_message_when_plugins_has_changes(){ I'm not sure it's necessary to separately test each client when they share the same code.

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

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

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

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

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

https://reviewboard.mozilla.org/r/44297/#review42853 > I find checkPrefName to be a bit vague for the property name. > > (I actually don't like the "checked" naming anywhere as it's also vague and I believe "updated" would be more descriptive and accurate IIUC. I'm not sure if it's too late to fix that now?) > > Since there is no such thing as a reference to a preference I think the "Name" suffix can be dropped so it's not too long. How about "updateTimePref" or "lastUpdatePref"? Or if you're using "check" instead of "update" since there may be a check with no actual change then perhaps "lastCheckTimePref"? For now, I renamed to lastCheckTimePref. As for the «check» notion, we can change that in another contribution (since it will conflict with other patches about to land). BTW it is a bit different than «update», it is more like a «ping». We'll think of something to make this crystal clear. Also, we may want to abstract a bit the notion «kinto» (eg. «storage») and add more namespacing in the pref name (eg. "services.kinto.onecrl.checked" → "services.storage.blocklist.onecrl.checked") > Is there ever a case where we would want an exception in the callback to prevent the updateLastCheck call? If not, I suggest that the try…catch move from updateJSONBlocklist to here. Yes, if an exception occurs then we don't want to update the last check. > I'm not sure it's necessary to separately test each client when they share the same code. I refactored the tests to avoid duplication! I'd prefer to keep some tests that assert the parameters they were instantiated with.

Comment on attachment 8741703 [details] MozReview Request: Bug 1257556 - Address some of MattN comments,r=MattN https://reviewboard.mozilla.org/r/46695/#review43435 ::: services/common/KintoBlocklist.js:11 (Diff revision 1) > + "FILENAME_ADDONS_JSON", > + "FILENAME_GFX_JSON", > + "FILENAME_PLUGINS_JSON"]; Note that I wasn't suggesting that you export these, I was suggesting that the test reach into the BackstagePass and use them without exporting. I think it's fine to do that from a test for the specific module. ::: services/common/kinto-updater.js:32 (Diff revision 1) > +// Add a blocklist client for testing purposes. Do not use for any other purpose > +this.addTestKintoClient = (name, client) => { gBlocklistClients[name] = client; } > + > // This is called by the ping mechanism. > // returns a promise that rejects if something goes wrong > -this.checkVersions = function() { > +this.checkVersions = function checkVersions() { In this case the JS engine can already figure out the name from the LHS so you don't need the name with the function.

Comment on attachment 8741716 [details] MozReview Request: Bug 1257556 - Rename checkPrefName to lastCheckTimePref,r=MattN https://reviewboard.mozilla.org/r/46705/#review43437

Comment on attachment 8741726 [details] MozReview Request: Bug 1257556 - Rename blocklist clients instances,r=MattN https://reviewboard.mozilla.org/r/46707/#review43439

Comment on attachment 8741798 [details] MozReview Request: Bug 1257556 - Add proper tests for time saved in preferences,r=MattN https://reviewboard.mozilla.org/r/46761/#review43441

Comment on attachment 8741799 [details] MozReview Request: Bug 1257556 - Refactor tests to avoid duplication between clients,r=MattN https://reviewboard.mozilla.org/r/46763/#review43443 Note that you don't need to flag me for review again if I give a "Ship it with issues". You simply fix the issues then push back to review with r=… and then land/checkin-needed.

https://reviewboard.mozilla.org/r/46695/#review43435 > Note that I wasn't suggesting that you export these, I was suggesting that the test reach into the BackstagePass and use them without exporting. I think it's fine to do that from a test for the specific module. I think it would still be useful to have those constants available in order to use them in `nsBlocklistService.js` when loading them during startup.

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

Comment on attachment 8738091 [details] MozReview Request: Bug 1257556 - Generalize Kinto blocklist client to addons/plugins/gfx,r=MattN Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44297/diff/1-2/

Comment on attachment 8738091 [details] MozReview Request: Bug 1257556 - Generalize Kinto blocklist client to addons/plugins/gfx,r=MattN Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44297/diff/2-3/

Hi Mathieu, I was going to land this but there are conflicts with some that I already landed. I started to resolve the conflicts myself but I'm afraid I'll make a mistake. Can you please rebase this on a more recent revision then re-push that back here so it's ready for review (e.g. with r=MattN in the commit messages). Then you can add the `checkin-needed` bug keyword.

Comment on attachment 8738085 [details] MozReview Request: Bug 1257556 - Rename KintoBlocklist client,r=MattN Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44295/diff/2-3/

Comment on attachment 8738091 [details] MozReview Request: Bug 1257556 - Generalize Kinto blocklist client to addons/plugins/gfx,r=MattN Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44297/diff/3-4/

https://hg.mozilla.org/integration/fx-team/rev/102f375bffd1 https://hg.mozilla.org/integration/fx-team/rev/253a857a1dcc

https://hg.mozilla.org/mozilla-central/rev/102f375bffd1 https://hg.mozilla.org/mozilla-central/rev/253a857a1dcc