562622 - Implementation of the automatic vs. manual update design mockups

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The latest design mockups for updating addons have to be implemented. See the given URL for details. Not sure if those are the final ones yet. I will bring it up in our todays meeting.

Comment 1

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Update Now Mockups for Default and Manual Updating in Add-ons Manager

I'm attaching the mocks we looked at last week, where a menu within the add-ons manager provides "extra" tasks, including updating add-ons now.  Available Updates is a tab which only appears if the user has one or more add-ons marked for manual updates, while Recent Updates is a permanent tab which shows the last few add-on updates.

Comment 2

[Tony Chung [:tchung]]

These dont include updates to plugins found.   And since many plugins require something like PFS or external websites; we should consider how to design for that.

Reminder, with current addons manager, clicking Find Updates takes user to https://www.mozilla.com/en-US/plugincheck/.  Where in the new addons manager design will this carry over?

Comment 3

[Nikita Popov]

One should be able to update the add-ons with one click. Most people aren't interested in deactivating automatic updates or viewing the update log, but only want to check for updates fast ('cause an add-on doesn't work, aso.)

This could be done using these buttons that are both button and drop down. (Like the Win7 Shutdown Button.)

Comment 4

[Paul [pwd]]

I'm pretty sure these are separate bugs, but I was directed here, so I'll state em here:

1/ Users should be able to set add-ons to "Download but prompt for installation authorisation". At the moment it's either Update Automatically or Update Manually.

2/ Users should be able to set add-on permissions en-masse (to either Update Manually, Download but prompt for installation or Update Automatically".

3/ The date within the Add-On's Manager list seems out of place. If it's the last updated date, it should be displayed next to the add-on title only when "Show More Information" is clicked. And it should display "Add-On Title last updated XX/XX/XXXX" at the moment, I have no idea what that date represents.

4/ Add-On's should only be given the ability to update automatically if they've been reviewed by Mozilla. At the moment it seems like a security risk waiting to happen.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #2)
> These dont include updates to plugins found.   And since many plugins require
> something like PFS or external websites; we should consider how to design for
> that.
> 
> Reminder, with current addons manager, clicking Find Updates takes user to
> https://www.mozilla.com/en-US/plugincheck/.  Where in the new addons manager
> design will this carry over?

Austin, will we still have to use the external web page for the plugin checker or are there plans to have an integrated pane available for Firefox 3.7/4.0?

Comment 6

[Austin King [:ozten]]

(In reply to comment #5)
It's been mentioned a couple times. I'm not sure of the priority of that work.

Comment 7

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #2)
> Reminder, with current addons manager, clicking Find Updates takes user to
> https://www.mozilla.com/en-US/plugincheck/.  Where in the new addons manager
> design will this carry over?

Tony - the "Available Updates" pane, which appears if the user selects "Update Add-ons Now" but has one or more add-ons selected to install manually, will take over the case of plugincheck.  I don't believe there's a reason to link to it from the add-on manager any longer.(In reply to comment #4)
> 1/ Users should be able to set add-ons to "Download but prompt for installation
> authorisation". At the moment it's either Update Automatically or Update
> Manually.

The update manually case does in fact download the updates and prompt for installation.

> 2/ Users should be able to set add-on permissions en-masse (to either Update
> Manually, Download but prompt for installation or Update Automatically".

They can - see attachment.

> 4/ Add-On's should only be given the ability to update automatically if they've
> been reviewed by Mozilla. At the moment it seems like a security risk waiting
> to happen.

While it's true that an add-on author could make an update malicious, I feel we have to respect user choice here.  The user has already said that they trust a particular add-on when they install it.  For us to require manual updates on non-reviewed add-ons means overriding the user's preference.  Also, we plan to allow any update to an add-on to be backed out.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #7)
> (In reply to comment #2)
> > Reminder, with current addons manager, clicking Find Updates takes user to
> > https://www.mozilla.com/en-US/plugincheck/.  Where in the new addons manager
> > design will this carry over?
> 
> Tony - the "Available Updates" pane, which appears if the user selects "Update
> Add-ons Now" but has one or more add-ons selected to install manually, will
> take over the case of plugincheck.  I don't believe there's a reason to link to
> it from the add-on manager any longer.

Unfortunately, there's not a 1:1 mapping between what plugins Firefox marks as being outdated (see bug 514327), and what the Plugin Check page suggests there is a new version for. I'm not sure we're actually marking any plugins as outdated yet, but when that happens its likely going to be just the highly recommended upgrades; whereas the Plugin Check page is far more granular and has a lot more information.

Furthermore, there is currently no automated way to upgrade plugins - users still need to manually download newer versions from the vendor's site and install it themselves. I doubt that is going to change any time soon.

Comment 9

[Nikita Popov]

The circle with the number of available updates seems a little bit out of place right now. Can't it be put in front of the "Available Updates" so it gets "(5) Available Updates"?

Comment 10

[Nikita Popov]

Furthermore the "Install Updates" (manually) button seems a little bit lost. There's much white space around. Imho it would look better and make more sense if it was place next to the settings menu (left of it).

Another thing I do not understand: Why is the process indicator for manual and automatic updates different?

Comment 11

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #10)
> Furthermore the "Install Updates" (manually) button seems a little bit lost.
> There's much white space around. Imho it would look better and make more sense
> if it was place next to the settings menu (left of it).

"Burying" Install updates under a menu is deliberate.  It's an expert user action, and not one we want to present in the main UI.  One reason is that presenting the option suggests that manually updating add-ons is needed, where we'd rather users rely on automatic updates.  For another, users who want to manually update are likely either testing a .xpi or are aware of an update, ie expert users who would be more likely to know manual install is possible.

> Another thing I do not understand: Why is the process indicator for manual and
> automatic updates different?

These are different because of where they are placed in the UI.  Manual updates happen in their own panel, and thus need a persistent button.  Automatic updates don't actually have a process indicator, but I'm assuming you mean a one-time manual check, which is in a menu because it's not common functionality.  If you're referring to final visual styling, the above are mockups rather than final graphics.

Comment 12

[Paul [pwd]]

There's still no option, or even talk of an option to 'download updates, but ask before installing (with the option to install later or dismiss)'. This is an option that's there for Firefox, so why is the option ignored for extensions? After all, wasn't this [akin to] the default option of 3.6.x

Comment 13

[Nikita Popov]

Attachment: "Install Updates" in menu bar

(In reply to comment #11)
I'm not sure whether we're talking about the same thing. Or at least I do not understand, why placing "Install Updates" in the upper menu when manual updates are to be applied is less "expert" than placing it underneath the upper menu. To make more clear what I meant I created a Paint picture (see attachment).

@Second Point: I'm referring to the one-time manual check ("Updating (14 seconds left)"). I don't see why this should be different. Why does one of them have a progress bar, while the other shows seconds left? Why is one abortable, but the other isn't?

Comment 16

[al_9x]

> The update manually case does in fact download the updates and prompt for
> installation.

Nothing should be downloaded unless chosen.

Current (3.6) manual update behavior is quite reasonable, why not retain it?

it should be possible to manually detect update availability for each add-on class and then to choose updates to download and install

Comment 17

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #16)
> > The update manually case does in fact download the updates and prompt for
> > installation.
> 
> Nothing should be downloaded unless chosen.
> 
> Current (3.6) manual update behavior is quite reasonable, why not retain it?
> 
> it should be possible to manually detect update availability for each add-on
> class and then to choose updates to download and install

It is possible to manually detect update available for each add-on and then install all (or some) of those updates.  It is only be default that updates are installed automatically.  This decision was made because of the effort currently required to manually update add-ons - being interrupted with an add-on update notification on startup, manually updating the add-ons, and then restarting.  If a user has downloaded and installed an add-on, this constitutes a choice to use this add-on and subsequent updates rarely change the experience enough to constitute requiring this work of the user.

Comment 18

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #12)
> There's still no option, or even talk of an option to 'download updates, but
> ask before installing (with the option to install later or dismiss)'. This is
> an option that's there for Firefox, so why is the option ignored for
> extensions? After all, wasn't this [akin to] the default option of 3.6.x

This is basically manual update mode: add-on updates are checked for and downloaded, but not enabled until the user chooses to update add-ons or discard those updates.

Comment 19

[al_9x]

> This is basically manual update mode: add-on updates are checked for and
> downloaded, but not enabled until the user chooses to update add-ons or discard
> those updates.

Are you saying that in this new and "improved" "manual" mode, Fx will still check for updates and still download them all, just not install them?

If so, I have to ask, why are you destroying current manual mode? Who is asking for it?  I put Fx on manual because I don't want ANY checking/downloading that I did not explicitly initiate.  Why would you take that away?  FULLY manual update mode should be retained.

Comment 20

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #19)
> > This is basically manual update mode: add-on updates are checked for and
> > downloaded, but not enabled until the user chooses to update add-ons or discard
> > those updates.
> 
> Are you saying that in this new and "improved" "manual" mode, Fx will still
> check for updates and still download them all, just not install them?
> 
> If so, I have to ask, why are you destroying current manual mode? Who is asking
> for it?  I put Fx on manual because I don't want ANY checking/downloading that
> I did not explicitly initiate.  Why would you take that away?  FULLY manual
> update mode should be retained.

We're changing the default from manual to automatic because of all the feedback we've gotten that most people would prefer not to manually update their add-ons.  We absolutely know and respect that some people prefer to manually update, which is why we are leaving that option for those that choose it.

Comment 21

[al_9x]

> 
> We're changing the default from manual to automatic because of all the feedback
> we've gotten that most people would prefer not to manually update their
> add-ons.  We absolutely know and respect that some people prefer to manually
> update, which is why we are leaving that option for those that choose it.

I am not worried about the default, as it can be changed.  I asked you to clarify how you intend the manual mode to work.  So again, are you saying that in this new "manual" mode, Fx will nevertheless automatically check for updates and automatically download them all, just not install them?

Comment 23

[al_9x]

Observation on 4.0 beta1 build2:

1) I've been asking for a clarification as to whether or not, in manual mode (i.e. with automatic updates off), Fx would still be auto checking for updates and even downloading them, just not installing.

The answer, thankfully, appears to be no. Is that correct?

2) The manual (user triggered) update process, however, is problematic.
 
a) There is no "check for available updates" functionality, only "update addons"
b) clicking "update addons" not only downloads updates without asking, but also installs them

Installing updates without confirmation is obviously wrong, the user may not want certain updates.

Downloading updates before getting permission to install also makes no sense.  It would lead to downloading the same rejected updates, over and over, on every manual check, a waste of time and bandwidth.

The current (3.6) manual update procedure is sane and adequate.  It checks for update availability, presents you with a list, lets you chose what you want to update and only then downloads and installs your choices.  That's the way it should work.

Comment 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP v1

Work in progress - not quite there yet. Still missing:
* Update all button
* Update button for each update should be a checkbox
* Release notes
* Dropdown in header is ugly (might wait til we do the visual refresh to fix that)
* Tests
* Maybe something else I've forgotten

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Not quite done yet, as I'm still working on other tests. But the code is review-ready.

Comment 28

[Dave Townsend [:mossop]]

Comment on attachment 460519 [details] [diff] [review]
Patch v1

Before landing I think this needs something slightly better than the blank menupopup, we probably want to be using a button type="menu" like the new popup notifications. Clicking the button does the update check, the drop down gives the further options.
As a follow-up get a bug filed on having the recent updates pane default to sorting by update date by default.

>diff --git a/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd

> <!-- addon updates -->
>-<!ENTITY updates.updateNow.label              "Update add-ons">
>+<!ENTITY updates.updateNow.label              "Update Add-ons Now">

You need to rename this entity.

>+<!ENTITY addon.releaseNotes.label             "Release Notes:">
>+<!ENTITY addon.loadingReleaseNotes.label      "Loadingâ¦">

fscking bugzilla!

>diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js

>+const UPDATES_RECENT_TIMESPAN = 172800000; // 2 days (in milliseconds)

I'd prefer to see the calculation here, at least 2 * 24 * 3600000. It makes maintenance a little easier. We may want to make it a pref too but this is probably fine for now.

>       doCommand: function(aAddon) {
>         var listener = {
>           onUpdateAvailable: function(aAddon, aInstall) {
>             gEventManager.delegateAddonEvent("onUpdateAvailable",
>                                              [aAddon, aInstall]);
>-            aInstall.install();
>+            if (aAddon.applyBackgroundUpdates !== false)
>+              aInstall.install();

Why not just |if (aAddon.applyBackgroundUpdates)| ? Same elsewhere in this patch

>+function hasState(aInstall, aState) {
>+  var state = AddonManager["STATE_" + aState.toUpperCase()];
>+  return aInstall.state == state;
>+}

This should be called isState as an install can only ever be in one state.

> var gCategories = {
>   node: null,
>   _search: null,
>+  _contextualCategories: ["addons://search/"],

The changes in this and the next few hunks seem unnecessary, any reason for them that I'm missing?

>+var gUpdatesView = {
>+  node: null,
>+  _listBox: null,
>+  _updatePrefs: null,
>+  _backgroundUpdateCheck: null,
>+  _categoryItem: null,
>+  _numManualUpdaters: 0,
>+
>+  initialize: function() {
>+    this.node = document.getElementById("updates-view");
>+    this._listBox = document.getElementById("updates-list");
>+    this._emptyNotice = document.getElementById("updates-list-empty");

Declare _emptyNotice above.

>+    this._backgroundUpdateCheck = document.getElementById("config-backgroudUpdateCheck");
>+    this._categoryItem = gCategories.get("addons://updates/available");
>+
>+    this._updatePrefs = Services.prefs.getBranch("extensions.update.");
>+    this._updatePrefs.QueryInterface(Ci.nsIPrefBranch2);
>+    this._updatePrefs.addObserver("", this, false);
>+    this.updateBackgroundCheck();
>+    this.updateManualUpdatersCount(true);
>+    this.updateAvailableCount(true);
>+
>+    AddonManager.addAddonListener(this);
>+    AddonManager.addInstallListener(this);
>+  },
>+  
>+  shutdown: function() {
>+    AddonManager.removeInstallListener(this);

Need to removeAddonListener too.

>+  _showRecentUpdates: function(aRequest) {
>+    var self = this;
>+    AddonManager.getAllAddons(function(aAddonsList) {
>+      if (gViewController && aRequest != gViewController.currentViewRequest)
>+        return;
>+
>+      let now = Date.now();

Subtract UPDATES_RECENT_TIMESPAN from Date.now() here and compare against that to save having to do the calculation in the loop.

>+      for (let i = 0; i < aAddonsList.length; i++) {

Use aAddonsList.forEach

>+  _showAvailableUpdates: function(aIsRefresh, aRequest) {
>+    var self = this;
>+    AddonManager.getAllInstalls(function(aInstallsList) {
>+      if (!aIsRefresh && gViewController && aRequest != gViewController.currentViewRequest)
>+        return;
>+
>+      if (aIsRefresh) {
>+        while (self._listBox.itemCount > 0)
>+          self._listBox.removeItemAt(0);
>+      }
>+
>+      for (let i = 0; i < aInstallsList.length; i++) {

Use aInstallsList.forEach

>         buildNextInstall();
>       }, "application/x-xpinstall");
>     }
> 
>     buildNextInstall();
> 
>     aEvent.preventDefault();
>   }
>-};
>+};
>\ No newline at end of file

Add a newline at the end please.

>diff --git a/toolkit/mozapps/extensions/content/extensions.xml b/toolkit/mozapps/extensions/content/extensions.xml

>         <xul:hbox class="status-container">
>           <xul:hbox anonid="checking-update" hidden="true">
>             <xul:image class="spinner"/>
>             <xul:label value="&addon.checkingForUpdates.label;"/>
>           </xul:hbox>
>+          <xul:hbox anonid="update-available" hidden="true">
>+            <xul:label value="An update is available"/>
>+            <xul:button class="addon-control" label="Update Now"
>+                        oncommand="document.getBindingParent(this).upgrade();"/>

Needs moar l10n

>+          var dataReq = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"]
>+                              .createInstance(Components.interfaces.nsIXMLHttpRequest);
>+          dataReq.open("GET", aURI.spec, true);
>+          dataReq.addEventListener("load", function(aEvent) {
>+            if (dataReq.responseXML &&
>+                dataReq.responseXML.documentElement.namespaceURI != XMLURI_PARSE_ERROR) {
>+              relNotesData = dataReq.responseXML;
>+              showRelNotes();
>+            } else {
>+              handleError();
>+            }
>+          }, false);
>+          dataReq.addEventListener("error", function(aEvent) {
>+            handleError();
>+          }, false);

Why not just pass handleError rather than the anonymous function?
Also feels like these two requests share enough code to simplify a little, but not too bothered if that is possible or not.

>+          dataReq.send(null);
>+
>+          var styleReq = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"]
>+                              .createInstance(Components.interfaces.nsIXMLHttpRequest);
>+          styleReq.open("GET", UPDATES_RELEASENOTES_TRANSFORMFILE, true);

The old code used overrideMimetype here, I presume it was unnecessary?

>+      <method name="toggleReleaseNotes">
>+        <body><![CDATA[
>+          if (this.hasAttribute("show-relnotes")) {
>+            this._relNotesContainer.style.height = "0px";
>+            this.removeAttribute("show-relnotes");
>+            this._relNotesToggle.setAttribute(
>+              "label",
>+              this._relNotesToggle.getAttribute("showlabel")
>+            );
>+            this._relNotesToggle.setAttribute(
>+              "tooltiptext",
>+              this._relNotesToggle.getAttribute("showtooltip")
>+            );

I think I'd prefer to see this done as two buttons, show/hide them depending on the show-relnotes attribute. Or it might be better to use the expander element, though that is needing some fixes I have some of it somewhere.

Comment 29

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #28)
> Comment on attachment 460519 [details] [diff] [review]
> Patch v1
> 
> Before landing I think this needs something slightly better than the blank
> menupopup, we probably want to be using a button type="menu" like the new popup
> notifications. Clicking the button does the update check, the drop down gives
> the further options.

Talked with Boriss about this: doing so would expose the manual update check too much - resulting in neurotic button clicking, and the assumption that updates aren't automatic. It would also mean that only update related items could go in the menu. So we'd still need to add another menu for other utility functions (such as the Install From File item in bug 567127).

Comment 30

[Dave Townsend [:mossop]]

(In reply to comment #29)
> (In reply to comment #28)
> > Comment on attachment 460519 [details] [diff] [review] [details]
> > Patch v1
> > 
> > Before landing I think this needs something slightly better than the blank
> > menupopup, we probably want to be using a button type="menu" like the new popup
> > notifications. Clicking the button does the update check, the drop down gives
> > the further options.
> 
> Talked with Boriss about this: doing so would expose the manual update check
> too much - resulting in neurotic button clicking, and the assumption that
> updates aren't automatic. It would also mean that only update related items
> could go in the menu. So we'd still need to add another menu for other utility
> functions (such as the Install From File item in bug 567127).

Fair enough, I still want something better than a blank menupopup though.

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #28)
> >       doCommand: function(aAddon) {
> >         var listener = {
> >           onUpdateAvailable: function(aAddon, aInstall) {
> >             gEventManager.delegateAddonEvent("onUpdateAvailable",
> >                                              [aAddon, aInstall]);
> >-            aInstall.install();
> >+            if (aAddon.applyBackgroundUpdates !== false)
> >+              aInstall.install();
> 
> Why not just |if (aAddon.applyBackgroundUpdates)| ? Same elsewhere in this
> patch

Because its an optional property - doing that would make addon types that don't implement applyBackgroundUpdates default to not updating automatically.


> > var gCategories = {
> >   node: null,
> >   _search: null,
> >+  _contextualCategories: ["addons://search/"],
> 
> The changes in this and the next few hunks seem unnecessary, any reason for
> them that I'm missing?

Opps - that was leftover from a previous iteration. Reverted this.


> >+          var dataReq = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"]
> >+                              .createInstance(Components.interfaces.nsIXMLHttpRequest);
> >+          dataReq.open("GET", aURI.spec, true);
> >+          dataReq.addEventListener("load", function(aEvent) {
> >+            if (dataReq.responseXML &&
> >+                dataReq.responseXML.documentElement.namespaceURI != XMLURI_PARSE_ERROR) {
> >+              relNotesData = dataReq.responseXML;
> >+              showRelNotes();
> >+            } else {
> >+              handleError();
> >+            }
> >+          }, false);
> >+          dataReq.addEventListener("error", function(aEvent) {
> >+            handleError();
> >+          }, false);
> 
> Why not just pass handleError rather than the anonymous function?
> Also feels like these two requests share enough code to simplify a little, but
> not too bothered if that is possible or not.

Done.


> >+          dataReq.send(null);
> >+
> >+          var styleReq = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"]
> >+                              .createInstance(Components.interfaces.nsIXMLHttpRequest);
> >+          styleReq.open("GET", UPDATES_RELEASENOTES_TRANSFORMFILE, true);
> 
> The old code used overrideMimetype here, I presume it was unnecessary?

Yea, seems to have been unnecessary. Unless you know of a case when it was needed?


> >+      <method name="toggleReleaseNotes">
> >+        <body><![CDATA[
> >+          if (this.hasAttribute("show-relnotes")) {
> >+            this._relNotesContainer.style.height = "0px";
> >+            this.removeAttribute("show-relnotes");
> >+            this._relNotesToggle.setAttribute(
> >+              "label",
> >+              this._relNotesToggle.getAttribute("showlabel")
> >+            );
> >+            this._relNotesToggle.setAttribute(
> >+              "tooltiptext",
> >+              this._relNotesToggle.getAttribute("showtooltip")
> >+            );
> 
> I think I'd prefer to see this done as two buttons, show/hide them depending on
> the show-relnotes attribute. Or it might be better to use the expander element,
> though that is needing some fixes I have some of it somewhere.

I considered that, but I want to keep focus on the button (for accessibility - and without forcing focus to a different button).

Still working on tests - new patch up when I have better coverage.

Comment 32

[Blair McBride [:Unfocused] (UNAVAILABLE)]

In the Recent Updates pane, I've been thinking about ignoring any addon in the application scope. Then discovered bug 582841, which blocks that possibility.

Comment 33

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

Alright, bunch more tests now. And Dave's review comments seen to. And a couple of extra things, like fixing what the tests found, and adding release notes to the items in the recent updates pane.

Dave's probably not able to review anything before B3, so it'd be great of you could do this, Rob.

Comment 34

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Uh, forgot about making the menu button thing less ugly for B3. I'll out up another patch later, with the only difference being an added icon and some CSS work for that.

Comment 35

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

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

Not sure when I'll be able to get to this but I'll definitely try to... couple items from a quick scan

>diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js
>--- a/toolkit/mozapps/extensions/content/extensions.js
>+++ b/toolkit/mozapps/extensions/content/extensions.js
>@@ -45,27 +45,33 @@ Cu.import("resource://gre/modules/Servic
> Cu.import("resource://gre/modules/PluralForm.jsm");
> Cu.import("resource://gre/modules/DownloadUtils.jsm");
> Cu.import("resource://gre/modules/AddonManager.jsm");
> Cu.import("resource://gre/modules/AddonRepository.jsm");
> 
> 
> const PREF_DISCOVERURL = "extensions.webservice.discoverURL";
> const PREF_MAXRESULTS = "extensions.getAddons.maxResults";
>+const PREF_BACKGROUND_UPDATE = "extensions.update.enabled";
> 
> const LOADING_MSG_DELAY = 100;
> 
> const SEARCH_SCORE_MULTIPLIER_NAME = 2;
> const SEARCH_SCORE_MULTIPLIER_DESCRIPTION = 2;
> 
> // Use integers so search scores are sortable by nsIXULSortService
> const SEARCH_SCORE_MATCH_WHOLEWORD = 10;
> const SEARCH_SCORE_MATCH_WORDBOUNDRY = 6;
> const SEARCH_SCORE_MATCH_SUBSTRING = 3;
> 
>+const UPDATES_RECENT_TIMESPAN = 2 * 24 * 3600000; // 2 days (in milliseconds)
What do you think about making this configurable via a hidden preference with this being the default?

>@@ -1499,16 +1572,293 @@ var gDetailView = {
>...
>+  onInstallEnded: function(aAddon) {
>+    if (aAddon.applyBackgroundUpdates === false) {
>+      this._numManualUpdaters++;
>+      this.maybeShowCategory();
>+    }
>+  },
>+
>+  /* XXXunf
>+    onPropertyChanged doesn't garentee that a property changed,
>+    just that it *may* have changed. So, sadly, we have to iterate over all
>+    addons, to keep an accurate count of this; rather than this nicer method.
>+    
>+
>+  onApplyBackgroundUpdatesChanged: function(aAddon) {
>+    if (!("applyBackgroundUpdates" in aAddon))
>+      return;
>+    if (aAddon.applyBackgroundUpdates)
>+      this._numManualUpdaters--;
>+    else
>+      this._numManualUpdaters++;
>+    this.maybeShowCategory();
>+  }
>+  */
What's up with this?

>+  onPropertyChanged: function(aAddon, aProperties) {
>+    if (aProperties.indexOf("applyBackgroundUpdates") != -1)
>+      this.updateManualUpdatersCount();
>   }
> };

Comment 36

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #35)
> Comment on attachment 461745 [details] [diff] [review]
> Patch v2
> 
> Not sure when I'll be able to get to this but I'll definitely try to... couple
> items from a quick scan

I unexpectedly needed to take Friday (and Saturday) off due to sickness, which really messed up my plans here. Dave said he's going to try to review the rest of this tomorrow.


> >+const UPDATES_RECENT_TIMESPAN = 2 * 24 * 3600000; // 2 days (in milliseconds)
> What do you think about making this configurable via a hidden preference with
> this being the default?

That was mentioned in comment 28 too - I'd rather it be in a followup, as this bug is complex enough already. Just filed bug 583534.


> >@@ -1499,16 +1572,293 @@ var gDetailView = {
> >...
> >+  onInstallEnded: function(aAddon) {
> >+    if (aAddon.applyBackgroundUpdates === false) {
> >+      this._numManualUpdaters++;
> >+      this.maybeShowCategory();
> >+    }
> >+  },
> >+
> >+  /* XXXunf
> >+    onPropertyChanged doesn't garentee that a property changed,
> >+    just that it *may* have changed. So, sadly, we have to iterate over all
> >+    addons, to keep an accurate count of this; rather than this nicer method.
> >+    
> >+
> >+  onApplyBackgroundUpdatesChanged: function(aAddon) {
> >+    if (!("applyBackgroundUpdates" in aAddon))
> >+      return;
> >+    if (aAddon.applyBackgroundUpdates)
> >+      this._numManualUpdaters--;
> >+    else
> >+      this._numManualUpdaters++;
> >+    this.maybeShowCategory();
> >+  }
> >+  */
> What's up with this?


See bug 562599 comment 4. Talked with Dave about this - the only existing case where onPropertyChanged was used (for applyBackgroundUpdates) does actually guarantee that the property changed, but the API spec officially said it wasn't guaranteed that the property changed. So the API spec (and documentation) is changing to guarantee that. Next patch will use the commented out code.

Comment 37

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v3

Comment 38

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot of utilities menu-button

Here's what the menubutton looks like with the latest patch. It'll change when the rest of the styling changes, but at least it won't be so ugly until then.

Comment 39

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v3.1

No exciting changes here - I just changed the IDs of the utilities button and menu items to have "utils" in them, instead of "config" (making it more generic for the likes of bug 567127).

Comment 40

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Landed: https://hg.mozilla.org/mozilla-central/rev/488ea306526b

The automated tests cover this pretty well - I can't think of anything else I'd want tested.

Comment 41

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Fix test

Test failure on OSX - seems background update checks are disabled there by default?

Comment 42

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Also fixed a message typo in that test as well.

Comment 43

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

(In reply to comment #41)
> Created attachment 461987 [details] [diff] [review]
> Fix test
> 
> Test failure on OSX - seems background update checks are disabled there by
> default?
Doubtful though the test harness may have a different default setting... this is probably worth investigating further

http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#149

Comment 44

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #43)
> (In reply to comment #41)
> > Created attachment 461987 [details] [diff] [review] [details]
> > Fix test
> > 
> > Test failure on OSX - seems background update checks are disabled there by
> > default?
> Doubtful though the test harness may have a different default setting... this
> is probably worth investigating further
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#149

Yea, I meant specific to some configuration for either the test harness, or the build config, or the build machine. Its the only explanation I can come up with :\

Comment 45

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Temp test fix

Ugh, that's not it. Temporarily commenting out the offending part of the test so we fix the orange, and I'll investigate further.

Comment 46

[Justin Dolske [:Dolske]]

Comment on attachment 461991 [details] [diff] [review]
Temp test fix

rs=me per irc discussion

Comment 47

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Temp fixed landed as http://hg.mozilla.org/mozilla-central/rev/99807f92a67c

I still have to figure out why that part of the test is failing on OSX, and fix it. Filed followup bug 583668.

Comment 48

[pal-moz]

Attachment: screenshot

no scrollbar.
so cannot scroll to the "Restart now" button.

Comment 49

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #48)
> Created attachment 461997 [details]
> screenshot
> 
> no scrollbar.
> so cannot scroll to the "Restart now" button.

That looks like bug 562924. None of the changes in this bug should have caused that by itself.

As a side-note, the "Restart now to complete installation" link at the top of the Addons Manager should restart Firefox. The "Restart Now" buttons for individual items are actually redundant here (and will eventually disappear with bug 553460).

Comment 50

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #40)
> The automated tests cover this pretty well - I can't think of anything else I'd
> want tested.

Well, this something completely new and I think we should have some manual tests here. Think about restarts for extension updates. I will have to figure out what's necessary. Also for Mozmill tests keep in mind that we can run tests against all localized builds now, which extends your automated tests.

Comment 51

[Marek Stępień [:marcoos, inactive]]

Could you add a localization note for the addon.update.postfix entity, stating at least whether it is a noun or a verb - which would be translated differently into languages other than English?

Comment 52

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #51)
> Could you add a localization note for the addon.update.postfix entity, stating
> at least whether it is a noun or a verb - which would be translated differently
> into languages other than English?

Filed bug 583945 for this - thanks!

Comment 53

[al_9x]

Is there a global override/default for the per add-on "Update Automatically" setting?  So that one doesn't have to uncheck it for all existing add-ons one by one and also new ones?

Comment 54

[Tony Mechelynck [:tonymec]]

(In reply to comment #53)
> Is there a global override/default for the per add-on "Update Automatically"
> setting?  So that one doesn't have to uncheck it for all existing add-ons one
> by one and also new ones?

Not AFAIK, or "not yet", unless this fix includes what was requested by comment #4 point 2. Bug 571527 is for adding it on SeaMonkey; I don't know whether it's also foreseen for Firefox and/or Thunderbird.

Comment 55

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #54)
> (In reply to comment #53)
> > Is there a global override/default for the per add-on "Update Automatically"
> > setting?  So that one doesn't have to uncheck it for all existing add-ons one
> > by one and also new ones?

Not yet. Really want it, but not sure if we'd get it in before 4.0. I think Boriss is still trying to figure out how to best expose it. Just realised we don't have a bug for that, so just filed bug 586574.

Comment 56

[L.A.R. Grizzly]

I noticed that the add-ons are not automatically installed now in b3 (great), but I never got an alert to the updates pending. I was just browsing my add-ons and noticed that two of them were waiting for my permission to update them. An alert when FF opens (as in previous versions) would be nice.

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100826 Minefield/4.0b5pre.

Remaining issues have been filed as depending bugs.

(In reply to comment #56)
> I noticed that the add-ons are not automatically installed now in b3 (great),
> but I never got an alert to the updates pending. I was just browsing my add-ons
> and noticed that two of them were waiting for my permission to update them. An
> alert when FF opens (as in previous versions) would be nice.

See bug 591189.

Comment 58

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This bug should now be pretty good covered by:
https://litmus.mozilla.org/show_test.cgi?id=15175
https://litmus.mozilla.org/show_test.cgi?id=15176
https://litmus.mozilla.org/show_test.cgi?id=15177
https://litmus.mozilla.org/show_test.cgi?id=15178