As per our UI concept in bug 1421690 and (finally) following the spec guidelines (https://www.w3.org/TR/webstorage/#privacy) we'd like to merge the cookies settings into the "Site Data" section and move them to top-level. This also means removing individual cookie management from about:preferences.

Comment on attachment 8951213 [details] Bug 1432743 - Move cookie settings to the Site Data section and remove the cookies dialog from preferences. https://reviewboard.mozilla.org/r/220474/#review228234 ::: commit-message-27fd0:10 (Diff revision 2) > +more visibility and to unify site data and cookies. > + > +Our UR has shown that our differentiation between site data and cookies > +is not helpful to users and that they struggle with discovering the > +cookie settings that are hidden in the "custom history" menu. > + The commit message needs to mention that you're actually changing the control types for some of the cookie controls here, and why/how. And that some strings are changing. ::: browser/components/preferences/in-content/privacy.js:824 (Diff revision 2) > var menu = document.getElementById("keepCookiesUntil"); > > // enable the rest of the UI for anything other than "disable all cookies" > var acceptCookies = (pref.value != 2); > > + accept.value = pref.value; So... I may be misunderstanding this (in my defense: this pref is silly-hairy!) but it seems like both the third party cookie control and (now) this radio group are affected by the same preference value. This is confusing because the third-party cookie control recognizes 4 different values for the preference (0-3 inclusive). So it seems here we can write e.g. 3. But the radiogroup only has 2 values (0 and 2). As you noted on IRC, it also seems like we should be returning the int (stringified, perhaps) instead of the boolean. ::: browser/components/preferences/in-content/privacy.xul:110 (Diff revision 2) > - <description>&rememberActions.pre.label;<label > + <description>&dontrememberActions.pre.label;<label > class="text-link" id="historyRememberClear" > - >&rememberActions.clearHistory.label;</label>&rememberActions.middle.label;<label > + >&dontrememberActions.clearHistory.label;</label>&dontrememberActions.post.label;</description> Please check with flod if reusing the dontrememberActions strings here is OK. I realize that in English the strings are exactly the same, and they're in a separate <description>, but they relate to the previous sentence, which is different here from in the other case where we use these labels. In some language that may make a difference (e.g. whether the previous sentence had a negation or not). I know there's been other cases where we couldn't reuse a string for this reason.

Comment on attachment 8951213 [details] Bug 1432743 - Move cookie settings to the Site Data section and remove the cookies dialog from preferences. https://reviewboard.mozilla.org/r/220474/#review228234 > The commit message needs to mention that you're actually changing the control types for some of the cookie controls here, and why/how. And that some strings are changing. Good idea. > So... I may be misunderstanding this (in my defense: this pref is silly-hairy!) but it seems like both the third party cookie control and (now) this radio group are affected by the same preference value. > > This is confusing because the third-party cookie control recognizes 4 different values for the preference (0-3 inclusive). > > So it seems here we can write e.g. 3. But the radiogroup only has 2 values (0 and 2). As you noted on IRC, it also seems like we should be returning the int (stringified, perhaps) instead of the boolean. I agree on the silly hairy part, but I don't think we're changing anything except turning a 2-value checkbox into a 2-value radiogroup, so I'm not sure what's wrong about this (except from the correct nit that we should return the value). The third-party menulist indeed changes the pref more granularly but it should not affect the visual state because it can not change it to 2 (always block). > Please check with flod if reusing the dontrememberActions strings here is OK. I realize that in English the strings are exactly the same, and they're in a separate <description>, but they relate to the previous sentence, which is different here from in the other case where we use these labels. In some language that may make a difference (e.g. whether the previous sentence had a negation or not). I know there's been other cases where we couldn't reuse a string for this reason. This will be removed in bug 1436568, so I don't think it matters much. I'm going to drop the issue if it's okay for you.

(In reply to Johann Hofmann [:johannh] from comment #6) > > So... I may be misunderstanding this (in my defense: this pref is silly-hairy!) but it seems like both the third party cookie control and (now) this radio group are affected by the same preference value. > > > > This is confusing because the third-party cookie control recognizes 4 different values for the preference (0-3 inclusive). > > > > So it seems here we can write e.g. 3. But the radiogroup only has 2 values (0 and 2). As you noted on IRC, it also seems like we should be returning the int (stringified, perhaps) instead of the boolean. > > I agree on the silly hairy part, but I don't think we're changing anything > except turning a 2-value checkbox into a 2-value radiogroup, so I'm not sure > what's wrong about this (except from the correct nit that we should return > the value). > > The third-party menulist indeed changes the pref more granularly but it > should not affect the visual state because it can not change it to 2 (always > block). Sorry, I should have been more explicit. AIUI, we call readAcceptCookies for onsyncfrompreference. It returns the pref value raw, and then the preferences implementation takes that value and assigns it to the radio group's value property and/or attribute (depends on whether the xbl binding is bound or not). That might work in practice, but it's not really 'right' - the correct value is either 0 or 1 (the value attribute on the individual <radio> elements), and instead we're assigning some value between 0 and 3. Apparently validation in XBL / XUL is making this still do the right thing, but we shouldn't be relying on that. This is why the function used to return the boolean - which then got assigned into the checked property/attribute.

Yeah, that makes sense. I didn't really consider it a problem but it's better to be explicit about it. I'll update the patch. Thanks for being thorough!

Comment on attachment 8951213 [details] Bug 1432743 - Move cookie settings to the Site Data section and remove the cookies dialog from preferences. https://reviewboard.mozilla.org/r/220474/#review228566 Thanks!

Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/36be9b83bcc9 Move cookie settings to the Site Data section and remove the cookies dialog from preferences. r=Gijs

This bug was verified during Site Storage UI Redesign feature testing. I will mark as Verified fixed.