Last Comment Bug 541665 - Sync Suite pref-smartupdate.* with mozilla-central
: Sync Suite pref-smartupdate.* with mozilla-central
Status: RESOLVED FIXED
[parity-fx][parity-tb]
:
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Philip Chee
:
Mentors:
Depends on:
Blocks: 547931
  Show dependency treegraph
 
Reported: 2010-01-23 07:19 PST by Philip Chee
Modified: 2010-03-03 20:04 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 (12.08 KB, patch)
2010-01-23 07:23 PST, Philip Chee
iann_bugzilla: review-
Details | Diff | Review
Patch v1.1 (11.71 KB, patch)
2010-01-25 06:26 PST, Philip Chee
no flags Details | Diff | Review
Patch v1.2 Ignore legacy mode 2 in app.update.mode and treat it as a boolean. (11.10 KB, patch)
2010-01-28 20:34 PST, Philip Chee
neil: superreview-
Details | Diff | Review
Patch v1.3 Fixed Review Comments. (12.15 KB, patch)
2010-02-06 03:10 PST, Philip Chee
no flags Details | Diff | Review
Patch v1.4 Fix more review comments. (11.66 KB, patch)
2010-02-10 01:37 PST, Philip Chee
neil: superreview+
Details | Diff | Review
Patch v1.5 Remove support for 1.9.2 sr=Neil (12.58 KB, patch)
2010-02-22 06:09 PST, Philip Chee
iann_bugzilla: review+
philip.chee: superreview+
Details | Diff | Review

Description Philip Chee 2010-01-23 07:19:49 PST
1. Add UI for app.update.auto app.update.mode
2. Enable/disable the UI based on the "locked" states of associated preferences.
Comment 1 Philip Chee 2010-01-23 07:23:40 PST
Created attachment 423140 [details] [diff] [review]
Patch v1.0

Initial patch as proposed.
Comment 2 Ian Neal 2010-01-23 15:27:32 PST
Comment on attachment 423140 [details] [diff] [review]
Patch v1.0

> function Startup()
> {
>-  toggleFrequency("extensions");
>-  toggleFrequency("app");
>+  UpdateAddonsItems();
>+  UpdateAppItems();
>+  UpdateAutoItems();
>+  UpdateModeItems();
Drop the calls to UpdateAutoItems() and UpdateModeItems() from here, then have UpdateAppItems() call UpdateAutoItems() and have UpdateAutoItems() call UpdateModeItems().

>+function UpdateAddonsItems()
> {
>+  document.getElementById("extensionsUpdatesEnabled").disabled =
>+    !document.getElementById("xpinstall.enabled").value ||
>+    document.getElementById("extensions.update.enabled").locked;
>+
>+  var locked = !document.getElementById("extensions.update.enabled").value ||
>+               document.getElementById("extensions.update.interval").locked ||
>+               !document.getElementById("xpinstall.enabled").value;
>+  document.getElementById("extensionsFreqDaily").disabled = locked;
>+  document.getElementById("extensionsFreqWeekly").disabled = locked;
Why not have here:
var disable = !document.getElementById("xpinstall.enabled").value ||
              document.getElementById("extensions.update.enabled").locked;
document.getElementById("extensionsUpdatesEnabled").disabled = disable;

disable = disable || !document.getElementById("extensions.update.enabled").value;
document.getElementById("extensionsFreqDaily").disabled = disable;
document.getElementById("extensionsFreqWeekly").disabled = disable;
> }
>+
>+function UpdateAppItems()
>+{

>+
>+  var enabledPref = document.getElementById("app.update.enabled");
>+  var appUpdatesEnabled = document.getElementById("appUpdatesEnabled");
This variable is only used once, so just inline it.
>+
>+  appUpdatesEnabled.disabled = !canCheckForUpdates || enabledPref.locked;
>+
>+  var locked = !enabledPref.value ||
>+               document.getElementById("app.update.interval").locked ||
>+               !canCheckForUpdates;
Keep the var name consistent so use disable as the name here, also put the !canCheckForUpdates on the same line as !enabledPref.value.
>+
>+  document.getElementById("appFreqDaily").disabled = locked;
>+  document.getElementById("appFreqWeekly").disabled = locked;
Call UpdateAutoItems() at then end of this function.
>+}

>+function UpdateAutoItems()
>+{
>+  var enabledPref = document.getElementById("app.update.enabled");
>+  var autoPref = document.getElementById("app.update.auto");
>+  var updateModeLabel = document.getElementById("updateModeLabel");
>+  var updateMode = document.getElementById("updateMode");
These last three variables are used only once, so inline them.
>+
>+  var disable = enabledPref.locked ||
>+                !enabledPref.value ||
>+                autoPref.locked;
Put the two enabledPref on the same line.
>+  updateModeLabel.disabled = disable;
>+  updateMode.disabled = disable;
Call UpdateModeItems() from here.
>+}

>+function UpdateModeItems()
>+{
>+  var enabledPref = document.getElementById("app.update.enabled");
>+  var autoPref = document.getElementById("app.update.auto");
>+  var modePref = document.getElementById("app.update.mode");
>+  var warnIncompatible = document.getElementById("warnIncompatible");
These last two variables are only used once so inline them.
>+
>+  var disable = enabledPref.locked ||
>+                !enabledPref.value ||
>+                autoPref.locked ||
>+                !autoPref.value ||
>+                modePref.locked;
Put the two enabledPref and the two autoPref on the same lines.
>+  warnIncompatible.disabled = disable;
>+}

>+function ReadAddonWarn()
>+{
>+  var preference = document.getElementById("app.update.mode");
>+  var doNotWarn = preference.value != 0;
>+  gModePreference = doNotWarn ? preference.value : 1;
>+  return doNotWarn;
>+}

>+function WriteAddonWarn()
Use the following and just pass "this" as argument from the caller.
function WriteAddonWarn(aWarn)
{
  return !aWarn.checked ? 0 : gModePreference;
}

>+function ShowUpdateHistory()
>+{
>+  Components.classes["@mozilla.org/updates/update-prompt;1"]
>+             .createInstance(Components.interfaces.nsIUpdatePrompt)
>+             .showUpdateHistory(window);
Indentation appears to be incorrect.

>--- a/suite/common/pref/pref-smartupdate.xul

>+      <preference id="xpinstall.enabled"
>+                  name="xpinstall.enabled"
>+                  type="bool"
>+                  onchange="UpdateAddonsItems();"/>
>+      <preference id="extensions.update.enabled"
>+                  name="extensions.update.enabled"
>+                  type="bool"
>+                  onchange="UpdateAddonsItems();"/>
>       <preference id="extensions.update.interval"
>-                  name="extensions.update.interval" type="int"/>
>+                  name="extensions.update.interval"
>+                  type="int"/>
>+
>+      <preference id="app.update.enabled"
>+                  name="app.update.enabled"
>+                  type="bool"
>+                  onchange="UpdateAppItems();UpdateAutoItems();UpdateModeItems();"/>
Just call UpdateAppItems() here, as that will be sufficient with the changes previously mentioned.
>+      <preference id="app.update.auto"
>+                  name="app.update.auto"
>+                  type="bool"
>+                  onchange="UpdateAutoItems();UpdateModeItems();"/>
Just call UpdateAutoItems() here, as that will be sufficient with the changes previously mentioned.

>+    <groupbox>
>+      <caption id="updateModeLabel"
>+               label="&whenUpdatesFound.label;"
>+               control="updateMode"/>
>+      <radiogroup id="updateMode"
>+                  class="indent"
>+                  preference="app.update.auto">
>+        <radio id="ask"
>+               label="&askMe.label;"
>+               accesskey="&askMe.accesskey;"
>+               value="false"/>
>+        <radio id="automatic"
>+               label="&modeAutomatic.label;"
>+               accesskey="&modeAutomatic.accesskey;"
>+               value="true"/>
Are these ids unique enough? Might be better to be updateModeAsk and updateModeAutomatic, if they're needed at all.

>+        <hbox class="indent">
>+          <checkbox id="warnIncompatible"
>+                    label="&modeAutoAddonWarn.label;"
>+                    accesskey="&modeAutoAddonWarn.accesskey;"
>+                    flex="1"
>+                    preference="app.update.mode"
>+                    onsyncfrompreference="return this.pane.ReadAddonWarn();"
>+                    onsynctopreference="return this.pane.WriteAddonWarn();"/>
Pass "this" as an argument for WriteAddonWarn
Comment 3 Philip Chee 2010-01-25 06:26:21 PST
Created attachment 423343 [details] [diff] [review]
Patch v1.1

> Drop the calls to UpdateAutoItems() and UpdateModeItems() from here, then have
> UpdateAppItems() call UpdateAutoItems() and have UpdateAutoItems() call
> UpdateModeItems().
Fixed.

>>+function UpdateAddonsItems()
>> {
>>+  document.getElementById("extensionsUpdatesEnabled").disabled =
>>+    !document.getElementById("xpinstall.enabled").value ||
>>+    document.getElementById("extensions.update.enabled").locked;
>>+
>>+  var locked = !document.getElementById("extensions.update.enabled").value ||
>>+               document.getElementById("extensions.update.interval").locked ||
>>+               !document.getElementById("xpinstall.enabled").value;
>>+  document.getElementById("extensionsFreqDaily").disabled = locked;
>>+  document.getElementById("extensionsFreqWeekly").disabled = locked;

> Why not have here:
> var disable = !document.getElementById("xpinstall.enabled").value ||
>               document.getElementById("extensions.update.enabled").locked;
> document.getElementById("extensionsUpdatesEnabled").disabled = disable;
> 
> disable = disable ||
> !document.getElementById("extensions.update.enabled").value;
> document.getElementById("extensionsFreqDaily").disabled = disable;
> document.getElementById("extensionsFreqWeekly").disabled = disable;

One uses:
document.getElementById("extensions.update.enabled").locked;

The other uses:
!document.getElementById("extensions.update.enabled").value

Or are you saying that they are equivalent?
(Fix pending clarification...)

>>+  var appUpdatesEnabled = document.getElementById("appUpdatesEnabled");
> This variable is only used once, so just inline it.
Fixed.

>>+  var locked = !enabledPref.value ||
>>+               document.getElementById("app.update.interval").locked ||
>>+               !canCheckForUpdates;
> Keep the var name consistent so use disable as the name here, also put the
> !canCheckForUpdates on the same line as !enabledPref.value.
Fixed.

>>+  document.getElementById("appFreqDaily").disabled = locked;
>>+  document.getElementById("appFreqWeekly").disabled = locked;
> Call UpdateAutoItems() at then end of this function.
Fixed.

>>+  var autoPref = document.getElementById("app.update.auto");
>>+  var updateModeLabel = document.getElementById("updateModeLabel");
>>+  var updateMode = document.getElementById("updateMode");
> These last three variables are used only once, so inline them.
Fixed.

>>+  var disable = enabledPref.locked ||
>>+                !enabledPref.value ||
>>+                autoPref.locked;
> Put the two enabledPref on the same line.
Fixed.

>>+  updateModeLabel.disabled = disable;
>>+  updateMode.disabled = disable;
> Call UpdateModeItems() from here.
Fixed.

>>+  var modePref = document.getElementById("app.update.mode");
>>+  var warnIncompatible = document.getElementById("warnIncompatible");
> These last two variables are only used once so inline them.
Fixed.

>>+  var disable = enabledPref.locked ||
>>+                !enabledPref.value ||
>>+                autoPref.locked ||
>>+                !autoPref.value ||
>>+                modePref.locked;
>Put the two enabledPref and the two autoPref on the same lines.
Fixed.

>>+function WriteAddonWarn()
> Use the following and just pass "this" as argument from the caller.
> function WriteAddonWarn(aWarn)
> {
>   return !aWarn.checked ? 0 : gModePreference;
> }
Fixed.

>>+  Components.classes["@mozilla.org/updates/update-prompt;1"]
>>+             .createInstance(Components.interfaces.nsIUpdatePrompt)
>>+             .showUpdateHistory(window);
> Indentation appears to be incorrect.
Fixed.

>>+      <preference id="app.update.enabled"
>>+                  name="app.update.enabled"
>>+                  type="bool"
>>+                  onchange="UpdateAppItems();UpdateAutoItems();UpdateModeItems();"/>
> Just call UpdateAppItems() here, as that will be sufficient with the changes
> previously mentioned.
Fixed.

>>+      <preference id="app.update.auto"
>>+                  name="app.update.auto"
>>+                  type="bool"
>>+                  onchange="UpdateAutoItems();UpdateModeItems();"/>
> Just call UpdateAutoItems() here, as that will be sufficient with the changes
> previously mentioned.
Fixed.

>>+        <radio id="ask"
>>+        <radio id="automatic"
> Are these ids unique enough? Might be better to be updateModeAsk and
> updateModeAutomatic, if they're needed at all.
Fixed.

>>+          <checkbox id="warnIncompatible"
>>+                    label="&modeAutoAddonWarn.label;"
>>+                    accesskey="&modeAutoAddonWarn.accesskey;"
>>+                    flex="1"
>>+                    preference="app.update.mode"
>>+                    onsyncfrompreference="return this.pane.ReadAddonWarn();"
>>+                    onsynctopreference="return this.pane.WriteAddonWarn();"/>
> Pass "this" as an argument for WriteAddonWarn
Fixed.
Comment 4 Philip Chee 2010-01-28 20:34:21 PST
Created attachment 424182 [details] [diff] [review]
Patch v1.2 Ignore legacy mode 2 in app.update.mode and treat it as a boolean.

Changes since the last patch:

> + * The app.update.mode preference is converted into a true/false value for
> + * use in determining whether the "Warn me if this will disable extensions
> + * or themes" checkbox is checked. Unlike other toolkit applications we
> + * don't care about supporting legacy mode 2. 

[2010-01-28 22:51:45] <RattyAway> so just have 0 and 1?
[2010-01-28 22:51:51] <RattyAway> and ignore > 1
[2010-01-28 22:51:58] * NeilAway doesn't think the pref window needs to cope with power lusers setting bogus pref values
[2010-01-28 22:52:05] <RattyAway> or treat anything >1 as 1?
[2010-01-28 22:52:31] <NeilAway> RattyAway: treat non-0 as 1, yes

> +      <preference id="app.update.disable_button.showUpdateHistory"
> +                  name="app.update.disable_button.showUpdateHistory"
> +                  type="bool"/> 
Somehow I missed out this preference until now.

> +          <checkbox id="warnIncompatible"
> +                    preference="app.update.mode"
> +                    onsyncfrompreference="return document.getElementById('app.update.mode').value != 0;"
> +                    onsynctopreference="return !this.checked ? 0 : 1;"/> 
In-lined these two functions and removed ReadAddonWarn()/WriteAddonWarn(aWarn) from pref-smartupdate.js
Comment 5 neil@parkwaycc.co.uk 2010-01-29 02:17:10 PST
Eww, accesskey alert - use of narrow letters and descenders! Try this:
a daily
b Warn me if this will disable any of my add-ons
c Automatically download and install the update
d daily
e Allowed Sites
h Help
k weekly
m Add-on Manager
n Ask me what I want to do
o Installed Add-ons
s SeaMonkey
u Show Update History
w weekly
x Allow web sites to install extensions and updates
Comment 6 neil@parkwaycc.co.uk 2010-01-29 02:35:29 PST
Comment on attachment 424182 [details] [diff] [review]
Patch v1.2 Ignore legacy mode 2 in app.update.mode and treat it as a boolean.

>+function UpdateAppItems()
>+{
>+  var aus = Components.classes["@mozilla.org/updates/update-service;1"]
Hmm, I don't have an update service in my build, makes testing harder ;-)

>+      <radiogroup id="updateMode"
>+                  class="indent"
Nothing to indent under. (Unlike the checkbox, which is correctly indented.)

>+        <hbox class="indent">
Lose the hbox and put the class on the checkbox.

>+          <checkbox id="warnIncompatible"
>+                    label="&modeAutoAddonWarn.label;"
>+                    accesskey="&modeAutoAddonWarn.accesskey;"
>+                    flex="1"
Don't need the flex (with or without the hbox)

>+                    onsyncfrompreference="return document.getElementById('app.update.mode').value != 0;"
Fortunately checkboxes already know how to convert integers to booleans.

>+                    onsynctopreference="return !this.checked ? 0 : 1;"/>
Switch the ? : around so you don't need the !
Ideally <preference>::getElementValue::getValue would be smarter and only try to convert strings to integers or booleans as necessary.
Comment 7 neil@parkwaycc.co.uk 2010-01-29 02:45:21 PST
Comment on attachment 424182 [details] [diff] [review]
Patch v1.2 Ignore legacy mode 2 in app.update.mode and treat it as a boolean.

>+  document.getElementById("extensionsUpdatesEnabled").disabled =
>+    !document.getElementById("xpinstall.enabled").value ||
Is this some sort of "master" preference?

>+function UpdateAppItems()
>+{
>+  var aus = Components.classes["@mozilla.org/updates/update-service;1"]
>+                      .getService(Components.interfaces.nsIApplicationUpdateService);
>+  // For 1.9.2 branch
>+  if (!("canCheckForUpdates" in aus))
>+    aus.QueryInterface(Components.interfaces.nsIApplicationUpdateService2);
>+  var canCheckForUpdates = aus.canCheckForUpdates;
>+
>+  var enabledPref = document.getElementById("app.update.enabled");
>+  document.getElementById("appUpdatesEnabled")
>+          .disabled = !canCheckForUpdates || enabledPref.locked;
This bit doesn't belong here since it never changes.

>+  var disabled = !enabledPref.value || !canCheckForUpdates ||
>+                 document.getElementById("app.update.interval").locked;
>+
>+  document.getElementById("appFreqDaily").disabled = disabled;
>+  document.getElementById("appFreqWeekly").disabled = disabled;
>+  UpdateAutoItems();
Or it would possibly be fairer to say that these belong together.

>+function UpdateAutoItems()
>+{
>+  var enabledPref = document.getElementById("app.update.enabled");
>+  var disable = enabledPref.locked || !enabledPref.value ||
That's not true, the locked state of app.update.enabled is irrelevant.
(But I guess canCheckForUpdates is.)

>+  var disable = enabledPref.locked || !enabledPref.value ||
>+                autoPref.locked || !autoPref.value ||
Again, locked state of other prefs is irrelevant.
Comment 8 Ian Neal 2010-01-31 16:38:10 PST
Comment on attachment 424182 [details] [diff] [review]
Patch v1.2 Ignore legacy mode 2 in app.update.mode and treat it as a boolean.

I'll wait til the next version of the patch.
Comment 9 Philip Chee 2010-02-06 03:10:12 PST
Created attachment 425633 [details] [diff] [review]
Patch v1.3 Fixed Review Comments.

> Eww, accesskey alert - use of narrow letters and descenders! Try this:
> a daily
> b Warn me if this will disable any of my add-ons
> c Automatically download and install the update
> d daily
> e Allowed Sites
> H Help
> k weekly
> M Add-on Manager
> n Ask me what I want to do
> o Installed Add-ons
> S SeaMonkey
> u Show Update History
> w weekly
> x Allow web sites to install extensions and updates
Fixed.

>>+      <radiogroup id="updateMode"
>>+                  class="indent"
> Nothing to indent under. (Unlike the checkbox, which is correctly indented.)
Fixed.

>>+        <hbox class="indent">
>Lose the hbox and put the class on the checkbox.
Fixed.

>>+          <checkbox id="warnIncompatible"
>>+                    label="&modeAutoAddonWarn.label;"
>>+                    accesskey="&modeAutoAddonWarn.accesskey;"
>>+                    flex="1"
> Don't need the flex (with or without the hbox)
Fixed.

>>+                    onsyncfrompreference="return document.getElementById('app.update.mode').value != 0;"
> Fortunately checkboxes already know how to convert integers to booleans.
Fixed.

>>+                    onsynctopreference="return !this.checked ? 0 : 1;"/>
> Switch the ? : around so you don't need the !
> Ideally <preference>::getElementValue::getValue would be smarter and only try
> to convert strings to integers or booleans as necessary.
Fixed.

>>+  document.getElementById("extensionsUpdatesEnabled").disabled =
>>+    !document.getElementById("xpinstall.enabled").value ||
> Is this some sort of "master" preference?
I guess you could say this. If you can't install extensions, you can't update them either.

>>+function UpdateAppItems()
>>+{
>>+  var aus = Components.classes["@mozilla.org/updates/update-service;1"]
>>+                      .getService(Components.interfaces.nsIApplicationUpdateService);
>>+  // For 1.9.2 branch
>>+  if (!("canCheckForUpdates" in aus))
>>+    aus.QueryInterface(Components.interfaces.nsIApplicationUpdateService2);
>>+  var canCheckForUpdates = aus.canCheckForUpdates;
>>+
>>+  var enabledPref = document.getElementById("app.update.enabled");
>>+  document.getElementById("appUpdatesEnabled")
>>+          .disabled = !canCheckForUpdates || enabledPref.locked;
> This bit doesn't belong here since it never changes.
Moved down to the next section.

>>+  var disabled = !enabledPref.value || !canCheckForUpdates ||
>>+                 document.getElementById("app.update.interval").locked;
>>+
>>+  document.getElementById("appFreqDaily").disabled = disabled;
>>+  document.getElementById("appFreqWeekly").disabled = disabled;
>>+  UpdateAutoItems();
> Or it would possibly be fairer to say that these belong together.
Fixed.

>>+function UpdateAutoItems()
>>+{
>>+  var enabledPref = document.getElementById("app.update.enabled");
>>+  var disable = enabledPref.locked || !enabledPref.value ||
> That's not true, the locked state of app.update.enabled is irrelevant.
> (But I guess canCheckForUpdates is.)
Fixed. Made gCanCheckForUpdates a global so that it can be referenced by UpdateAutoItems().

>>+  var disable = enabledPref.locked || !enabledPref.value ||
>>+                autoPref.locked || !autoPref.value ||
> Again, locked state of other prefs is irrelevant.
Fixed.
Comment 10 neil@parkwaycc.co.uk 2010-02-06 09:59:46 PST
Comment on attachment 425633 [details] [diff] [review]
Patch v1.3 Fixed Review Comments.

>  * ***** END LICENSE BLOCK ***** */
>+var gCanCheckForUpdates;
Nit: blank line after licence block

>+  // Make it easier to access the pref pane from onsync.
>+  document.getElementById("warnIncompatible").pane = this;
Unused.

>+  document.getElementById("extensionsUpdatesEnabled").disabled =
>+    !document.getElementById("xpinstall.enabled").value ||
>+    document.getElementById("extensions.update.enabled").locked;
[See below.]

>+  document.getElementById("extensionsFreqDaily").disabled = locked;
>+  document.getElementById("extensionsFreqWeekly").disabled = locked;
Hmm, why did we never think to disable the <radiogroup> directly?

>+  document.getElementById("appUpdatesEnabled")
>+          .disabled = !gCanCheckForUpdates || enabledPref.locked;

>+  var disable = !enabledPref.value || !autoPref.value ||
>+                document.getElementById("app.update.mode").locked;
>+  document.getElementById("warnIncompatible").disabled = disable;
So, you seem to have three different disabling styles :-(
Comment 11 Philip Chee 2010-02-10 01:37:43 PST
Created attachment 426206 [details] [diff] [review]
Patch v1.4 Fix more review comments.

>>  * ***** END LICENSE BLOCK ***** */
>>+var gCanCheckForUpdates;
> Nit: blank line after licence block
Fixed.

>>+  // Make it easier to access the pref pane from onsync.
>>+  document.getElementById("warnIncompatible").pane = this;
> Unused.
Fixed.

>>+  document.getElementById("extensionsUpdatesEnabled").disabled =
>>+    !document.getElementById("xpinstall.enabled").value ||
>>+    document.getElementById("extensions.update.enabled").locked;
> [See below.]
>
>>+  document.getElementById("extensionsFreqDaily").disabled = locked;
>>+  document.getElementById("extensionsFreqWeekly").disabled = locked;
> Hmm, why did we never think to disable the <radiogroup> directly?
Fixed. Now changed to disabling both extensionsUpdateFrequency and appUpdateFrequency.

>>+  document.getElementById("appUpdatesEnabled")
>>+          .disabled = !gCanCheckForUpdates || enabledPref.locked;
>
>>+  var disable = !enabledPref.value || !autoPref.value ||
>>+                document.getElementById("app.update.mode").locked;
>>+  document.getElementById("warnIncompatible").disabled = disable;
> So, you seem to have three different disabling styles :-(
I've tried to make things more consistent. I hope I've fixed all the bits.
Comment 12 neil@parkwaycc.co.uk 2010-02-10 01:49:48 PST
Comment on attachment 426206 [details] [diff] [review]
Patch v1.4 Fix more review comments.

There was one thing that did cross my mind when testing this... you have a checkbox for extensions, then a checkbox for SeaMonkey and a checkbox for extensions, then a radiogroup for SeaMonkey. In particular, the first checkbox disables the third checkbox, and the second checkbox disables the radiogroup. I just thought it looked a little odd, but I don't think we can switch around the rows due to the presence of the Add-on Manager button.
Comment 13 neil@parkwaycc.co.uk 2010-02-21 09:44:21 PST
Comment on attachment 426206 [details] [diff] [review]
Patch v1.4 Fix more review comments.

>+  // For 1.9.2 branch
>+  if (!("canCheckForUpdates" in aus))
>+    aus.QueryInterface(Components.interfaces.nsIApplicationUpdateService2);
This can go ;-) (plus the one in utilityOverlay.js while you're there)
Comment 14 Philip Chee 2010-02-22 06:09:42 PST
Created attachment 428195 [details] [diff] [review]
Patch v1.5 Remove support for 1.9.2 sr=Neil

Carrying forward sr=Neil

>>+  // For 1.9.2 branch
>>+  if (!("canCheckForUpdates" in aus))
>>+    aus.QueryInterface(Components.interfaces.nsIApplicationUpdateService2);
> This can go ;-)
Fixed.

> (plus the one in utilityOverlay.js while you're there)
Sigh. Fixed.
Comment 15 Ian Neal 2010-02-22 14:56:40 PST
Comment on attachment 428195 [details] [diff] [review]
Patch v1.5 Remove support for 1.9.2 sr=Neil

Could you make sure that there is bug open for updating the help to reflect the changes in this bug (or that you do it in this bug).
Comment 16 Philip Chee 2010-02-23 00:52:17 PST
> Could you make sure that there is bug open for updating the help to reflect the
> changes in this bug (or that you do it in this bug).

Opened Bug 547931.
Comment 17 Justin Wood (:Callek) 2010-03-03 20:04:41 PST
Pushed to c-c: http://hg.mozilla.org/comm-central/rev/18bd4c198a74

Note You need to log in before you can comment on or make changes to this bug.