605494 - Need UI for restartless addons

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Currently, when you install a bootstrapped (restartless) addon from the addon search pane, there is no UI. We should probably:

1.) Move the addon to the "installed addons" area.
2.) Use the correct binding for installed addons.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Fix v1

Here's a first run at this. I'm adding madhava too, to make sure this is the desired behavior. This makes it so that:

1.) If you install the restartless addon from the "search"/"repository" area, we move it up into the "installed" items area, and fix its formatting.

2.) If you install a restartless addon from a webpage (AMO for example), we add it to the "installed" addons area, and show a notification that says, "Add-on installed". Clicking it takes you to the add-on manager.

3.) If you uninstall a bootstrapped addon, we immediately delete it from the list (i.e. there is no way to cancel the uninstall like there is for normal addons).

4.) Disabling or enabling the addon will just change the state of the addon, without affecting restart notifications.

5.) The update button should enable itself when you install a restartless addon that can update. Worrying about the uninstall case (or even this case, why do we bother disabling it at all?) feels a bit like overkill to me. 

... whew. Probably missing something, but that's all I found right now. Seems like a good place to start and file other bugs as they're found(?)

Comment 2

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

(In reply to comment #1)

> ... whew. Probably missing something, but that's all I found right now. Seems
> like a good place to start and file other bugs as they're found(?)

Yes, that's fine. I talked to Madhava and he is OK with the UI.

Comment 3

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

Comment on attachment 485169 [details] [diff] [review]
Fix v1

>diff --git a/chrome/content/extensions.js b/chrome/content/extensions.js
>--- a/chrome/content/extensions.js
>+++ b/chrome/content/extensions.js
>@@ -270,59 +270,64 @@ var ExtensionsView = {
>   },
> 
>   hideOnSelect: function ev_handleEvent(aEvent) {
>     // When list selection changes, be sure to close up any open options sections
>     if (aEvent.target == this._list)
>       this.hideOptions();
>   },
> 
>+  _createLocalAddon: function ev__createLocalAddon(aAddon) {
>+    return listitem;
>+  },
>   getAddonsFromLocal: function ev_getAddonsFromLocal() {

Need a blank line

>+  removeItem : function ev_moveItem(aItem) {
>+    this._list.removeChild(aItem);
>+  },
>   enable: function ev_enable(aItem) {

Same

>-  appendSearchResults: function(aAddons, aShowRating) {
>+  appendSearchResults: function(aAddons, aShowRating, aSection) {
> 
>-      let item = this._list.appendChild(listitem);
>+      let item = this.addItem(listitem, aSection);

Do we need the aSection passed in? Can't we just hard code it in the addItem call? We should only be calling appendSearchResults for the "repo" section.

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

> alertAddonsInstalling=Installing add-on
> alertAddonsInstalled=Installation complete. Restart required.
>+alertAddonsInstalled.noRestart=Installation complete. 

We only seem to use a period (.) for multiple sentences

r- for the "repo" cleanup. You have some | if( | in there too. 

Do we have tests for this?

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Nits addressed, but need to test again (I've destroyed my testing environment). I'll write some automated browser-chrome tests for this stuff too.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: WIP: Tests

Mostly ready tests. Added two new tests for installing addons (bootstrapped and non-bootstrapped) with the addon manager closed. Also updated the current tests to make sure that bootstrapped addons move to the correct place in the UI.

These (almost) pass, but with a few caveots:

1.) We can't get hold of alert-notifications on all platforms. Need to provide fall backs in other cases (or skip those particular tests?).
2.) I tried to kill all of the setTimeouts in here, but added a one more. It is in onInstallEnded to give the UI time to move a boostrapped addon from the search to the installed area.
3.) On desktop at least, I am getting the desktop "Install addon" confirm prompt. I have no idea why this is occurring. It wasn't at first, but halfway through writing these it happened. The only errors I am seeing are related to this problem, and passed until it started happening.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: WIP: Tests

Oops. Wrong file.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Tests

These test installing addons from the urlbar and addon manager, for both bootstrapped and non-bootstrapped addons. They test a lot, and add a few events to catch to help with race conditions.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Tests

Whoops. Fixed a few "if(" mistypes.

Comment 10

[Tony Chung [:tchung]]

Wes, do you have any restartless addons that can be installed for testing?  if so, please attach them here and QA can use them for regression testing later.  Also to verify the fix.

Comment 11

[Wesley Johnston (:wesj)]

I have two addons (bootstrapped and not) set up at:

file:///home/wes/Dropbox/Public/Testing/extensions/testInstallAndUpgrade.html

Comment 12

[Wesley Johnston (:wesj)]

Oops. Correct link:

http://dl.dropbox.com/u/72157/Testing/extensions/testInstallAndUpgrade.html

Comment 13

[Tony Chung [:tchung]]

Thanks for the test addons.   will store them away and use it for future testing

Comment 14

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

Comment on attachment 489312 [details] [diff] [review]
Tests

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>+
>+    let event = document.createEvent("Events");
>+    event.initEvent("MozAlertShown", true, false);
>+    container.dispatchEvent(event);

Drop the "Moz", this is a chrome only event and it's Fennec only.

>diff --git a/chrome/tests/browser_addons.js b/chrome/tests/browser_addons.js

This file could use some line breaks between functions.

>diff --git a/components/PromptService.js b/components/PromptService.js

No to all of these changes. We already fire an event before showing a dialog: "DOMWillOpenModalDialog"

http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#2164

Comment 15

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

Comment on attachment 489308 [details] [diff] [review]
Patch

I want you to figure out the existing add-on restart bugs before we land this patch. Any fixes would bit rot this patch. This patch would only make it harder to fix the restart bugs.

We can land the string by itself for now.

Comment 16

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

pushed string only:
http://hg.mozilla.org/mobile-browser/rev/a4c19a702753

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Unbitrotted all of this.

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Tests v2

Unbitrotted.

Comment 19

[Wesley Johnston (:wesj)]

Comment on attachment 496004 [details] [diff] [review]
Tests v2

Whoops. Just remembered that these test a lot of strings, something we are trying to move away from. I'll try to pull those bits out and repost.

Comment 20

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

Comment on attachment 496002 [details] [diff] [review]
Patch v2


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

>     // can see more by pressing the "See More" button
>-    this.appendSearchResults(aRecommendedAddons, false, aRecommendedAddons.length);
>-    this.appendSearchResults(aBrowseAddons, true, (aRecommendedAddons.length >= kAddonPageSize ? 0 : kAddonPageSize));
>+    this.appendSearchResults(aRecommendedAddons, false, aRecommendedAddons.length, "repo");
>+    this.appendSearchResults(aBrowseAddons, true, (aRecommendedAddons.length >= kAddonPageSize ? 0 : kAddonPageSize), "repo");

Don't pass "repo" to appendSearchResults. Those results are always added to the repo section.

   onInstallEnded: function(aInstall, aAddon) {
>-    if (aInstall.existingAddon && (aInstall.existingAddon.pendingOperations & AddonManager.PENDING_UPGRADE))
>-      ExtensionsView.showRestart("update");
>-    else if (aAddon.pendingOperations & AddonManager.PENDING_INSTALL)
>-      ExtensionsView.showRestart("normal");
>+    dump("install ended\n");

remove dump

>+    // if we already have a mode, then we need to show a restart notification
>+    // otherwise, we are likely a bootstrapped addon
>+    if (needsRestart)
>+      ExtensionsView.showRestart(mode);
>+    dump("need restart: " + needsRestart + "\n");

same

r+ with nits. I assume tests are still passing

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Nits addressed. browser_addons tests pass here on my local machine.

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Tests v3

Tests

Comment 23

[Wesley Johnston (:wesj)]

Attachment: Patch

Unbitrotted.

Comment 24

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

Comment on attachment 501117 [details] [diff] [review]
Tests v3

Can we use a | waitFor | that checks AlertsHelper.container.hidden instead of adding a new event?

r+ if we can remove the event

Comment 25

[Wesley Johnston (:wesj)]

Attachment: Tests v4

Use waitFor.

Comment 26

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

Comment on attachment 501148 [details] [diff] [review]
Tests v4

You can remove the | info("testing\n") | before checkin

Comment 27

[Wesley Johnston (:wesj)]

Pushed:

Bad push of tests (empty diff): http://hg.mozilla.org/mobile-browser/rev/d54b2ffcad27
Good push of tests: http://hg.mozilla.org/mobile-browser/rev/db2288604bb2
Patch: http://hg.mozilla.org/mobile-browser/rev/b8893fcabc1d

Comment 28

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Android; Linux armv71; rv:2.0b9pre) Gecko/20110110 Firefox/4.0b9pre Fennec/4.0b4pre.  need to verify on maemo also

Comment 29

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b10pre) Gecko/20110119 Firefox/4.0b10pre Fennec/4.0b4pre