342606 - Compatibility Wizard is broken in oh so many ways (unreachable code paths, messages not shown, etc.)

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Robert Strong (they/them - no direct email)]

This is a spin off of bug 342350.

Comment 1

[Robert Strong (they/them - no direct email)]

Requesting blocking Firefox 2.0. While cleaning up this code I've found a couple of bugs with this wizard and I should have a patch ready soon.

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: screenshot after cleanup

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: patch - removes a lot of unused code

I still need to verify this does the right thing in regards to hashes.

Comment 4

[Robert Strong (they/them - no direct email)]

beltzner, could you take a look at the screenshots? If there are more string changes needed I'd prefer to fix them in another bug so I can get this landed when it is ready.

Comment 5

[Robert Strong (they/them - no direct email)]

beltzner, for starters the word restart is no longer necessary since we don't display any other app ui and perform the restart no matter what when the wizard is closed.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

--> blocking Fx2b2; I'd like to get it into Fx2b1 if possible, though, but that'll be depending on getting a review in time and making an assertion of risk.

(string comments coming)

Comment 7

[Robert Strong (they/them - no direct email)]

beltzner, so, the error details window is not used but it should be. We currently just go on happily when an update error happens instead of informing the user that the install failed. The old strings for this were:
installErrorDescription=The following components could not be installed due to errors (the file could not be downloaded, was corrupt, or for some other reason).
checkingErrorDescription=%S could not check for updates to the following components (either the update server(s) did not respond, or the update service(s) were not found). 

They are displayed on top of a list and I'll try to get screenshots submitted shortly.

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: error dialog screenshot

beltzner, this is pretty damn awful IMO. This is the error dialog that needs those two strings in my previous comment... I'm going to try to clean up this ui some but it should give you an idea of where it is used.

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: patch

Benjamin, I highly suspect you won't much like this patch. All I can say is the original code is full of cruft, dead end paths, etc. and this should at least make it easier to work with by removing said cruft, etc. while showing the messages that were broken since the app update code was removed from this code.

Comment 10

[Robert Strong (they/them - no direct email)]

Attachment: patch - forgot a file in the last one

Comment 11

[Robert Strong (they/them - no direct email)]

Attachment: wider error dialog

beltzner, this should be wide enough to display the errors we might have. It is still sucky but this should be less sucky. btw: I made it the same width as the wizard that opens it.

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 228222 [details] [diff] [review]
patch - forgot a file in the last one

Some comments to hopefully help with the review

>Index: toolkit/mozapps/update/content/errors.xul
...
> <dialog id="errors"
>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>         title="&errors.title;"
>         onload="init()"
>-        style="width: 28em;"
>+        style="width: 40em;"
We weren't displaying the entire error message due to the width being too small.

>@@ -59,17 +59,17 @@
...
>-      var str = updateStrings.getFormattedString(state + "ErrorDescription", [brandShortName]);
>+      var str = updateStrings.getFormattedString(state + "ErrorDesc", [brandShortName]);
l10n change

>@@ -99,14 +99,14 @@
...
>-  <separator/>
>+  <separator class="thin"/>
>   <listbox id="extensions" rows="7"/>
>-  <separator/>
>+  <separator class="thin"/>
There was too much space between the text and the list (see screenshot).

>Index: toolkit/mozapps/extensions/content/update.js
...
>@@ -15,95 +15,85 @@

>-//
>-// This UI can be opened from the following places, and in the following modes:
>-//
>-// - from the Version Compatibility Checker at startup
>-//    as the application starts a check is done to see if the application being
>-//    started is the same version that was started last time. If it isn't, a
>-//    list of UpdateItems that are incompatible with the verison being 
>-//    started is generated and this UI opened with that list. This UI then
>-//    offers to check for updates to those UpdateItems that are compatible
>-//    with the version of the application being started. 
>-//    
>-//    In this mode, the wizard is opened to panel 'mismatch'. 
>+// This UI is only opened from the Extension Manager when the app is upgraded.
The old comment was bogus.

>-const PREF_UPDATE_EXTENSIONS_AUTOUPDATEENABLED  = "extensions.update.autoUpdateEnabled";
I don't think this pref was ever implemented... it isn't used now at least.

> 
>-var gShowMismatch = null;
This was initialized to null and never changed though it was checked in if statements.

> var gUpdateWizard = {
>-  // The items to check for updates for (e.g. an extension, some subset of extensions, 
>-  // all extensions, a list of compatible extensions, etc...)
>+  // When synchronizing app compatibility info this contains all installed
>+  // add-ons. When checking for compatible versions this contains only
>+  // incompatible add-ons.
comment cleanup.

>   items: [],
>   // The items that we found updates available for
>   itemsToUpdate: [],
>-  // The items that we successfully installed updates for
>-  updatedCount: 0,
not used anywhere

>   shouldSuggestAutoChecking: false,
>   shouldAutoCheck: false,
>   xpinstallEnabled: false,
>   xpinstallLocked: false,
>-  remainingExtensionUpdateCount: 0,
not used anywhere

>-    this.shouldSuggestAutoChecking = 
>-      gShowMismatch && 
>-      !pref.getBoolPref(PREF_UPDATE_EXTENSIONS_AUTOUPDATEENABLED);
>+
>+    try {
>+      this.shouldSuggestAutoChecking = 
>+        !pref.getBoolPref(PREF_UPDATE_EXTENSIONS_ENABLED);
>+    }
>+    catch (e) {
>+    }
Mainly to protect XULRunner apps since other apps specify this.

>-      pref.setBoolPref(PREF_UPDATE_EXTENSIONS_ENABLED, this.shouldAutoCheck); 
>+      pref.setBoolPref(PREF_UPDATE_EXTENSIONS_ENABLED, this.shouldAutoCheck);
sorry, whitespace cleanup.

>@@ -129,28 +119,30 @@
...
>+  // Displays a list of items that had an error during the update check. We
>+  // don't display the actual error that occured since
>+  // nsIAddonUpdateCheckListener doesn't return the error details.
We don't return actual errors regretfully so all we have here is success and failure when checking an update rdf. Misspelled occurred and fixed locally.

...
>-      errors.push({ name: this.errorItems[i].name, error: true, 
>-                    item: this.errorItems[i] });
>+      errors.push({ name: this.errorItems[i].name, error: true });
The nsIUpdateItem was never used in errors.xul

...
>-    if (this.errorOnGeneric || this.errorItems.length > 0)
>+    if (this.errorItems.length > 0)
this.errorOnGeneric is not decalred or set anywhere and has been around since 1.0.

>@@ -248,34 +240,36 @@
...
> var gUpdatePage = {
>+  _totalCount: 0,
Added _totalCount to gUpdatePage for consistency with the other wizard pages.

> var gFoundPage = {
>-  _nonAppItems: [],
>-  
>-  _newestInfo: null,
No longer needed and they weren't referenced anywhere.

>-  buildAddons: function ()
It is not possible to init this page more than once so there is no need for the buildAddons function.

>+  onPageShow: function ()
>   {
>-    var hasExtensions = false;
No longer needed since this code no longer handles app update.

>-    var foundAddonsList = document.getElementById("found.addons.list");
>-    var uri = Components.classes["@mozilla.org/network/standard-url;1"]
>-                        .createInstance(Components.interfaces.nsIURI);
>+    gUpdateWizard.setButtonLabels(null, true, 
>+                                  "installButtonText", false, 
>+                                  null, false);
>+
>+    var foundUpdates = document.getElementById("found.updates");
>     var itemCount = gUpdateWizard.itemsToUpdate.length;
>     for (var i = 0; i < itemCount; ++i) {
>       var item = gUpdateWizard.itemsToUpdate[i];
>-      var checkbox = document.createElement("checkbox");
>-      foundAddonsList.appendChild(checkbox);
>-      checkbox.setAttribute("type", "update");
>-      checkbox.label        = item.name + " " + item.version;
>-      checkbox.setAttribute("URL", item.xpiURL);
>-      checkbox.setAttribute("hash", item.xpiHash);
>-      checkbox.infoURL      = "";
>-      checkbox.internalName = "";
>-      uri.spec              = item.xpiURL;
>-      checkbox.setAttribute("source", uri.host);
>-      checkbox.checked      = true;
>-      hasExtensions         = true;
>+      var listItem = foundUpdates.appendItem(item.name + " " + item.version,
>+                                             item.xpiURL);
>+      listItem.setAttribute("type", "checkbox");
>+      listItem.setAttribute("checked", "true");
>+      listItem.setAttribute("URL", item.xpiURL);
>+      listItem.setAttribute("hash", item.xpiHash);
The wizard now uses a listbox.

>-    if (hasExtensions) {
>-      var addonsHeader = document.getElementById("addons");
>-      var strings = document.getElementById("updateStrings");
>-      addonsHeader.label = strings.getFormattedString("updateTypeAddons", [itemCount]);
>-      addonsHeader.collapsed = false;
>-    }
This was needed to select the radio for extensions when we had app update, etc. included in this code and this is no longer needed.

>-  _initialized: false,
It is not possible to init this page more than once so there is no need to know it has been initialized.

...
>     if (!gUpdateWizard.xpinstallEnabled) {
>-      document.documentElement.getButton("next").disabled = true;
handled by the call to updateNextButton.

>+    else {
>+      //XXXrstrong - this string should be set in update.xul from update.dtd
>+      // for Gecko Version 1.9.
>+      var strings = document.getElementById("updateStrings");
>+      var text = strings.getString("foundInstructions");
>+      var foundInstructions = document.getElementById("foundInstructions");
>+      foundInstructions.hidden = false;
>+      foundInstructions.appendChild(document.createTextNode(text));
This really should be done in the dtd. Since there are string changes should it be done now?


>+    var foundUpdates = document.getElementById("found.updates");
>+    var updates = foundUpdates.getElementsByTagName("listitem");;
>+    for (var i = 0; i < updates.length; ++i) {
>+      if (!updates[i].checked)
>+        continue;
>+      oneChecked = true;
>+      break;
>     }
Changed to support the listbox

> 
> var gInstallingPage = {
>   _installing       : false,
>-  _restartRequired  : false,
No longer needed and we always restart anyways.

>-    var updates = document.getElementById("found.updates");
>-    var checkboxes = updates.selectedItem.getElementsByTagName("checkbox");
>-    for (var i = 0; i < checkboxes.length; ++i) {
>-      if (checkboxes[i].type == "update" && checkboxes[i].checked) {
>-        items.push(checkboxes[i].URL);
>-        hashes.push(checkboxes[i].hash ? checkboxes[i].hash : null);
>-        this._objs.push({ name: checkboxes[i].label });
>-      }
>+    var foundUpdates = document.getElementById("found.updates");
>+    var updates = foundUpdates.getElementsByTagName("listitem");;
>+    for (var i = 0; i < updates.length; ++i) {
>+      if (!updates[i].checked)
>+        continue;
>+      items.push(updates[i].value);
>+      hashes.push(updates[i].getAttribute("hash") ? updates[i].getAttribute("hash") : null);
>+      this._objs.push({ name: updates[i].label });
>     }
Changed to support the listbox

>@@ -475,95 +431,87 @@
>     case nsIXPIProgressDialog.INSTALL_START:
>       var label = strings.getFormattedString("installingPrefix", [this._objs[aIndex].name]);
>       var actionItem = document.getElementById("actionItem");
>       actionItem.value = label;
>       this._installing = true;
>       break;
>     case nsIXPIProgressDialog.INSTALL_DONE:
>       switch (aValue) {
>-      case 999: 
>-        this._restartRequired = true;
>-        break;
>-      case 0: 
>-        --gUpdateWizard.remainingExtensionUpdateCount;
Set but not used anywhere

>+      case 999:
>+      case 0:
>         break;
>+      default:
>+        this._errors.push({ name: this._objs[aIndex].name, error: aValue });
>       }
So we can display install errors

>   
>-  _objs: [],
declared twice

>-  _errors: false,
Now it just uses the _errors array

> var gErrorsPage = {
>   onPageShow: function ()
>   {
>     document.documentElement.getButton("finish").focus();
>   },
>   
>   onShowErrors: function ()
>   {
>-    gUpdateWizard.showErrors("install", gInstallingPage._objs);
>+    gUpdateWizard.showErrors("install", gInstallingPage._errors);
gInstallingPage._objs never contained errors so installs were always shown as successful and errors were never shown.

> var gFinishedPage = {
>   onPageShow: function ()
>   {
>     gUpdateWizard.setButtonLabels(null, true, null, true, null, true);
>     document.documentElement.getButton("finish").focus();
>     
>-    if (gUpdateWizard.xpinstallLocked)
>-    {
>+    if (gUpdateWizard.xpinstallLocked) {
>       document.getElementById("adminDisabled").hidden = false;
>       document.getElementById("incompatibleRemainingLocked").hidden = false;
>+      document.getElementById("finishedMismatch").hidden = false;
>+      document.getElementById("incompatibleAlert").hidden = false;
>     }
>     else {
>       document.getElementById("updated").hidden = false;
>       document.getElementById("incompatibleRemaining").hidden = false;
>       if (gUpdateWizard.shouldSuggestAutoChecking) {
>+        document.getElementById("finishedEnableCheckingDesc").hidden = false;
>         document.getElementById("finishedEnableChecking").hidden = false;
>         document.getElementById("finishedEnableChecking").click();
>       }
>     }
>-    
>-    if (gShowMismatch || gUpdateWizard.xpinstallLocked) {
>-      document.getElementById("finishedMismatch").hidden = false;
>-      document.getElementById("incompatibleAlert").hidden = false;
>-    }
xpinstallLocked is handled above and gShowMismatch is always null.

> var gNoUpdatesPage = {
>   onPageShow: function (aEvent)
>   {
>     gUpdateWizard.setButtonLabels(null, true, null, true, null, true);
>     document.documentElement.getButton("finish").focus();
>-    if (gShowMismatch) {
>-      document.getElementById("introUser").hidden = true;
>-      document.getElementById("introMismatch").hidden = false;
>-      document.getElementById("mismatchNoUpdates").hidden = false;
>+    document.getElementById("introUser").hidden = true;
>+    document.getElementById("introMismatch").hidden = false;
>+    document.getElementById("mismatchNoUpdates").hidden = false;
gShowMismatch is always null so there was missing ui.

>-      if (gUpdateWizard.shouldSuggestAutoChecking) {
>-        document.getElementById("mismatchIncompatibleRemaining").hidden = true;
>-        document.getElementById("mismatchIncompatibleRemaining2").hidden = false;
>-        document.getElementById("mismatchFinishedEnableChecking").hidden = false;
>-      }
>+    if (gUpdateWizard.shouldSuggestAutoChecking) {
>+      document.getElementById("mismatchIncompatibleRemaining").hidden = true;
>+      document.getElementById("mismatchIncompatibleRemaining2").hidden = false;
>+      document.getElementById("mismatchFinishedEnableChecking").hidden = false;
>+      document.getElementById("mismatchFinishedEnableChecking").click();
Sorry, more whitespace.

>Index: toolkit/mozapps/extensions/content/update.xul
...
>@@ -33,34 +33,32 @@
> # decision by deleting the provisions above and replace them with the notice
> # and other provisions required by the GPL or the LGPL. If you do not delete
> # the provisions above, a recipient may use your version of this file under
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK *****
> 
> <?xml-stylesheet href="chrome://global/skin/" type="text/css"?> 
>-<?xml-stylesheet href="chrome://mozapps/content/extensions/update.css" type="text/css"?> 
no longer needed

> <wizard id="updateWizard"
>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>         title="&updateWizard.title;"
>         windowtype="Update:Wizard"
>         onload="gUpdateWizard.init();"
>         onwizardfinish="gUpdateWizard.onWizardFinish();"
>         onclose="return gUpdateWizard.onWizardClose(event);"
>-        style="width: 47em; min-height: 35em;"
Use the default dimensions for a wizard

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

String comments for the screenshots in attachment 228118 [details] (as read top-to-bottom, left-to-right) cc'ing reed for orthography:

screen 1: 
   panel title: "Checking Add-On Compatibility"
   progress text: "Checking your add-ons to see if they are compatible…"

screen 2:
   panel title: "Found Incompatible Add-Ons"
   text: "The following add-ons are not compatible with this version of %S and have been disabled:"
   text: "%S can check to see if updates are available to make these add-ons compatible with this version."

(NOTE: shouldn't we be doing the xpinstall check *before* we bother to check for available updates?)

screen 3:
   panel title: "Checking for Compatibility Updates"
   text: "Checking for available updates to incompatible add-ons ..."
   - just need the text below the progress indicator, remove the stuff above

screen 4:
   panel title: "Unable to Install Updates"
   text: "There are updates available, but they can't be installed because software installation for Minefield has been disabled. Please contact your system administrator for assistance."
   - the "don't check" button shouldn't be here
   - the "Finish" button should be at the right, selected by default

screen 5:
   panel title: "Found Compatibility Updates"
   text: "You may select the updates you would like %S to install." 
   
screen 6:
   panel title: "Found Compatibility Updates"
   text: "The selected updates can't be installed because software installation is currently disabled. You can change this setting below." 

screen 7:
   panel title: "Installing Compatibility Updates"
   text: "Downloading and installing updates to your add-ons"
   progress indication text is good

screen 8:
   panel title: "Compatibility Updates Installed"
   text: "%S has installed the updates to your add-ons."

screen 9:
   panel title: "No Compatibility Updates Found"
   text: "%S was unable to find any updates for your add-ons to make them compatible with this version. These updates may be available in the future. %S will check periodically and inform you when compatible versions of your add-ons are found."

screen 10:
   panel title: "Some Compatibility Updates Found"
   text: "%S was unable to find any updates for some of your add-ons to make them compatible with this version. These updates may be available in the future. %S can check periodically and inform you when compatible versions of your add-ons are found."
   - checkText is good as is

screen 11:
   panel title: "No Compatibility Updates Found"
   text: "%S was unable to find any updates for your add-ons to make them compatible with this version. These updates may be available in the future. %S can check periodically and inform you when compatible versions of your add-ons are found."
   - checkText is good as is

Comment 14

[Robert Strong (they/them - no direct email)]

Thank you beltzner!

(In reply to comment #13)
...
> screen 1: 
>    panel title: "Checking Add-On Compatibility"
We've consistently used Add-on and Add-ons when it should be capitalized

...
> screen 2:
>    panel title: "Found Incompatible Add-Ons"
Same re: Add-ons

>    text: "The following add-ons are not compatible with this version of %S and
> have been disabled:"
>    text: "%S can check to see if updates are available to make these add-ons
> compatible with this version."
The sentence structure seems a bit awkward - "can check to see". I still don't like it but something along the lines of:
    text: "%S can check for compatible versions of these add-ons."

> (NOTE: shouldn't we be doing the xpinstall check *before* we bother to check
> for available updates?)
The reason it is done at this point is if no updates are found then this ui doesn't need to ask the user to enable xpinstall.

> screen 3:
>    panel title: "Checking for Compatibility Updates"
>    text: "Checking for available updates to incompatible add-ons ..."
No space between "add-ons" and "..." right?

>    - just need the text below the progress indicator, remove the stuff above
The text will display the add-on name that is being checked similar to screen 7. I just caught it when it was first starting. Do you still want it removed?

> screen 4:
>    panel title: "Unable to Install Updates"
>    text: "There are updates available, but they can't be installed because
> software installation for Minefield has been disabled. Please contact your
> system administrator for assistance."
>    - the "don't check" button shouldn't be here
>    - the "Finish" button should be at the right, selected by default
Would it be ok if the "don't check" button was a "cancel" button and disabled as it is elsewhere?

> screen 5:
>    panel title: "Found Compatibility Updates"
>    text: "You may select the updates you would like %S to install." 
> 
> screen 6:
>    panel title: "Found Compatibility Updates"
>    text: "The selected updates can't be installed because software installation
> is currently disabled. You can change this setting below." 
For screen 5 and 6 - I had problems with the existing ui since in both it is possible to check / uncheck add-ons to update but only in one is it stated "You may select the updates you would like %S to install."

> screen 7:
>    panel title: "Installing Compatibility Updates"
>    text: "Downloading and installing updates to your add-ons"
>    progress indication text is good
> 
> screen 8:
>    panel title: "Compatibility Updates Installed"
>    text: "%S has installed the updates to your add-ons."
> 
> screen 9:
>    panel title: "No Compatibility Updates Found"
>    text: "%S was unable to find any updates for your add-ons to make them
> compatible with this version. These updates may be available in the future. %S
> will check periodically and inform you when compatible versions of your add-ons
> are found."
"These updates may be available in the future." seems a bit superfluous in relation to the sentence that follows it.

> screen 10:
>    panel title: "Some Compatibility Updates Found"
>    text: "%S was unable to find any updates for some of your add-ons to make
> them compatible with this version. These updates may be available in the
> future. %S can check periodically and inform you when compatible versions of
> your add-ons are found."
Same here

> screen 11:
>    panel title: "No Compatibility Updates Found"
>    text: "%S was unable to find any updates for your add-ons to make them
> compatible with this version. These updates may be available in the future. %S
> can check periodically and inform you when compatible versions of your add-ons
> are found."
Same here

Comment 15

[Robert Strong (they/them - no direct email)]

beltzner, from comment #7 there are a couple of more.
installErrorDescription=The following components could not be installed due to errors (the file could not be downloaded, was corrupt, or for some other reason).
checkingErrorDescription=%S could not check for updates to the following components (either the update server(s) did not respond, or the update service(s) were not found). 

See attachment #228230 [details] for context

Comment 16

[Reed Loden [:reed]]

(In reply to comment #13)
> String comments for the screenshots in attachment 228118 [details] [edit] (as read
> top-to-bottom, left-to-right) cc'ing reed for orthography:

Glad to help. Ask me any time. :)

> screen 1: 
>    panel title: "Checking Add-On Compatibility"

Why "Add-On" in title and "add-ons" in text (difference in plurality)? I'd go with "Add-Ons" in title, too.

>    progress text: "Checking your add-ons to see if they are compatible…"

Compatible with what? Maybe "compatible with this version"?

> screen 3:
>    panel title: "Checking for Compatibility Updates"
>    text: "Checking for available updates to incompatible add-ons ..."
>    - just need the text below the progress indicator, remove the stuff above

Remove the space between "add-ons" and "...". Also, shouldn't you be using "…" instead of "..." here?

> screen 4:
>    panel title: "Unable to Install Updates"
>    text: "There are updates available, but they can't be installed because
> software installation for Minefield has been disabled. Please contact your
> system administrator for assistance."
>    - the "don't check" button shouldn't be here
>    - the "Finish" button should be at the right, selected by default

s/Minefield/%S/

> screen 7:
>    panel title: "Installing Compatibility Updates"
>    text: "Downloading and installing updates to your add-ons"
>    progress indication text is good

I'd go with "…" ("...") after "add-ons" (just to coincide with progress bar).

> screen 8:
>    panel title: "Compatibility Updates Installed"
>    text: "%S has installed the updates to your add-ons."

Shouldn't you mention something about restarting here?

> screen 9:
>    panel title: "No Compatibility Updates Found"
>    text: "%S was unable to find any updates for your add-ons to make them
> compatible with this version. These updates may be available in the future. %S
> will check periodically and inform you when compatible versions of your add-ons
> are found."

Do you really want to say "versions" here? Other references to "version(s)" in earlier proposed text are directed towards the application itself and not the add-on. How about "updates" (or a synonym) instead of "versions"?

> screen 10:
>    panel title: "Some Compatibility Updates Found"
>    text: "%S was unable to find any updates for some of your add-ons to make
> them compatible with this version. These updates may be available in the
> future. %S can check periodically and inform you when compatible versions of
> your add-ons are found."

Title and text contradict each other. You say "Updates Found" in title but "unable to find" in text. Also, you might need to explain that some updates were found and that a restart is required to finish installation. Don't spend too much time on one side of the fence. I'd straddle the fence by spending a sentence or two on both the found and not found updates.

See comment in earlier screen 9 outline.

> screen 11:
>    panel title: "No Compatibility Updates Found"
>    text: "%S was unable to find any updates for your add-ons to make them
> compatible with this version. These updates may be available in the future. %S
> can check periodically and inform you when compatible versions of your add-ons
> are found."

See comment in earlier screen 9 outline.

Comment 17

[Reed Loden [:reed]]

(In reply to comment #15)
> beltzner, from comment #7 there are a couple of more.
> installErrorDescription=The following components could not be installed due to
> errors (the file could not be downloaded, was corrupt, or for some other
> reason).

Need something after and/or before "or for some other reason" to end the sentence gracefully. Maybe "or for some other reason was invalid" or "or was for some other reason invalid". Also, I bet s/components/add-ons/.

> checkingErrorDescription=%S could not check for updates to the following
> components (either the update server(s) did not respond, or the update
> service(s) were not found). 

Maybe "for the following" instead of "to the following" in the first sentence (however, the for...for sounds weird). Again, probably s/components/add-ons/. The comma after "respond" needs to be removed.

Comment 18

[Robert Strong (they/them - no direct email)]

(In reply to comment #16)
> > screen 3:
> >    panel title: "Checking for Compatibility Updates"
> >    text: "Checking for available updates to incompatible add-ons ..."
> >    - just need the text below the progress indicator, remove the stuff above
> 
> Remove the space between "add-ons" and "...". Also, shouldn't you be using
> "…" instead of "..." here?
I didn't get the memo... why are any decimals in dtd's being changed to the html entity equivalent?

> > screen 4:
> >    panel title: "Unable to Install Updates"
> >    text: "There are updates available, but they can't be installed because
> > software installation for Minefield has been disabled. Please contact your
> > system administrator for assistance."
> >    - the "don't check" button shouldn't be here
> >    - the "Finish" button should be at the right, selected by default
> 
> s/Minefield/%S/
Having the value is better for me so there is no question whether it should be the short brand name, full brand name, app version, etc.

> > screen 8:
> >    panel title: "Compatibility Updates Installed"
> >    text: "%S has installed the updates to your add-ons."
> 
> Shouldn't you mention something about restarting here?
See previous comment #5 about restart not being necessary.

Comment 19

[Reed Loden [:reed]]

(In reply to comment #18)
> (In reply to comment #16)
> > > screen 3:
> > >    panel title: "Checking for Compatibility Updates"
> > >    text: "Checking for available updates to incompatible add-ons ..."
> > >    - just need the text below the progress indicator, remove the stuff above
> > 
> > Remove the space between "add-ons" and "...". Also, shouldn't you be using
> > "…" instead of "..." here?
> I didn't get the memo... why are any decimals in dtd's being changed to the
> html entity equivalent?

beltzner used … in screen 1, I believe, so either way, the same format needs to be used in all of them.

> > > screen 4:
> > >    panel title: "Unable to Install Updates"
> > >    text: "There are updates available, but they can't be installed because
> > > software installation for Minefield has been disabled. Please contact your
> > > system administrator for assistance."
> > >    - the "don't check" button shouldn't be here
> > >    - the "Finish" button should be at the right, selected by default
> > 
> > s/Minefield/%S/
> Having the value is better for me so there is no question whether it should be
> the short brand name, full brand name, app version, etc.

Meaning you prefer the %S here, correct?

> > > screen 8:
> > >    panel title: "Compatibility Updates Installed"
> > >    text: "%S has installed the updates to your add-ons."
> > 
> > Shouldn't you mention something about restarting here?
> See previous comment #5 about restart not being necessary.

Oops, sorry, didn't see that. :)

Comment 20

[Robert Strong (they/them - no direct email)]

(In reply to comment #19)
> Meaning you prefer the %S here, correct?
No, %S may mean short brand name, full brand name, or some other string so providing either the actual text or a generic version of the actual text (e.g. shortBrandName, fullBrandName, etc.) removes any question as to what is meant.

Comment 21

[Robert Strong (they/them - no direct email)]

repost:
beltzner, from comment #7 there are a couple of more.
installErrorDescription=The following components could not be installed due to errors (the file could not be downloaded, was corrupt, or for some other reason).
checkingErrorDescription=%S could not check for updates to the following components (either the update server(s) did not respond, or the update service(s) were not found). 

See attachment #228230 [details] [edit] for context

Comment 22

[(not reading, please use [e-mail] instead)]

Comment on attachment 228222 [details] [diff] [review]
patch - forgot a file in the last one

r=sspitzer, robert explained this patch to me face-to-face.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #21)
> beltzner, from comment #7 there are a couple of more.

Gah, I'm well beyond my initial ETA of 20 minutes :( Your strings, sire:

> installErrorDescription=The following components could not be installed due to
> errors (the file could not be downloaded, was corrupt, or for some other
> reason).

"%S was unable to install the updates for the following add-ons:"

> checkingErrorDescription=%S could not check for updates to the following
> components (either the update server(s) did not respond, or the update
> service(s) were not found). 

"%S was unable to successfully check for updates for the following add-ons:"

(These strings assume, as your screenshot shows, that the error reasons will be shown in the textbox with the add-on names. My feeling is that there's no need to say that it was due to an error - the very fact that we were unable to perform the action is, in itself, an error!)

Also, the "OK" button should become part of the wizard, or become a "Next" button or something to fit into the overall flow.

I also folded in your and Reed's comments to get: 

screen 1: 
   panel title: "Checking Add-ons Compatibility"
   progress text: "Checking your add-ons…"

screen 2:
   panel title: "Found Incompatible Add-ons"
   text: "The following add-ons are not compatible with this version of %S and
have been disabled:"
   text: "%S can check to see if compatible updates to these add-ons are available."

screen 3:
   panel title: "Checking for Compatibility Updates"
   text: "Checking for available updates to incompatible add-ons…"
   progress text: "Checking for update to %S."

screen 4:
   panel title: "Unable to Install Updates"
   text: "There are updates available, but they can't be installed because
software installation for %S has been disabled. Please contact your
system administrator for assistance."
   - the "don't check" button should be a disabled "Cancel" button

screen 5:
   panel title: "Found Compatibility Updates"
   text: "Select the updates you would like to install:" 

screen 6:
   panel title: "Found Compatibility Updates"
   text: "Select the updates you would like to install:" 
   text: "The selected updates can't be installed because software installation
is currently disabled. You can change this setting below." 

screen 7:
   panel title: "Installing Compatibility Updates"
   text: "Downloading and installing updates to your add-ons…"
   progress text as current

screen 8:
   panel title: "Compatibility Updates Installed"
   text: "%S has installed the updates to your add-ons."

screen 9:
   panel title: "No Compatibility Updates Found"
   text: "%S was unable to find any updates for your incompatible add-ons. %S will continue to check periodically and inform you when compatible updates for these add-ons are found."

screen 10:
   panel title: "Some Compatibility Updates Found"
   text: "%S was unable to find any updates for some of your incompatible add-ons. %S will continue to check periodically and inform you when compatible updates for these add-ons are found."
   - checkText is good as is

screen 11:
   panel title: "No Compatibility Updates Found"
   text: "%S was unable to find any updates for your incompatible add-ons. %S will continue to check periodically and inform you when compatible updates for these add-ons are found."
   - checkText is good as is

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #23)
> screen 10:
>    panel title: "Some Compatibility Updates Found"

Er,  panel title: "Some Compatibility Updates Not Found"

Comment 25

[Robert Strong (they/them - no direct email)]

I opened Bug 345353 for the string changes that aren't required to fix the broken code that is fixed by this bug since it will require additional code.

Re-opening bug... I haven't landed this yet. :P

Comment 26

[Robert Strong (they/them - no direct email)]

(In reply to comment #23)
> > installErrorDescription=The following components could not be installed due to
> > errors (the file could not be downloaded, was corrupt, or for some other
> > reason).
> 
> "%S was unable to install the updates for the following add-ons:"
We provide decent errors here so this is good.

> > checkingErrorDescription=%S could not check for updates to the following
> > components (either the update server(s) did not respond, or the update
> > service(s) were not found). 
> 
> "%S was unable to successfully check for updates for the following add-ons:"
We don't provide good errors here so I'm going to use:
%S was unable to successfully check for updates for the following add-ons (either the update server(s) did not respond, or the update service(s) were not found):

> Also, the "OK" button should become part of the wizard, or become a "Next"
> button or something to fit into the overall flow.
This is an information only dialog that can be opened from the wizard and standard is to use an OK button in this case since it is not part of the wizard pages.

Comment 27

[Robert Strong (they/them - no direct email)]

Checked in to trunk

Comment 28

[Robert Strong (they/them - no direct email)]

Comment on attachment 228222 [details] [diff] [review]
patch - forgot a file in the last one

This code needed some major cleanup since the last time it was changed.

Comment 29

[Mike Connor [:mconnor]]

Comment on attachment 228222 [details] [diff] [review]
patch - forgot a file in the last one

a=mconnor on behalf of drivers for checkin to the 1.8 branch

Comment 30

[Robert Strong (they/them - no direct email)]

Checked in to MOZILLA_1_8_BRANCH