329045 - Firefox 2.0 Extension Manager user interface update

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

This bug is for the work I am doing on the Extension Manager user interface for Firefox 2.0. This is needed to support Extension Locales (Bug 285848) and to better support Extension Dependencies (bug 298497) and Extension Blacklisting (bug 271166). There are also several other bugs I will be fixing at the same time that involve user interface modifications.

Comment 1

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

Attachment: rough screenshot 1 (work in progress)

Comment 2

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

Attachment: rough screenshot 2 w/ blocklisted item that has an update (work in progress)

All of the images aren't present and haven't even been decided if they are all necessary.

I have also started a wiki page for this and please post comments / ideas / etc. there.
http://wiki.mozilla.org/Extension_Manager_UI:Scratchpad

Comment 3

[Justin Kerk]

The dependencies for this should be "depends on" rather than "blocks" I would think.

Comment 4

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

I should have mentioned this when I added the dependencies.
This bug depends on bug 271166 due to that patch needing to land before this one.
The bugs this bug blocks does so due to those bugs being fixed by the work being done in this bug.

Comment 5

[Ben Basson]

(In reply to comment #2)
> Created an attachment (id=213855) [edit]
> rough screenshot 2 w/ blocklisted item that has an update (work in progress)

Seems like there are buttons all over the place to me. I saw the following a while back and I think it might be a more logical grouping (disregard the fact that this is part of the main options UI, though):
http://wiki.mozilla.org/Firefox:Extension_Manager_UI

Also, having PrefWindowV-style tabs for Extensions / Themes / Plugins / Search Engines (or whatever) might be more intuitive than having a drop-down list.

Comment 6

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

Responded on discussion page of wiki
http://wiki.mozilla.org/Talk:Extension_Manager_UI:Scratchpad

Comment 7

[Worcester12345]

(In reply to comment #1)
> Created an attachment (id=213854) [edit]
> rough screenshot 1 (work in progress)

I'd put the install update next to disable and make it stand out. It seems clumsy to have two rows of buttons. 

Also, would it be possible to have a date field and a name field, that way you click on the header and sort by date or name (or other attributes)?  That would allow people to sort by name or whatever else just like in all other similar interfaces; Thunderbird message view and Windows Explorer being the two closest to home off the top of my head.

Comment 8

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

Attachment: extensions and themes windows screenshots (normal display)

The other screenshots don't show the normal use case where an extension doesn't have the additional information... these do.

Comment 9

[Grey Nicholson (they/them)]

Attachment: Suggested Install button

I suggest using something like this (Firefox 0.9.3's Options button) as the Install button. Thunderstripe (a Thunderbird theme imitating Winstripe) uses this and it fits nicely.

Comment 10

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

Attachment: updated screenshots

Most of the ui is completed (I used an existing similar image to the suggested install button for install and install updates). Just need to finish up an couple of issues with the selected element size changes and cleanup the code a bit.

Comment 11

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

Attachment: Addon Global Notifications screenshot

Comment 12

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

Attachment: all (rev 1)

I'll attach images and separate patches for review shortly.

Comment 13

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

Attachment: placeholder images for winstripe

Images for winstripe. Also, I haven't finished the css for pinstripe yet

Comment 14

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

Attachment: changes to browser (checked in)

Comment 15

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

Attachment: changes to mail (checked in)

Comment 16

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

Attachment: changes to calendar

Comment 17

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

Attachment: Toolkit changes

Comment 18

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

Comment on attachment 219743 [details] [diff] [review]
Toolkit changes

Some comments that will hopefully assist in the review... I'll add more later.

>Index: mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl,v
>retrieving revision 1.41.2.3
>diff -u -8 -r1.41.2.3 nsIExtensionManager.idl
>--- mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl	14 Mar 2006 21:59:15 -0000	1.41.2.3
>+++ mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl	25 Apr 2006 11:07:22 -0000
>@@ -404,16 +404,29 @@
...
>+
>+  /**
>+   * Sorts addons of the specified type by the specified property in the
>+   * Extensions Datasource container starting from the top of their container.
>+   * If the addons are already sorted then no action is performed.
>+   * @param   type
>+   *          The nsIUpdateItem type of the items to sort.
>+   * @param   propertyName
>+   *          The RDF property name used for sorting.
>+   * @param   isAscending
>+   *          true to sort ascending and false to sort descending
>+   */
>+  void sortTypeByProperty(in unsigned long type, in AString propertyName, in boolean isAscending);
All add-ons are now sorted everytime the ui is opened and whenever an install or upgrade occurs.

>Index: mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v
>retrieving revision 1.144.2.34
>diff -u -8 -r1.144.2.34 nsExtensionManager.js.in
>--- mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in	14 Mar 2006 22:18:19 -0000	1.144.2.34
>+++ mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in	25 Apr 2006 11:07:49 -0000
>@@ -233,16 +233,20 @@
...
>+function EM_D(integer) {
>+  return gRDF.GetDateLiteral(integer);
>+}
This is to support the installDate property I added from the StartupCache which is the last modified time on the add-on's directory (e.g. last install / upgrade date time). It isn't used by the ui (I don't think we should add it) but the cost is minimal and extension authors have asked for it.

>@@ -1606,17 +1610,17 @@
...
>-          jarFile.copyTo(chromeDir, jarFile.fileName);
>+          jarFile.copyTo(chromeDir, jarFile.leafName);
This threw while working on this patch and it turns out it is not an nsIURL

>@@ -3822,17 +3826,17 @@
...
>-      if (oldExtensionsDS.GetTarget(oldRes, EM_R("disabled"), true))
>+      if (oldExtensionsDS.GetTarget(oldRes, EM_R("isDisabled"), true))
>         ds.setItemProperty(item.id, EM_R("userDisabled"), EM_L("true"));
The RDF template used disabled and it was playing havoc with focus / selection since the XUL element also had this attribute. I changed disabled to isDisabled throughout the EM to denote an EM disabled add-on.

>@@ -4602,16 +4606,18 @@
...
>   installRequiresRestart: function(id, type) {
>     switch (type) {
>     case nsIUpdateItem.TYPE_EXTENSION:
>+    case nsIUpdateItem.TYPE_LOCALE:
>+    case nsIUpdateItem.TYPE_PLUGIN:
Treat locales and plug-ins the same as extensions

>@@ -4642,17 +4648,16 @@
...
>-    ds.updateProperty(id, "displayDescription");
Removed displayDescription throughout... this was added to display messages in the ui and was a pain to maintain so I moved it to the ui.

>@@ -4836,57 +4840,44 @@
...
>   uninstallItem: function(id) {
>-    var em = this;
>-    /**
...
>-    function cleanUpPendingStageFile(id) {
...
>-    }
Simplified and moved cleanUpPendingStageFile into uninstallItem

>@@ -4909,34 +4900,40 @@
>     else if (appDisabled == OP_NEEDS_DISABLE || userDisabled == OP_NEEDS_DISABLE) {
>       this._setOp(id, OP_NEEDS_DISABLE);
>       this._notifyAction(id, EM_ITEM_DISABLED);
>     }
>     else if (appDisabled == OP_NEEDS_ENABLE || userDisabled == OP_NEEDS_ENABLE) {
>       this._setOp(id, OP_NEEDS_ENABLE);
>       this._notifyAction(id, EM_ITEM_ENABLED);
>     }
>+    else {
>+      this._setOp(id, OP_NONE);
>+      this._notifyAction(id, EM_ITEM_CANCEL);
>+    }
Handles userDisabled == "true" when all others don't match

...
>+    if (op == OP_NEEDS_INSTALL || op == OP_NEEDS_UPGRADE)
>+      ds.updateDownloadState(PREFIX_ITEM_URI + id, "success");
Store install / upgrade state. This is stored in _progressData and kept around until restart so the ui can display the Installs view appropriately.

>@@ -5228,17 +5221,16 @@
...
>-      ds.updateProperty(id, "blocklistDetailsURL");
Moved blocklistDetailsURL into the ui.

>@@ -5326,31 +5318,49 @@
...
>   _confirmCancelDownloadsOnQuit: function(subject) {
>     if (this._downloadCount > 0) {
>+      // The observers will be notified again after this so set the download
>+      // count to 0 to prevent this dialog from being displayed again.
>+      this._downloadCount = 0;
This is part of the solution to the multiple cancel download prompts on exiting.

...
>+      if (subject instanceof Components.interfaces.nsISupportsPRBool)
>+        subject.data = result;
Cleanup for consistency.

...
>   _cancelDownloads: function() {
>     for (var i = 0; i < this._transactions.length; ++i)
>       gOS.notifyObservers(this._transactions[i], "xpinstall-progress", "cancel");
>-    gOS.removeObserver(this, "offline-requested");
>-    gOS.removeObserver(this, "quit-application-requested");
Now removed in onStateChange.

>@@ -5452,33 +5463,47 @@
...
>   addDownloads: function(items, itemCount, fromChrome) { 
>+    var ds = this.datasource;
>+    // Add observers only if they aren't already added for an active download
>+    if (this._downloadCount == 0) {
>+      gOS.addObserver(this, "offline-requested", false);
>+      gOS.addObserver(this, "quit-application-requested", false);
>+    }
...
>-    // Kick off the download process for this transaction
>-    gOS.addObserver(this, "offline-requested", false);
>-    gOS.addObserver(this, "quit-application-requested", false);
Everytime a download was added we added the observers!

>@@ -5525,76 +5550,71 @@
...
>   onStateChange: function(transaction, addon, state, value) {
>-    var url = addon.xpiURL;
>-    if (!(url in this._progressData)) 
>-      this._progressData[url] = { };
>-    this._progressData[url].state = state;
This _progressData was only used on exit to flush the download data to the datasource which provided no value whatsoever except to make sure we had partial downloads in the datasource. I removed it from ExtensionsManager and I am now using it in the datasource to provide state and progress without writing state and progress to the datasource.

...
>+    var ds = this.datasource;
>+    var id = addon.id != addon.xpiURL ? PREFIX_ITEM_URI + addon.id : addon.xpiURL;
Updates need the em item prefix to while new installs only have the url

...
>-    if (addon.id != addon.xpiURL) {
>-      var ds = this.datasource;
>-      ds.updateProperty(addon.id, "displayDescription");
>-      ds.updateProperty(addon.id, "availableUpdateURL");
>-      ds.updateProperty(addon.id, "updateable"); 
This is now done at the start of the update since we don't want a second updates to be kicked off.

>@@ -5707,17 +5727,17 @@
...
>-      this.removeDownload(addon.xpiURL, addon.type);
>+      this.removeDownload(addon.xpiURL);
This doesn't take a second param

>@@ -7192,16 +7209,31 @@
...
>   updateProperty: function(id, property) {
>     var item = getResourceForID(id);
>+    this._updateProperty(item, property);
>+  },
>+  
>+  /**
>+   * Notify views that this propery has changed (this is for properties that
>+   * are implemented by this datasource rather than by the inner in-memory
>+   * datasource and thus do not get free change handling). This allows updating
>+   * properties for download items which don't have the em item prefix in there
>+   ( resource value. In most instances updateProperty should be used.
>+   * @param   item
>+   *          The item to update the property for.
>+   * @param   property
>+   *          The property (less EM_NS) to update.
>+   */
>+  _updateProperty: function(item, property) {
updateProperty takes an id and then prefixes it with the em item prefix... this supports updating urls which don't have the prefix.

>@@ -7223,67 +7255,129 @@
...
>+  /**
>    * Determines if an Item is an active download
>    * @param   id
>-   *          The GUID of the item
>+   *          The ID of the item. This will be a uri scheme without the
>+   *          em item prefix so getProperty shouldn't be used.
>    * @returns true if the item is an active download, false otherwise.
>    */
>   isDownloadItem: function(id) {
>-    return this.getItemProperty(id, "downloadURL") != "";
>+    var downloadURL = stringData(this.GetTarget(gRDF.GetResource(id), EM_R("downloadURL"), true));
>+    return downloadURL && downloadURL != "";
A failed download doesn't have an em item prefix so this was badly broken.


>   addIncompatibleUpdateItem: function(name, url, type, version) {
>-    // type must be TYPE_EXTENSION for a multi_xpi to display in the manager.
>-    if (type == nsIUpdateItem.TYPE_MULTI_XPI)
>-      type = nsIUpdateItem.TYPE_EXTENSION;
I fixed the logic when getting icon urls when one isn't available so that if it isn't a theme it defauls to the generic xpinstall icon

>@@ -7400,64 +7526,54 @@
...
>-   * A hash of Addon IDs that we are currently looking for updates to. 
>-   */
>-  _updateURLs: { },
Was used by displayDescrion which has been moved to the ui.

Comment 19

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

The remainder of nsExtensionManager.js.in

>Index: mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>===================================================================
>@@ -7639,106 +7782,29 @@
>@@ -7786,60 +7852,49 @@
>-  _rdfGet_disabled: function(item, property) {
>+  _rdfGet_isDisabled: function(item, property) {
>     var id = stripPrefix(item.Value, PREFIX_ITEM_URI);
>     if (this.getItemProperty(id, "userDisabled") == "true" ||
>         this.getItemProperty(id, "appDisabled") == "true" ||
>         this.getItemProperty(id, "userDisabled") == OP_NEEDS_ENABLE ||
>         this.getItemProperty(id, "appDisabled") == OP_NEEDS_ENABLE)
>       return EM_L("true");
>-
>-    // Migrate disabled in the extensions datasource to userDisabled or
>-    // appDisabled as appropriate.
>-    var oldDisabled = this._inner.GetTarget(item, property, true);
>-    if (oldDisabled instanceof Components.interfaces.nsIRDFLiteral) {
>-      this._inner.Unassert(item, property, oldDisabled);
>-      if (this.getItemProperty(id, "compatible") == "true")
>-        this.setItemProperty(id, EM_R("userDisabled"), EM_L("true"));
>-      else
>-        this.setItemProperty(id, EM_R("appDisabled"), EM_L("true"));
>-
>-      return EM_L("true");
>-    }
This was added for nightly users and is already migrated on upgrade so it is safe to remove

>+  _rdfGet_addonID: function(item, property) {
>+    var id = this._inner.GetTarget(item, EM_R("downloadURL"), true) ? item.Value :
>+                                                                      stripPrefix(item.Value, PREFIX_ITEM_URI);
>+    return EM_L(id);
>+  },
This slipped in... it is intended to help get the id when rdf templates use real ids. I can remove it if you prefer.

...
>   _rdfGet_updateable: function(item, property) {
>     var id = stripPrefix(item.Value, PREFIX_ITEM_URI);
>     var opType = this.getItemProperty(id, "opType");
>     if (opType == OP_NEEDS_INSTALL || opType == OP_NEEDS_UNINSTALL ||
>         opType == OP_NEEDS_UPGRADE ||
>         this.getItemProperty(id, "appManaged") == "true")
>       return EM_L("false");
> 
>-    try {
>-      if (!gPref.getBoolPref(PREF_EM_ITEM_UPDATE_ENABLED.replace(/%UUID%/, id)))
>-        return EM_L("false");
>-    }
>-    catch (e) { }
>+    if (getPref("getBoolPref", (PREF_EM_ITEM_UPDATE_ENABLED.replace(/%UUID%/, id), false)) == true)
>+      return EM_L("false");
Cleanup

>@@ -7896,41 +7951,41 @@
...
>-   * Get the em:name property (name of the item - template builder calls
>-   * GetTargets on this one for some reason).
>-   */
>-  _rdfGets_name: function(item, property) {
>-    return this._getLocalizablePropertyValues(item, property);
>-  },
Foxed this in GetTargets

>@@ -7945,16 +8000,20 @@
...
>   HasAssertion: function(source, property, target, truthValue) {
>     if (!source || !property || !target)
>       return false;
>+
>+    var getter = "_rdfGet_" + stripPrefix(property.Value, PREFIX_NS_EM);
>+    if (getter in this)
>+      return this[getter](source, property) == target;
Needed by the template builder for properties implemented by the datasource.

Comment 20

[Scott MacGregor]

Comment on attachment 219741 [details] [diff] [review]
changes to mail (checked in)

Thanks Rob.

Comment 21

[Benjamin Smedberg]

Comment on attachment 219743 [details] [diff] [review]
Toolkit changes

>-      if (oldExtensionsDS.GetTarget(oldRes, EM_R("disabled"), true))
>+      if (oldExtensionsDS.GetTarget(oldRes, EM_R("isDisabled"), true))

You're renaming the property to avoid the UI issues with a disabled attribute as mentioned in extensions.js?

>   installRequiresRestart: function(id, type) {
>     switch (type) {
>     case nsIUpdateItem.TYPE_EXTENSION:
>+    case nsIUpdateItem.TYPE_LOCALE:
>+    case nsIUpdateItem.TYPE_PLUGIN:
>       return true;

perhaps this should be the default: case, and make TYPE_THEME the special-case?

>+      // Addons with an opType of OP_NEEDS_INSTALL only have a staged xpi file
>+      // and are removed immediately since the uninstall can't be canceled.

Isn't it possible for an extension to be already installed and also have OP_NEEDS_INSTALL (when upgrading)? In this case wouldn't you need to remove both the staged XPI and the extension files? (Or perhaps there's an OP_NEEDS_UPGRADE flag that I missed)?

>Index: mozilla/toolkit/mozapps/extensions/content/extensions.xml

>-# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>-# ***** BEGIN LICENSE BLOCK *****

>-# ***** END LICENSE BLOCK *****

Why did you remove the license block? Seems to me that's not kosher.

>+<!-- Override of richlistbox's _setItemSelection. Allows us to prevent focus
>+     when using the keyboard to navigate views and to ensure an element is
>+     visible after it has had a chance to reflow. -->

Dare I ask why we need to do this here, instead of in richlistbox directly? (What is special about EM that doesn't apply to other possible users of richlistbox?) Overriding private methods of the base binding sounds fragile to me.

>+<!-- based on preferences.xml paneButton -->
>+  <binding id="viewbutton" extends="chrome://global/content/bindings/radio.xml#radio">

Have you run this by aaronlev or mpilgrim? It looks like it has the accessibility hooks, but the accessibility code for XUL is more than a bit fragile.

>Index: mozilla/toolkit/mozapps/extensions/content/extensions.xul

>-<?xml-stylesheet href="chrome://global/skin/" type="text/css"?> 
> <?xml-stylesheet href="chrome://mozapps/content/extensions/extensions.css"?>
> <?xml-stylesheet href="chrome://mozapps/skin/extensions/extensions.css"?>
>+<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>

This is unusual: you should generally include global/skin first, then override as necessary.

> <window xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+        xmlns:xhtml2="http://www.w3.org/TR/xhtml2"
>+        xmlns:wairole="http://www.w3.org/2005/01/wai-rdf/GUIRoleTaxonomy#"

wairole is accessibility, right? What is xhtml2 for? I don't see either of these used later down in the patch.

>Index: mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd

>+<!ENTITY themePreview.width               "220">

Does this need to be localizable?

>+<!ENTITY plugins.label                    "Plug-ins">

It's add-ons, but plugins.

>+<!ENTITY cmd.restartApp.tooltip           "Restarts &brandShortName; to finish the installation">

"Restarts" sounds a little strange to me in a tooltip, for some reason; not sure if there's a way to avoid the passivity.

>+<!ENTITY cmd.enableAll.label              "Enable All">
>+<!ENTITY cmd.enableAll.accesskey          "a">
>+<!ENTITY cmd.enableAll.tooltip            "Enable all displayed Add-ons">
>+<!ENTITY cmd.disableAll.label             "Disable All">
>+<!ENTITY cmd.disableAll.accesskey         "a">
>+<!ENTITY cmd.disableAll.tooltip           "Disable all displayed Add-ons">

enableAll and disableAll have the same accesskey: are they used in different contexts?

>+<!-- Status Messsages -->
>+<!ENTITY needsDependencies.label          "Requires additional items">
>+<!ENTITY blocklisted.label                "Disabled for your protection">
>+<!ENTITY needsDisable.label               "Will be disabled when &brandShortName; is restarted">
>+<!ENTITY needsEnable.label                "Will be enabled when &brandShortName; is restarted">
>+<!ENTITY needsInstall.label               "Will be installed when &brandShortName; is restarted">
>+<!ENTITY needsUninstall.label             "Will be uninstalled when &brandShortName; is restarted">
>+<!ENTITY needsUpgrade.label               "Will be upgraded when &brandShortName; is restarted">
>+<!ENTITY needsDisableBlocklisted.label    "Will be disabled for your protection when &brandShortName; is restarted">

I think I need to look at a mockup again... how are these labels used? The sentence fragments with initial capitals but without periods look a little odd to me.

>+# restartBeforeEnableMessage=Will be enabled when %S is restarted.
>+# restartBeforeDisableMessage=Will be disabled when %S is restarted.
>+# restartBeforeDisableBlocklisted=Will be disabled when %S is restarted for your protection.
>+# restartBeforeUninstallTitle=Uninstall
>+# restartBeforeUninstallMessage=Will be uninstalled when %S is restarted.
>+# restartBeforeInstallMessage=Will be installed when %S is restarted.
>+# restartBeforeUpgradeMessage=Will be upgraded when %S is restarted.

This will cause issues with l10n tools. These should either be there or removed, not commented out.

>+installAdoonPickerTitle=Select an addon to install

add-on

>+blocklistDetailsURL=https://www.mozilla.com/blocklist/

This URL should not be in the toolkit (e.g. songbird will run their own blocklist, not a mozilla.com blocklist). I imagine this should be a pref.

r- based on the blocklistURL issue only, the rest are just nits that you can fix on checkin.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 219742 [details] [diff] [review]
changes to calendar

Over to mvl for review, as this seems to be in Sunbird & extension-specific code, and I'm not too familiar with that.

Comment 23

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

Benjamin, thank you for the quick review! New patch coming up.

(In reply to comment #21)
> You're renaming the property to avoid the UI issues with a disabled attribute
> as mentioned in extensions.js?
Yes

> perhaps this should be the default: case, and make TYPE_THEME the special-case?
Fixed

> Isn't it possible for an extension to be already installed and also have
> OP_NEEDS_INSTALL (when upgrading)? In this case wouldn't you need to remove
> both the staged XPI and the extension files? (Or perhaps there's an
> OP_NEEDS_UPGRADE flag that I missed)?
There is an OP_NEEDS_UPGRADE and if opType is OP_NEEDS_UPGRADE when uninstalling it will remove the staged file and set OP_NEEDS_UNINSTALL to the extension on restart.

> Why did you remove the license block? Seems to me that's not kosher.
Ooops! Fixed

> >+<!-- Override of richlistbox's _setItemSelection. Allows us to prevent focus
> >+     when using the keyboard to navigate views and to ensure an element is
> >+     visible after it has had a chance to reflow. -->
> 
> Dare I ask why we need to do this here, instead of in richlistbox directly?
> (What is special about EM that doesn't apply to other possible users of
> richlistbox?) Overriding private methods of the base binding sounds fragile to
> me.
The richlistbox implementation always focuses the richlistbox if there is a last-selected element specified. When using the keyboard to navigate the different views this will cause the richlistbox to receive focus which interrupts keyboard navigation... this suppresses it. This same code in richlistbox occurs prior to reflow finsihing on the element which makes ensureElementIsVisible not do the right thing when the content expands as in the case of the buttons and status messages. I'll file followup bugs on richlistbox for these two issues.

> >+<!-- based on preferences.xml paneButton -->
> >+  <binding id="viewbutton" extends="chrome://global/content/bindings/radio.xml#radio">
> 
> Have you run this by aaronlev or mpilgrim? It looks like it has the
> accessibility hooks, but the accessibility code for XUL is more than a bit
> fragile.
I'll run it by them... this uses the same accessibility hooks as preferences.xml so will hopefully be good to go.

> >Index: mozilla/toolkit/mozapps/extensions/content/extensions.xul
> 
> >-<?xml-stylesheet href="chrome://global/skin/" type="text/css"?> 
> > <?xml-stylesheet href="chrome://mozapps/content/extensions/extensions.css"?>
> > <?xml-stylesheet href="chrome://mozapps/skin/extensions/extensions.css"?>
> >+<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
> 
> This is unusual: you should generally include global/skin first, then override
> as necessary.
Fixed.

> >+        xmlns:xhtml2="http://www.w3.org/TR/xhtml2"
> >+        xmlns:wairole="http://www.w3.org/2005/01/wai-rdf/GUIRoleTaxonomy#"
> 
> wairole is accessibility, right? What is xhtml2 for? I don't see either of
> these used later down in the patch.
I'll run this by aaronlev or mpilgrim as well. I stole this from the prefpanels implementation and it is used on the radiogroup.
<radiogroup id="viewGroup" xhtml2:role="wairole:list" persist="last-selected"

> >+<!ENTITY themePreview.width               "220">
> 
> Does this need to be localizable?
It doesn't... good catch and I moved it into extensions.xul.

> It's add-ons, but plugins.
Fixed

> >+<!ENTITY cmd.restartApp.tooltip           "Restarts &brandShortName; to finish the installation">
> 
> "Restarts" sounds a little strange to me in a tooltip, for some reason; not
> sure if there's a way to avoid the passivity.
I'll ask beltzner for a better string in bug 308916

> enableAll and disableAll have the same accesskey: are they used in different
> contexts?
Fixed. I haven't added the buttons for these yet and the plan is to only have one shown at a time but this may change.

> >+<!-- Status Messsages -->
> >+<!ENTITY needsDependencies.label          "Requires additional items">
> >+<!ENTITY blocklisted.label                "Disabled for your protection">
> >+<!ENTITY needsDisable.label               "Will be disabled when &brandShortName; is restarted">
> >+<!ENTITY needsEnable.label                "Will be enabled when &brandShortName; is restarted">
> >+<!ENTITY needsInstall.label               "Will be installed when &brandShortName; is restarted">
> >+<!ENTITY needsUninstall.label             "Will be uninstalled when &brandShortName; is restarted">
> >+<!ENTITY needsUpgrade.label               "Will be upgraded when &brandShortName; is restarted">
> >+<!ENTITY needsDisableBlocklisted.label    "Will be disabled for your protection when &brandShortName; is restarted">
> 
> I think I need to look at a mockup again... how are these labels used? The
> sentence fragments with initial capitals but without periods look a little odd
> to me.
Added periods and removed the needsDisableBlocklisted which is no longer necessary (e.g. we display a message that the item is blocklisted and the needs disable message separately... I prefer not to special case this.

> This will cause issues with l10n tools. These should either be there or
> removed, not commented out.
Removed

> >+installAdoonPickerTitle=Select an addon to install
> 
> add-on
Fixed

> This URL should not be in the toolkit (e.g. songbird will run their own
> blocklist, not a mozilla.com blocklist). I imagine this should be a pref.
Fixed

Comment 24

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

Attachment: Updated to comments (inlcudes three additional minor changes)

This should address the comments and it also includes:
1) In nsExtensionManager.js.in I removed setting _downloadCount to 0 from _confirmCancelDownloadsOnOffline. This is still a bit broken and I will file a followup bug for it.
2) In extensions.js I added a check for xpinstall.enabled to installWithFilePicker.
3) I added a new status message for install success when a restart is needed.

Comment 25

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

Comment on attachment 219810 [details] [diff] [review]
Updated to comments (inlcudes three additional minor changes)

Forgot to mention that the original patch didn't include extensions/content/extensions.css and this one does

Comment 26

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

Attachment: all (rev 2)

Comment 27

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

Attachment: all (rev 3)

Includes changes to pinstripe

Comment 28

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

Comment on attachment 219863 [details] [diff] [review]
all (rev 3)

bah... I didn't include the jar.mn for pinstripe. The toolkit patch for review will include it.

Comment 29

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

Attachment: Toolkit changes (rev 3)

Besides the items listed in comment #24 this also has the pinstripe changes. I believe additional tweaking will be needed for pinstripe after alpha 2 and this can be done during the visual refresh.

Comment 30

[Benjamin Smedberg]

Comment on attachment 219864 [details] [diff] [review]
Toolkit changes (rev 3)

There's still one stray "addons", but I think you're leaving the strings for touchup later anyway.

Comment 31

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

Attachment: toolkit patch includes XULRunner prefs (checked in)

Comment 32

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

Comment on attachment 219742 [details] [diff] [review]
changes to calendar

I opened Bug 335618 for the Calendar changes

Comment 33

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

Comment on attachment 219971 [details] [diff] [review]
toolkit patch includes XULRunner prefs (checked in)

I went ahead and fixed the cases where it should be Add-on

Comment 34

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

Fixed on trunk. Please file new bugs for any fallout from this. Thanks.

BTW: The strange behavior when scrolling and selecting on the trunk pre-existed this patch landing and is Bug 330073 - Weird jumpy selection behaviour with richlistbox (e.g. extension manager and download manager)

Comment 35

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

Filed Bug 335645 for the review of the accessibility hooks.
Filed Bug 335642 for the richlistbox overrides that were needed.

Comment 36

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

Note: both Bug 310843 and Bug 326274 should land on the branch to allow installing xpi's as locales and plugins if / when this lands on the branch

Comment 37

[Robert Kaiser]

Does Firefox follow different rules or is it a bug that everything says "Add-ons", but the Firefox changes says "Addons"?

Comment 38

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

(In reply to comment #37)
> Does Firefox follow different rules or is it a bug that everything says
> "Add-ons", but the Firefox changes says "Addons"?
No, that is incorrect... it should be "Add-ons". Please file a bug for it

Comment 39

[Robert Kaiser]

(In reply to comment #38)
> No, that is incorrect... it should be "Add-ons". Please file a bug for it

Done, see bug 335871

Comment 40

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

Checked in on MOZILLA_1_8_BRANCH after getting a verbal a+ from bsmedberg, mconnor, and mscott

Comment 41

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

*** Bug 340259 has been marked as a duplicate of this bug. ***

Comment 42

[Worcester12345]

This and 337696 are branch bugs,(and Bugzilla Bug 341166
Problems with dead progress bars and the restart button in which fixed that) is for trunk. Not sure how that affects this, but it should be noted so it isn't missed down the road.

Comment 43

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

Let's not add depends on or blocks bugs to this closed meta bug that was used for the Firefox 2.0 release since that is long gone.

Comment 44

[logo design]

Thanks for the step by step instructions. I now know how to add link using html codes. [url=http://www.logo-genie.com]logo design[/url] . <a href="http://www.logo-genie.com/">logo design</a>