Allow add-ons to opt out of metadata collection

VERIFIED FIXED in mozilla2.0b8

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: mossop, Assigned: Margaret)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla2.0b8
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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).
(Reporter)

Updated

7 years ago
blocking2.0: --- → final+
Would that be configurable by the user then? Means flipping this pref will enable the ping?
(Reporter)

Comment 2

7 years ago
They'd be able to set it manually in about:config, but otherwise not visible in the UI.
(Reporter)

Comment 3

7 years ago
And the pref will default to enabled so opt-out.
(Assignee)

Comment 4

7 years ago
Created attachment 485326 [details] [diff] [review]
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?
Assignee: nobody → margaret.leibovic
Attachment #485326 - Flags: feedback?(dtownsend)
(Reporter)

Comment 5

7 years ago
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
Attachment #485326 - Flags: feedback?(dtownsend) → feedback-
(Assignee)

Comment 6

7 years ago
Created attachment 485949 [details] [diff] [review]
patch
Attachment #485326 - Attachment is obsolete: true
Attachment #485949 - Flags: review?(dtownsend)
(Assignee)

Updated

7 years ago
Whiteboard: [good first bug] → [good first bug][needs review dtownsend]
(Reporter)

Comment 7

7 years ago
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.
Attachment #485949 - Flags: review?(dtownsend) → review-
(Assignee)

Updated

7 years ago
Whiteboard: [good first bug][needs review dtownsend] → [needs updated patch]

Updated

7 years ago
Duplicate of this bug: 597970
(Assignee)

Comment 9

7 years ago
Created attachment 496356 [details] [diff] [review]
patch v2
Attachment #485949 - Attachment is obsolete: true
Attachment #496356 - Flags: review?(dtownsend)
(Assignee)

Updated

7 years ago
Whiteboard: [needs updated patch] → [needs review dtownsend]
(Reporter)

Comment 10

7 years ago
Comment on attachment 496356 [details] [diff] [review]
patch v2

Nice work, looks good
Attachment #496356 - Flags: review?(dtownsend) → review+
(Reporter)

Updated

7 years ago
Whiteboard: [needs review dtownsend]
(Assignee)

Comment 11

7 years ago
http://hg.mozilla.org/mozilla-central/rev/d3ca4963f4b2
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Updated

7 years ago
Keywords: dev-doc-needed
(Reporter)

Updated

7 years ago
Depends on: 628703
(Reporter)

Comment 12

7 years ago
Posted some docs here https://developer.mozilla.org/en/Addons/Working_with_AMO
Keywords: dev-doc-needed → dev-doc-complete
(Reporter)

Updated

7 years ago
Target Milestone: --- → mozilla2.0b8
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: in-litmus-

Updated

5 years ago
Blocks: 762929
You need to log in before you can comment on or make changes to this bug.