Closed Bug 605494 Opened 11 years ago Closed 10 years ago

Need UI for restartless addons

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: wesj, Assigned: wesj)

References

Details

(Whiteboard: [strings] [fennec-checkin-postb3])

Attachments

(2 files, 11 obsolete files)

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.
Assignee: nobody → wjohnston
Attached patch Fix v1 (obsolete) — Splinter Review
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(?)
Attachment #485169 - Flags: ui-review?
Attachment #485169 - Flags: review?(mark.finkle)
blocking2.0: --- → ?
tracking-fennec: --- → ?
(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.
blocking2.0: ? → ---
Attachment #485169 - Flags: ui-review? → ui-review?(madhava)
tracking-fennec: ? → 2.0+
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?
Attachment #485169 - Flags: ui-review?(madhava)
Attachment #485169 - Flags: ui-review+
Attachment #485169 - Flags: review?(mark.finkle)
Attachment #485169 - Flags: review-
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #485169 - Attachment is obsolete: true
Attached patch WIP: Tests (obsolete) — Splinter Review
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.
Attached patch WIP: Tests (obsolete) — Splinter Review
Oops. Wrong file.
Attachment #488611 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Attachment #488374 - Attachment is obsolete: true
Attachment #489308 - Flags: review?(mark.finkle)
Attached patch Tests (obsolete) — Splinter Review
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.
Attachment #488612 - Attachment is obsolete: true
Attachment #489310 - Flags: review?(mark.finkle)
Attached patch Tests (obsolete) — Splinter Review
Whoops. Fixed a few "if(" mistypes.
Attachment #489310 - Attachment is obsolete: true
Attachment #489312 - Flags: review?(mark.finkle)
Attachment #489310 - Flags: review?(mark.finkle)
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.
I have two addons (bootstrapped and not) set up at:

file:///home/wes/Dropbox/Public/Testing/extensions/testInstallAndUpgrade.html
Thanks for the test addons.   will store them away and use it for future testing
Flags: in-litmus?
Whiteboard: [strings]
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
Attachment #489312 - Flags: review?(mark.finkle) → review-
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.
Attachment #489308 - Flags: review?(mark.finkle)
Attached patch Patch v2 (obsolete) — Splinter Review
Unbitrotted all of this.
Attachment #489308 - Attachment is obsolete: true
Attachment #496002 - Flags: review?(mark.finkle)
Attached patch Tests v2 (obsolete) — Splinter Review
Unbitrotted.
Attachment #489312 - Attachment is obsolete: true
Attachment #496004 - Flags: review?(mark.finkle)
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.
Attachment #496004 - Flags: review?(mark.finkle)
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
Attachment #496002 - Flags: review?(mark.finkle) → review+
Attached patch Patch v3 (obsolete) — Splinter Review
Nits addressed. browser_addons tests pass here on my local machine.
Attachment #496002 - Attachment is obsolete: true
Attachment #497615 - Flags: review+
Whiteboard: [strings] → [strings] [fennec-checkin-postb3]
Attached patch Tests v3 (obsolete) — Splinter Review
Tests
Attachment #496004 - Attachment is obsolete: true
Attachment #501117 - Flags: review?(mark.finkle)
Attached patch PatchSplinter Review
Unbitrotted.
Attachment #497615 - Attachment is obsolete: true
Attachment #501121 - Flags: review+
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
Attachment #501117 - Flags: review?(mark.finkle) → review-
Attached patch Tests v4Splinter Review
Use waitFor.
Attachment #501117 - Attachment is obsolete: true
Attachment #501148 - Flags: review?(mark.finkle)
Comment on attachment 501148 [details] [diff] [review]
Tests v4

You can remove the | info("testing\n") | before checkin
Attachment #501148 - Flags: review?(mark.finkle) → review+
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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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
Verified fix on Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b10pre) Gecko/20110119 Firefox/4.0b10pre Fennec/4.0b4pre
Status: RESOLVED → VERIFIED
Depends on: 627911
You need to log in before you can comment on or make changes to this bug.