603409 - Allow add-ons to opt out of metadata collection

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

We should allow add-on developers to specify that their add-ons should not be included in the list of add-ons that we ping AMO for metadata for. This should be done by just having them set a pref (extensions.<ID>.getAddons.cache.enabled probably).

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Would that be configurable by the user then? Means flipping this pref will enable the ping?

Comment 2

[Dave Townsend [:mossop]]

They'd be able to set it manually in about:config, but otherwise not visible in the UI.

Comment 3

[Dave Townsend [:mossop]]

And the pref will default to enabled so opt-out.

Comment 4

[:Margaret Leibovic]

Attachment: WIP

Is this the right idea? Also, I'm not sure how to test this. Do you have any add-ons manager testing strategies I can use?

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 485326 [details] [diff] [review]
WIP

This is along the right lines however it has a couple of problems.

There are two methods that download and cache metadata for add-ons, repopulateCache and cacheAddons, you need to cover both cases (it might be possible to just merge a lot of the code from those two methods but isn't necessary).

The patch as is still allows the request for the data to go to AMO and then just doesn't cache it, instead you should remove the add-on from the list of IDs we request in the first place.

No need to warn if the pref isn't there, that will probably be the most common case.

We have a lot of xpcshell tests for the add-ons manager, including ones that pull metadata, see for example the testcase in http://hg.mozilla.org/mozilla-central/rev/59f185971b5c (covers the cacheAddons method), also the whole of test_AddonRepository_cache.js which verifies repopulateCache

Comment 6

[:Margaret Leibovic]

Attachment: patch

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 485949 [details] [diff] [review]
patch

>diff --git a/toolkit/mozapps/extensions/AddonRepository.jsm b/toolkit/mozapps/extensions/AddonRepository.jsm
>--- a/toolkit/mozapps/extensions/AddonRepository.jsm
>+++ b/toolkit/mozapps/extensions/AddonRepository.jsm
>@@ -528,16 +528,28 @@ var AddonRepository = {
>     // Completely remove cache if caching is not enabled
>     if (!this.cacheEnabled) {
>       this._addons = null;
>       this._pendingCallbacks = null;
>       AddonDatabase.delete(aCallback);
>       return;
>     }
> 
>+    aIds.forEach(function(aId, aIndex){
>+      let preference = "extensions." + aId + ".getAddons.cache.enabled";
>+      let enabled = true;
>+      try {
>+        enabled = Services.prefs.getBoolPref(preference);
>+      } catch(e) {
>+        // If pref doesn't exist, default to enabled = true.
>+      }
>+      if (!enabled)
>+        aIds.splice(aIndex);
>+    });

Mutating the array that was passed in my the caller is kind of a no-no. Instead how about using aIds.filter to construct a new array. You should also make sure this does something sensible in the event that there are no add-ons left to get the data for.

>    */
>   cacheAddons: function(aIds, aCallback) {
>     if (!this.cacheEnabled) {
>       if (aCallback)
>         aCallback();
>       return;
>     }
> 
>+    aIds.forEach(function(aId, aIndex){
>+      let preference = "extensions." + aId + ".getAddons.cache.enabled";
>+      let enabled = true;
>+      try {
>+        enabled = Services.prefs.getBoolPref(preference);
>+      } catch(e) {
>+        // If pref doesn't exist, default to enabled = true.
>+      }
>+      if (!enabled)
>+        aIds.splice(aIndex);
>+    });

Ditto

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_AddonRepository_cache.js b/toolkit/mozapps/extensions/test/xpcshell/test_AddonRepository_cache.js

> // Tests repopulateCache when search returns no results
> function run_test_4() {
>   Services.prefs.setCharPref(PREF_GETADDONS_BYIDS, GETADDONS_EMPTY);
> 
>   AddonRepository.repopulateCache(ADDON_IDS, function() {
>+    check_initialized_cache([false, false, false], run_test_41);
>+  });
>+}
>+
>+// Tests repopulateCache when caching is disabled for a single add-on
>+function run_test_41() {
>+  Services.prefs.setBoolPref(PREF_ADDON0_CACHE_ENABLED, false);
>+
>+  AddonRepository.repopulateCache([ADDON_IDS[0]], function() {
>+    // Reset pref for next test
>+    Services.prefs.setBoolPref(PREF_ADDON0_CACHE_ENABLED, true);
>     check_initialized_cache([false, false, false], run_test_5);
>   });
> }

These tests don't seem to be testing anything, in both cases the search results return nothing and so there is nothing cached for the add-on regardless of the caching pref. You'd probably get differing results if you did it from run_test_5 instead.

> // Tests cacheAddons when the search returns no results
> function run_test_8() {
>   Services.prefs.setCharPref(PREF_GETADDONS_BYIDS, GETADDONS_EMPTY);
> 
>   AddonRepository.cacheAddons(ADDON_IDS, function() {
>+    check_initialized_cache([false, false, false], run_test_81);
>+  });
>+}
>+
>+// Tests cacheAddons when caching is disabled for a single add-on
>+function run_test_81() {
>+  Services.prefs.setBoolPref(PREF_ADDON0_CACHE_ENABLED, false);
>+
>+  AddonRepository.cacheAddons([ADDON_IDS[0]], function() {
>+    // Reset pref for next test
>+    Services.prefs.setBoolPref(PREF_ADDON0_CACHE_ENABLED, true);
>     check_initialized_cache([false, false, false], run_test_9);
>   });
> }

Ditto.

I'd also really like to see an additional test in test_install.js like run_test_19 to make sure that the data isn't cached at install time when the preference says not to.

Comment 9

[:Margaret Leibovic]

Attachment: patch v2

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 496356 [details] [diff] [review]
patch v2

Nice work, looks good

Comment 11

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/d3ca4963f4b2

Comment 12

[Dave Townsend [:mossop]]

Posted some docs here https://developer.mozilla.org/en/Addons/Working_with_AMO

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre