Closed Bug 436730 Opened 12 years ago Closed 12 years ago

Migrate SeaMonkey's Cookies preferences to new pref window

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a1

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Patch for pref-cookies v0.1 (obsolete) — Splinter Review
No description provided.
Attachment #323252 - Flags: superreview?(neil)
Attachment #323252 - Flags: review?(mnyromyr)
Comment on attachment 323252 [details] [diff] [review]
Patch for pref-cookies v0.1

>+    <script type="application/x-javascript"
>+            src="chrome://communicator/content/permissions/permissionsOverlay.js"/>
Does this have to be here, rather than in preferences.xul?

>+                   size="4"
>+                   maxlength="3"
Maybe size="3" too?

>+  var button = document.getElementById("viewCookieButton");
>+  var pref = document.getElementById(button.getAttribute("preference"));
Unnecessary.

>+const cookies_disabled = "2";
>+ 
>+const accept_for_n_days = "3";
>+const ask_before_accepting = "1";
Can you make these be numbers rather than strings?

>+  if (!days.disabled && setFocus)
>+    days.focus();
I vaguely remember commenting somewhere else about the problem with this and setting the preference in about:config while the prefwindow is open...

>+function SetButton(aDisable)
>+{
>+  document.getElementById("viewCookieButton").disabled = aDisable;
>+}
I don't see the point of this at all, once a preference is locked it's locked.
Depends on: 436800
Attachment #323252 - Flags: superreview?(neil)
Attachment #323252 - Flags: review?(mnyromyr)
Attached patch Patch for pref-cookies v0.2 (obsolete) — Splinter Review
Changes since v0.1:
* Moved permissionsOverlay.js to preferences.xul and made the required changes to pref-images.xul and pref-smartupdate.xul
* Changed size to 3 on textbox - assuming 999 days will be enough.
* Removed unnecessary code for disabling button
* Changed constants from strings to numbers
Attachment #323252 - Attachment is obsolete: true
Attachment #323320 - Flags: superreview?(neil)
Attachment #323320 - Flags: review?(mnyromyr)
Attachment #323320 - Flags: superreview?(neil) → superreview+
Comment on attachment 323320 [details] [diff] [review]
Patch for pref-cookies v0.2

Just some nits:

>Index: pref-cookies.xul
>===================================================================
>+        <radio value="2" label="&disableCookies.label;"
>+               accesskey="&disableCookies.accesskey;"/>

One attribute per line, please, if you need more than one line.
(Several more of that in the rest of the file.)

>+          <radio value="3" label="&acceptforNDays.label;"
>+                 accesskey="&acceptforNDays.accesskey;"/>
>+          <textbox id="lifetimeDays"
>+                   type="number"
>+                   max="999"
>+                   min="0"
>+                   size="3"
>+                   maxlength="3"
>+                   preference="network.cookie.lifetime.days"/>
>+          <label value="&days.label;" control="lifetimeDays"/>

You may need some a a11y attributes here (control, aria-labelledby). (Ask MarcoZ?)

>Index: pref-cookies.js
>===================================================================

You forgot to patch jar.mn... ;-)

>+const cookies_disabled = 2;
>+ 
>+const accept_for_n_days = 3;
>+const ask_before_accepting = 1;

These should be all UPPERCASE NAMES.

>+function SetDisables(setFocus)

Parameters should have an 'a' prefix.
Attachment #323320 - Flags: review?(mnyromyr) → review-
Attached patch Patch for pref-cookies v0.3 (obsolete) — Splinter Review
Changes since v0.2:
* Added changes for jar.mn
* Added aria-labelledby to parts of xul
* kName format used for const
* One attribute per line in xul
* Parameters now start with 'a' prefix

Carrying for sr= and requesting r=
Attachment #323320 - Attachment is obsolete: true
Attachment #325179 - Flags: superreview+
Attachment #325179 - Flags: review?(mnyromyr)
Comment on attachment 325179 [details] [diff] [review]
Patch for pref-cookies v0.3

>Index: pref/pref-cookies.xul
>===================================================================
>+          <radio id="acceptForNDays"
>+                 value="3"
>+                 label="&acceptforNDays.label;"
>+                 accesskey="&acceptforNDays.accesskey;"
>+                 aria-labelledby="acceptForNDays lifetimeDays daysLabel"/>
>+          <textbox id="lifetimeDays"
>+                   type="number"
>+                   max="999"
>+                   min="0"
>+                   size="3"
>+                   maxlength="3"
>+                   preference="network.cookie.lifetime.days"
>+                   aria-labelledby="acceptForNDays lifetimeDays daysLabel"/>
>+          <label id="daysLabel"
>+                 value="&days.label;"/>

The label is part of the "control group" and thus also needs the aria-labelledby attribute.

>Index: pref/pref-cookies.js
>===================================================================
>+function Startup()
>+{
>+  SetDisables(false);
>+}
>+
>+const kCookiesDisabled = 2;
>+ 
>+const kAcceptForNDays = 3;
>+const kAskBeforeAccepting = 1;
>+
>+function SetDisables(aSetFocus)
>+{

Only constants used in several functions justify being defined globally (and then should be defined at the top of the file's code).
So move them to the top inside of SetDisables, sorted by value.

r=me with that.
Attachment #325179 - Flags: review?(mnyromyr) → review+
Changes since v0.3:
* Moved const declarations into SetDisables function as requested

Having discussed aria-labelledby on IRC agreed with mnyromyr that label element does not need that attribute.

Carrying forward r/sr
Attachment #325179 - Attachment is obsolete: true
Attachment #325201 - Flags: superreview+
Attachment #325201 - Flags: review+
Comment on attachment 325201 [details] [diff] [review]
Patch for pref-cookies v0.3a (Checked in)

Checking into trunk
jar.mn;
new revision: 1.50; previous revision: 1.49
pref/pref-cookies.xul;
new revision: 1.42; previous revision: 1.41
pref/pref-cookies.js;
initial revision: 1.1
pref/pref-images.xul;
new revision: 1.16; previous revision: 1.15
pref/pref-smartupdate.xul;
new revision: 1.53; previous revision: 1.52
pref/preferences.xul;
new revision: 1.19; previous revision: 1.18
pref/preftree.xul;
new revision: 1.121; previous revision: 1.120
done
Attachment #325201 - Attachment description: Patch for pref-cookies v0.3a → Patch for pref-cookies v0.3a (Checked in)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.