696532 - (aom) Add-on manager

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[[:fabrice] Fabrice Desré]

Either build a new one or make the current about:addons mobile friendly

Comment 1

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

I have a patch started to make about:addons mobile friendly

Comment 2

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

Attachment: WIP 1

Really basic WIP. Adds an aboutAddons.xhtml, based on aboutHome.xhtml, and lists the installed add-ons and search engines.

Next up:
* Separate DTD file
* Add support for Enable/Disable/Uninstall

Comment 3

[Madhava Enros [:madhava]]

I spent some time yesterday thinking through what the basic structure of this UI should be. There are two basic models we could follow, I think. They are shown here: 
http://www.flickr.com/photos/madhava_work/6301559203/sizes/o/in/photostream/

Even though it's more of a departure from what we have now, I _think_ I prefer the latter for its flexibility and familiarity to Android users.

Comment 4

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

Attachment: WIP 2

This patch adds:
* Support for enable/disable/uninstall (and cancel uninstall for non-restartless) buttons in each row
* 'Options' button but no real support for it yet
* Minor style tweaks to remove the about:home header
* DTD and CSS files for aboutHome

To do:
* Add an add-on install listener so the page can update when new add-ons are installed
* More style tweaks moving toward the mockups

Note:
* Current mockups add a new flow, where add-on details are on a new page, not inline in the list. This patch still uses the inline list approach.

Comment 5

[Dave Townsend [:mossop]]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Created attachment 571318 [details] [diff] [review] [diff] [details] [review]
> WIP 2
> 

Doesn't seem to actually include the xhtml file.

Comment 6

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

Attachment: WIP 2.1

git would have magically added the file

Comment 7

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

Attachment: patch - basic changeover

This patch adds an AddonInstallListener to the about page, so we can update the page as we add or remove addons. There is no sense putting code for displaying taost or notifications in the about page since it may not be open most of the time.

This patch is enough to start iterating, so I wanted to get a review and land it instead of waiting much longer. The current add-on manager page is useless in mobile.

I plan to put the toast/notification part in browser.js in a different bug.

Things to change about this:
* Add newly installed addons into the "sort appropriate" location in the list
* Separate search add-ons?
* Style it like UX mock ups

Comment 8

[Madhava Enros [:madhava]]

This is the add-install flow I've got:
http://www.flickr.com/photos/madhava_work/6310042574/sizes/o/in/photostream/

Comment 9

[Patryk Adamczyk [:patryk] UX]

I recieved another bug for this...
https://bugzilla.mozilla.org/show_bug.cgi?id=698599
That's where my mock up is.

But the latest designs can be found here:
http://www.flickr.com/photos/patrykdesign/sets/72157628030736602/

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 573103 [details] [diff] [review]
patch - basic changeover

Review of attachment 573103 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a pretty good first draft to me. Made some comments as I looked through. There's probably tons of bugs, but I'd be fine getting something simple in so that we could split those up and parallelize the work more.

::: mobile/chrome/content/aboutAddons.xhtml
@@ +80,5 @@
> +    div[opType="needs-install"] .hide-on-install,
> +    div[opType="needs-enable"] .hide-on-enable,
> +    div[opType="needs-disable"] .hide-on-disable {
> +      display: none;
> +    }

These seemed confusing to me at first. I get it now, but kinda wonder how much overlap there is here that makes the classes useful? Could we instead do something like:

.extension[opType="needs-disable"] #enableButton

without adding to much to this list?

@@ +144,5 @@
> +      AddonManager.removeInstallListener(Addons);
> +    }
> +
> +    var Addons = {
> +      _createItem: function _createItem(aAddon) {

Looks like as good a place as any to abstract this dom creation stuff a bit? Maybe in here for now, or in a separate library for other things (are there other things beyond this and about:config?). Some sort of createNode(aNodeName, aText, aAttributes, aListeners) type?

Heck, mabye instead we can use innerHTML? Create a template of some sort, fill in the correct values and then just append the html. That might make the .setAttribute stuff below a bit more difficult... Something to look at

It looks like patrick's mockups also include the old "type" off to the right (i.e. "Search", "Addon", "Theme"). I don't see that here?

@@ +175,5 @@
> +
> +        let enableBtn = document.createElement("button");
> +        enableBtn.classList.add("show-on-disable");
> +        enableBtn.classList.add("hide-on-enable");
> +        enableBtn.classList.add("hide-on-uninstall");

'className = "show-on-disable hide-on-enable hide-on-uninstall"' is probably the faster path here.

@@ +176,5 @@
> +        let enableBtn = document.createElement("button");
> +        enableBtn.classList.add("show-on-disable");
> +        enableBtn.classList.add("hide-on-enable");
> +        enableBtn.classList.add("hide-on-uninstall");
> +        enableBtn.textContent = "Enable";

Localization?

@@ +239,5 @@
> +            break;
> +        }
> +
> +        let item = this._createItem(aAddon);
> +        item.setAttribute("isDisabled", !aAddon.isActive);

This guy probably needs to be set for everything, not just addons. opType too maybe?

@@ +244,5 @@
> +        item.setAttribute("appDisabled", aAddon.appDisabled);
> +        item.setAttribute("appManaged", appManaged);
> +        item.setAttribute("opType", opType);
> +        item.setAttribute("updateable", updateable);
> +        item.setAttribute("isReadonly", !uninstallable);

This does nothing for us now. Some of the pieces above it are in the same boat. Do we want to hold onto it for now? I'm fine filing follow ups to make sure we track hooking these up.

@@ +263,5 @@
> +      getAddons: function getAddons() {
> +        // Clear all content before filling the addons
> +        let list = document.getElementById("newAddonsList");
> +        while (list.firstElementChild)
> +          list.removeChild(list.firstElementChild);

Again, we can use innerHTML="" to make this super fun and fast.

@@ +334,5 @@
> + 
> +          //aItem.addon.userDisabled = false;
> +          //aItem.setAttribute("isDisabled", false);
> +        } else {
> +          aItem.addon.userDisabled = false;

userDisabled isn't used by anyone here. Do we need it?

@@ +360,5 @@
> +          aItem.addon.engine.hidden = true;
> +          opType = "needs-disable";
> +        } else if (aItem.addon.type == "theme") {
> +          aItem.addon.userDisabled = true;
> +          aItem.setAttribute("isDisabled", true);

I assume we'll remove this for now, along with the commented out section above?

@@ +437,5 @@
> +      },
> +
> +      openAMO: function openAMO() {
> +        let chromeWin = getChromeWin();
> +        chromeWin.BrowserApp.addTab("https://addons.mozilla.org/mobile");

Whoops. This shows off a bug I introduced. We need to select this tab in browser.js so that it shows. Do we want to close this AddonManager tab?

@@ +464,5 @@
> +        } else {
> +          if (element.getAttribute("typeName") == "search") {
> +            list.removeChild(element);
> +            element = this._createItemForAddon(aAddon);
> +            list.appendChild(element);

What does this do? Do we just need to update the item, but only for search engines?

Comment 11

[Patryk Adamczyk [:patryk] UX]

Add on manager's colour values.
http://www.flickr.com/photos/patrykdesign/6335035963/in/photostream

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

The current add-ons manager is broken on my phone. After selecting add-ons from the menu the page stays Loading... indefinitely. Also, if I leave the browser open to this page and leave my phone for a while, when I come back to use my phone it is very slow to respond the the fps has dropped to 1 and stays there consistently. 

I mention this in case it is useful while creating the new mobile add-ons manager.

Comment 14

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

Attachment: patch - the basics

OK, let's get a basic, but ugly version of a working add-on manager landed and move forward.

This patch has the basic listing and commands ready to go:
* Add-ons and search engines are listed
* Add-ons can be enabled/disabled and uninstalled
* Add-on options are not supported yet, but the options button will show if the add-on has options specified
* Search engines have no options
* Popup notifications are handled through the XPIInstallObserver in browser.js, so we can be notified even if the add-on manager is not visible.

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 575881 [details] [diff] [review]
patch - the basics

Review of attachment 575881 [details] [diff] [review]:
-----------------------------------------------------------------

Overall I think this is fine for a first start. I know most of the code is moved over from xul fennec and the behavior seems fine. I'd rather tackle any issues in smaller bugs. Some nits/notes as I read through below.

::: mobile/android/app/mobile.js
@@ +205,5 @@
>  pref("extensions.update.interval", 86400);
>  pref("extensions.dss.enabled", false);
>  pref("extensions.dss.switchPending", false);
>  pref("extensions.ignoreMTimeChanges", false);
> +pref("extensions.logging.enabled", true);

Not sure we want this on for everyone. Note, this log info gets written to disk on errors.

::: mobile/android/chrome/content/aboutAddons.xhtml
@@ +49,5 @@
> +   - ***** END LICENSE BLOCK ***** -->
> +
> +<html xmlns="http://www.w3.org/1999/xhtml">
> +<head>
> +  <title></title>

Need a title or this shows up in the tabs list as about:addons.

@@ +80,5 @@
> +    div[opType="needs-install"] .hide-on-install,
> +    div[opType="needs-enable"] .hide-on-enable,
> +    div[opType="needs-disable"] .hide-on-disable {
> +      display: none;
> +    }

Do we need these separate from the aboutAddons.css file?

@@ +245,5 @@
> +        return item;
> +      },
> +
> +      _getElementForAddon: function(aKey) {
> +        Services.console.logStringMessage("finding: " + aKey)

Removing logging

@@ +269,5 @@
> +
> +          // Load the search engines
> +          let defaults = Services.search.getDefaultEngines({ }).map(function (e) e.name);
> +          function isDefault(aEngine)
> +            defaults.indexOf(aEngine.name) != -1

Is this used? Here or in XUL Fennec?

@@ +287,5 @@
> +            let item = self._createItem(addon);
> +            item.setAttribute("isDisabled", !addon.isActive);
> +            item.setAttribute("opType", "");
> +            item.addon = addon;
> +            list.appendChild(item);

For XUL Fennec, we use appManaged to determine whether or not an addon/search engine can be uninstalled:

http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/extensions.js#357
and:
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/bindings/extensions.xml#180

We'll need to mimick that here (maybe in combo with the isReadonly property we're using on items?) Since we have no XBL constructor, I guess we might as well do that here? There are a few other properties there that might be nice to drag in as well.

@@ +335,5 @@
> +    
> +          if (aItem.addon.pendingOperations & AddonManager.PENDING_ENABLE) {
> +            this.showRestart();
> +          } else {
> +            aItem.removeAttribute("isDisabled");

We use isDisabled="false" on the two above, but removeAttribute("isDisabled") here?

@@ +428,5 @@
> +      showRestart: function showRestart(aMode) {
> +      },
> +
> +      hideRestart: function hideRestart(aMode) {
> +      },

I assume these are gone for some reason? Follow up bug?

@@ +433,5 @@
> +
> +      openAMO: function openAMO() {
> +        let chromeWin = getChromeWin();
> +        chromeWin.BrowserApp.addTab("https://addons.mozilla.org/mobile");
> +      },

Not used anymore.

::: mobile/android/chrome/content/browser.js
@@ +2100,5 @@
>      }
> +  },
> +
> +  onInstallEnded: function(aInstall, aAddon) {
> +    dump("Installed ended");

Remove

@@ +2108,5 @@
> +    else if (aAddon.pendingOperations & AddonManager.PENDING_INSTALL)
> +      needsRestart = true;
> +
> +    if (needsRestart) {
> +      dump("Install needs restart");

Remove

@@ +2127,5 @@
> +
> +      let message = Strings.browser.GetStringFromName("notificationRestart.normal");
> +      NativeWindow.doorhanger.show(message, "addon-app-restart", buttons, BrowserApp.selectedTab.id, { persistence: -1 });
> +    } else {
> +      dump("Install does not need restart");

Remove

Comment 16

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

(In reply to Wesley Johnston (:wesj) from comment #15)

> ::: mobile/android/app/mobile.js

> > +pref("extensions.logging.enabled", true);
> 
> Not sure we want this on for everyone. Note, this log info gets written to
> disk on errors.

Reverted to false

> ::: mobile/android/chrome/content/aboutAddons.xhtml

> > +  <title></title>
> 
> Need a title or this shows up in the tabs list as about:addons.

Added "Add-ons Manager" string

> > +    div[opType="needs-install"] .hide-on-install,
> > +    div[opType="needs-enable"] .hide-on-enable,
> > +    div[opType="needs-disable"] .hide-on-disable {
> > +      display: none;
> > +    }
> 
> Do we need these separate from the aboutAddons.css file?

I left these in the file, since they are not real "theme" CSS

> Removing logging

I removed all the debug logging

> > +          // Load the search engines
> > +          let defaults = Services.search.getDefaultEngines({ }).map(function (e) e.name);
> > +          function isDefault(aEngine)
> > +            defaults.indexOf(aEngine.name) != -1
> 
> Is this used? Here or in XUL Fennec?

It is used to disable the "uninstall" button for built-in search engines. I added code to use it.

> > +            let item = self._createItem(addon);
> > +            item.setAttribute("isDisabled", !addon.isActive);
> > +            item.setAttribute("opType", "");

> For XUL Fennec, we use appManaged to determine whether or not an
> addon/search engine can be uninstalled:

Added support for that

> We'll need to mimick that here (maybe in combo with the isReadonly property
> we're using on items?) Since we have no XBL constructor, I guess we might as
> well do that here? There are a few other properties there that might be nice
> to drag in as well.

I added support for some of that in _createItem

> > +          if (aItem.addon.pendingOperations & AddonManager.PENDING_ENABLE) {
> > +            this.showRestart();
> > +          } else {
> > +            aItem.removeAttribute("isDisabled");
> 
> We use isDisabled="false" on the two above, but
> removeAttribute("isDisabled") here?

I switched to use setAttribute("isDisabled", "false") here too, for consistency

> > +      showRestart: function showRestart(aMode) {
> > +      },
> > +
> > +      hideRestart: function hideRestart(aMode) {
> > +      },
> 
> I assume these are gone for some reason? Follow up bug?

Filed bug 704406

> > +      openAMO: function openAMO() {
> > +        let chromeWin = getChromeWin();
> > +        chromeWin.BrowserApp.addTab("https://addons.mozilla.org/mobile");
> > +      },
> 
> Not used anymore.

Removed

Comment 17

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

And so lands the ugliest Add-on Manager you will ever see:
https://hg.mozilla.org/projects/birch/rev/8bb653dcddc9

Filed bug 704415 to make it pretty

Comment 18

[Aaron Train [:aaronmt]]

Tagging [QA+] but I would hold until more functionality lands

Comment 19

[Adrian Tamas (:AdrianT)]

The Addon Manager is available on Firefox Mobile 16.0b1, Aurora 17.0a2 2012-09-04 and Nightly 18.0a1 2012-09-04 

Verified on:
Samsung Galaxy R (Android 2.3.4)
Samsung Galaxy Tab 2 7.0 Android (4.0.4)

Comment 20

[Adrian Tamas (:AdrianT)]

Test cases for the Add-on Manager are available in the Add-ons suite in MozTrap: https://moztrap.mozilla.org/manage/cases/?filter-suite=91