Last Comment Bug 639968 - Add checkbox to Software Installation preferences to opt out of personalized add-on recommendations
: Add checkbox to Software Installation preferences to opt out of personalized ...
Status: RESOLVED FIXED
: privacy
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b3
Assigned To: rsx11m
:
Mentors:
Depends on: 606482
Blocks: 641252
  Show dependency treegraph
 
Reported: 2011-03-08 12:45 PST by rsx11m
Modified: 2011-03-13 16:25 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (8.02 KB, patch)
2011-03-08 12:51 PST, rsx11m
no flags Details | Diff | Review
Proposed patch (v2) (7.93 KB, patch)
2011-03-09 10:12 PST, rsx11m
no flags Details | Diff | Review
Proposed patch (v3) (8.19 KB, patch)
2011-03-09 11:50 PST, rsx11m
no flags Details | Diff | Review
Patch after bug 606482 (v7) (8.09 KB, patch)
2011-03-10 10:49 PST, rsx11m
neil: ui‑review-
Details | Diff | Review
Patch (v4) after bug 606482 (v8) (7.83 KB, patch)
2011-03-11 09:08 PST, rsx11m
neil: ui‑review+
Details | Diff | Review
Patch (v5) after bug 606482 (v8) (7.89 KB, patch)
2011-03-11 10:07 PST, rsx11m
rsx11m.pub: ui‑review+
Details | Diff | Review
Patch (v6) after bug 606482 (v8) (7.67 KB, patch)
2011-03-11 11:33 PST, rsx11m
no flags Details | Diff | Review
Patch (v7) after bug 606482 (v8) (6.81 KB, patch)
2011-03-11 11:43 PST, rsx11m
rsx11m.pub: ui‑review+
Details | Diff | Review
Proposed patch (v8) (9.75 KB, patch)
2011-03-12 16:27 PST, rsx11m
iann_bugzilla: review+
rsx11m.pub: ui‑review+
Details | Diff | Review
Screenshot of (v8) (15.08 KB, image/png)
2011-03-12 16:33 PST, rsx11m
no flags Details
Final patch [Checkin: comment 34] (10.65 KB, patch)
2011-03-13 15:44 PDT, rsx11m
rsx11m.pub: review+
rsx11m.pub: ui‑review+
Details | Diff | Review

Description rsx11m 2011-03-08 12:45:13 PST
Given that toolkit is likely no longer accepting any string changes for Gecko 2.0, but on the other hand making the extensions.getAddons.cache.enabled pref accessible for users who don't want to submit their list of installed add-ons to AMO to identify personalized recommendations, I'm filing this for SM 2.1 to add a checkbox to the Software Installation preference pane instead. Even if toolkit adds it to the options menu of the add-on manager itself, redundancy
in this case cannot hurt and is given by the add-on update settings anyway. This addition should be minimal and safe for 2.1b3 and has l10n changes, thus nominating for blocking as stated in the meeting minutes.

(Quoting bug 639722 description)
> +++ This bug was initially created as a clone of Bug #635115 +++
> 
> Per bug 635115 comment #13, there is currently no UI element for easy opting
> out of the "Recommended for you" feature. This is a follow-up bug to introduce
> such a switch. Users concerned about privacy would like to disable any feature
> that appears to send user-profiling information to whatever provider (and it
> this case the feature actually does so). A hidden preference does not
> adequately address such concerns.
Comment 1 rsx11m 2011-03-08 12:51:53 PST
Created attachment 517844 [details] [diff] [review]
Proposed patch

This patch creates a dedicated "Add-on Manager" group and also moves the button to start the add-on manager from the "Updates" group into it (this seems to be a better match and leaves the "Updates" section more specific). The only other item in the new group is the checkbox for the new preference setting.

Note that I changed the button label from "Add-On Manager" to "Start Manager" to avoid many "Add-On" strings next to each other. Also, the "Start" context is more explicit than just "Manage Add-Ons" (if followed the example in the first group) to indicate that an action outside the current dialog will be happening once the button is pushed. The help content has been updated respectively.
Comment 2 neil@parkwaycc.co.uk 2011-03-09 08:59:50 PST
(In reply to comment #1)
> Note that I changed the button label from "Add-On Manager" to "Start Manager"
> to avoid many "Add-On" strings next to each other. Also, the "Start" context is
> more explicit than just "Manage Add-Ons" (if followed the example in the first
> group) to indicate that an action outside the current dialog will be happening
> once the button is pushed. The help content has been updated respectively.
Sorry, but the "Start" just doesn't appeal to me. I'd be happy with just "Manage Add-Ons" though. Since it doesn't open in a dialog we can't even use ... to show that, but maybe we should be using a text-link instead; this would be a better indication that the action applies in the browser rather than in the dialog.
Comment 3 rsx11m 2011-03-09 10:12:27 PST
Created attachment 518100 [details] [diff] [review]
Proposed patch (v2)

Updated patch, now (1) using a text link rather than a button and (2) with the label text changed according to comment #2.

Note that there is some discussion in the companion bugs that the pref also has other functions (see bug 639722 comment #4 and following as well as bug 640033 comment #3). Per Joe's link in the latter comment, the pref implies disabling meta checks, but that appears to be considered consistent in the last section of that page for users who want to limit broadcasting installation details.

I've kept the label the same, it may be simplifying the actual function but in general reflects the intention. I could expand on it in the help text a little more if desired.
Comment 4 rsx11m 2011-03-09 11:50:19 PST
Created attachment 518134 [details] [diff] [review]
Proposed patch (v3)

Fixed a few minor things vs. (v2):
 - Capitalization of labels now matches the rest of the pane;
 - expanded the help text as proposed before;
 - corrected a URL typo in the "toEM()" call as a drive-by fix.
Comment 5 Jens Hatlak (:InvisibleSmiley) 2011-03-09 12:32:37 PST
@Ian/Neil: Note that the changes here at least partly conflict with bug 606482 (which already has a positive review and an open ui-review request for Neil).

(In reply to comment #4)
>  - corrected a URL typo in the "toEM()" call as a drive-by fix.

I already found that in bug 606482 comment 17. Edmund added it to the next patch but in later ones, including the current one, the change is gone again. I guess only the result counts. ;-)
Comment 6 rsx11m 2011-03-09 13:01:08 PST
Oh well, the magic of bitrotting... :-)

I don't really see those two patches conflicting other than that one will bitrot the other. The two major changes proposed here are:

 - adding a checkbox for the "submit installed add-ons to site" pref;
 - changing the "Manage Add-ons" button into a link.

Both can be done incremental to Edmund's patch, replacing the current startAddonManager button with these two UI items, or Edmund can incorporate
any changes made here into his patch. Neil's call...
Comment 7 rsx11m 2011-03-10 10:49:03 PST
Created attachment 518442 [details] [diff] [review]
Patch after bug 606482 (v7)

While I'm on it, this is the patch on top of attachment 515267 [details] [diff] [review].

It's simpler than the previous patch as Edmund already splits off the separate add-on group, so this leaves only the need for making the additional changes.
I've added the new checkbox just underneath allowing add-on installation, given that the context is still on installation rather than update here. I left the link (previously button) to start the add-on manager at the bottom right of that group, but changed its id to indicate the change of UI element here.

This should be suitable for Neil's ui-review, but with further changes pending in bug 606482, the technical review may only be useful after the patch there has been finalized.
Comment 8 rsx11m 2011-03-10 10:56:03 PST
BTW: Both attachment 518134 [details] [diff] [review] and attachment 518442 [details] [diff] [review] have the
same patch for help content as attachment 515267 [details] [diff] [review] doesn't update it.
Comment 9 neil@parkwaycc.co.uk 2011-03-11 08:33:45 PST
Comment on attachment 518442 [details] [diff] [review]
Patch after bug 606482 (v7)

The new checkbox looks out of place where you've put it. I think it would look better on the same line as the link (which I like).
Comment 10 rsx11m 2011-03-11 09:08:10 PST
Created attachment 518738 [details] [diff] [review]
Patch (v4) after bug 606482 (v8)

This patch goes on top of attachment 518702 [details] [diff] [review] which has all approvals.
Hence, I'm also canceling review requests for (v3) here per comment #5.

Neil, you weren't specific whether or not to keep it in the context of the top-most checkbox, thus I kept the indent and also disable the box when XPinstall is unchecked. Let me know if you want to flush it all the way to
the left and if disabling it is the right thing to do here, easy to modify.

I'm not sure what to do with the help content, it seems to require a major overhaul (especially given that the Data Manager now takes care of the install permissions, thus the entire description is outdated as is and needs updates). If this has time after the string freeze, I'd suggest to spin this off to a separate bug and just add the current text as a place-holder for now, thus to do a thorough update of the help text with all considerations as needed.
Comment 11 neil@parkwaycc.co.uk 2011-03-11 09:41:44 PST
Comment on attachment 518738 [details] [diff] [review]
Patch (v4) after bug 606482 (v8)

>+  document.getElementById("enablePersonalized").disabled =
>+    addOnsCheck;
>+
>   document.getElementById("addOnsUpdatesEnabled").disabled =
>     addOnsCheck ||
>     document.getElementById("extensions.update.enabled").locked;
You need to account for the locked case...

>+        <hbox class="indent">
I have a slight preference for no indent.
Comment 12 rsx11m 2011-03-11 09:45:33 PST
(In reply to comment #11)
> Comment on attachment 518738 [details] [diff] [review]
> Patch (v4) after bug 606482 (v8)
> 
> >+  document.getElementById("enablePersonalized").disabled =
> >+    addOnsCheck;
> >+
> >   document.getElementById("addOnsUpdatesEnabled").disabled =
> >     addOnsCheck ||
> >     document.getElementById("extensions.update.enabled").locked;
> You need to account for the locked case...
> 
> >+        <hbox class="indent">
> I have a slight preference for no indent.
Comment 13 rsx11m 2011-03-11 09:48:10 PST
Oops, sorry for that empty reply, it went off before I had a chance to respond.

(In reply to comment #11)
> You need to account for the locked case...

You mean extensions.update.enabled? This doesn't seem to apply here, it's not for updates but for new installations.

> I have a slight preference for no indent.

Ok, but keeping the box disabled when disabling software installation, yes?
Comment 14 rsx11m 2011-03-11 10:07:32 PST
Created attachment 518752 [details] [diff] [review]
Patch (v5) after bug 606482 (v8)

Ok, got it - the new preference itself can be locked, done based on the example of the update option. It remains disabled when disabling software installation, otherwise you wouldn't have made the first request... ;-)

Also removed the indentation of that checkbox, looks better - I agree.

Carrying forward ui-review+ from attachment 518738 [details] [diff] [review], waiting for Ian.
Comment 15 neil@parkwaycc.co.uk 2011-03-11 10:08:48 PST
(In reply to comment #13)
> (In reply to comment #11)
> > You need to account for the locked case...
> You mean extensions.update.enabled? This doesn't seem to apply here, it's not
> for updates but for new installations.
No, I mean extensions.getAddons.cache.enabled

> > I have a slight preference for no indent.
> Ok, but keeping the box disabled when disabling software installation, yes?
Well, if we went with no indent, then I wouldn't disable the box. But I'd have to check with Mossop/Unfocused to see whether that makes sense, that is to say, would the Addons Manager still suggest add-ons when installation is disabled.
Comment 16 rsx11m 2011-03-11 10:13:46 PST
Actually, the add-ons manager apparently remains fully active even with software installation disabled, it will stop you once you actually try to install an add-on. Thus, I think keeping it active all the time makes sense.
Comment 17 rsx11m 2011-03-11 11:33:05 PST
Created attachment 518770 [details] [diff] [review]
Patch (v6) after bug 606482 (v8)

Ok, so here is how it works after some more testing:

 - Add-on recommendations are active regardless of xpinstall.enabled and the
   respective discovery pane shown as long as we don't disable personalization;
 - if you click on a suggested add-on, you are brought to its AMO page;
 - clicking the installation button there opens a dialog, saying it's currently
   not allowed and offering you to enable software installation at that time.

Consequently, I've updated the patch to keep the checkbox active independently unless the underlying preference is locked (also a minor white-space change).

This still needs attachment 518702 [details] [diff] [review] until bug 606482 is checked in.
Comment 18 neil@parkwaycc.co.uk 2011-03-11 11:39:05 PST
Comment on attachment 518770 [details] [diff] [review]
Patch (v6) after bug 606482 (v8)

>+  document.getElementById("enablePersonalized").disabled =
>+    document.getElementById("extensions.getAddons.cache.enabled").locked;
In that case you can ditch this change, because the preference element automatically disables the control if it is locked. It's only if you have handlers that update the status based on other controls that you have to think about the locked status as well, to avoid erroneously overriding it.
Comment 19 rsx11m 2011-03-11 11:43:00 PST
Created attachment 518774 [details] [diff] [review]
Patch (v7) after bug 606482 (v8)

Updated per comment #18 with explicit locked-check removed.
Comment 20 Jens Hatlak (:InvisibleSmiley) 2011-03-12 11:08:46 PST
(In reply to comment #10)
> I'm not sure what to do with the help content, it seems to require a major
> overhaul (especially given that the Data Manager now takes care of the install
> permissions, thus the entire description is outdated as is and needs updates).
> If this has time after the string freeze, I'd suggest to spin this off to a
> separate bug and just add the current text as a place-holder for now, thus to
> do a thorough update of the help text with all considerations as needed.

I had waited a bit to see whether Ian would reply to this, but before this gets lost I'll add my 2c:

Generally Help is exempted from l10n freeze and may even be modified on stable branches as long as no new files are introduced. Still I'd like to at least achieve that all Preferences panel descriptions are up-to-date ASAP.

Since bug 606482 already modified the Software Installation panel I propose to create a Help bug to cover both that and this bug regarding Preferences and make it a dependency of meta bug 636993. If changes elsewhere in Help are needed wrt. the changes made here, please file yet another Help bug for that (without meta dependency). Thanks.
Comment 21 rsx11m 2011-03-12 13:20:42 PST
My thinking too, thanks for confirming the l10n-freeze policy. I've just filed bug 641252 on updating the help content, where I'll leave the current text for the preference in this patch given that text will only have to be reordered then to reflect the new layout.
Comment 22 Jens Hatlak (:InvisibleSmiley) 2011-03-12 13:30:56 PST
(In reply to comment #2)
> Since it doesn't open in a dialog we can't even use
> ... to show that, but maybe we should be using a text-link instead; this would
> be a better indication that the action applies in the browser rather than in
> the dialog.

Sorry for being late, but I'm a bit concerned about consistency here. Yes, the Add-on Manager opens in a browser window (content area), but so does the Data Manager, so Allowed Websites would have to be a link, too, no? FWIW I don't think that "opens in the content area" qualifies for "use a link instead of a button", but that only "opens a website" would (cf. the Sync panel) but hey, I'm not a reviewer.

Oh, while you're at it, I think Show Update History needs an ellipsis at the end (given that it opens a dialog).
Comment 23 Jens Hatlak (:InvisibleSmiley) 2011-03-12 13:39:07 PST
Comment on attachment 518774 [details] [diff] [review]
Patch (v7) after bug 606482 (v8)

>+        <hbox>
>+          <checkbox id="enablePersonalized" flex="1"
>+                    label="&enablePersonalized.label;"
>+                    accesskey="&enablePersonalized.accesskey;"
>+                    preference="extensions.getAddons.cache.enabled"/>
>+          <label id="addonManagerLink" class="text-link"
>+                 onclick="toEM('addons://list/extension');"
>+                 value="&addonManagerLink.label;"/>
>         </hbox>

Judging from what I see with the patch applied I'd say this hbox needs align="center" to make the link appear on the same vertical level as the checkbox and label.
Comment 24 neil@parkwaycc.co.uk 2011-03-12 13:54:18 PST
(In reply to comment #23)
> >+        <hbox>
> Judging from what I see with the patch applied I'd say this hbox needs
> align="center" to make the link appear on the same vertical level as the
> checkbox and label.
Good catch. It probably looked OK in the theme I was using; I should have checked both themes on one platform, if not multiple platforms.

(In reply to comment #22)
> Sorry for being late, but I'm a bit concerned about consistency here. Yes, the
> Add-on Manager opens in a browser window (content area), but so does the Data
> Manager, so Allowed Websites would have to be a link, too, no?
I was going to suggest fixing all the panels at once in a followup.

> FWIW I don't think that "opens in the content area" qualifies for "use a
> link instead of a button", but that only "opens a website" would (cf. the
> Sync panel) but hey, I'm not a reviewer.
My argument is that people would expect links to open in the browser, which this does, even though it's not technically a website.
Comment 25 rsx11m 2011-03-12 16:27:59 PST
Created attachment 518983 [details] [diff] [review]
Proposed patch (v8)

This patch addresses all of the previous comments relative to (v7):

 - "Allowed Websites" is now a link too (renamed id to indicate change)
 - align="center" has been added to the add-on <hbox>
 - ellipses appended to "Show Update History..."
Comment 26 rsx11m 2011-03-12 16:33:49 PST
Created attachment 518985 [details]
Screenshot of (v8)

Current appearance on Windows, for reference.
Comment 27 Jens Hatlak (:InvisibleSmiley) 2011-03-13 03:20:14 PDT
Comment on attachment 518983 [details] [diff] [review]
Proposed patch (v8)

>-<!ENTITY updateHistory.label                  "Show Update History">
>+<!ENTITY updateHistory.label                  "Show Update History…">

Entity value changes (other than typo corrections) require an entity name change so that localizers notice.

(No need to add another patch until you got review.)
Comment 28 rsx11m 2011-03-13 08:40:53 PDT
My first thinking was that the modification wasn't substantial enough to
warrant a revised entity name, but that's an implied context change, so I'd
agree with you on a second thought.
Comment 29 neil@parkwaycc.co.uk 2011-03-13 10:11:51 PDT
(In reply to comment #27)
> >-<!ENTITY updateHistory.label                  "Show Update History">
> >+<!ENTITY updateHistory.label                  "Show Update History…">
> Entity value changes (other than typo corrections) require an entity name
> change so that localizers notice.
> 
> (No need to add another patch until you got review.)
Assuming you get review ;-) I can't remember for sure but an ellipsis might not be required in this case since the opening of the dialog is the intended result (an example where an ellipsis is required would be the find or open dialog; the actual find or open takes effect once the dialog has been accepted).
Comment 30 rsx11m 2011-03-13 10:22:06 PDT
It's not quite the same as in "Set Default Browser" or "Restore Default" which perform an action and then they are done; the "..." in general indicates that another dialog window will open. So, maybe comparable to the "Add..." action
in the Browser > Languages or Mail > Send Format dialogs. But I see your point, it's sort of both in this case (dialog but done, or somewhere inbetween).
Comment 31 Ian Neal 2011-03-13 15:09:02 PDT
Comment on attachment 518983 [details] [diff] [review]
Proposed patch (v8)

>+          <label id="allowedSitesLink" class="text-link"
>+                 value="&allowedSitesLink.label;"
>+                 onclick="toDataManager('|permissions');"/>
Nit: one attribute per line please.

>+        <hbox align="center">
>+          <checkbox id="enablePersonalized" flex="1"
>+                    label="&enablePersonalized.label;"
>+                    accesskey="&enablePersonalized.accesskey;"
>+                    preference="extensions.getAddons.cache.enabled"/>
>+          <label id="addonManagerLink" class="text-link"
>+                 onclick="toEM('addons://list/extension');"
>+                 value="&addonManagerLink.label;"/>
Nit: one attribute per line please.

>+++ b/suite/locales/en-US/chrome/common/pref/pref-smartupdate.dtd	Sat Mar 12 18:20:49 2011 -0600
> <!ENTITY addOnsAllow.label                    "Allow websites to install add-ons and updates">
>-<!ENTITY addOnsAllow.accesskey                "b">
>-<!ENTITY allowedWebsites.label                "Allowed Websites">
>-<!ENTITY allowedWebsites.accesskey            "W">
>+<!ENTITY addOnsAllow.accesskey                "w">
I think "b" is still the better key.

> <!ENTITY addOnsModeAutomatic.label            "Automatically download and install the updates">
> <!ENTITY addOnsModeAutomatic.accesskey        "c">
As you are freeing up "m", you could use it here.

>+<!ENTITY enablePersonalized.label             "Personalize add-on recommendations">
>+<!ENTITY enablePersonalized.accesskey         "m">
What's wrong with using "P" here?

r=me with those addressed/fixed
Comment 32 rsx11m 2011-03-13 15:44:09 PDT
Created attachment 519056 [details] [diff] [review]
Final patch [Checkin: comment 34]

Modified as requested per comment #27 and comment #31; ui-r=Neil, r=IanN.
Comment 33 rsx11m 2011-03-13 15:46:02 PDT
(In reply to comment #31)
> >+<!ENTITY enablePersonalized.accesskey         "m">
> What's wrong with using "P" here?

That accesskey was originally occupied by the old "Manage Permissions" label, thus I went with "m" for the new checkbox and didn't notice/change it later.

Thanks for the reviews and the feedback, push for trunk please.
Comment 34 Jens Hatlak (:InvisibleSmiley) 2011-03-13 16:20:26 PDT
Comment on attachment 519056 [details] [diff] [review]
Final patch [Checkin: comment 34]

http://hg.mozilla.org/comm-central/rev/62f7bd45e71f

Note You need to log in before you can comment on or make changes to this bug.