Closed Bug 296566 Opened 15 years ago Closed 14 years ago

Move Extension Update into Extension Manager

Categories

(Toolkit :: Application Update, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8final

People

(Reporter: bugs, Assigned: bugs)

References

Details

Attachments

(3 files, 5 obsolete files)

The Extension System relies on the Update Service to handle update checking from
its front end and to do regular background update checks. Since the Extension
Update system is slightly different to application update and the UI for the two
features is going to diverge, it makes sense to split the Extension related
sections out of the Update Service and make the Extension front end call the
Extension Manager directly to do it.

This work will be done in two stages:

1. move manual update checking from the UI and mismatch checking to invoke EM
directly from the appropriate places instead of invoking the update service

2. move the background checking into the EM from the update service (this is
less critical to do right away since its UI is more hidden).
Priority: -- → P1
Target Milestone: --- → Firefox1.1
Comment on attachment 185288 [details] [diff] [review]
changes in Extension Manager etc. 

>Index: toolkit/mozapps/extensions/public/nsIExtensionManager.idl

>+  void getIncompatibleItemList(in AString id, 
>+                               in AString version,
>+                               in unsigned long type,
>+                               out unsigned long itemCount,
>+                               [retval, array, size_is(itemCount)] out nsIUpdateItem items);

Please bump the uuid of the interface.


>Index: toolkit/xre/Makefile.in

>+	nsUpdateDriver.cpp \

this change is part of my other patch.


r=darin
Attachment #185288 - Flags: review?(darin) → review+
Blocks: 292163
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050607
Firefox/1.0+

If I click an extension (any) and go to update it using either the right click
or the button at bottom, an update box comes up with a check in the box. There
is nothing next to the box to describe it. I tried to "Install now" but it
didn't do anything. Just had to "X" out of it. Just an FYI.
In nsExtensionManager.js.in
+      var sourceEvent = Components.classes["@mozilla.org/supports-PRUint8;1"]
+                                 
.createInstance(Components.interfaces.nsISupportsPRBool);
Shouldn't that be supports-PRBool?
It appears that update.css was added to extensions for winstripe and qute but
not for pinstripe.
I am now working on a large change to move background update in, and tidy up a
few other loose ends in EM. 
Status: NEW → ASSIGNED
Ben, just want to give you a heads up regarding a patch that touches this code
in bug 245082
Robert, land your patch and I'll take on the task of merging it with this one.
(note this patch is partial work, not yet complete - I'd appreciate it if you
could get your other work in asap to reduce my merge hell - I know it's coming,
would rather get it out of the way sooner than later ;-)

My plan here is as follows:
- replace all the ugly observer notifications with a nsIAddonUpdateCheckListener
interface that the front end uses to listen to update checks. 
- internalize update to the Extensions window itself, removing the clunky wizard
UI which is now sort of hobbled having removed app update from it. The visual
will be something like this:
http://www.bengoodger.com/software/mb/extensions/emui2.png

This should make updates to Extensions more obvious to users. 

The update check progress will be shown inline in the Extension manager as each
item is updated. 

I will then add a Background Update timer using the new Timer Manager service to
kick off this operation periodically. I may lower the interval to daily. (Will
need to check with umo/ops folks)
Ben, the new patch still needs review so go ahead and land... if any of the
functionality is still needed afterwards I'll create a new patch within a day or
two.
and if you want to take anything from either of the patches in bug 245082 or
make it obsolete I won't be offended in any way.

Also, if anyone could see their way to landing the patches for any or all of the
following bugs I would appreciate it. I don't have CVS access and I haven't had
any luck when asking on irc
bug 299887
bug 300442
bug 300265
bug 298498
Mano was kind enough to land the patches. I suspect that these are enough to
cause a bit of merge hell but hopefully not too much.
Thanks Robert, I'll continue down my development path here and merge with your
changes in a little bit. 

Here's a screenshot of what I have right now:
http://www.bengoodger.com/software/mb/extensions/inline-update.png

Attached patch closer... (obsolete) — Splinter Review
(not merged yet, that should be fun)
Attachment #189392 - Attachment is obsolete: true
I merged it and really like the changes. I have a couple of concerns that I
suspect you have already thought of.
If the list is long (e.g. items out of view) and an update check is performed on
all items there is the possibility that there will be no visual indication that
there is an update for the items out of view waiting to be installed.

If an update check finds multiple updates there is no way to install them all
with one action.
That's why I'm planning a prompt dialog to appear after the update completes
(when invoked by the user). 

We could move items with updates to the top but that might be annoying. 

Not sure what else we might do. 
(In reply to comment #17)
> That's why I'm planning a prompt dialog to appear after the update completes
> (when invoked by the user). 
Perhaps this would only be needed when multiple items are checked.

> We could move items with updates to the top but that might be annoying. 
A new type of item could be created similar to a download to avoid rearranging
the items. I'd suggest appending them to the bottom due to the perf impact of
reordering the items in the container when inserting. After the last item is
added ensureElementIsVisible could be called on the last item added. Items of
this type would be removed if the user installs it and any items of this type
could be removed during the next load of the extensions datasource.
Hey Robert, did you mean you merged this with your error reporting patch...? I
can't find that bug now but it looked like you made error reporting more fine
grained than it was, and I don't know that my nsIAddonUpdateCheckListener
interface patch does - so I was wondering if you made code changes to basically
bring the two together - if so, maybe you should land this patch as part of your
work... :-) 
No, I just merged it with everything that had already landed. From looking over
this patch it should be fairly easy to update the patch in bug 245082 for the
changes.
Hey Ben, any word on when this will land? I'm holding off on a patch for bug 264750.
FYI - I told rob that I'm getting closer to having all this work, ETA is like 1d
or so. 
Attached patch closer... (obsolete) — Splinter Review
still need to make the prompt work and come up with a solution for mismatch...
Attachment #189499 - Attachment is obsolete: true
Attached patch closer... (obsolete) — Splinter Review
OK, this pretty much works except mismatch is broken. Once I get that done,
this will be ready for review.
Attachment #189837 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
OK this is pretty much done. I'll write some notes on what's going on here
tomorrow and then request review.
Attachment #189852 - Attachment is obsolete: true
Comment on attachment 189972 [details] [diff] [review]
patch

>Index: browser/app/profile/firefox.js

>-pref("extensions.update.lastUpdateDate", 0);    // UTC offset when last Extension/Theme 
>-                                                // update was performed. 

No need for this pref anymore, it is now handled by the Timer Manager


>+pref("extensions.update.interval", 86400);  // Check for updates to Extensions and 
>+                                            // Themes every day

Allow the Extension System to check for updates more frequently (every day...)

>Index: browser/base/content/browser.js

>-  var updatePanel = document.getElementById("softwareupdate");
>-  try {
>-    updatePanel.init();
>-  }
>-  catch (e) { }
>-

Remove the rogue update notifier panel from the browser UI - it's showing up as
an empty button...

>-  // Ensure the Software Update item is visible on the menubar on Windows and
>-  // Linux, and on the navigation toolbar on MacOSX (since we can't put items on
> ..
>-    theToolbar.appendChild(updateItem);

ditto

>Index: browser/base/content/browser.xul

>-      <toolbaritem id="softwareupdate-item" title="&updatesItem.title;" align="center" pack="center">
>-        <toolbarbutton type="updates" id="softwareupdate"/>
>-      </toolbaritem>

ditto

>Index: browser/components/preferences/advanced.js

> ...
>+      const URI_EXTENSIONS_WINDOW = 
>+        "chrome://mozapps/content/extensions/extensions.xul?type=extensions";
>+      openDialog(URI_EXTENSIONS_WINDOW, "", features, "updatecheck");
> ...

Extension Update is now performed in the Extensions window, not in the wizard
UI, 
which is reserved solely for mismatch checking. 

>Index: toolkit/locales/jar.mn

>-  locale/@AB_CD@/mozapps/extensions/finalize.dtd                  (%chrome/mozapps/extensions/finalize.dtd)

Remove unnecessary files

>Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd

>-<!ENTITY cmd.update.label                 "Update">
>+<!ENTITY cmd.update.label                 "Find Updates">

This text is more accurate

> <!ENTITY cmd.update.accesskey             "U">
>-<!ENTITY cmd.update.tooltip               "Checks for Updates to your Extensions">
>+<!ENTITY cmd.update.tooltip               "Finds Updates to your Extensions">

ditto

>+<!ENTITY installNow.label                 "Update Now">
>+

Label for the Update Now button on each Extension that has an available update
ready
for download. 

>Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties

>+updatingMessage=Looking for updates to %S...
>+updateAvailableMessage=A new version of %S (%S) is available.

Text to display in the Extension's entry in the Extension window when an update
check is being performed and when an update is available.

>+updateFailedMsg=Update failed for this item.
>+updateDisabledMsg=Update is disabled for this item.

Some status error text to display if necessary

>-# Note: here is the unit 'em' necessary
>+# Note: the unit string 'em' is necessary here!

Repair the English here.

>+updatesAvailableMessage1=%S found updates to the following items:
>+updatesAvailableMessage2=Click Install Now to download and install the updates now.
>+updatesAvailableAccept=Install Now
>+updatesAvailableCancel=Later
>+updatesAvailableTitle=Updates Found
>+itemFormat=%S %S (New version: %S)

Strings for the "Updates Available (to multiple extensions)" dialog that
appears when
there are updates found to multiple extensions after the user clicks the "Find
Update"
button. 

>Index: toolkit/locales/en-US/chrome/mozapps/extensions/finalize.dtd

Kill this file

>Index: toolkit/locales/en-US/chrome/mozapps/extensions/mismatch.dtd

ditto

>Index: toolkit/locales/en-US/chrome/mozapps/extensions/update.dtd

>-<!ENTITY  restart.title                   "Upgrade Complete">
>-<!ENTITY  restart.updated.label           "&brandShortName; successfully downloaded and installed updates. You will have to restart &brandShortName; to complete the update.">

Remove unnecessary strings

>-
>-<!ENTITY  resetHomepage.label             "Use &brandShortName; Start as my Home Page">
>-<!ENTITY  resetHomepage.accesskey         "H">
>+<!ENTITY  versioninfo.title               "Compatibility Updates">
>+<!ENTITY  versioninfo.intro               "Checking for compatibility updates to your Extensions...">

ditto, and add some more informative text for the VersionInfo check page. 

>Index: toolkit/mozapps/extensions/jar.mn
>-* content/mozapps/extensions/update.xul                         (content/update.xul)
>-* content/mozapps/extensions/update.xml                         (content/update.xml)
>-* content/mozapps/extensions/update.js                          (content/update.js)
>-  content/mozapps/extensions/update.css                         (content/update.css)
> * content/mozapps/extensions/about.xul                          (content/about.xul)
> * content/mozapps/extensions/about.js                           (content/about.js)
>-* content/mozapps/extensions/finalize.xul                       (content/finalize.xul)
>+* content/mozapps/extensions/list.xul                           (content/list.xul)
>+* content/mozapps/extensions/list.js                            (content/list.js)
>+* content/mozapps/extensions/update.xul                         (content/update.xul)
>+* content/mozapps/extensions/update.js                          (content/update.js)
>+* content/mozapps/extensions/update.xml                         (content/update.xml)
>+* content/mozapps/extensions/update.css                         (content/update.css)

Add, remove files. 

>Index: toolkit/mozapps/extensions/content/extensions.js

>-const URI_EXTENSION_UPDATE_DIALOG     = "chrome://mozapps/content/extensions/update.xul";
>-

The extensions window no longer invokes this dialog, only the mismatch checker,
so
we don't need this constnat anymore

>+function LOG(msg) {
>+  dump("*** " + msg + "\n");
>+}

Handy log function. 

>+function onExtensionUpdateNow(aEvent)
>+{
>+  var item = gExtensionManager.getItemForID(getIDFromResourceURI(
>+    aEvent.target.id));
>+  gExtensionManager.addDownloads([item], 1);
>+}

Handles user clicking on the Update Now button on an extension for which an
update
is available and ready to be installed. 

>+  gExtensionsView.addEventListener("extension-updatenow", onExtensionUpdateNow, false);

Add a listener to handle when the user clicks on the "Update Now" button on an
extension for which there is an update available and ready to install. 

>-  gObserverIndex = gExtensionManager.addDownloadListener(gDownloadManager);
>+  gObserverIndex = gExtensionManager.addUpdateListener(gDownloadManager);

API name change

>-    catch (e) { }
>+    catch (e) {
>+      if (window.arguments[0] == "updatecheck")
>+        performUpdate();
>+    }

If the window argument isn't a param block for initialization (i.e. we weren't
invoked
by XPInstall but some other place), perform an update check right away if
necessary.

>   gExtensionsView.removeEventListener("richview-select", onExtensionSelect, false);
>+  gExtensionsView.removeEventListener("extension-updatenow", onExtensionUpdateNow, false);

Tidy up. 

>-  gExtensionManager.removeDownloadListenerAt(gObserverIndex);
>+  gExtensionManager.removeUpdateListenerAt(gObserverIndex);

API Name change

>-
>-  removeDownload: function (aEvent)
>-  {

Unnecessary function

>+  getElementForAddon: function(aAddon) 
>+  {
>+    var element = document.getElementById(PREFIX_ITEM_URI + aAddon.id);
>+    if (aAddon.id == aAddon.xpiURL)
>+      element = document.getElementById(aAddon.xpiURL);
>+    return element;
>   },

Locate the <extension> element for a given nsIUpdateItem object. By default, 
look for an element for an already installed item by looking for an element
with the id of the Item URI prefix + its ID, but fall back to using the 
addon's XPI URL as the id - since that's the id for items that are being
downloaded and have not yet been installed. 

>-  // nsIExtensionDownloadListener
>-  onStateChange: function (aURL, aState, aValue)
>+  // nsIAddonUpdateListener
>+  onStateChange: function (aAddon, aState, aValue)

API change - takes a nsIUpdateItem now, not a URL.

>-    var element = document.getElementById(aURL);
>+    var element = this.getElementForAddon(aAddon);

Use our handy helper method (see above).

... and adjust the function to accomodate for the fact that we're given a 
nsIUpdateItem with an xpiURL property now, not just a URL string.

>-  onProgress: function (aURL, aValue, aMaxValue)
>+  onProgress: function (aAddon, aValue, aMaxValue)

API Change (given a nsIUpdateItem, not a URL)

>-    var element = document.getElementById(aURL);
>+    var element = this.getElementForAddon(aAddon);

Helper method,

also adjust function to accomodate new parameter types. 

>   QueryInterface: function (aIID) 
>   {
>-    if (!aIID.equals(Components.interfaces.nsIExtensionDownloadListener) &&
>+    if (!aIID.equals(Components.interfaces.nsIAddonUpdateListener) &&

Interface name change.

>+// Update Listener
>+//
>+function UpdateCheckListener() {

Listens to every stage of the Update Check process, collects a list of addons
for
which there are updates available and shows a dialog to the user indicating
that they
should update now if there is more than one found (if only one is found - just
show
the result inline since it'll be easy to spot). 

>+function performUpdate()
>+{
>+  gExtensionsViewController.commands.cmd_update(null);
>+}

Global function callback that other windows can use to invoke an update check
when
they locate this window using the window mediator. 

>   onCommandUpdate: function ()
>   {
>     var extensionsCommands = document.getElementById("extensionsCommands");
>-    for (var i = 0; i < extensionsCommands.childNodes.length; ++i) {
>-      var command = extensionsCommands.childNodes[i];
>-      if (this.isCommandEnabled(command.id))
>-        command.removeAttribute("disabled");
>-      else
>-        command.setAttribute("disabled", "true");
>-    }
>+    for (var i = 0; i < extensionsCommands.childNodes.length; ++i)
>+      this.updateCommand(extensionsCommands.childNodes[i]);
>+  },
>+  
>+  updateCommand: function (command) 
>+  {
>+    if (this.isCommandEnabled(command.id))
>+      command.removeAttribute("disabled");
>+    else
>+      command.setAttribute("disabled", "true");

Use a handy updater function that we can use to tickle enabledness from
elsewhere, too.

>     cmd_update: function (aSelectedItem)
>     { 
> ...
>-      var ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
>-                         .getService(Components.interfaces.nsIWindowWatcher);
>-      ww.openWindow(window, URI_EXTENSION_UPDATE_DIALOG, "", features, ary);

We don't open an update wizard UI anymore when the update button is clicked,
rather

>+      var items = [];
>+      if (!aSelectedItem)
>+        items = gExtensionManager.getItemList(gItemType, { });
>+      else {
>+        var id = getIDFromResourceURI(aSelectedItem.id);
>+        items = [gExtensionManager.getItemForID(id)];
>+      }
>+      var listener = new UpdateCheckListener();
>+      gExtensionManager.update(items, items.length, false, listener);

... we gather a list of items to be updated and tell the EM to update them,
providing
a listener and showing the update check notifications inline. 

>Index: toolkit/mozapps/extensions/content/extensions.xml
>+        <xul:stack class="extension-icon-stack">
>+          <xul:vbox pack="start" align="start">
>+            <xul:image class="extension-icon" xbl:inherits="src=image"
>+                       style="width: 32px; max-width: 32px; height: 32px; max-height: 32px;"/>
>+          </xul:vbox>
>+          <xul:vbox pack="end" align="end">
>+            <xul:image class="extension-badge"/>
>+          </xul:vbox>
>+        </xul:stack>

Badge the "Update Available" icon on the Addon's icon. 

>+    <implementation>
>+      <field name="eventPrefix">"extension-"</field>
>+    </implementation>
>+    <handlers>
>+      <handler event="command">
>+      <![CDATA[
>+        if (event.originalTarget.getAttribute("anonid") == "extension-install-button")
>+          this.fireEvent("updatenow");
>+      ]]>
>+      </handler>
>+    </handlers>

Add an "Update Now" button for Extensions that have updates available. 

>Index: toolkit/mozapps/extensions/content/extensions.xul

>+                     predicate="http://www.mozilla.org/2004/em-rdf#availableUpdateURL"
>+                     object="?available-update-url"/>
>+            <binding subject="?extension"

An arc that contains the URL of the XPI of the available update

>+                       availableUpdateURL="?available-update-url"

... store it on the extension in an attribute. 

>Index: toolkit/mozapps/extensions/content/finalize.xul

Remove this file.

>Index: toolkit/mozapps/extensions/content/list.js
>Index: toolkit/mozapps/extensions/content/list.xul

A dialog that can list extensions and show text about them. 

>Index: toolkit/mozapps/extensions/content/update.js
>-// window.arguments[1...] is an array of nsIUpdateItem implementing objects 
>+// window.arguments[...] is an array of nsIUpdateItem implementing objects 

Stop passing in useless parameters.

>-const PREF_UPDATE_EXTENSIONS_COUNT              = "extensions.update.count";
>-const PREF_UPDATE_EXTENSIONS_SEVERITY_THRESHOLD = "extensions.update.severity.threshold";
>-
>-const PREF_UPDATE_SEVERITY                      = "update.severity";

Remove unused prefs. 

>-    gUpdateTypes = window.arguments[0];
>-    gShowMismatch = window.arguments[1];
>-
>-    var items = window.arguments;
>-    if (window.arguments.length == 2) {

Remove useless parameters

>+    this.items = window.arguments;
>+    if (this.items.length == 0) {
>       var em = Components.classes["@mozilla.org/extensions/manager;1"]
>                          .getService(Components.interfaces.nsIExtensionManager);
>-      items = [0,0].concat(em.getUpdateableItemList(gUpdateTypes, { }));
>+      this.items = em.getUpdateableItemList(nsIUpdateItem.TYPE_ADDON, { });
>     }

Ditto. 

>-    
>-    for (var i = 2; i < items.length; ++i)
>-      this.items.push(items[i].QueryInterface(nsIUpdateItem));

No flattening needed since window.arguments == this.items now

>-    var pref = Components.classes["@mozilla.org/preferences-service;1"]
>-                         .getService(Components.interfaces.nsIPrefBranch);
>-    this.shouldSuggestAutoChecking = gShowMismatch && 
>-                                     !pref.getBoolPref(PREF_UPDATE_EXTENSIONS_AUTOUPDATEENABLED);
>+    var pref = 
>+        Components.classes["@mozilla.org/preferences-service;1"].
>+        getService(Components.interfaces.nsIPrefBranch);
>+    this.shouldSuggestAutoChecking = 
>+      gShowMismatch && 
>+      !pref.getBoolPref(PREF_UPDATE_EXTENSIONS_AUTOUPDATEENABLED);

Whitespace/formatting

> 
>-    if (gShowMismatch) 
>-      gMismatchPage.init();
>-    else
>-      document.documentElement.advance();
>-  },
>-  
>-  uninit: function ()
>-  {
>-    // Ensure all observers are unhooked, just in case something goes wrong or the
>-    // user aborts. 
>-    gUpdatePage.uninit();  

No uninit function needed.

>+    document.documentElement.currentPage = 
>+      document.getElementById("versioninfo");

Advanced past the dummy to the versioninfo check to get us started.

>-    
>-    if (this.succeeded) {
>-      // Downloading and Installed Extension
>-      this.clearExtensionUpdatePrefs();

Unused prefs. 

>-    // Send an event to refresh any FE notification components. 
>-    var os = Components.classes["@mozilla.org/observer-service;1"]
>-                       .getService(Components.interfaces.nsIObserverService);
>-    os.notifyObservers(null, "Update:Ended", "1");

We don't use notifications anymore

>-  clearExtensionUpdatePrefs: function ()
>-  {
>-    var pref = Components.classes["@mozilla.org/preferences-service;1"]
>-                         .getService(Components.interfaces.nsIPrefBranch);
>-    if (pref.prefHasUserValue(PREF_UPDATE_EXTENSIONS_COUNT)) 
>-      pref.clearUserPref(PREF_UPDATE_EXTENSIONS_COUNT);

Unused prefs.

More notes later.
Looks good.
>+      var items = [];
>+      if (!aSelectedItem)
>+        items = gExtensionManager.getItemList(gItemType, { });
This should use getUpdateableItemList.
Comment on attachment 189972 [details] [diff] [review]
patch

>+var gVersionInfoPage = {
> ...
>+    var em = Components.classes["@mozilla.org/extensions/manager;1"]
>+                       .getService(Components.interfaces.nsIExtensionManager);
>+    em.update(gUpdateWizard.items, gUpdateWizard.items.length, true, this);
>+  },

Do a VersionInfo check before showing the list of incompatible extensions -
some of the
items the user has installed may actually be compatible, just not have updated
VersionInfo
yet. 

>+  onUpdateEnded: function() {
>+    if (gUpdateWizard.items.length > 0) {
>+      // There are still incompatible addons, inform the user.
>+      document.documentElement.currentPage = 
>+        document.getElementById("mismatch");

If we still have incompatible addons, show the mismatch page listing them... 

>+    else {
>+      // VersionInfo compatibility updates resolved all compatibility problems,
>+      // close this window and continue starting the application...
>+      close();

... otherwise just close up and continue starting, we're all good. 

>+    if (status == nsIAUCL.STATUS_VERSIONINFO) {
>+      for (var i = 0; i < gUpdateWizard.items.length; ++i) {
>+        var item = gUpdateWizard.items[i].QueryInterface(nsIUpdateItem);
>+        if (addon.id == item.id) {
>+          gUpdateWizard.items.splice(i, 1);
>+          break;
>+        }
>+      }
>     }
>+    ++this._completeCount;
>+
>+    // Update the status text and progress bar
>+    var progress = document.getElementById("versioninfo.progress");
>+    progress.mode = "normal";
>+    progress.value = Math.ceil((this._completeCount / this._totalCount) * 100);

If we found a VersionInfo update, strip this item from the list of items that
are incompatible, update the progress bar and continue. 

>+
>+    var incompatible = document.getElementById("mismatch.incompatible");
>+    for (var i = 0; i < gUpdateWizard.items.length; ++i) {
>+      var item = gUpdateWizard.items[i].QueryInterface(nsIUpdateItem);
>+      var listitem = document.createElement("listitem");
>+      listitem.setAttribute("label", item.name + " " + item.version);
>+      incompatible.appendChild(listitem);
>+    }

Move initialization of the mismatch UI into its onPageShow handler

>-  _messages: ["Update:Extension:Started", 
>-              "Update:Extension:Ended", 
>-              "Update:Extension:Item-Started", 
>-              "Update:Extension:Item-Ended",
>-              "Update:Extension:Item-Error",
>-              "Update:Ended"],
>-  

API change means no more observers.

>-    var os = Components.classes["@mozilla.org/observer-service;1"]
>-                       .getService(Components.interfaces.nsIObserverService);
>-    for (var i = 0; i < this._messages.length; ++i)
>-      os.addObserver(this, this._messages[i], false);
>-

ditto

>-    em.update(gUpdateWizard.items, gUpdateWizard.items.length, false);
>+    em.update(gUpdateWizard.items, gUpdateWizard.items.length, false, this);

Need to pass a nsIAddonUpdateCheckListener now, too. 

>-  _destroyed: false,  
>-  uninit: function ()
>-  {
>-    if (this._destroyed)
>-      return;

No uninit necessary.

>+  onUpdateEnded: function() {
>+    var nextPage = document.getElementById("noupdates");
>+    if (gUpdateWizard.itemsToUpdate.length > 0)
>+      nextPage = document.getElementById("found");
>+    document.documentElement.currentPage = nextPage;

Report finding of the updates to the user. 

>-  _totalCount: 0,
>-  get totalCount()
> ...

No need to do fancy stuff to detect count of addons to be updated... we got
that at
the start with gUpdateWizard.items, remember?


>-  observe: function (aSubject, aTopic, aData)
>-  {

Move impl of this function into nsIAddonUpdateCheckListener methods. 

>+  onAddonUpdateEnded: function(addon, status) {
>+    if (status == nsIAUCL.STATUS_UPDATE)
>+      gUpdateWizard.itemsToUpdate.push(addon);
>+    else if (status == nsIAUCL.STATE_ERROR)
>+      gUpdateWizard.errorItems.push(addon);

Discover updates and errors. 

>-
>-      if (gUpdateTypes & nsIUpdateItem.TYPE_ADDON)
>-        this.buildAddons();
>+      this.buildAddons();

No need for the conditional goop. We're always building addons now since this
wizard
knows how to do nothing else. 

>Index: toolkit/mozapps/extensions/content/update.xul

>-        onunload="gUpdateWizard.uninit();"

No need for uninit. 

>+  
>+  <wizardpage id="dummy" pageid="dummy"/>

Since onPageShow fires before the onLoad event, and that really messes with our
mojo, start with a dummy page so that onLoad fires and we set the |currentPage|
property on the wizard so that events are fired in the right order. 


>+  <wizardpage id="versioninfo" pageid="versioninfo" next="mismatch"
>+              label="&versioninfo.title;"
>+              onpageshow="gVersionInfoPage.onPageShow();">
>+    <progressmeter id="versioninfo.progress" mode="undetermined"/>
>+    <hbox align="center">
>+      <image id="versioninfo.throbber" class="throbber"/>
>+      <label flex="1" crop="right">&versioninfo.intro;</label>
>+    </hbox>
>+    <separator/>
>+  </wizardpage>

This is the VersionInfo Check page. 

The User Experience here could be better - strictly speaking this check could
also discover updates to available extensions rather than having to do that
check again later, but I'm just trying to get this done nice and quickly so
rob et al can move on, we can fuss over the UI details later. 

>Index: toolkit/mozapps/extensions/locale/finalize.dtd

Remove file. 

>Index: toolkit/mozapps/extensions/locale/mismatch.dtd

Remove file. 

>Index: toolkit/mozapps/extensions/public/nsIExtensionManager.idl

>- * Interface for handling download and install notifications for Extensions
>- * and Themes.
>- */
>-[scriptable, uuid(9048223f-ec50-49e5-9866-80ee8f26179d)]
>-interface nsIExtensionDownloadListener : nsISupports

Rename this to nsIAddonUpdateListener (symmetry with
nsIAddonUpdateCheckListener)

>+   * @param   listener
>+   *          An nsIAddonUpdateCheckListener object which will be notified during
>+   *          the update check process. 
>    */
>    void update([array, size_is(itemCount)] in nsIUpdateItem items,
>                in unsigned long itemCount, 
>-               in unsigned long versionUpdateOnly);
>+               in unsigned long versionUpdateOnly,
>+               in nsIAddonUpdateCheckListener listener);

Take a listener to send notifications to as the check progresses.

>-  long addDownloadListener(in nsIExtensionDownloadListener listener);
>+  long addUpdateListener(in nsIAddonUpdateListener listener);

Nomenclature change. 

>- * Interface representing an object that can update a set of Extensions,
>- * Themes etc. 
>- */
>-[scriptable, uuid(c0b7517f-0b3a-41a2-bde8-ba3ac8a5af47)]
>-interface nsIExtensionItemUpdater : nsISupports

This interface isn't used for anything by anyone outside of 
nsExtensionManager.js

>+/**
>+ * Interface for handling download and install progress notifications for 
>+ * addons.
>+ */
>+[scriptable, uuid(bb86037c-98c1-4c22-8e03-1e4c9fc89a8e)]
>+interface nsIAddonUpdateListener : nsISupports

Like nsIExtensionDownloadListener, except with a newer, spiffier name, and
all the methods take nsIUpdateItems rather than URLs, so it's a lot easier for
us to do useful things when we're called. 

>+/**
>+ * Interface for handling notifications during the addon update check process.
>+ */
>+[scriptable, uuid(c946119f-9e7c-41aa-a794-803148045350)]
>+interface nsIAddonUpdateCheckListener : nsISupports

Handles update check notifications. Robert, you probably want to massage this
one
with your error reporting goodness. 

>Index: toolkit/mozapps/extensions/service/UpdateItem.java

Die WebService, Die! (This is where the awful name nsIUpdateItem came from,
btw... 
I wish to change it to nsIAddon, once the EM smoke clears)

>Index: toolkit/mozapps/extensions/service/Version.java

Remove file. 

>Index: toolkit/mozapps/extensions/service/VersionCheck.java

Remove file. 

>Index: toolkit/mozapps/extensions/service/new_web_service.sh

Remove file. 

>Index: toolkit/mozapps/extensions/service/unix_deploy.sh

Remove file. 

>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in

>-function getCharPrefWithDefault(aPrefName, aDefault) {
>-  try {
>-    return gPref.getCharPref(aPrefName);
>-  }
>-  catch (e) {
>-    return aDefault;
>-  }
>-}

Ew. No one documented this one, either. 

>+function getPref(func, preference, defaultValue) {

Replace with this one, more generic and useful. 

>   // Reading the registry may throw an exception, and that's ok.  In error
>   // cases, we just leave ourselves in the empty state.
>   try {
>-    this._readReg();
>+    var path = this._appKeyPath + "\\Extensions";
>+    var key = Components.classes["@mozilla.org/windows-registry-key;1"]
>+                        .createInstance(nsIWindowsRegKey);
>+    key.open(this._rootKey, path, nsIWindowsRegKey.ACCESS_READ);
>+    this._readAddons(key);
>   } catch (e) {
>+    if (key)
>+      key.close();
>   }
> }

Move the addons reading code into this location so that we can be sure that the
key
is properly closed after it is read!

>+  get _appKeyPath() {

A convenient property that holds the string path to the App's general Registry
key. 

>+  // test access by attempting to write to this location
>   get canAccess() {
>+    try {
>+      var key = Components.classes["@mozilla.org/windows-registry-key;1"]
>+                          .createInstance(nsIWindowsRegKey);
>+      key.open(this._rootKey, this._appKeyPath, nsIWindowsRegKey.ACCESS_WRITE);
>+      key.close();
>+      return true;
>+    }
>+    catch (e) {
>+    }

Access is not necessarily forbidden, do a test to see if we are actually
allowed to write
to this location. 

>-  gLoggingEnabled = getBoolPrefWithDefault(PREF_EM_LOGGING_ENABLED, false);
>+  gLoggingEnabled = getPref("getBoolPref", PREF_EM_LOGGING_ENABLED, false);

Use the new function. 

>-    gLoggingEnabled = getBoolPrefWithDefault(PREF_EM_LOGGING_ENABLED, false);
>+    gLoggingEnabled = getPref("getBoolPref", PREF_EM_LOGGING_ENABLED, false);

ditto. 

>     if (isDirty)
>-        return this._finishOperations();
>+      return this._finishOperations();
>+      
>+    this._startTimers();

Whitespace, + start background update check timers. 

>+    var tm = 
>+        Components.classes["@mozilla.org/updates/timer-manager;1"]
>+                  .getService(Components.interfaces.nsIUpdateTimerManager);
>+    var interval = getPref("getIntPref", PREF_EM_UPDATE_INTERVAL, 86400); 
>+    tm.registerTimer("addon-background-update-timer", this, interval);

Register a long-duration timer with the Timer Manager service. 

>+  notify: function(timer) {
>+    this.update([], 0, false, null);
>+  },

And actually perform the update on the timer. 

>+    var ignoreMTimeChanges = getPref("getBoolPref", PREF_EM_IGNOREMTIMECHANGES,
>+                                     false);

New function. 

>+    var currAppVersion = getPref("getCharPref", PREF_EM_APP_EXTENSIONS_VERSION,
>+                                 gApp.version);
>+    var lastAppVersion = getPref("getCharPref", PREF_EM_LAST_APP_VERSION, "");

more of the same. 

>-      var updateTypes = Components.classes["@mozilla.org/supports-PRUint8;1"]
>-                                  .createInstance(Components.interfaces.nsISupportsPRUint8);
>-      updateTypes.data = nsIUpdateItem.TYPE_ADDON;

Mismatch UI no longer needs any of this crap since it is single purpose now. 

>+    var currAppVersion = getPref("getCharPref", PREF_EM_APP_EXTENSIONS_VERSION,
>+                                 gApp.version);

New function. 

>-      _iinstallRDF: null,
>+      _installRDF: null,

Yikes. 

>-        gOS.addObserver(this, "Update:Extension:Started", false);
>-        gOS.addObserver(this, "Update:Extension:Item-Ended", false);
>-        gOS.addObserver(this, "Update:Extension:Item-Error", false);
>-        gOS.addObserver(this, "Update:Extension:Ended", false);

No more observer notifications! Yay!

>-        em.update([item], 1, true);
>+        em.update([item], 1, true, this);

Requires a listener. 

>-      observe: function(subject, topic, data) {

Don't need this anymore, implementation moves to the nsIAUCL methods. 

(... lots of implementation moving from observe: to nsIAUCL methods)

>+                  type            : EM_I(type),
>+                  availableUpdateURL    : null,
>+                  availableUpdateVersion: null };

Reset availableUpdateURL/Version values when we install a new item.

>+    ds.updateProperty(id, "displayDescription");
>+    ds.updateProperty(id, "availableUpdateURL");

Tickle the ds so the front end updates. 

>+                  type            : EM_I(type),
>+                  availableUpdateURL      : null,
>+                  availableUpdateVersion  : null };
>     for (var p in props)
>       ds.setItemProperty(id, EM_R(p), props[p]);
>+    ds.updateProperty(id, "displayDescription");
>+    ds.updateProperty(id, "availableUpdateURL");

ditto. 

>-  update: function(items, itemCount, versionUpdateOnly) {
>+  update: function(items, itemCount, versionUpdateOnly, listener) {

We take a listener now. 

>+    var appVersion = getPref("getCharPref", PREF_EM_APP_EXTENSIONS_VERSION,
>+                             gApp.version);

New function 

>-    updater.checkForUpdates(items, items.length, versionUpdateOnly);
>+    updater.checkForUpdates(items, items.length, versionUpdateOnly, listener);

Takes a listener. 

>+    var urls = [];
>     var txn = new ItemDownloadTransaction(this);
>     for (var i = 0; i < itemCount; ++i) {
>       var currItem = items[i];
>       var txnID = Math.round(Math.random() * 100);
>-      txn.addDownload(currItem.name, currItem.xpiURL, currItem.iconURL, 
>-                      currItem.type, txnID);
>+      txn.addDownload(currItem, txnID);
>       this._transactions.push(txn);
>+      urls.push(currItem.xpiURL);

Build up a list of XPI URLs we are downloading. Also, I changed the transaction
object's |addDownload| method to take a nsIUpdateItem, since it needs to notify
the FE through nsIAddonUpdateListener which requires one, not just a URL
string.

>+    
>+    // Cancel the existing xpimgr...
>+    gOS.notifyObservers(txn, "xpinstall-progress", "cancel");  
>+    
>+    // And start a new one...
>+    var xpimgr = 
>+        Components.classes["@mozilla.org/xpinstall/install-manager;1"].
>+        createInstance(Components.interfaces.nsIXPInstallManager);
>+    xpimgr.initManagerFromChrome(urls, urls.length, txn);

Cancel the existing XPInstallManager, if any, and create a new one for the
download. 
We do this because we want to share the same UI for new extension installation
with
updates - and in the new installation case the browser content handler created
a
XPInstallManager itself and the theatrics of dealing with that vs. one we
control
are simply too complicated, so we make sure that any XPInstallManager in use is
created by us. 

>-  onStateChange: function(transaction, url, state, value) {
>+  onStateChange: function(transaction, addon, state, value) {

All of these methods take nsIUpdateItems now. 

>-    for (var i = 0; i < this._downloadListeners.length; ++i)
>-      this._downloadListeners[i].onStateChange(url, state, value);
>+    for (var i = 0; i < this._updateListeners.length; ++i)
>+      this._updateListeners[i].onStateChange(addon, state, value);

Terminology change. 

>+    // If we're updating an installed item for which content is already built,
>+    // update the "displayDescription" property so the restart now message is
>+    // shown.
>+    if (addon.id != addon.xpiURL) {

This test here is interesting. For items that are not yet in the datasource, we
set
the ID of the nsIUpdateItem representing that item to be the download URL,
which is
the same as the .xpiURL. So when we do a addon.id != addon.xpiURL check, what
we're
saying is "are you something that's already installed" because if it is already
installed
it should have an ID distinct from its download URL. 

>+      var ds = this.datasource;
>+      ds.updateProperty(addon.id, "displayDescription");
>+      ds.updateProperty(addon.id, "availableUpdateURL");
>+      ds.updateProperty(addon.id, "updateable"); 

Tickle the DS. 

>+  onProgress: function(addon, value, maxValue) {

We take a nsIUpdateItem now.

>+    for (var i = 0; i < this._updateListeners.length; ++i)
>+      this._updateListeners[i].onProgress(addon, value, maxValue);

Terminology change. 

>+  _updateListeners: [],
>+  addUpdateListener: function(listener) {
>+    for (var i = 0; i < this._updateListeners.length; ++i) {
>+      if (this._updateListeners[i] == listener)

Terminology change. 

...

>   QueryInterface: function(iid) {
>     if (!iid.equals(Components.interfaces.nsIExtensionManager) &&
>+        !iid.equals(Components.interfaces.nsITimerCallback) &&

We implement this now too, for our background update listener. 

>+  addDownload: function(addon, id) {
>+    this._downloads.push({ addon: addon, waiting: true });
>+    this._manager.datasource.addDownload(addon);

Terminology change. 

>+/**
>+ * A listener object to the update check process that routes notifications to
>+ * the right places and keeps the datasource up to date.
>+ */
>+function AddonUpdateCheckListener(listener, datasource) {

This is the actual listener that is attached to the update process, and ensures
that both the datasource and the front end get notified of updates that are
found. 

>+  checkForUpdates: function(aItems, aItemCount, aVersionUpdateOnly, 
>+                            aListener) {
>+    this._listener = new AddonUpdateCheckListener(aListener, this._emDS);
>+    if (this._listener)
>+      this._listener.onUpdateStarted();

Create the item described above. 

>+    // If |targetAppInfo| is null, then this item does not exist in the 
>+    // datasource yet, probably because this update check operation was spawned
>+    // during a Phone Home operation and the item has not yet been installed.
>+    // Return!
>+    if (!targetAppInfo) 
>+      return;

The Update code previously was not applying version updates to the datasource
in the 
"VersionInfo only" query case because it assumed the only person doing that was
the 
Phone Home checker - which runs at a time when the item has not yet been
installed into
the datasource so there's no reason to update the target app info... however
the mismatch
UI now does a VersionInfo-only check first to update items that are compatible
but don't
think they are yet, and we need this info to be reflected into the datasource,
so 
we always now apply discovered changes into the datasource. If there is no
target app 
info object for a given item, that means it isn't installed yet (The phone home
case)
and we should just return early. 

>-      if (this._emDS.getItemProperty(aLocalItem.id, "disabled") == "true") 
>+      var op = StartupCache.entries[aLocalItem.installLocationKey][aLocalItem.id].op;
>+      if (op == OP_NEEDS_DISABLE ||
>+          this._emDS.getItemProperty(aLocalItem.id, "disabled") == "true")

This is important. If an item was disabled by the |checkForMismatches| function
before
we got here, it has not yet had the "disabled" property set on it since the app
has
not restarted... it only has the OP_NEEDS_DISABLE op set in the Startup Cache
and in
the PendingOperations list... so we need to check for either case here and
enable if
either the DS property exists OR the op is present. 

>-      getCharPrefWithDefault(PREF_EM_APP_EXTENSIONS_VERSION, gApp.version);
>+      getPref("getCharPref", PREF_EM_APP_EXTENSIONS_VERSION, gApp.version);

New function. 

>+  checkForDone: function(item, status) {
>+    if (this._listener) {
>+      try {
>+        this._listener.onAddonUpdateEnded(item, status);
>+      }
>+      catch (e) {
>+        LOG("ExtensionItemUpdater:checkForDone: Failure in listener's onAddonUpdateEnded: " + e);
>+      }
>+    }
>+    if (--this._responseCount == 0 && this._listener) {
>+      try {
>+        this._listener.onUpdateEnded();
>+      }
>+      catch (e) {
>+        LOG("ExtensionItemUpdater:checkForDone: Failure in listener's onUpdateEnded: " + e);
>+      }

Check for update ending, and carefully notify. 

>       if (!gPref.getBoolPref(PREF_EM_ITEM_UPDATE_ENABLED.replace(/%UUID%/, aItem.id))) {
>-        gOS.notifyObservers(aItem, "Update:Extension:Item-Ended", "update-check-disabled");
>-        this._updater._checkForDone();
>+        var status = nsIAddonUpdateCheckListener.STATUS_DISABLED;
>+        this._updater.checkForDone(aItem, status);

New ways of doing this. 

>-      gOS.notifyObservers(aItem, "Update:Extension:Item-Error", e);
>-      this._updater._checkForDone();
>+      this._updater.checkForDone(aItem, 
>+                                 nsIAddonUpdateCheckListener.STATUS_FAILURE);

Public vs. private method. Pass the item and the reason, too, so we can notify
using
the nsIAUCL interface. 

>-      if (!this._versionUpdateOnly) {
> ...
>+        this._updater._applyVersionUpdates(aLocalItem, sameItem);

See lengthy comment above about applying version info updates in all update
conditions. 

> ...

Lots more dealing with new nsIAUCL method of notification, checking for done,
etc. 

>-  onDatasourceError: function(aItem, aError) {

This method isn't used anymore, impl moves to onError:

>+    var appVersion = getPref("getCharPref", PREF_EM_APP_EXTENSIONS_VERSION,
>+                             gApp.version);

New function. 

>+      version = getPref("getCharPref", PREF_EM_APP_EXTENSIONS_VERSION,
>+                        gApp.version);

ditto.

>-                    "", /* XPI Update URL */
>+                    this.getItemProperty(id, "availableUpdateURL"),

When we construct a nsIUpdateItem object for an item, fill its xpiURL property
with an
available update URL, if present. 

>+   *          or null, if no target application data exists for the specified
>+   *          id in the supplied datasource.

See lengthy comment above about version info checks applying updates to
datasource. 

>+      var ver = getPref("getCharPref", PREF_EM_APP_EXTENSIONS_VERSION,
>+                        gApp.version);

New function. 

>     var targetApps = datasource.GetTargets(r, EM_R("targetApplication"), true);
>+    if (!targetApps)
>+      return null;

Safeguarding, see lengthy comment above. 

>-  addDownload: function(name, url, iconURL, type) {
>+  addDownload: function(addon) {

Function takes a nsIUpdateItem

>+    if (addon.id != addon.xpiURL)
>+      return;

See comment above about equality of these two properties and what that means. 

>-    ctr.RemoveElement(res, true);
>+    if (ctr.IndexOf(res) != -1) 
>+      ctr.RemoveElement(res, true);

Be more careful. 

>+  onAddonUpdateEnded: function(addon, status) {
>+    LOG("Datasource: Addon Update Ended: " + addon.id + ", status: " + status);
>+    this._updateURLs[addon.id] = null;
>+    var url = null, version = null;
>+    var updateAvailable = status == nsIAddonUpdateCheckListener.STATUS_UPDATE;
>+    if (updateAvailable) {
>+      url = EM_L(addon.xpiURL);
>+      version = EM_L(addon.version);
>+    }
>+    this.setItemProperty(addon.id, EM_R("availableUpdateURL"), url);
>+    this.updateProperty(addon.id, "availableUpdateURL");
>+    this.setItemProperty(addon.id, EM_R("availableUpdateVersion"), version);
>+    this.updateProperty(addon.id, "displayDescription");
>+  },

When we discover an update, assert information about it into the datasource. 

>+    var appVersion = getPref("getCharPref", PREF_EM_APP_EXTENSIONS_VERSION,
>+                             gApp.version);

New function. 

>+  
>+  /** 
>+   * Gets the em:availableUpdateURL - the URL to an XPI update package, if
>+   * present, or a literal string "novalue" if there is no update XPI URL.
>+   */
>+  _rdfGet_availableUpdateURL: function(item, property) {
>+    var value = this._inner.GetTarget(item, property, true);
>+    if (!value) 
>+      return EM_L("none");
>+    return value;
>+  },

New property getter. 

>+    if (id in this._updateURLs && this._updateURLs[id] == "checking")
>+      return getLiteral("updatingMessage", itemName, BundleManager.appName);
>+    var node = this._inner.GetTarget(item, EM_R("availableUpdateURL"), true); 
>+    if (node) {
>+      var version = this.getItemProperty(id, "availableUpdateVersion");
>+      return getLiteral("updateAvailableMessage", itemName, 
>+                        version);
>+    }

DisplayDescription can now also show a message saying that there's a newer
version
of this addon available. 

>     var installLocation = InstallLocations.get(this.getInstallLocationKey(id));
>-    if (!installLocation.canAccess)
>+    if (!installLocation || !installLocation.canAccess)
>       return EM_L("false");

Be more careful (sometimes this causes js errors without)

>Index: toolkit/mozapps/extensions/src/soapupdater.js

Remove this file. 

>Index: toolkit/themes/winstripe/mozapps/extensions/extensions.css

> extension {
>-  padding-top: 13px;
>-  padding-bottom: 13px;
>-  -moz-padding-start: 13px;
>-  -moz-padding-end: 10px;
>+  padding-top: 7px;
>+  padding-bottom: 7px;
>+  -moz-padding-start: 7px;
>+  -moz-padding-end: 7px;

Make extensions more streamlined so you can see more of them. 

> .extension-icon {
>-  -moz-margin-end: 10px; 
>+  -moz-margin-end: 2px; 

ditto. 

>+extension[availableUpdateURL="none"] .extension-badge {
>+  display: none;
>+}
>+
>+extension[loading="true"] .extension-badge {
>+  display: -moz-box;
>+  width: 16px;
>+  height: 16px;
>+  margin-bottom: -3px;
>+  -moz-margin-end: -2px;
>+  list-style-image: url("chrome://global/skin/throbber/Throbber-small.gif") !important;
>+}
>+
>+.extension-badge {
>+  display: -moz-box;
>+  width: 16px;
>+  height: 16px;
>+  margin-bottom: -3px;
>+  -moz-margin-end: -2px;
>+  list-style-image: url("chrome://mozapps/skin/update/extensionalert.png");
>+  -moz-image-region: rect(0px 48px 16px 32px);
>+}
>+
>+extension[availableUpdateURL="none"] .extension-install-button-box {
>+  display: none;
>+}
>+
>+.extension-install-button-box {
>+  display: -moz-box;
>+  margin: 0px;
>+}
>+
>+.throbber {
>+  width: 16px;
>+  height: 16px;
>+  list-style-image: url("chrome://global/skin/throbber/Throbber-small.gif") !important;
>+}
>+

Badge extensions with an update icon when there's one available. 

OK, that's it.
At a high level, I have:

- replaced the update check observer notifications with a new interface,
nsIAddonUpdateCheckListener

- renamed nsIExtensionDownloadListener to nsIAddonUpdateListener, and made its
methods all take nsIUpdateItems

- consolidated the downloading functionality so that all extension download
operations now go through a single XPInstallManager

- made various bug fixes to the ExtensionManager (WindowsRegistry location,
Datasource, etc)

- tweaked the mismatch UI so that it works better, and uses the new interfaces.

- brought Extension Update inline into the Extensions list, showing information
about available updates on the individual extension items. 

- updated the various other pieces that use the API or invoke UI to do updating. 

Would one (or more) of Robert, Benjamin or Darin please review this?
Attachment #189972 - Flags: review?(rob_strong)
Comment on attachment 189972 [details] [diff] [review]
patch

+updatesAvailableMessage2=Click Install Now to download and install the updates
now.
nit: "now" at the end of the sentence is implied.

+    case nsIAUCL.STATUS_DISABLED:
+      element.setAttribute("description",
strings.getString("updateDisabledMsg")); 
+      break;
+    }
+  },
There are several other cases (e.g. appManaged="true", no write access,
needs-uninstall, needs-upgrade, and needs-install though the last two are 
debatable). Using getUpdateableItemList will not return these items but it may
be more appropriate to return them and change the description as is done for
STATUS_DISABLED. I could go with either method you prefer though I am leaning
towards setting the description since it will make what is going on clearer to
the user. I can create a patch for this after this is checked in so this patch
isn't held up.

+  // test access by attempting to write to this location
   get canAccess() {
+    try {
+      var key = Components.classes["@mozilla.org/windows-registry-key;1"]
+			   .createInstance(nsIWindowsRegKey);
+      key.open(this._rootKey, this._appKeyPath,
nsIWindowsRegKey.ACCESS_WRITE);
+      key.close();
+      return true;
+    }
+    catch (e) {
+    }
     return false;
This tests write access to the regkey unlike the other canAccess which checks
for write access to the file system for a location. Is this to allow updating
app compatibility?

-    var appVersion = getCharPrefWithDefault(PREF_EM_APP_EXTENSIONS_VERSION,
-					     gApp.version);
+    var appVersion = getPref("getCharPref", PREF_EM_APP_EXTENSIONS_VERSION,
+			      gApp.version);
     if (version === undefined) {
-      version = getCharPrefWithDefault(PREF_EM_APP_EXTENSIONS_VERSION,
-					gApp.version);
+      version = getPref("getCharPref", PREF_EM_APP_EXTENSIONS_VERSION,
+			 gApp.version);
     }		    
This doesn't seem right though it didn't seem right before as well.

+  onUpdateEnded: function() {
+    LOG("Datasource: Update Ended");
+    this._updateItems = { };
+  },
Shouldn't that be this._updateURLs = { };

+  onAddonUpdateStarted: function(addon) {
+    LOG("Datasource: Addon Update Started: " + addon.id);
+    this._updateURLs[addon.id] = addon.id;
You use the id here but use the value "checking" to set the displayDescription

+    if (id in this._updateURLs && this._updateURLs[id] == "checking")
+      return getLiteral("updatingMessage", itemName, BundleManager.appName);


Other items to consider
The only indication that an update is being performed is that the Find Updates
button is disabled.

With bug 288054 preventing installing an update after finding one the user
should be notified that web sites must be allowed to install software, etc.
This also breaks upgrading an incompatible extension during an app version
mismatch.
Attachment #189972 - Flags: review?(rob_strong) → review-
(In reply to comment #30)
> Other items to consider
> The only indication that an update is being performed is that the Find Updates
> button is disabled.
scratch that... it was an earlier note before I noticed the cause.
> +  // test access by attempting to write to this location
>    get canAccess() {
> +    try {
> +      var key = Components.classes["@mozilla.org/windows-registry-key;1"]
> +			   .createInstance(nsIWindowsRegKey);
> +      key.open(this._rootKey, this._appKeyPath,
> nsIWindowsRegKey.ACCESS_WRITE);
> +      key.close();
> +      return true;
> +    }
> +    catch (e) {
> +    }
>      return false;
> This tests write access to the regkey unlike the other canAccess which checks
> for write access to the file system for a location. Is this to allow updating
> app compatibility?

Unless there is a very compelling use-case that I'm missing, I don't think that
we should ever have the registry install location have canAccess=true.
Why would you not give people the ability to configure extensions using the
registry? 
Attached patch newer patchSplinter Review
- text adjustment (remove "now")
- remove impl of |canAccess| (although it's very useful for testing, may be
worth finding a way to turn it on) 
- removed this line of code completely:
+    var appVersion = getPref("getCharPref", PREF_EM_APP_EXTENSIONS_VERSION,
+			      gApp.version);
... since it's not actually used by anyone and the supplied |version| parameter
takes its place
- fixed the updateItems vs updateURLs typo
- got rid of the check on "checking" since it's no longer necessary (oops)...
swapped it for a null check of this._updateItems[id] since an item's entry in
that hash is null'ed after its update ends.
Attachment #189972 - Attachment is obsolete: true
Attachment #190156 - Flags: review?(rob_strong)
> Why would you not give people the ability to configure extensions using the
> registry? 

I don't understand what this means... if an extension is installed via registry,
I think we must assume that we are not responsible in any way for
upgrading/uninstalling it, which is what I understand "canAccess" to be for.
I guess there's one school of thought that ties a particular registry location
to a particular use case. There's another that sees the location as a general
thing that can be used by anyone for any purpose. I discovered another use case
for that location ('development and testing')... there may be more... I was
assuming people writing applications to set the reg key would be including the
appManaged flag in their install.rdf to prevent the internal update checking
from being invoked, but if that's not a reasonable assumption maybe this is
something that is #ifdef DEBUG_ben ;-) 
I think that this should be DEBUG_ben at least for this release cycle... we can
document and implement something in the 1.9 cycle if we want to allow user
access to registry-installed locations, but I'm not sure we want to use the
appmanaged arc for this... I'll think more (I was actually planning on
disallowing any extension from using appmanaged unless they were app-global).
OK, my current patch has the code removed, I can just hack in something for my
own purposes when I'm testing something next time.
Comment on attachment 190156 [details] [diff] [review]
newer patch

>-   *
>+   * Whether or not this is the currently 
Looks like this inadvertently slipped in with nsIUpdateService.idl

A patch was checked in yesterday that changed richview-select to select in
extensions.js
Attachment #190156 - Flags: review?(rob_strong) → review+
(In reply to comment #37)
> I think that this should be DEBUG_ben at least for this release cycle... we can
> document and implement something in the 1.9 cycle if we want to allow user
> access to registry-installed locations, but I'm not sure we want to use the
> appmanaged arc for this... I'll think more (I was actually planning on
> disallowing any extension from using appmanaged unless they were app-global).
appManaged is only added to the extensions datasource for app-global items

http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#5912
Blocks: 300116
I just checked out the ui when updating themes and the list is not wide enough
to view the install button properly. You might want to consider the patch in bug
292617 for this issue.
Ben, can this land soon? I have a couple of patches I'm holding off on for this.
I will land this right quick.
Landed. Closing this bug out now, will be attacking any fallout in separate
bugs. Thanks Robert!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Blocks: 302167
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.