Split off from bug 238554: - Lifetime Policy groupbox should disable when Block is selected. - change for="lifetimeDays" into control="lifetimeDays" - consider renaming Cookie Lifetime Policy to Retention Policy

Well, this didn't make 1.8a ;-)

Does changes as per comment 0

Comment on attachment 173130 [details] [diff] [review] Cookie Pref disabling patch v0.1 I found the original disabled checks way easier to read. Any reason why you switched style?

The problem with the old code was the elements were being enabled and then disabled again later on in the code. Just adding the three lines required to do the disabling for cookieLifetime does make for a smaller patch but I did find the old code harder to understand than the new code. When doing this sort of coding elsewhere within the tree, the way the new code does things is the way I've been asked to do it.

It looks to me that the logic is reversed. original: if A is selected, B and C should be disabled new: B should be disabled if A is selected; C should be disabled if A is selected (not entirely fair, the number of lines is about the same) But if this way is common, i'm ok with it.

Thinking about it, i really don't like the reversed logic.

Changes since v0.1 * Revised how alwaysAcceptSession and lifetimeDays are disabled so cookieLifetime checking logic is not reversed.

Changes since v0.1a * Revised logic as per mvl's suggestion - I was assuming hierchial pref locking and shouldn't have.

Comment on attachment 173866 [details] [diff] [review] Updated cookie pref disabling patch v0.1b (Checked in) - Backed out r=mvl

Comment on attachment 173866 [details] [diff] [review] Updated cookie pref disabling patch v0.1b (Checked in) - Backed out Simple low risk patch - requesting a=

Comment on attachment 173866 [details] [diff] [review] Updated cookie pref disabling patch v0.1b (Checked in) - Backed out a=asa for checkin to 1.8b

Nice to see that most of my points from bug 238554 are being addressed in this patch, but can you please change: <!ENTITY viewP3P.label "View Privacy Settings"> into: <!ENTITY viewP3P.label "View Privacy Settings...">

(In reply to comment #12) > Nice to see that most of my points from bug 238554 are being addressed in this > patch, but can you please change: > > <!ENTITY viewP3P.label "View Privacy Settings"> > into: > <!ENTITY viewP3P.label "View Privacy Settings..."> No, because that's incorrect usage of the ellipsis. The ellipsis is only appropriate when the button triggers an action which requires further action on the part of the user. This is not the case in a button that opens a viewer, etc.

Comment on attachment 173866 [details] [diff] [review] Updated cookie pref disabling patch v0.1b (Checked in) - Backed out Checking in content/pref-cookies.xul; /cvsroot/mozilla/extensions/cookie/resources/content/pref-cookies.xul,v <-- pref-cookies.xul new revision: 1.26; previous revision: 1.25 done Checking in locale/en-US/pref-cookies.dtd; /cvsroot/mozilla/extensions/cookie/resources/locale/en-US/pref-cookies.dtd,v <-- pref-cookies.dtd new revision: 1.11; previous revision: 1.10 done

(In reply to comment #13) > No, because that's incorrect usage of the ellipsis. The ellipsis is only > appropriate when the button triggers an action which requires further action on > the part of the user. This is not the case in a button that opens a viewer, etc. Oh, but it is not a viewer but a manager, the cookiemanager in this case. Also note that on other pref panels that open a manager use of them was allowed previously so what about consistency?

Further action can be taken, but is not _required_ in this case. See the following links for some HIG guidelines on this. http://developer.gnome.org/projects/gup/hig/2.0/menus-design.html http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dnwue/html/ch14d.asp

Why was this patch not superreviewed?

Comment on attachment 173866 [details] [diff] [review] Updated cookie pref disabling patch v0.1b (Checked in) - Backed out From Neil on irc: {#developers} IanN: you missed a semicolon on cookieLifetime.disabled = false {#developers} IanN: and I don't like that revised if/else for the disabling

Changes since v0.1b * Revised cookies_disabled part of code

Comment on attachment 173957 [details] [diff] [review] Tweaked cookie pref disabling patch v0.1c (Checked in) >+ alwaysAcceptSession.disabled = cookiesDisabled || (cookieLifetime.value != ask_before_accepting); > > if (parent.hPrefWindow.getPrefIsLocked(alwaysAcceptSession.getAttribute("prefstring")) ) > alwaysAcceptSession.disabled = true; I hope mvl is still ok with this version, only I notice that this is now looking suspiciously like attachment 173130 [details] [diff] [review] - all it needs is an extra || condition ;-)

Comment on attachment 173957 [details] [diff] [review] Tweaked cookie pref disabling patch v0.1c (Checked in) Requesting a= for revised, low risk patch

Comment on attachment 173957 [details] [diff] [review] Tweaked cookie pref disabling patch v0.1c (Checked in) a=asa for 1.8b checkin

Comment on attachment 173957 [details] [diff] [review] Tweaked cookie pref disabling patch v0.1c (Checked in) Checking in content/pref-cookies.xul; /cvsroot/mozilla/extensions/cookie/resources/content/pref-cookies.xul,v <-- pref-cookies.xul new revision: 1.28; previous revision: 1.27 done Checking in locale/en-US/pref-cookies.dtd; /cvsroot/mozilla/extensions/cookie/resources/locale/en-US/pref-cookies.dtd,v <-- pref-cookies.dtd new revision: 1.13; previous revision: 1.12 done

Ian, is there more work to do here? The last patch landed a few months back.

Ooops, forgot to resolve :-S

Issues in comment 0 Verified FIXED with build 2005-05-05-06 on Windows XP Seamonkey trunk.