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.

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.

(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.

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.

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.

@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. ;-)

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...

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.

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 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).

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 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.

(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.

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?

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.

(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.

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.

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 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.

Updated per comment #18 with explicit locked-check removed.

(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.

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.

(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 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.

(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.

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..."

Current appearance on Windows, for reference.

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.)

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.

(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).

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 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

Modified as requested per comment #27 and comment #31; ui-r=Neil, r=IanN.

(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 on attachment 519056 [details] [diff] [review] Final patch [Checkin: comment 34] http://hg.mozilla.org/comm-central/rev/62f7bd45e71f