Sync Suite pref-smartupdate.* with mozilla-central

RESOLVED FIXED in seamonkey2.1a1

Status

SeaMonkey
Preferences
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

Trunk
seamonkey2.1a1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [parity-fx][parity-tb])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

7 years ago
1. Add UI for app.update.auto app.update.mode
2. Enable/disable the UI based on the "locked" states of associated preferences.
(Assignee)

Comment 1

7 years ago
Created attachment 423140 [details] [diff] [review]
Patch v1.0

Initial patch as proposed.
Attachment #423140 - Flags: superreview?(neil)
Attachment #423140 - Flags: review?(iann_bugzilla)

Comment 2

7 years ago
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
Attachment #423140 - Flags: review?(iann_bugzilla) → review-
(Assignee)

Comment 3

7 years ago
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.
Attachment #423140 - Attachment is obsolete: true
Attachment #423343 - Flags: review?(iann_bugzilla)
Attachment #423140 - Flags: superreview?(neil)
(Assignee)

Updated

7 years ago
Attachment #423343 - Flags: superreview?(neil)
(Assignee)

Comment 4

7 years ago
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
Attachment #423343 - Attachment is obsolete: true
Attachment #424182 - Flags: superreview?(neil)
Attachment #424182 - Flags: review?(iann_bugzilla)
Attachment #423343 - Flags: superreview?(neil)
Attachment #423343 - Flags: review?(iann_bugzilla)

Comment 5

7 years ago
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

7 years ago
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

7 years ago
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.
Attachment #424182 - Flags: superreview?(neil) → superreview-

Updated

7 years ago
Attachment #424182 - Flags: review?(iann_bugzilla)

Comment 8

7 years ago
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.
(Assignee)

Comment 9

7 years ago
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.
Attachment #424182 - Attachment is obsolete: true
Attachment #425633 - Flags: superreview?(neil)
Attachment #425633 - Flags: review?(iann_bugzilla)
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 :-(
(Assignee)

Comment 11

7 years ago
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.
Attachment #425633 - Attachment is obsolete: true
Attachment #426206 - Flags: superreview?(neil)
Attachment #426206 - Flags: review?(iann_bugzilla)
Attachment #425633 - Flags: superreview?(neil)
Attachment #425633 - Flags: review?(iann_bugzilla)
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.
Attachment #426206 - Flags: superreview?(neil) → superreview+
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)
(Assignee)

Comment 14

7 years ago
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.
Attachment #426206 - Attachment is obsolete: true
Attachment #428195 - Flags: superreview+
Attachment #428195 - Flags: review?(iann_bugzilla)
Attachment #426206 - Flags: review?(iann_bugzilla)

Comment 15

7 years ago
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).
Attachment #428195 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Updated

7 years ago
Blocks: 547931
(Assignee)

Comment 16

7 years ago
> 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.
Keywords: checkin-needed
Pushed to c-c: http://hg.mozilla.org/comm-central/rev/18bd4c198a74
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.