Closed Bug 260364 Opened 17 years ago Closed 10 years ago

Fixup radiogroups to use new .value functionality of radio.xml

Categories

(Core :: XUL, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

Details

Attachments

(7 files, 3 obsolete files)

Now that the fix to bug 250817 has landed there are certain parts of the code
that could hopefully be simplified now.
Status: NEW → ASSIGNED
Assignee: jag → bugzilla
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Attached patch Patch to cookieSplinter Review
Fixes radiogroup JS in p3p.xul
Attached patch Patch to venkmanSplinter Review
Updates venkman-bpprops.js
Attachment #159401 - Attachment description: Patch to cookies → Patch to cookie
Attached patch Patch to security (obsolete) — Splinter Review
Updates editcerts.js, editemailcert.xul, editsslcert.xul, pref-masterpass.js
and pref-masterpass.xul
Attached patch Patch to mailnews (obsolete) — Splinter Review
Patches askSendFormat.js, importDialog.js, am-addressing.js, aw-server.js,
smtpEditOverlay.js, searchTermOverlay.js and am-smime.js
Attached patch Patch to mailSplinter Review
Patches am-smime.js and pref-masterpass.xul in mail tree
Attached patch Patches to xpfeSplinter Review
Patches pref-applications-edit.xul
Attachment #159652 - Flags: review?(mscott)
Comment on attachment 159653 [details] [diff] [review]
Patches to xpfe

r+sr=me on attachment 159653 [details] [diff] [review] from Neil via IRC
Attachment #159653 - Flags: superreview+
Attachment #159653 - Flags: review+
Comment on attachment 159625 [details] [diff] [review]
Patch to security

for moa
Attachment #159625 - Flags: review?(kaie)
Attachment #159647 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 159401 [details] [diff] [review]
Patch to cookie

r+sr=neil via IRC
Attachment #159401 - Flags: superreview+
Attachment #159401 - Flags: review+
Attachment #159401 - Flags: approval1.8a4?
Comment on attachment 159653 [details] [diff] [review]
Patches to xpfe

Requesting approval for simple patch
Attachment #159653 - Flags: approval1.8a4?
Attachment #159479 - Flags: review?(rginda)
Comment on attachment 159479 [details] [diff] [review]
Patch to venkman

if this is going to break venkman in older builds, I don't see the point.
Attachment #159479 - Flags: review?(rginda) → review-
Comment on attachment 159653 [details] [diff] [review]
Patches to xpfe

a=asa for checkin to 1.8a4.
Attachment #159653 - Flags: approval1.8a4? → approval1.8a4+
Patches to xpfe checked in:
Checking in
mozilla/xpfe/components/prefwindow/resources/content/pref-applications-edit.xul;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-applications-edit.xul,v
 <--  pref-applications-edit.xul
new revision: 1.51; previous revision: 1.50
done
Comment on attachment 159401 [details] [diff] [review]
Patch to cookie

a=asa for 1.8a4 checkin.
Attachment #159401 - Flags: approval1.8a4? → approval1.8a4+
Patch to cookie checked in:
Checking in p3p.xul;
/cvsroot/mozilla/extensions/cookie/resources/content/p3p.xul,v  <--  p3p.xul
new revision: 1.14; previous revision: 1.13
done
Comment on attachment 159647 [details] [diff] [review]
Patch to mailnews

>-    else if ( gSearchBooleanRadiogroup.selectedItem.value == 'and')
>+    else if ( gSearchBooleanRadiogroup.value == 'and')
Nit: scope for whitespace cleanup

> function getBooleanAnd()
> {
>-    if (gSearchBooleanRadiogroup.selectedItem)
>-        return (gSearchBooleanRadiogroup.selectedItem.getAttribute("value") == "and") ? true : false;
>+    if (gSearchBooleanRadiogroup.value)
>+        return (gSearchBooleanRadiogroup.value == "and") ? true : false;
> 
>     // default to false
>     return false;
> }
gSearchBooleanRadiogroup.value == "and" already returns true or false; this
entire function call could be inlined into that expression.

askSendFormat needs a separate overhaul; making the radiobutton values
correspond to those in nsIMsgCompose.idl for nsIMsgCompSendFormat would
simplify the code enormously. r=me on everything else with the nits fixed.
Attachment #159647 - Flags: review?(neil.parkwaycc.co.uk) → review+
Takes on board Neil's comments, patch does not have askSendFormat.js changes
Attachment #159647 - Attachment is obsolete: true
Comment on attachment 160396 [details] [diff] [review]
Patch for mailnews v0.2

Carrying forward r=
Attachment #160396 - Flags: superreview?(bienvenu)
Attachment #160396 - Flags: review+
Attached patch Patch to askSendFormat (obsolete) — Splinter Review
Takes on board Neil's suggestions of overhaul of askSendFormat.js changing the
value for plainTextAndHtml in askSendFormat.xul from 0 to 3 to simplify the
coding.
Attachment #160396 - Flags: superreview?(bienvenu) → superreview+
Checking patch for mailnews v0.2
Checking in base/prefs/resources/content/aw-server.js;
/cvsroot/mozilla/mailnews/base/prefs/resources/content/aw-server.js,v  <-- 
aw-server.js
new revision: 1.43; previous revision: 1.42
done
Checking in base/prefs/resources/content/am-addressing.js;
/cvsroot/mozilla/mailnews/base/prefs/resources/content/am-addressing.js,v  <--
am-addressing.js
new revision: 1.8; previous revision: 1.7
done
Checking in base/prefs/resources/content/smtpEditOverlay.js;
/cvsroot/mozilla/mailnews/base/prefs/resources/content/smtpEditOverlay.js,v  <--
  smtpEditOverlay.js
new revision: 1.27; previous revision: 1.26
done
Checking in base/search/resources/content/searchTermOverlay.js;
/cvsroot/mozilla/mailnews/base/search/resources/content/searchTermOverlay.js,v
<--  searchTermOverlay.js
new revision: 1.37; previous revision: 1.36
done
Checking in extensions/smime/resources/content/am-smime.js;
/cvsroot/mozilla/mailnews/extensions/smime/resources/content/am-smime.js,v  <--
 am-smime.js
new revision: 1.14; previous revision: 1.13
done
Checking in import/resources/content/importDialog.js;
/cvsroot/mozilla/mailnews/import/resources/content/importDialog.js,v  <-- 
importDialog.js
new revision: 1.38; previous revision: 1.37
done
Attachment #160404 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 160404 [details] [diff] [review]
Patch to askSendFormat

This doesn't quite work because param.action is compared identically to an
integer in a switch statement (bypassing automatic conversions). You could
either convert the radiogroup value to an integer before assinging it to
param.action or change MsgComposeCommands to use a type-insensitive comparison.
Attachment #160404 - Flags: review?(neil.parkwaycc.co.uk) → review-
Uses parseInt to make param.action be an integer
Attachment #160404 - Attachment is obsolete: true
Attachment #162007 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 162007 [details] [diff] [review]
Patch to askSendFormat v0.2

I had hoped you'd change the caller ;-)
Attachment #162007 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #162007 - Flags: superreview?(bienvenu)
Comment on attachment 162007 [details] [diff] [review]
Patch to askSendFormat v0.2

       if (useDefault)
-	 radioSelect=defaultAction;
+	 format = defaultAction;
       else
-	 radioSelect=param.action;
+	 format = param.action;

now, can't this just be:

var format = (useDefault) ? defaultAction : param.action;

modulo that nit, sr=bienvenu
Attachment #162007 - Flags: superreview?(bienvenu) → superreview+
askSendFormat changes checked in with nit addressed.
Checking in askSendFormat.xul;
/cvsroot/mozilla/mailnews/compose/resources/content/askSendFormat.xul,v  <-- 
askSendFormat.xul
new revision: 1.23; previous revision: 1.22
done
Checking in askSendFormat.js;
/cvsroot/mozilla/mailnews/compose/resources/content/askSendFormat.js,v  <-- 
askSendFormat.js
new revision: 1.21; previous revision: 1.20
done
Attachment #159652 - Flags: review?(mscott)
Comment on attachment 159652 [details] [diff] [review]
Patch to mail

Requesting r= on mail patch
Attachment #159652 - Flags: review?(mscott)
Attachment #159652 - Flags: review?(mscott) → review+
Checking in patch for mail:
Checking in components/prefwindow/content/pref-masterpass.xul;
/cvsroot/mozilla/mail/components/prefwindow/content/pref-masterpass.xul,v  <-- 
pref-masterpass.xul
new revision: 1.4; previous revision: 1.3
done
Checking in extensions/smime/content/am-smime.js;
/cvsroot/mozilla/mail/extensions/smime/content/am-smime.js,v  <--  am-smime.js
new revision: 1.5; previous revision: 1.4
done
Comment on attachment 159625 [details] [diff] [review]
Patch to security

Your change looks correct.

r=kaie assuming you have tested it still works.
Attachment #159625 - Flags: review?(kaie) → review+
Changes since last version:
* Unbitrotted it.
* Fixed an incorrect = vs == which I spotted during testing elsewhere in the file.
Attachment #159625 - Attachment is obsolete: true
Attachment #537342 - Flags: review?(kaie)
Comment on attachment 537342 [details] [diff] [review]
Unbitrotted pki patch [Checked in: trunk comment 32]

r=kaie

Thanks a lot, especially for finding the =/== issue!
Attachment #537342 - Flags: review?(kaie) → review+
Ian, if for any reason you cannot land the "pki" patch very soon, let's at least land the correctness fix.

If you do land this soon, then please mark bug 663362 fixed, too.
(I've filed that bug primarily for landing it on other branches.)
Thanks!
Attachment #537342 - Attachment description: Unbitrotted pki patch → Unbitrotted pki patch [Checked in: trunk comment 32]
Comment on attachment 537342 [details] [diff] [review]
Unbitrotted pki patch [Checked in: trunk comment 32]

http://hg.mozilla.org/mozilla-central/rev/b7502e9fbd30
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.