474480 - Update the Add-ons Manager UI

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

I've wireframed out the UI I'd like to see for the fennec add-ons manager here:

https://wiki.mozilla.org/Mobile/UI/Designs/TouchScreen/workingUI/add-ons_concepts

from the third image on.

It's a big vertically pannable list.  There are two sections, one for add-ons you have, and one for add-ons you can get, which is filled, by default, with five items from the fennec recommended add-on set (provided by the api).  The two sections are sequential, in that you can pan past the first one to the second, but there are also table of contents buttons at the top that skip you immediately (or quickly, with animation) to the section in question.

A add-on row expands when tapped to reveal its delicious contents -- enable and disable buttons.  The mockups show a button to open a prefs panel -- instead (mockup to follow shortly), we should embed the add-on's prefs in the expanded section.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

What about plugins? We'll have to think about Themes too in the long term.

Comment 2

[Madhava Enros [:madhava]]

Yeah - not sure about plugins.  For themes <cue controversy> I think I'd like to see them interleaved with the extension -- i.e. not separate lists.  The tag in the upper right-hand corner tells you whether what you're looking at is a theme or extension.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 1

This patch adds much of the Add-on Manager functionality:
* Displays local add-ons
* Allows enable, disable and uninstall of local add-ons
* Shows local add-ons options in an inline iframe
* Displays recommended and search results from AMO
* Installs add-ons from AMO
* Installs add-ons from webpages (XPI files)
* Displays a "restart needed" notification for actions that require it

This patch does not:
* Display progress or alerts (start and end) about add-on installation
* Handle canceling some of the actions (enable/disable/uninstall)

This does start using common richlistbox/richlistitem CSS rules to create a common look and feel. These rules are currently in themes/*/browser.css but might be better served in themes/*/platform.css.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 2

Same as previous patch, but it adds a nsIXPIDialogService implmentation so we can control the XPI "confirm install" dialog and any progress dialogs. And by "control" I mean "avoid".

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

Here's a decent patch to get landed. It supports all add-on manager functionality regarding add-ons:
* lists installed add-ons:
  * supports enable, disable and uninstall action and shows the restart notif
  * supports toggling actions (enable, disable and uninstall) and hides the restart notif, if possible
* lists add-ons from AMO
  * recommended and search modes
  * search show a "search terms" textbox
  * shows a progressmeter during installation and a restart notif afterward
  * shows a popup alert when starting and completing installation (only if the add-ons manager is not visible)

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 372505 [details] [diff] [review]
patch


>+/* addons states ----------------------------------------------------------- */
>+.hide-on-enable,
>+.show-on-uninstall,
>+.show-on-install,
>+.show-on-restart,
>+richlistitem[isDisabled="true"] .hide-on-disable {
>+  display: none;
>+}
>+
>+richlistitem[opType="needs-restart"] .show-on-restart,
>+richlistitem[opType="needs-uninstall"] .show-on-uninstall,
>+richlistitem[opType="needs-install"] .show-on-install,
>+richlistitem[opType="needs-enable"] .show-on-enable,
>+richlistitem[opType="needs-disable"] .show-on-disable,
>+richlistitem[isDisabled="true"] .show-on-disable {
>+  display: -moz-box;
>+}
>+
>+richlistitem[opType="needs-restart"] .hide-on-restart,
>+richlistitem[opType="needs-uninstall"] .hide-on-uninstall,
>+richlistitem[opType="needs-install"] .hide-on-install,
>+richlistitem[opType="needs-enable"] .hide-on-enable,
>+richlistitem[opType="needs-disable"] .hide-on-disable {
>+  display: none;
>+}

I am having second thoughts on putting these rules in the theme folders. I am thinking this should go into the content/browser.css file as it has no real style presentation rules.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 372505 [details] [diff] [review]
patch

Adding Dave to check over the parts I pulled from the toolkit add-on manager.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

I forgot to remove "extensions.css" from themes\wince\jar.mn

It's been fixed locally. I'll wait for review comments to post an updated patch.

Comment 9

[Dave Townsend [:mossop]]

(In reply to comment #6)
> (From update of attachment 372505 [details] [diff] [review])
> 
> I am having second thoughts on putting these rules in the theme folders. I am
> thinking this should go into the content/browser.css file as it has no real
> style presentation rules.

Yeah put them into the content styles. I keep meaning to do the same for the similar rules in the toolkit themes.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 372505 [details] [diff] [review]
patch

This is a first overview pass, will look a little more at the details in an hour or so. Something feels a little wrong with the control of the restart notification bar but I can't quite put my finger on it.

Just as a thought, it might be worth just using an nsIRDFObserver on the EM datasource in order to keep the local items up to date, as a way of avoiding templates but still keeping things in better sync over time. It could also detect new add-on installed from content.

>+  showMessage: function em_showMessage(aMsg, aValue, aButtonLabel, aShowCloseButton, aNotifyData) {
>+    let notification = this._msg.getNotificationWithValue(aValue);
>+    if (notification)
>+      return;
>+
>+    let buttons = null;
>+    if (aButtonLabel) {
>+      buttons = [ {
>+        label: aButtonLabel,
>+        accesskey: "",
>+        data: aNotifyData,
>+        callback: function(aNotification, aButton) {
>+          let os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>+          os.notifyObservers(null, "addons-message-notification", aButton.data);
>+          aNotification.close();
>+          return true;
>+        }
>+      } ];
>+    }

I wouldn't bother dispatching the callback through the observer service, just pass aButton.data to a function on ExtensionsView. I think the observer notification in toolkit is left over from before the notificationbox widget even existed. Also I think the notification gets closed automatically depending on the return from the callback.

>+    let self = this;
>+    setTimeout(function() {
>+      self.getAddonsFromLocal();
>+      self.getAddonsFromRepo("");
>+    }, 0);

Might be worth just initialising the list when the add-ons container is first shown, should save you some time if the user never uses it.

>+  getAddonsFromLocal: function em_getAddonsFromLocal() {
>+    let items = this._extmgr.getItemList(Ci.nsIUpdateItem.TYPE_ANY, {});
>+
>+    var rdf = Cc["@mozilla.org/rdf/rdf-service;1"].getService(Ci.nsIRDFService);
>+
>+    for (let i = 0; i < items.length; i++) {
>+      let isDisabled = false;
>+      let desc = "";
>+      let optionsURL = "";
>+
>+      // Some information is not directly accessible from the extmgr
>+      var itemResource = rdf.GetResource(PREFIX_ITEM_URI + items[i].id);
>+      if (itemResource) {
>+        var ds = this._extmgr.datasource;
>+
>+        var isDisabledTarget = ds.GetTarget(itemResource, rdf.GetResource(PREFIX_NS_EM + "isDisabled"), true);
>+        if (isDisabledTarget && isDisabledTarget instanceof Ci.nsIRDFLiteral)
>+          isDisabled = (isDisabledTarget.Value == "true");
>+
>+        var descTarget = ds.GetTarget(itemResource, rdf.GetResource(PREFIX_NS_EM + "description"), true);
>+        if (descTarget && descTarget instanceof Ci.nsIRDFLiteral)
>+          desc = descTarget.Value;
>+
>+        var optionsTarget = ds.GetTarget(itemResource, rdf.GetResource(PREFIX_NS_EM + "optionsURL"), true);
>+        if (optionsTarget && optionsTarget instanceof Ci.nsIRDFLiteral)
>+          optionsURL = optionsTarget.Value;
>+      }
>+
>+      let listitem = this._createItem(items[i], "local");
>+      listitem.setAttribute("isDisabled", isDisabled);
>+      listitem.setAttribute("description", desc);
>+      listitem.setAttribute("optionsURL", optionsURL);
>+      this._list.insertBefore(listitem, this._repoItem);

You probably want to get the opType for the item and add that too. isDisabled covers a variety of reasons for being disabled, at the very least you want to get appDisabled and userDisabled. If the item is appDisabled then the user shouldn't be offered the choice to enable (it won't work anyway but it's crappy UI). Perhaps for a later revision but you'd want to get blocklisted, blocklistedsoft, compatible, providesUpdatesSecurely for the full set of states.

>+  enable: function em_enable(aItem) {
>+    this._extmgr.enableItem(this._getIDFromURI(aItem.id));
>+    if (aItem.getAttribute("isDisabled") == "true")
>+      this.showRestart();
>+    else
>+      this.hideRestart();
>+    aItem.setAttribute("opType", "needs-enable");

You actually need to get the new opType for the item from the rdf and update the restart message based on that. In some cases enableItem may not actually enable the item so you want to check that something is actually pending for real. Same goes for the other operations.

>+  cancelUninstall: function em_cancelUninstall(aItem) {
>+    this._extmgr.uninstallItem(this._getIDFromURI(aItem.id));
>+    hideRestart();
>+    aItem.removeAttribute("opType");
>+  },

This doesn't seem to cancel the uninstall.

>+
>+  _installCallback: function em__installCallback(aItem, aStatus) {
>+    if (aStatus == -210) {
>+      // TODO: User cancelled
>+    }
>+    else if (aStatus < 0) {
>+      // TODO: Some other error
>+    }
>+    else {
>+      // Sucess
>+    aItem.setAttribute("opType", "needs-restart");

Again, get the actual opType I think.

>+  confirmInstall: function(aParent, aPackages, aCount) {
>+    // Return true to get the install started
>+    return true;
>+  },

No confirmation dialog? Shaver would not be happy I think.

>+  openProgressDialog: function(aPackages, aCount, aManager) {

I think you can basically strip this function and just call back to aManager.openProgressDialog rather than duplicating what it does.

Comment 11

[Dave Townsend [:mossop]]

Another probably easy way to handle adding newly installed items to the list would be to either just do it from onInstallEnded or just refresh the entire list in onInstallsCompleted.

Comment 12

[Dave Townsend [:mossop]]

(In reply to comment #10)
> >+  _installCallback: function em__installCallback(aItem, aStatus) {
> >+    if (aStatus == -210) {
> >+      // TODO: User cancelled
> >+    }
> >+    else if (aStatus < 0) {
> >+      // TODO: Some other error
> >+    }
> >+    else {
> >+      // Sucess
> >+    aItem.setAttribute("opType", "needs-restart");
> 
> Again, get the actual opType I think.

Yeah ignore that. But maybe instead delete the item from the search results and add it to the local list?

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 372505 [details] [diff] [review]
patch

>+  onInstallsCompleted: function() {
>+    let strings = document.getElementById("bundle_browser");
>+
>+    // If even one add-on succeeded, display the restart notif
>+    if (this._succeeded.length > 0) {
>+      ExtensionsView.showMessage(strings.getString("addonsRestart"), "restart-app",
>+                                 strings.getString("addonsRestartButton.label"), false, "addons-restart-app");
>+    }

I think you want to be calling showRestart instead to keep _restartCount at the right level.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #10)
> (From update of attachment 372505 [details] [diff] [review])
> Just as a thought, it might be worth just using an nsIRDFObserver on the EM
> datasource in order to keep the local items up to date, as a way of avoiding
> templates but still keeping things in better sync over time. It could also
> detect new add-on installed from content.

I need to talk to Madhava about how to best display newly installed add-ons. Will revisit this in a followup bug

> 
> >+        callback: function(aNotification, aButton) {
> >+          let os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
> >+          os.notifyObservers(null, "addons-message-notification", aButton.data);
> >+          aNotification.close();
> >+          return true;
> 
> I wouldn't bother dispatching the callback through the observer service, just
> pass aButton.data to a function on ExtensionsView. I think the observer
> notification in toolkit is left over from before the notificationbox widget
> even existed. Also I think the notification gets closed automatically depending
> on the return from the callback.

Done.

> >+    let self = this;
> >+    setTimeout(function() {
> >+      self.getAddonsFromLocal();
> >+      self.getAddonsFromRepo("");
> >+    }, 0);
> 
> Might be worth just initialising the list when the add-ons container is first
> shown, should save you some time if the user never uses it.

Done. We wait until the panel is actually shown to display the lists.

> >+  getAddonsFromLocal: function em_getAddonsFromLocal() {
>
> >+      let listitem = this._createItem(items[i], "local");
> >+      listitem.setAttribute("isDisabled", isDisabled);
> >+      listitem.setAttribute("description", desc);
> >+      listitem.setAttribute("optionsURL", optionsURL);
> >+      this._list.insertBefore(listitem, this._repoItem);
> 
> You probably want to get the opType for the item and add that too. isDisabled
> covers a variety of reasons for being disabled, at the very least you want to
> get appDisabled and userDisabled. If the item is appDisabled then the user
> shouldn't be offered the choice to enable (it won't work anyway but it's crappy
> UI). Perhaps for a later revision but you'd want to get blocklisted,
> blocklistedsoft, compatible, providesUpdatesSecurely for the full set of
> states.

OK. I added "appDisabled" and "opType" here too.
I will add the others to a followup bug

> >+  enable: function em_enable(aItem) {
> >+    this._extmgr.enableItem(this._getIDFromURI(aItem.id));
> >+    if (aItem.getAttribute("isDisabled") == "true")
> >+      this.showRestart();
> >+    else
> >+      this.hideRestart();
> >+    aItem.setAttribute("opType", "needs-enable");
> 
> You actually need to get the new opType for the item from the rdf and update
> the restart message based on that. In some cases enableItem may not actually
> enable the item so you want to check that something is actually pending for
> real. Same goes for the other operations.

Done. We now pull the "opType" from RDF and use it when checking for a needed restart.

> >+  cancelUninstall: function em_cancelUninstall(aItem) {
> >+    this._extmgr.uninstallItem(this._getIDFromURI(aItem.id));
> >+    hideRestart();
> >+    aItem.removeAttribute("opType");
> >+  },
> 
> This doesn't seem to cancel the uninstall.

It helps if I call this._extmgr.cancelUninstallItem

> >+  _installCallback: function em__installCallback(aItem, aStatus) {
>
> >+    else {
> >+      // Sucess
> >+    aItem.setAttribute("opType", "needs-restart");
> 
> Again, get the actual opType I think.

I skipped this for now. We need a better story for the newly installed add-ons

> >+  confirmInstall: function(aParent, aPackages, aCount) {
> >+    // Return true to get the install started
> >+    return true;
> >+  },
> 
> No confirmation dialog? Shaver would not be happy I think.

Followup bug (as much as I don't want it)

> 
> >+  openProgressDialog: function(aPackages, aCount, aManager) {
> 
> I think you can basically strip this function and just call back to
> aManager.openProgressDialog rather than duplicating what it does.

I can't do that because the aManager is the nsXPInstallManager, which will try to open a dialog in ::OpenProgressDialog

http://mxr.mozilla.org/mozilla-central/source/xpinstall/src/nsXPInstallManager.cpp#540

(In reply to comment #13)
> (From update of attachment 372505 [details] [diff] [review])
> >+  onInstallsCompleted: function() {
> >+    let strings = document.getElementById("bundle_browser");
> >+
> >+    // If even one add-on succeeded, display the restart notif
> >+    if (this._succeeded.length > 0) {
> >+      ExtensionsView.showMessage(strings.getString("addonsRestart"), "restart-app",
> >+                                 strings.getString("addonsRestartButton.label"), false, "addons-restart-app");
> >+    }
> 
> I think you want to be calling showRestart instead to keep _restartCount at the
> right level.

Done

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #15)
> > >+  openProgressDialog: function(aPackages, aCount, aManager) {
> > 
> > I think you can basically strip this function and just call back to
> > aManager.openProgressDialog rather than duplicating what it does.
> 
> I can't do that because the aManager is the nsXPInstallManager, which will try
> to open a dialog in ::OpenProgressDialog
> 
> http://mxr.mozilla.org/mozilla-central/source/xpinstall/src/nsXPInstallManager.cpp#540

It will try to open a dialog unless the dialog is already open, so you could just set PREF_XPINSTALL_STATUS_DLG_CHROME to something that is already open, say the Fennec main UI. No big deal really, just saves some code duplication.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Followup bug is bug 489229

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 373727 [details] [diff] [review]
patch 2

diff --git a/chrome/content/bindings/extensions.xml b/chrome/content/bindings/extensions.xml

>++  <binding id="extension-local" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>++    <content orient="vertical">
>+      <xul:hbox align="start">

Same comment as the download patch (orient="vertical" vs. hbox, applies to all bindings).

>+      <method name="toggleOptions">

>+            let box = document.getAnonymousElementByAttribute(this, "anonid", "options-box");

>+            let frame = document.getAnonymousElementByAttribute(this, "anonid", "options-frame");

_optionsHandler.box/frame?

diff --git a/chrome/content/extensions.js b/chrome/content/extensions.js

>+var ExtensionsView = {

>+  _isXPInstallEnabled: function isXPInstallEnabled() {

>+/*
>+    var msgText = getExtensionString(locked ? "xpinstallDisabledMsgLocked" :
>+                                              "xpinstallDisabledMsg");
>+    var buttonLabel = locked ? null : getExtensionString("enableButtonLabel");
>+    var buttonAccesskey = locked ? null : getExtensionString("enableButtonAccesskey");
>+    var notifyData = locked ? null : "addons-enable-xpinstall";
>+    showMessage(URI_NOTIFICATION_ICON_WARNING,
>+                msgText, buttonLabel, buttonAccesskey,
>+                !locked, notifyData);
>+*/

Why is this commented out?

>+  _clearSection: function ev_clearSection(aSectionStart, aSectionEnd) {

This is kind of confusing... perhaps it should just take a section name (e.g. "repo"), and hardcode the start/end items, that way there's only one place to change if we change the sort order or XUL layout of the items?

>+  init: function ev_init() {

>+    let self = this;
>+    let panels = document.getElementById("panel-items");
>+    panels.addEventListener("select",

Hmm, I guess this is here to avoid doing the work if you're just showing one of the other panels? Listener could remove itself after the first time, right?

>+  _installCallback: function ev__installCallback(aItem, aStatus) {
>+    Components.reportError("callback: " + aItem.id)

Remove this?

>+  displaySearchResults: function ev_displaySearchResults(aAddons, aTotalResults, aIsRecommended) {

>+    if (aIsRecommended) {

>+    if (aAddons.length == 0) {

Doesn't matter too much, but this check seems like it should go before the previous one.

>+      // Strip out any items with potentially unsafe urls
>+      let unsafe = false;
>+      for (var j = 0; j < urlproperties.length; j++) {
>+        if (!this._isSafeURI(addon[urlproperties[j]])) {
>+          unsafe = true;
>+          break;
>+        }
>+      }
>+      if (unsafe)
>+        continue;

if (urlproperties.some(function (p) !this._isSafeURI(addon[p]), this))
  continue;

>+  resetSearch: function ev_resetSearch() {
>+    let mode = document.getElementById("addons-repo-mode").value = "recommended";
>+    this.toggleMode();

"mode" is unused?

>+function XPInstallDownloadManager() {

>+  addDownloads: function (aParams, aManager) {

>+    for (var i = 0; i < count;) {

>+      let uri = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService).newURI(url, null, null);

Probably should cache the ioService globally, given its use here and in isSafeURI (also in a loop).

>+  onInstallEnded: function(aAddon, aStatus) {
>+    // Track success/failure for each addon
>+    if (aStatus < 0)

!Components.isSuccessCode(aStatus) (also later in this function)

>+  onInstallsCompleted: function() {

>+      message = strings.getString("alertAddonsFailed");

String name doesn't match:

>+alertAddonsFail=Installation failed

>+  QueryInterface: function (aIID) {

Worth using xpcomutils?

diff --git a/components/xpiDialogService.js b/components/xpiDialogService.js

>+function XPInstallDialogService() { }

>+  openProgressDialog: function(aPackages, aCount, aManager) {

>+    dpb.SetInt(0, 2);                       // OK and Cancel buttons

This doesn't actually appear to be used by the "xpinstall-download-started" observer... worth a separate bug on fixing this in general (both for us and the nsXPInstallManager code)?

>+  onStateChange: function(aIndex, aState, aError) { debugger; Components.reportError("onstatechange"); },
>+  onProgress: function(aIndex, aValue, aMax) { debugger; Components.reportError("onprogress"); }

Remove these debugger statements and reportErrors before landing?

diff --git a/locales/en-US/chrome/browser.properties b/locales/en-US/chrome/browser.properties

>+addonsSearchFailed.label=%S couldn't retrieve add-ons
>+addonsSearchFailed.button=OK

These appear to be unused?

diff --git a/themes/hildon/browser.css b/themes/hildon/browser.css

>+richlistitem .show-on-select {
>+  visibility: collapse;
>+}
>+
>+richlistitem[selected="true"] .show-on-select {
>+  visibility: visible;
>+}
>+
>+richlistitem .hide-on-select {
>+  visibility: visible;
>+}
>+
>+richlistitem[selected="true"] .hide-on-select {
>+  visibility: collapse;
>+}

Shouldn't these be in content/ (same applies to the other theme)?

Would be good to get specific followups on the TODO comments, and on the confirmation prompt (should be separate from bug 489229, I think).

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #18)

> >++  <binding id="extension-local" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
> >++    <content orient="vertical">
> >+      <xul:hbox align="start">
> 
> Same comment as the download patch (orient="vertical" vs. hbox, applies to all
> bindings).

I was able to fix one binding. The other two were different.

> >+            let box = document.getAnonymousElementByAttribute(this, "anonid", "options-box");
> >+            let frame = document.getAnonymousElementByAttribute(this, "anonid", "options-frame");
> 
> _optionsHandler.box/frame?

Done

> >+/*
> >+    var msgText = getExtensionString(locked ? "xpinstallDisabledMsgLocked" :
> >+                                              "xpinstallDisabledMsg");
> >+    var buttonLabel = locked ? null : getExtensionString("enableButtonLabel");
> >+    var buttonAccesskey = locked ? null : getExtensionString("enableButtonAccesskey");
> >+    var notifyData = locked ? null : "addons-enable-xpinstall";
> >+    showMessage(URI_NOTIFICATION_ICON_WARNING,
> >+                msgText, buttonLabel, buttonAccesskey,
> >+                !locked, notifyData);
> >+*/
> 
> Why is this commented out?

I didn't know if we should deal with it the same way. I can add to a followup bug.

> >+  _clearSection: function ev_clearSection(aSectionStart, aSectionEnd) {
> 
> This is kind of confusing... perhaps it should just take a section name (e.g.
> "repo"), and hardcode the start/end items, that way there's only one place to
> change if we change the sort order or XUL layout of the items?

Done

> >+    let self = this;
> >+    let panels = document.getElementById("panel-items");
> >+    panels.addEventListener("select",
> 
> Hmm, I guess this is here to avoid doing the work if you're just showing one of
> the other panels? Listener could remove itself after the first time, right?

If I stopped using an anonymous function. I'll look into it.

> >+  _installCallback: function ev__installCallback(aItem, aStatus) {
> >+    Components.reportError("callback: " + aItem.id)
> 
> Remove this?

Done

> >+  displaySearchResults: function ev_displaySearchResults(aAddons, aTotalResults, aIsRecommended) {
> 
> >+    if (aIsRecommended) {
> 
> >+    if (aAddons.length == 0) {
> 
> Doesn't matter too much, but this check seems like it should go before the
> previous one.

I moved the .length check before the aIsRecommended check

> >+      // Strip out any items with potentially unsafe urls
> >+      let unsafe = false;
> >+      for (var j = 0; j < urlproperties.length; j++) {
> >+        if (!this._isSafeURI(addon[urlproperties[j]])) {
> >+          unsafe = true;
> >+          break;
> >+        }
> >+      }
> >+      if (unsafe)
> >+        continue;
> 
> if (urlproperties.some(function (p) !this._isSafeURI(addon[p]), this))
>   continue;

Done

> >+  resetSearch: function ev_resetSearch() {
> >+    let mode = document.getElementById("addons-repo-mode").value = "recommended";
> >+    this.toggleMode();
> 
> "mode" is unused?

Removed

> >+function XPInstallDownloadManager() {
> 
> >+  addDownloads: function (aParams, aManager) {
> 
> >+    for (var i = 0; i < count;) {
> 
> >+      let uri = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService).newURI(url, null, null);
> 
> Probably should cache the ioService globally, given its use here and in
> isSafeURI (also in a loop).

Done.

> >+  onInstallEnded: function(aAddon, aStatus) {
> >+    // Track success/failure for each addon
> >+    if (aStatus < 0)
> 
> !Components.isSuccessCode(aStatus) (also later in this function)

Changed

> >+      message = strings.getString("alertAddonsFailed");
> 
> String name doesn't match:

Fixed

> >+  openProgressDialog: function(aPackages, aCount, aManager) {
> 
> >+    dpb.SetInt(0, 2);                       // OK and Cancel buttons
> 
> This doesn't actually appear to be used by the "xpinstall-download-started"
> observer... worth a separate bug on fixing this in general (both for us and the
> nsXPInstallManager code)?

I'll check with Mossop

> >+  onStateChange: function(aIndex, aState, aError) { debugger; Components.reportError("onstatechange"); },
> >+  onProgress: function(aIndex, aValue, aMax) { debugger; Components.reportError("onprogress"); }
> 
> Remove these debugger statements and reportErrors before landing?

Removed

> >+addonsSearchFailed.label=%S couldn't retrieve add-ons
> >+addonsSearchFailed.button=OK
> 
> These appear to be unused?

No, just didn't match the string in the code. Fixed.

> >+richlistitem .show-on-select {
> >+  visibility: collapse;
> >+}
> >+
> >+richlistitem[selected="true"] .show-on-select {
> >+  visibility: visible;
> >+}
> >+
> >+richlistitem .hide-on-select {
> >+  visibility: visible;
> >+}
> >+
> >+richlistitem[selected="true"] .hide-on-select {
> >+  visibility: collapse;
> >+}
> 
> Shouldn't these be in content/ (same applies to the other theme)?

Moved to content/browser.css

> Would be good to get specific followups on the TODO comments, and on the
> confirmation prompt (should be separate from bug 489229, I think).

OK, I'll file some bugs

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Added bug 489569 for the installation prompt

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

http://hg.mozilla.org/mobile-browser/rev/c6bc51b6f102

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

verified with beta3