Last Comment Bug 562622 - Implementation of the automatic vs. manual update design mockups
: Implementation of the automatic vs. manual update design mockups
Status: VERIFIED FIXED
[rewrite]
: user-doc-needed
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: P1 normal with 2 votes (vote)
: mozilla2.0b3
Assigned To: Blair McBride [:Unfocused] (UNAVAILABLE)
:
: Andy McKay [:andym]
Mentors:
: 571994 576226 582080 (view as bug list)
Depends on: 565293 570128 584035 591189 553503 562599 573062 582841 584006 584330 587967 587970 591024 591027 593315 597397
Blocks: 571950 583534 583544 550048 553488 562935 571527 582082 583668 583945 586574 612053
  Show dependency treegraph
 
Reported: 2010-04-29 02:33 PDT by Henrik Skupin (:whimboo)
Modified: 2012-06-09 02:23 PDT (History)
26 users (show)
blair: in‑testsuite+
hskupin: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta3+


Attachments
Update Now Mockups for Default and Manual Updating in Add-ons Manager (673.34 KB, image/png)
2010-04-29 12:41 PDT, Jennifer Morrow [:Boriss] (UX)
no flags Details
"Install Updates" in menu bar (124.43 KB, image/png)
2010-05-21 05:34 PDT, Nikita Popov
no flags Details
WiP v1 (43.24 KB, patch)
2010-07-04 14:43 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
no flags Details | Diff | Splinter Review
Patch v1 (64.27 KB, patch)
2010-07-27 06:57 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
dtownsend: review-
Details | Diff | Splinter Review
Patch v2 (77.96 KB, patch)
2010-07-30 21:33 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
no flags Details | Diff | Splinter Review
Patch v3 (81.18 KB, patch)
2010-07-31 20:14 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
no flags Details | Diff | Splinter Review
Screenshot of utilities menu-button (11.82 KB, image/png)
2010-07-31 20:19 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
no flags Details
Patch v3.1 (81.16 KB, patch)
2010-08-01 16:27 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
dtownsend: review+
Details | Diff | Splinter Review
Fix test (1.69 KB, patch)
2010-08-01 20:56 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
no flags Details | Diff | Splinter Review
Temp test fix (1.45 KB, patch)
2010-08-01 21:26 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
dolske: review+
Details | Diff | Splinter Review
screenshot (86.16 KB, image/jpeg)
2010-08-01 22:41 PDT, pal-moz
no flags Details

Description Henrik Skupin (:whimboo) 2010-04-29 02:33:54 PDT
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) 2010-04-29 12:41:57 PDT
Created attachment 442478 [details]
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] 2010-04-30 09:33:10 PDT
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 2010-04-30 10:01:01 PDT
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] 2010-04-30 10:18:43 PDT
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) 2010-04-30 13:06:32 PDT
(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] 2010-05-01 09:17:34 PDT
(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) 2010-05-04 18:48:59 PDT
(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) 2010-05-04 20:02:29 PDT
(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 2010-05-05 03:55:37 PDT
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 2010-05-05 04:00:30 PDT
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) 2010-05-20 14:31:36 PDT
(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] 2010-05-20 14:45:56 PDT
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 2010-05-21 05:34:20 PDT
Created attachment 446694 [details]
"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 14 Dave Townsend [:mossop] 2010-06-14 15:16:01 PDT
*** Bug 571994 has been marked as a duplicate of this bug. ***
Comment 15 Henrik Skupin (:whimboo) 2010-06-14 15:37:55 PDT
*** Bug 571950 has been marked as a duplicate of this bug. ***
Comment 16 al_9x 2010-06-14 15:45:56 PDT
> 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) 2010-06-29 21:34:40 PDT
(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) 2010-06-29 21:35:44 PDT
(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 2010-06-29 22:23:45 PDT
> 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) 2010-06-30 09:23:44 PDT
(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 2010-06-30 12:09:55 PDT
> 
> 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 22 Henrik Skupin (:whimboo) 2010-07-01 01:53:26 PDT
*** Bug 576226 has been marked as a duplicate of this bug. ***
Comment 23 al_9x 2010-07-01 22:05:39 PDT
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) 2010-07-04 14:43:13 PDT
Created attachment 455981 [details] [diff] [review]
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 25 Dave Townsend [:mossop] 2010-07-26 14:50:12 PDT
*** Bug 582080 has been marked as a duplicate of this bug. ***
Comment 26 Blair McBride [:Unfocused] (UNAVAILABLE) 2010-07-27 06:57:26 PDT
Created attachment 460519 [details] [diff] [review]
Patch v1

Not quite done yet, as I'm still working on other tests. But the code is review-ready.
Comment 27 Justin Dolske [:Dolske] 2010-07-28 14:49:21 PDT
*** Bug 580993 has been marked as a duplicate of this bug. ***
Comment 28 Dave Townsend [:mossop] 2010-07-28 16:49:34 PDT
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) 2010-07-28 18:03:47 PDT
(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] 2010-07-28 18:05:04 PDT
(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) 2010-07-28 19:34:29 PDT
(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) 2010-07-28 20:25:36 PDT
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) 2010-07-30 21:33:24 PDT
Created attachment 461745 [details] [diff] [review]
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) 2010-07-30 21:35:10 PDT
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 [:rstrong] (use needinfo to contact me) 2010-07-30 23:18:48 PDT
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) 2010-07-31 20:06:56 PDT
(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) 2010-07-31 20:14:27 PDT
Created attachment 461850 [details] [diff] [review]
Patch v3
Comment 38 Blair McBride [:Unfocused] (UNAVAILABLE) 2010-07-31 20:19:29 PDT
Created attachment 461851 [details]
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) 2010-08-01 16:27:54 PDT
Created attachment 461954 [details] [diff] [review]
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) 2010-08-01 20:04:27 PDT
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) 2010-08-01 20:56:35 PDT
Created attachment 461987 [details] [diff] [review]
Fix test

Test failure on OSX - seems background update checks are disabled there by default?
Comment 42 Blair McBride [:Unfocused] (UNAVAILABLE) 2010-08-01 20:57:49 PDT
Also fixed a message typo in that test as well.
Comment 43 Robert Strong [:rstrong] (use needinfo to contact me) 2010-08-01 21:00:08 PDT
(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) 2010-08-01 21:02:37 PDT
(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) 2010-08-01 21:26:35 PDT
Created attachment 461991 [details] [diff] [review]
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] 2010-08-01 21:28:01 PDT
Comment on attachment 461991 [details] [diff] [review]
Temp test fix

rs=me per irc discussion
Comment 47 Blair McBride [:Unfocused] (UNAVAILABLE) 2010-08-01 21:47:58 PDT
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 2010-08-01 22:41:02 PDT
Created attachment 461997 [details]
screenshot

no scrollbar.
so cannot scroll to the "Restart now" button.
Comment 49 Blair McBride [:Unfocused] (UNAVAILABLE) 2010-08-02 04:38:28 PDT
(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) 2010-08-02 05:41:03 PDT
(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] 2010-08-02 10:55:22 PDT
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) 2010-08-02 18:35:18 PDT
(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 2010-08-11 06:16:44 PDT
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] 2010-08-11 20:04:45 PDT
(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) 2010-08-11 22:29:43 PDT
(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 2010-08-13 11:43:39 PDT
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) 2010-08-27 01:02:49 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.