Implementation of the automatic vs. manual update design mockups

VERIFIED FIXED in mozilla2.0b3

Status

()

Toolkit
Add-ons Manager
P1
normal
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: whimboo, Assigned: Unfocused)

Tracking

(Depends on: 4 bugs, Blocks: 3 bugs, {user-doc-needed})

Trunk
mozilla2.0b3
user-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus +

Firefox Tracking Flags

(blocking2.0 beta3+)

Details

(Whiteboard: [rewrite])

Attachments

(6 attachments, 5 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Keywords: uiwanted
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.
Depends on: 553503

Updated

7 years ago
Blocks: 554007

Comment 2

7 years ago
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?
No longer blocks: 554007

Comment 3

7 years ago
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

7 years ago
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.
(Reporter)

Comment 5

7 years ago
(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

7 years ago
(In reply to comment #5)
It's been mentioned a couple times. I'm not sure of the priority of that work.
(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.
(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.

Updated

7 years ago
Blocks: 562935

Comment 9

7 years ago
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

7 years ago
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?
Depends on: 565293
blocking2.0: --- → beta1+
(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

7 years ago
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

7 years ago
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?
(Reporter)

Updated

7 years ago
Depends on: 570128
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Depends on: 562599
Blocks: 571527
Duplicate of this bug: 571994
(Reporter)

Updated

7 years ago
Duplicate of this bug: 571950

Comment 16

7 years ago
> 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
blocking2.0: beta1+ → beta2+
Depends on: 573062
(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.
(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

7 years ago
> 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.
(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

7 years ago
> 
> 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?
(Reporter)

Updated

7 years ago
Duplicate of this bug: 576226

Comment 23

7 years ago
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.
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
Attachment #455981 - Flags: feedback?(dtownsend)
blocking2.0: beta2+ → final+
blocking2.0: final+ → beta3+
Duplicate of this bug: 582080
Blocks: 582082
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.
Attachment #455981 - Attachment is obsolete: true
Attachment #460519 - Flags: review?(dtownsend)
Attachment #455981 - Flags: feedback?(dtownsend)
Duplicate of this bug: 580993
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.
Attachment #460519 - Flags: review?(dtownsend) → review-
(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).
(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.
(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.
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.
Depends on: 582841
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.
Attachment #460519 - Attachment is obsolete: true
Attachment #461745 - Flags: review?(robert.bugzilla)
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 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();
>   }
> };
(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.
Created attachment 461850 [details] [diff] [review]
Patch v3
Attachment #461745 - Attachment is obsolete: true
Attachment #461850 - Flags: review?(dtownsend)
Attachment #461745 - Flags: review?(robert.bugzilla)
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.
Blocks: 583534, 583544
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).
Attachment #461850 - Attachment is obsolete: true
Attachment #461954 - Flags: review?(dtownsend)
Attachment #461850 - Flags: review?(dtownsend)
Attachment #461954 - Flags: review?(dtownsend) → review+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Created attachment 461987 [details] [diff] [review]
Fix test

Test failure on OSX - seems background update checks are disabled there by default?
Attachment #461987 - Flags: review?
Also fixed a message typo in that test as well.
(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
(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 :\
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.
Attachment #461987 - Attachment is obsolete: true
Attachment #461991 - Flags: review?
Attachment #461987 - Flags: review?
Comment on attachment 461991 [details] [diff] [review]
Temp test fix

rs=me per irc discussion
Attachment #461991 - Flags: review? → review+

Updated

7 years ago
Keywords: user-doc-needed
Blocks: 583668
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

7 years ago
Created attachment 461997 [details]
screenshot

no scrollbar.
so cannot scroll to the "Restart now" button.
(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).
Keywords: uiwanted
(Reporter)

Comment 50

7 years ago
(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.
Flags: in-litmus- → in-litmus?
Target Milestone: --- → mozilla2.0b3
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?
(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!
Blocks: 583945

Updated

7 years ago
Depends on: 584006

Updated

7 years ago
Depends on: 584035
Depends on: 584330

Comment 53

7 years ago
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?
(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.
Blocks: 586574
(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

7 years ago
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.

Updated

7 years ago
Depends on: 587967

Updated

7 years ago
Depends on: 587970

Updated

7 years ago
Depends on: 590096
No longer depends on: 590096
(Reporter)

Updated

7 years ago
Depends on: 591189
(Reporter)

Updated

7 years ago
Depends on: 591024
(Reporter)

Updated

7 years ago
Depends on: 591027
(Reporter)

Comment 57

7 years ago
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.
Status: RESOLVED → VERIFIED
(Reporter)

Updated

7 years ago
Blocks: 553488

Updated

7 years ago
Depends on: 593315
(Reporter)

Updated

7 years ago
Blocks: 571950

Updated

7 years ago
Depends on: 597397

Updated

7 years ago
Depends on: 603185

Updated

7 years ago
Depends on: 603187
No longer depends on: 603187
No longer depends on: 603185

Updated

7 years ago
Blocks: 612053
Flags: in-litmus? → in-litmus?(vlad.maniac)
(Reporter)

Comment 58

7 years ago
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
Flags: in-litmus?(vlad.maniac) → in-litmus+
You need to log in before you can comment on or make changes to this bug.