299716 - ([e-mail]) Need for em:targetApplication marker for "the toolkit"

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Allan Beaufour]

It should be possible to create extensions that target "the toolkit" in general,
so it's f.x. possible to create an XPI that can be installed generally for all
XULRunner applications. For that we need a "toolkit" em:targetApplication.

Example extensions are DOM Inspector, Venkman, and XForms (bug 298431).

Comment 1

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

To check for compatibility we would also need a toolkit version as well. 

Comment 2

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

For app compatibility the pref value from app.extensions.version is used... for
this perhaps a new pref like app.extensions.toolkit.version? If an extension
doesn't specify a targetApplication (e.g. GUID) that matches the current
targetAppliction then we would look for the toolkit targetAppliication in the
install.rdf and use the value specified in app.extensions.toolkit.version to
check against its minVersion and maxVersion.

Comment 3

[Darin Fisher]

I'd say that an unknown targetApplication should cause the extension to be
disabled.  However, if no targetApplication is specified, then perhaps it could
be assumed that the extension is targeting any toolkit app.  How about we use
targetToolkit for this then?  On the other hand, it is possible for an extension
to specify multiple targetApplication arcs, and would you really want an
extension to be used in application without explicit testing against said
application?

Comment 4

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

I think that is simpler. If there are no targetApplication arcs then check for a
targetToolkit arc. If that isn't compatible when version checked against the
pref value or it doesn't exist then the extension is not compatible, etc. It
would probably be easiest to add a + to toolkit.extensions.version at the same
time app.extensions.version gets a +. This could also be opt in by not checking
for targetToolkit unless the pref is set for the app.

As for whether or not an extension should be used with an app it hasn't been
tested with I suspect there will be problems. For example, DOMi has issues with
Thunderbird and if I am not mistaken this is due to an overlay being provided by
Thunderbird that doesn't have a dtd that is required. I still think this is
appropriate to implement even with the potential problems.

Comment 5

[Benjamin Smedberg]

I *do* want some extensions to work in any gecko application without testing,
e.g. NPAPI plugins, venkman, xforms. I don't know why we need a separate
toolkitTarget arc.

Shaver and I agreed that the ID should be "[e-mail]". I
don't see why it can't just be another targetApplication, but instead of using
the app.extensions.version pref we should use the gecko ID (which should
currently be "1.7+"), which I can hopefully add to nsIXULAppInfo relatively easily.

Comment 6

[Darin Fisher]

> I *do* want some extensions to work in any gecko application without testing,
> e.g. NPAPI plugins, venkman, xforms. I don't know why we need a separate
> toolkitTarget arc.

We don't necessarily need a separate targetToolkit arc.  I suggested it as a
matter of convenience.  The code that checks the targetApplication might be more
complicated if it has to treat "[e-mail]" in a special
way, or maybe not *shrug* :-)


> ... we should use the gecko ID (which should
> currently be "1.7+"), which I can hopefully add to nsIXULAppInfo relatively 
> easily.

Yeah, I agree.  We should expose the Gecko/Toolkit version in the same way that
we expose the application version ("+" syntax and all).

Comment 7

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

Benjamin - I'll start on the EM code so it will hopefully be ready when you add
a gecko ID that can be version checked to nsIXULAppInfo.

Comment 8

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

Note to self: need to add toolkitVersion to software update.

Comment 9

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

bah... we have to support notifying the user that an extension is not compatible with the toolkit as we do when an extension is not compatible with the application.

Comment 10

[Benjamin Smedberg]

Can't you reuse the same notification and substitute in "the Mozilla platform" or something like that?

Comment 11

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

True but I highly suspect that the definition of "the Mozilla Platform" is not known to the vast majority of users.

Comment 12

[Benjamin Smedberg]

True, but they aren't going to know what "1.8" or "1.8.1" or "1.9" mean anyway, so I'm really not sure it matters. Perhaps beltzner can come up with a more elegant solution.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #9)
> bah... we have to support notifying the user that an extension is not
> compatible with the toolkit as we do when an extension is not compatible with
> the application.

Couldn't we use the same notification that we do at the app level? The fact that Firefox2 (or Songbird 1.0) is built on Gecko1.8.1 won't matter to a user. What will matter is that they can't use DOMInspector2.0 with their application. The only tricky part is if we're committed to telling them what version of the application/toolkit they should be trying to get, and I'm not convinced (especially in the case of toolkit version incompatibilities) that this information is useful/helpful.

Comment 14

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

(In reply to comment #13)
> Couldn't we use the same notification that we do at the app level? The fact
> that Firefox2 (or Songbird 1.0) is built on Gecko1.8.1 won't matter to a user.
> What will matter is that they can't use DOMInspector2.0 with their application.
> The only tricky part is if we're committed to telling them what version of the
> application/toolkit they should be trying to get, and I'm not convinced
> (especially in the case of toolkit version incompatibilities) that this
> information is useful/helpful.
The version info in the message has been very handy in the community when troubleshooting.
http://forums.mozillazine.org/viewtopic.php?t=383793

Comment 15

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

Mook, I am guessing that since you unassigned this from me that you are thinking about taking this on. I've already added support in blocklisting for this but there are several other areas that need to be changed to support this... especially since the current interfaces only support passing one id, minVersion, maxVersion and this will require a minimum of two. At the same time it would be appropriate to also fix bug 301236 since it will also require changes to some of these same interfaces.

Comment 16

[:Mook]

Oop, sorry Robert, all I wanted to do was CC myself.  I apologize for the gaffe, and thanks for telling me about it.

Comment 17

[Alex Vincent [:WeirdAl]]

requesting blocking-firefox3; this is seriously hurting Inspector and Venkman for XULRunner.

Comment 18

[Alex Vincent [:WeirdAl]]

Attachment: sample install.rdf file, version 1

I'm going to start work on this bug.  I'm providing this attachment as (hopefully) a sample of what a toolkit-extension install.rdf looks like.  I'd appreciate someone taking a look and seeing what needs changing in this sample.

Comment 19

[Alex Vincent [:WeirdAl]]

Attachment: sample install.rdf file, version 1.1

like, getting the min- and max-version numbers right :)

Comment 20

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

(In reply to comment #19)
> Created an attachment (id=248455) [edit]
> sample install.rdf file, version 1.1
> 
> like, getting the min- and max-version numbers right :)
That looks correct to me

Comment 21

[Alex Vincent [:WeirdAl]]

So by adjusting ExtensionsDataSource.prototype.isCompatible, I was able to force the EM to install the extension... but after the restart, the extension is disabled again.  After several hours, I can't locate the code that determines the isDisabled property for the rich list items (i.e. what sets it in the composite rdf).  Help!

On a related note, per comment 0, should apps targeted at toolkit should be installed in XULRunner's dist/bin/extensions (as opposed to the application's extensions directory)?

Comment 22

[Nickolay_Ponomarev]

> On a related note, per comment 0, should apps targeted at toolkit should be
> installed in XULRunner's dist/bin/extensions (as opposed to the application's
> extensions directory)?

This restriction seems totally unnecessary. For example, an application's extension might depend on such a "toolkit extension" (say, jslib), in which case it makes more sense to allow installation of the toolkit extension in the profile or in the application's extensions directory.

Comment 23

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

When one refuses to give up, one eventually finds a solution.  :)

The update mechanism changes have NOT been tested yet.  Nor, for that matter, has this been tested with installing and uninstalling currently acceptable extensions.  I plan on running those smoketests tonight.

Comment 24

[Dão Gottwald [::dao]]

(In reply to comment #23)
> -  getItemProperty: function(id, property) { 
> +  getItemProperty: function getItemProperty(id, property) {

You removed a trailing space, fine, but why |function getItemProperty|?

Comment 25

[Alex Vincent [:WeirdAl]]

(In reply to comment #24)
> You removed a trailing space, fine, but why |function getItemProperty|?

Debugging anonymous functions (as opposed to named functions) is painful, more than the cost of the few bytes needed.  Think error stack traces.  Also, although it doesn't apply yet in this case, Venkman likes named functions too.  It's one of those little JavaScript nuances that one learns the hard way how important it is.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Because named functions are better for debugging.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Note to self: read all bugmail for a bug before responding! (sorry for the spam).

Comment 28

[Dão Gottwald [::dao]]

Does make sense, although I wonder if that should be part of a patch (I didn't see it anywhere else). Anyway, just a few bytes as you said -- enough noise from me for the day.

Comment 29

[Alex Vincent [:WeirdAl]]

Comment on attachment 248599 [details] [diff] [review]
patch, v1

meh, updates is broken

Comment 30

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.1

update bustage fixed.

Comment 31

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

I suspect that Software Update will also need to be patched. When an update to the app is advertised the new version of the toolkit will need to be checked when looking for incompatible extensions prior to upgrading for Software Update's incompatible extensions user interface.

Comment 32

[Alex Vincent [:WeirdAl]]

Robert, can you point me to the code that handles that, and preferably a good testcase for it?

Comment 33

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

I don't believe that Software Update has a testcase for this. Basically, it will need to retrieve the new version of the toolkit from the xml and perform the same checks as it does when checking against the new version of the application.
http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/update/content/updates.js#636

Comment 34

[Alex Vincent [:WeirdAl]]

I think that case is actually covered by this patch, just from a cursory look at the code.  updates.js#708 gets the extension manager service.  Line 710 calls on nsIExtensionManager::getIncompatibleItemList(), which lives at nsExtensionManager.js.in#5391.

This in turn calls the datasource's getIncompatibleItemList() method (nsExtensionManager.js.in#6654), which calls on isCompatible() (nsExtensionManager.js.in#6557).  My patch upgrades isCompatible to handle the toolkit target case.

Comment 35

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

The new version of the toolkit needs to be set in Software Spdate update xml (it doesn't exist currently), Software Update will then need to retrieve it, and it will need to pass it to the EM for checking against, etc. This all happens before the download of the new version of the app and it couldn't possibly already be accounted for.

Comment 36

[Alex Vincent [:WeirdAl]]

So there has to be a change to either the Checker code (http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/update/src/nsUpdateService.js.in#1812 ) which handles the XMLHttpRequest to the server, or to the server-side code which feeds XML markup to the checker.

In the former case, we need the Checker object to fetch two different URL's.  I think I can do that.

Comment 37

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.2

This patch adds a new Checker() for toolkit apps, and adjusts the update listener to handle both of them at the same time.

Comment 38

[Alex Vincent [:WeirdAl]]

Comment on attachment 248755 [details] [diff] [review]
patch, v1.2

dammit, something else is broken.

Comment 39

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.3

That should do it, and I've fully tested it from start to finish.  I'll attach a zip file with four sample XPI's (each containing only an install.rdf file) and an update.rdf in a moment.

Notes:
* I wish we could port this back to Firefox 2, but alas, I had to change nsIUpdateItem to add a new property (targetAppID).
* ExtensionManager implemented nsIClassInfo, but the QueryInterface function denied it...
* In my testing, one of the steps I did was to drag an older-versioned XPI into the addons window; I was surprised to see the addons window accept that.  I don't believe that's a behavior I caused to regress.

Comment 40

[Alex Vincent [:WeirdAl]]

Attachment: zipped set of update.rdf + 4 XPI's containing install.rdf files only

Steps to test:
(1) Unzip the zip file into a new directory.
(2) Manually adjust the <em:updateLink/> elements in the update.rdf file to refer to the local directory.
(3) Manually adjust the install.rdf files in the XPI's at their respective <<em:updateURL>/> elements to refer to the local directory.
(4) Apply the patch and rebuild toolkit/mozapps.
(5) Install the foo_point_one.xpi and oh_point_one.xpi files.
(6) Restart the application as directed.
(7) xulwidgets-foo and xulwidgets-bar should both be installed at version 0.1.
(8) Update the xulwidgets-foo and xulwidgets-bar extensions and restart.
(9) xulwidgets-foo and xulwidgets-bar should both be installed at version 0.5.2.
(10) Uninstall both extensions.

We could probably eliminate steps 2 and 3 of this test suite if the files were installed at build time into a resource:// URL.  Maybe we could automate the rest of it in a make check test if some state were to change between each restart (say, a preference or a mozstorage field), and then used that state to determine where we were in the test.  Anyone care to think about that a bit?

Comment 41

[Mike Beltzner [:beltzner, not reading bugmail]]

This is more of a XULRunner/platform issue to do with the toolkit, so clearing the Firefox 3 blocking nom and adding [wanted-1.9]

Comment 42

[Alex Vincent [:WeirdAl]]

Is there anyone who could reliably review this patch?  It has sat for over four and a half months with nary a comment from rob_strong, including repeated attempts to contact him by e-mail and by irc.

If review cycles take this long for this patch and for follow-on patches, it will become IMPOSSIBLE to land this in Gecko 1.9, and impossible for Inspector, Venkman, etc., to support applications beyond the standard mozilla.org offerings.  

Comment 43

[Alex Vincent [:WeirdAl]]

mconnor, bsmedberg, Toolkit review page says both of you are reviewers for the
extension manager; can you please lend a hand here???

Comment 44

[Dave Townsend [:mossop]]

I haven't really looked at this in any depth or tested in any way, but it looks like there might be an issue if the install.rdf specifies compatibility for both toolkit, and the particular app. I think the info that takes precedence might be the last one in the file. I'm not sure if that's the best behaviour. I would think that taking the app compatability info in preference to toolkit would be more sensible.

Also if toolkit info is used to make the judgement, and the addon still turns out to be incompatible then you are going to get a confusing dialog:

<addon> could not be installed because it is not compatible with <current app> <current app version>. (<addon> will only work with <current app> <target toolkit version>)

Comment 45

[Mike Connor [:mconnor]]

This was nominated previously, but we didn't minus it explicitly.  Not a blocker, but wanted.  I think Dave's comments need a response before its worth reviewing anything.

Comment 46

[Alex Vincent [:WeirdAl]]

As annoyed as I was by mconnor stating this wasn't worth reviewing before I answered Mossop's comments (considering how long it was in the review queue before said comments), it turns out Mossop is exactly right.  *sigh*

Comment 47

[Dave Townsend [:mossop]]

Alex, I've just realised that the patch I posted for bug 314915 yesterday is going to conflict with yours somewhat, perhaps you could take a look at it then we can have a chat about the best way to handle that.

Comment 48

[Alex Vincent [:WeirdAl]]

Spec question:  suppose an extension advertises support for Firefox and Toolkit, but the version test for Firefox fails and the version test for Toolkit passes.  Should the extension manager install the extension as enabled or as disabled?

Similarly, suppose the Firefox version test passes for an extension, but the Toolkit one fails.  Enable the extension or disable it?

Comment 49

[Benjamin Smedberg]

The application setting should override the platform setting in all cases.

Comment 50

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.4

This patch passes a xpcshell-based test, based on bug 382752's test harness - with seven different extensions, each with two XPI's (one for the extension's 0.1 version, another for 0.2 to test updating).  The test files are not included in this patch due to CVS difficulties; I will attach a patch for that shortly.

Comment 51

[Alex Vincent [:WeirdAl]]

Attachment: test files, based on xpcshell test harness

As Mossop wrote the test harness, he might be a good person to review the tests I've written here.  :)

Comment 52

[Alex Vincent [:WeirdAl]]

Attachment: A long list of assertion stacks

I had to wallpaper over assertions in my test files - something I'm not happy about at all.  Basically, there's a bunch which this test reveals, and I don't want to block the patch for them (considering both the test and the tested code are JS-based).

I was planning on filing separate bugs blocking this one for the assertions if the patches herein passed review.  Hopefully we could get them knocked out for 1.9, and remove that ugly nsIEnvironment hack so that assertions would cause real test failures.

<bsmedberg> the not thread-safe assertion is a known bug
<bsmedberg> WeirdAl: the "bogus trigger" assertions sounds like something you should investigate

I'd like to get comments about the rest of these - where responsibility lies.

Comment 53

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

Comment on attachment 271806 [details] [diff] [review]
patch, v1.4

Alex, should be able to get to this by no this weekend... maybe sooner. Sorry about the delay.

Comment 54

[Dave Townsend [:mossop]]

Comment on attachment 271806 [details] [diff] [review]
patch, v1.4

>@@ -696,37 +696,49 @@ function getAddonTypeFromInstallManifest
>  */
> function showIncompatibleError(installData) {
>   var extensionStrings = BundleManager.getBundle(URI_EXTENSIONS_PROPERTIES);
>   var params = [extensionStrings.GetStringFromName("type-" + installData.type)];
>   var title = extensionStrings.formatStringFromName("incompatibleTitle", 
>                                                     params, params.length);
>   var message;
>   var targetAppData = installData.currentApp;
>+
>+  var targetAppName;
>+  var targetVersion;
>+  if (targetAppData.id == TOOLKIT_ID) {
>+    targetAppName = "Gecko Toolkit";
>+    targetVersion = gApp.platformVersion;
>+  } else {
>+    targetAppName = gApp.name;
>+    targetVersion = gApp.version;
>+  }
>+

targetAppData may be null so this will be throwing a warning at least.

>   if (!targetAppData) {
>-    params = [installData.name, installData.version, BundleManager.appName];
>+    params = [installData.name, installData.version, targetAppName];
>     message = extensionStrings.formatStringFromName("incompatibleMessageNoApp", 
>                                                     params, params.length);

You've dropped the localisation of the app name here, any particular reason? Regardless I think this localisation of this function needs a little more though. Maybe check in with Pike or someone else appropriate (if you already have then fine just ignore this bit), but I think rather than having a localised form of "Gecko Toolkit" we probably just want another incompatibleMsgSingleAppVersion and incompatibleMsg for the gecko case. This gives the localisers more flexibility I think.

>@@ -3968,17 +3983,18 @@ ExtensionManager.prototype = {
>       var updatedMinVersion = null;
>       var updatedMaxVersion = null;
>       var targetApps = oldExtensionsDS.GetTargets(oldRes, EM_R("targetApplication"), true);
>       while (targetApps.hasMoreElements()) {
>         var targetApp = targetApps.getNext();
>         if (targetApp instanceof Components.interfaces.nsIRDFResource) {
>           try {
>             var foundAppID = stringData(oldExtensionsDS.GetTarget(targetApp, EM_R("id"), true));
>-            if (foundAppID != currAppID) // Different target application
>+            // Different target application?
>+            if ((foundAppID != currAppID) && (foundAppID != TOOLKIT_ID))
>               continue;

Not sure there is a great deal of benefit in affecting this import from the 1.0 schema since there should be no update app info for toolkit since we never checked for it before, however as it is it is broken since you attempt to make it do something for a toolkit target app and it then compares the min/max to the app version.

>+  _isValidUpdate: function _isValidUpdate(aLocalItem, aRemoteItem) {
>+    var appExtensionsVersion = (aRemoteItem.targetAppID != TOOLKIT_ID) ?
>+                               gApp.version :
>+                               gApp.platformVersion;
>+
>+    // Check the update id.
>+    if (aRemoteItem.id != aLocalItem.id)
>+      return false;

Is this ever the case?

>+
>+    var min = aRemoteItem.minAppVersion;
>+    var max = aRemoteItem.maxAppVersion;
>+    // Check if the update will only run on a newer version of the application.
>+    if (aRemoteItem.minAppVersion &&
>+        gVersionChecker.compare(appExtensionsVersion, min) < 0)

May as well do |if (min &&| and ditto below.

>@@ -6209,21 +6232,26 @@ RDFItemUpdater.prototype = {
>     catch (e) {
>       LOG("RDFItemUpdater:checkForUpdates: There was an error loading the \r\n" + 
>           " update datasource for: " + dsURI + ", item = " + aItem.id + ", error: " + e);
>       this._updater.checkForDone(aItem, 
>                                  nsIAddonUpdateCheckListener.STATUS_FAILURE);
>       return;
>     }
> 
>+    if (!aItem.targetAppID) {
>+      throw Components.results.NS_ERROR_UNEXPECTED;
>+    }
>+

I'm concerned by this, it changes the API for update checking. Can we not determine automatically which targetApp is currently enforcing compatibility for this addon? I think we should do so and note on the update method that only certain properties of the updateitem are used for the update check.

>+      // Because we searched for aLocalItem.id above, we can reuse it here.
>+      aUpdateData.id = aLocalItem.id;
>+

UpdateData doesn't have an id property and the only place I see it used is in the _isValidUpdate method where you compare it to aLocalItem.id.

>       var targetApp = targetApps.getNext().QueryInterface(Components.interfaces.nsIRDFResource);
>-      var id = this._getPropertyFromResource(aDataSource, targetApp, "id", aLocalItem);
>-      if (id != this._updater._appID)
>+      var appID = this._getPropertyFromResource(aDataSource, targetApp, "id", aLocalItem);
>+      if ((appID != this._updater._appID) && (appID != TOOLKIT_ID))
>         continue;

I think you need to apply some logic here to make sure that the aUpdateData ends up with the application targetAppInfo details in preference to the toolkit one.

>         var maxAppVersion = this._getPropertyFromResource(aDataSource, targetApp, "maxVersion", aLocalItem);
>         var minAppVersion = this._getPropertyFromResource(aDataSource, targetApp, "minVersion", aLocalItem);
>-        if (this._updater._isValidUpdate(aLocalItem, version, minAppVersion, maxAppVersion)) {
>-          aUpdateData.found = true;
>+
>+        aUpdateData.minVersion = minAppVersion;
>+        aUpdateData.maxVersion = maxAppVersion;
>+

May as well get rid of the minAppVersion and maxAppVersion vars declared since they are only used once. Looks like some dodgy tabbing around there too, quite possibly my fault ;)

>         var anon = gRDF.GetAnonymousResource();
>-        this._inner.Assert(anon, idRes, installManifest.GetTarget(newVersionInfo, idRes, true), true);
>-        this._inner.Assert(anon, minVersionRes, installManifest.GetTarget(newVersionInfo, minVersionRes, true), true);
>-        this._inner.Assert(anon, maxVersionRes, installManifest.GetTarget(newVersionInfo, maxVersionRes, true), true);
>+        var idTarget = installManifest.GetTarget(newVersionInfo, idRes, true);
>+        var minVersionTarget = installManifest.GetTarget(newVersionInfo, minVersionRes, true);
>+        var maxVersionTarget = installManifest.GetTarget(newVersionInfo, maxVersionRes, true);
>+
>+        this._inner.Assert(anon, idRes, idTarget, true);
>+        this._inner.Assert(anon, minVersionRes, minVersionTarget, true);
>+        this._inner.Assert(anon, maxVersionRes, maxVersionTarget, true);

I'm not seeing any actual change here, am I missing something?

>@@ -7881,25 +7940,37 @@ ExtensionsDataSource.prototype = {
>   },
> 
>   /**
>    * Get the em:compatible property (whether or not this item is compatible)
>    */
>   _rdfGet_compatible: function(item, property) {
>     var id = stripPrefix(item.Value, PREFIX_ITEM_URI);
>     var targetAppInfo = this.getTargetApplicationInfo(id, this);
>+    if (targetAppInfo && (typeof targetAppInfo.appID == "undefined")) {
>+      targetAppInfo.appID = gApp.ID;
>+    }
>+

Why is appID ever not set? Also this is broken. getTargetApplicationInfo returns the first targetAppInfo it finds that is toolkit or app. We need the app one in preference.

>     var getter = "_rdfGet_" + stripPrefix(property.Value, PREFIX_NS_EM);
>-    if (getter in this)
>+    if (getter in this) {
>       target = this[getter](source, property);
>+      if (target) {
>+        return target;
>+      }
>+    }
> 
>-    return target || this._inner.GetTarget(source, property, truthValue);
>+    return this._inner.GetTarget(source, property, truthValue);
>   },

Unless I'm missing something there is no difference here?

There are a bunch of issues stemming from getTargetApplicationInfo not returning the correct targetAppInfo block in some cases, also you need to do some work with setTargetAppInfo, setUpdatedTargetAppInfo etc. They all just blindly update either of the app and toolkit target app info blocks for an add-on. You probably want to be passing in the appid to them or something.

I haven't really looked over the updateservice bits yet.

Comment 55

[Alex Vincent [:WeirdAl]]

(In reply to comment #54)
> I haven't really looked over the updateservice bits yet.

Should I wait for the completion of your review?  Also, should I request review on the next patch from you instead of Robert?

Comment 56

[Alex Vincent [:WeirdAl]]

Pike, please respond to this:

(In reply to comment #54)
> >   if (!targetAppData) {
> >-    params = [installData.name, installData.version, BundleManager.appName];
> >+    params = [installData.name, installData.version, targetAppName];
> >     message = extensionStrings.formatStringFromName("incompatibleMessageNoApp", 
> >                                                     params, params.length);
> 
> You've dropped the localisation of the app name here, any particular reason?
> Regardless I think this localisation of this function needs a little more
> though. Maybe check in with Pike or someone else appropriate (if you already
> have then fine just ignore this bit), but I think rather than having a
> localised form of "Gecko Toolkit" we probably just want another
> incompatibleMsgSingleAppVersion and incompatibleMsg for the gecko case. This
> gives the localisers more flexibility I think.

Comment 57

[Axel Hecht [:Pike]]

From an l10n point of view, the complete section http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties&rev=1.35&mark=53-70#50 is pretty much a nightmare. Not that we have the technical infrastructure to solve it better, but we shouldn't spend too much effort on making this suck less for Gecko than it already does for Firefox and Thunderbird, and for Extensions vs. Themes. All of those can have different genders or vocal harmonies already, so localizers have likely already worked around this.

You may want to poke the l10n newsgroup once you have a concrete proposal for localizing Gecko. I'm neither sure what it should be really called (I've seen Gecko, Toolkit, Gecko Toolkit here so far), nor where it should be put. I'm not aware of any existing strings that hold that.

Another point that should be resolved is to what the translation should actually be. If we'd mention Gecko, should it be translated, or rather transcribed? Sounds like a question for legal, maybe.

Comment 58

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

I'm not convinced that we should be that specific in the UI and we can always log more specific info to the error console.

cc'ing beltzner for his take on what the text should be.

Comment 59

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

Alex, since we already use [e-mail] for blocklisting and I got buyin to use that name way back I'd like the toolkit id to also be [e-mail]. I've already ran this by shaver and bsmedberg and they are both fine with it.

Comment 60

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.5 (includes test files)

I've cleaned up the assertion which I was responsible for (the empty URI) by making sure XPInstallManager doesn't get any empty URI's.

This patch is twice as big as the preceding one, simply because it merges the extmgr patch with the test files patch.  In other words, not much really changes here.

(In reply to comment #54)
>Also you need to do
> some work with setTargetAppInfo, setUpdatedTargetAppInfo etc. They all just
> blindly update either of the app and toolkit target app info blocks for an
> add-on. You probably want to be passing in the appid to them or something.

I have to admit, I do not understand this part.  Can you write a testcase in addition to these modified tests which demonstrates a bug in this code?

> I haven't really looked over the updateservice bits yet.

(In reply to comment #57)
> You may want to poke the l10n newsgroup once you have a concrete proposal for
> localizing Gecko. I'm neither sure what it should be really called (I've seen
> Gecko, Toolkit, Gecko Toolkit here so far), nor where it should be put. I'm not
> aware of any existing strings that hold that.

For now, I'm calling it Gecko Toolkit.  I believe this shouldn't hold up the patch.  (beltzner hasn't replied yet.)

> Another point that should be resolved is to what the translation should
> actually be. If we'd mention Gecko, should it be translated, or rather
> transcribed? Sounds like a question for legal, maybe.

Again, I don't want to block the patch for this.  What's the contact for MoCoFo legal?

Comment 61

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

Don't worry about the string / translation issues. I'll discuss this with beltzner next week.

Comment 62

[Dave Townsend [:mossop]]

Comment on attachment 276412 [details] [diff] [review]
patch, v1.5 (includes test files)

Ok We're getting closer now, there are a few issues still present, see the comments below for most of them and some minor nits.

For the setTargetApplicationInfo issue I've attached a testcase that demonstrates the failure (it won't until one of the typos below is fixed). The problem is that calls to both setTargetApplicationInfo and setUpdatedTargetApplication make assertions for both the toolkit and app targetApps if present when only one is correct. In the best case the author always includes the right info in the update.rdf and we only end up putting incorrect information into a targetApp in the datasource that we never use, but this testcase demonstrates the worst case.

Also as I told you already, you are mistaken about what changes need to be made to the update service. There should still be only one update check, however the update xml format should be altered to include both the updated app's version and toolkit version. Contact me on IRC if this still doesn't make sense.

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

>+      // If somehow we end up with a null URL, the item isn't valid.  Move on.
>+      if (!currItem.xpiURL) {
>+        continue;
>+      }
Where are these empty urls coming from? Do we have some bug passing them in, if so please file one.
So if you're going to do this you'll have to rework this a bit. Make sure _downloadCount is incremented by the number of real downloads, bail out at the end of the loop if there were no real downloads added, don't add the observers if there were no real downloads added.
I'm not sure if we shouldn't be throwing in the event of bad downloads specified.
Minor nit, skip the braces around single statements and elsewhere in this patch.

>-        var maxAppVersion = this._getPropertyFromResource(aDataSource, targetApp, "maxVersion", aLocalItem);
>-        var minAppVersion = this._getPropertyFromResource(aDataSource, targetApp, "minVersion", aLocalItem);
>-        if (this._updater._isValidUpdate(aLocalItem, version, minAppVersion, maxAppVersion)) {
>-          aUpdateData.found = true;
>+        aUpdateData.minVersion = this._getPropertyFromResource(aDataSource, targetApp, "maxVersion", aLocalItem);
>+        aUpdateData.maxVersion = this._getPropertyFromResource(aDataSource, targetApp, "minVersion", aLocalItem);
You're flipping the min/max versions here

>+        aUpdateData.found = (this._updater._isValidUpdate(aLocalItem, aUpdateData));
Nit: drop the surrounding brackets.

>-        var versionChecker = getVersionChecker();
>-        return ((versionChecker.compare(version, minVersion) >= 0) &&
>+        rv = ((versionChecker.compare(version, minVersion) >= 0) &&
                 (versionChecker.compare(version, maxVersion) <= 0));
Nit: pull the versionCheckers into line

>+      if (id == TOOLKIT_ID) {
>+        rv =  ((versionChecker.compare(toolkitVersion, minVersion) >= 0) &&
>+               (versionChecker.compare(toolkitVersion, maxVersion) <= 0));
>+        // Keep looping, in case the app id is later.
>     }
>-    return false;
>+    }
>+    return rv;
Nit: clean up the indentation here.

>-            return { id        : id,
>+            outData = { id        : id,
                      minVersion: stringData(updatedMinVersion),
                      maxVersion: stringData(updatedMaxVersion) };
Nit: line up the properties.

>-          return { minVersion: stringData(datasource.GetTarget(targetApp, EM_R("minVersion"), true)),
>+          outData = { appID: foundAppID,
>+                      minVersion: stringData(datasource.GetTarget(targetApp, EM_R("minVersion"), true)),
                    maxVersion: stringData(datasource.GetTarget(targetApp, EM_R("maxVersion"), true)) };
Nit: line this up

>+          if (foundAppID == appID) {
>+            return outdata;
>+          }
Typo, should be outData I believe.

>Index: toolkit/mozapps/extensions/test/unit/test_bug299716.js

>+var env = Components.classes["@mozilla.org/process/environment;1"]
>+                    .getService(Components.interfaces.nsIEnvironment);
>+env.set("XPCOM_DEBUG_BREAK", "stack");
This isn't done in any other unit tests, why is it necessary here?

>+const updateListener = {
>+  _state: -1,
>+
>+  // nsIAddonUpdateListener
>+  onStateChange: function onStateChange(aAddon, aState, aValue) {
>+    if ((this._state == -1) &&
>+        (aState == Components.interfaces.nsIXPIProgressDialog.DIALOG_CLOSE)) {
>+      this._state = aState;
>+      next_test();
>+    }
>+  },
>+
>+  onProgress: function onProgress(aAddon, aValue, aMaxValue) {
>+    // do nothing.
>+  }
>+};

>+  onAddonUpdateEnded: function onAddonUpdateEnded(aAddon, aStatus) {
>+    for (var i = 0; i < ADDONS.length; i++) {
>+      if (ADDONS[i].id == aAddon.id) {
>+        ADDONS[i].newItem = aAddon;
>+        return;
>+      }
>+    }
Nit: Throughout this test you flip between two different ways of iterating the array. Please pick one.

>+  if (aAddonsEntry.installed) {
>+    do_check_eq(aItem.version, aVersion);
>+  } else {
>+    do_check_eq(aItem.version, "");
>+    do_check_eq(aItem.installLocationKey, "");
>+    do_check_eq(aItem.minAppVersion, "");
>+    do_check_eq(aItem.maxAppVersion, "");
>+    do_check_eq(aItem.name, "");
>+    do_check_eq(aItem.updateRDF, "");
>+    do_check_eq(aItem.type, "");
This is nasty and will be going away. If bug 391899 lands before this you'll have to compare aItem to null.

>+  // Make sure we can actually get our data files.
>+  const xpiFile = addonsDir.clone();
>+  xpiFile.append("test_bug299716_a_2.xpi");
>+  do_check_true(xpiFile.exists());
Don't really see why this is necessary

>+  // Make sure we can fetch the files over HTTP.
>+  const C_i = Components.interfaces;
>+  const xhr = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"]
>+                        .createInstance(C_i.nsIXMLHttpRequest)
>+  xhr.open("GET", "http://localhost:4444/addons/test_bug299716_a_2.xpi", false);
>+  xhr.send(null);
>+  do_check_true(xhr.status == 200);
>+
>+  xhr.open("GET", "http://localhost:4444/data/test_bug299716.rdf", false);
>+  xhr.send(null);
>+  do_check_true(xhr.status == 200);
Why are you testing the http server ?

>+function run_test_pt2() {
Nit: Can you put a quick comment at the start of these functions to say what they are for.

>Index: toolkit/mozapps/extensions/test/unit/data/test_bug299716.rdf

>+<!DOCTYPE RDF:RDF [
So I'm not entirely sure about using these entities, do you think it really makes it more readable?

Comment 63

[Dave Townsend [:mossop]]

Attachment: Additional testcase

Testcase mentioned in previous comment

Comment 64

[Alex Vincent [:WeirdAl]]

Quick comments, to explain certain changes:

(In reply to comment #62)
> >Index: toolkit/mozapps/extensions/public/nsIExtensionManager.idl
> Where are these empty urls coming from? Do we have some bug passing them in, if
> so please file one.

I added this to prevent the mURL.IsEmpty() assertions from firing - my testcase was passing in items with null URL's (due to their not having installed).  Although this is technically an abuse of the API, I think it's better to handle it inside the extmgr instead of fixing the testcase.  That way, invalid input is handled by the extmgr, and the assertion doesn't fire.

> >+var env = Components.classes["@mozilla.org/process/environment;1"]
> >+                    .getService(Components.interfaces.nsIEnvironment);
> >+env.set("XPCOM_DEBUG_BREAK", "stack");
> This isn't done in any other unit tests, why is it necessary here?

###!!! ASSERTION: nsStandardURL not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file c:/mozilla.org/trunk/mozilla/netwerk/base/src/nsStandardURL.cpp, line 931

###!!! ASSERTION: nsChromeRegistry not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file c:/mozilla.org/trunk/mozilla/chrome/src/nsChromeRegistry.cpp, line 438

If we checked this test in without the wallpaper, the test would fail with these assertions every time.  Hence the big XXX comment I put in right before those lines.  Without this wallpaper, the patch would be backed out in three minutes flat.  See comment 52.

I refuse to believe my patch to a JS-based component with a JS-based testcase should be held hostage to thread-safety assertions.  :)

I don't like it any more than you do (in fact, I like it a lot less), but here it appears to be a necessary evil.

> >+  // Make sure we can actually get our data files.
> >+  const xpiFile = addonsDir.clone();
> >+  xpiFile.append("test_bug299716_a_2.xpi");
> >+  do_check_true(xpiFile.exists());
> Don't really see why this is necessary
> 
> >+  // Make sure we can fetch the files over HTTP.
> >+  const C_i = Components.interfaces;
> >+  const xhr = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"]
> >+                        .createInstance(C_i.nsIXMLHttpRequest)
> >+  xhr.open("GET", "http://localhost:4444/addons/test_bug299716_a_2.xpi", false);
> >+  xhr.send(null);
> >+  do_check_true(xhr.status == 200);
> >+
> >+  xhr.open("GET", "http://localhost:4444/data/test_bug299716.rdf", false);
> >+  xhr.send(null);
> >+  do_check_true(xhr.status == 200);
> Why are you testing the http server ?

Technically speaking, I don't need these lines at all.  These are here just to make sure we're properly set up to run the real test.  (There were a couple times where my test failed because I hadn't set up properly.)

> >+<!DOCTYPE RDF:RDF [
> So I'm not entirely sure about using these entities, do you think it really
> makes it more readable?

Perhaps not, but it saves on both disk space and repeated typing.  Very handy when we dropped "components." from the toolkit ID.  It also makes sure that our RDF code can handle entities.  ;-)

Comment 65

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.6 (-w)

Answering all other nits and including Mossop's test case.  Also, I changed several dump() statements in my own test case to do_throw(), since we presume they aren't being called.

Comment 66

[Alex Vincent [:WeirdAl]]

One minor thing I missed:
http://lxr.mozilla.org/seamonkey/search?string=BUILD_TARGET

The various apps need to be updated for the new value - and the update service too, probably.

Comment 67

[Dave Townsend [:mossop]]

Comment on attachment 276593 [details] [diff] [review]
patch, v1.6 (-w)

This is looking good now from the EM point of view, couple of minor comments is all. However the nsUpdateService.js.in work still requires implementing.

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

>   void init(in AString id, in AString version, 
>             in AString installLocationKey, in AString minAppVersion, 
>             in AString maxAppVersion, in AString name,
>             in AString downloadURL, in AString xpiHash, in AString iconURL,
>-            in AString updateURL, in long type);
>+            in AString updateURL, in long type, in AString targetAppID);
I'm just wondering if the targetAppID argument should go before the minAppVersion one. Thoughts?

>       var currItem = items[i];
>+      // If somehow we end up with a null URL, the item isn't valid.  Move on.
>+      if (!currItem.xpiURL) {
>+        continue;
>+      }

Please throw an exception here, I guess NS_ERROR_INVALID_ARG and make a note in the idl that it will fail with invalid items.
Nit: Drop the braces around a single statement.


>   getTargetApplicationInfo: function(id, datasource) {
...
>+          if (foundAppID == appID) {
>+            return outData;
>+          }
Nit: drop the braces.

Comment 68

[Dave Townsend [:mossop]]

To provide clarification, here are a pair of update information files:

https://aus2.mozilla.org/update/1/Firefox/2.0.0.5/2007071317/WINNT_x86-msvc/en-US/release/update.xml
https://aus2.mozilla.org/update/1/Firefox/1.5.0.12/2007050813/WINNT_x86-msvc/en-US/release/update.xml

To make the user experience right those would have to include a toolkitVersion attribute on the update tag. So the build team would have to make that happen. Then in nsUpdateService.js.in you need to update the calls to getIncompatibleItemList such that it will return extensions that wont work in the new app/toolkit combo.

Comment 69

[Alex Vincent [:WeirdAl]]

http://lxr.mozilla.org/seamonkey/search?string=getIncompatibleItemList

This is a reminder to myself that not only update mgr calls getIncompatibleItemList.

Comment 70

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.7 (-w, wild guess on update manager)

The update manager code needs some tests.  I don't have much time (if any) to write them.  I took a best-guess shot at fixing it, but I do not vouch for it being correct.

Comment 71

[Dave Townsend [:mossop]]

Comment on attachment 277677 [details] [diff] [review]
patch, v1.7 (-w, wild guess on update manager)

So this is looking good but I need to give it a closer look to be sure and I think testcases would be required, not sure when I'll get chance to look at that, probably not this week. In the meantime I'm afraid I missed in the last review that you didn't fully answer the review in comment 62, specifically for hte addDownloads method, which is a bit simpler now we just fail:

> So if you're going to do this you'll have to rework this a bit. Make sure
> _downloadCount is incremented by the number of real downloads, bail out at the
> end of the loop if there were no real downloads added, don't add the observers
> if there were no real downloads added.

Comment 72

[Alex Vincent [:WeirdAl]]

There's some kind of really subtle bug here.  I added this right to the beginning of addDownloads():

    for (i = 0; i < itemCount; ++i) {
      var currItem = items[i];
      // If somehow we end up with a null URL, the item isn't valid.
      if (!currItem.xpiURL)
        throw Components.results.NS_ERROR_ILLEGAL_VALUE;
    }

In this scenario, this._downloadCount increments only when all the items are valid.  There's just one problem.  Suddenly only the last item will update.

I'm well and truly stumped over this.

Comment 73

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.8

Comment 74

[Alex Vincent [:WeirdAl]]

Comment on attachment 278009 [details] [diff] [review]
patch, v1.8

I don't know why, but it's working now.  The interdiff says the only change is moving the exception throw to the top of addDownloads.

Comment 75

[Alex Vincent [:WeirdAl]]

Comment on attachment 278009 [details] [diff] [review]
patch, v1.8

I think I should've included this too.

>   addDownloads: function(items, itemCount, fromChrome) { 
      if (itemCount == 0) {
        throw Components.results.NS_ERROR_ILLEGAL_VALUE;
      }
>+    for (i = 0; i < itemCount; ++i) {
>+      var currItem = items[i];
>+      // If somehow we end up with a null URL, the item isn't valid.
>+      if (!currItem.xpiURL)
>+        throw Components.results.NS_ERROR_ILLEGAL_VALUE;
>+    }

Dave, if there's anything else you need for extmgr, or I'm *still* not grokking it, can you just show me what you have in mind?

Comment 76

[Dave Townsend [:mossop]]

Comment on attachment 278009 [details] [diff] [review]
patch, v1.8

>   addDownloads: function(items, itemCount, fromChrome) { 
>+    for (i = 0; i < itemCount; ++i) {
>+      var currItem = items[i];
>+      // If somehow we end up with a null URL, the item isn't valid.
>+      if (!currItem.xpiURL)
>+        throw Components.results.NS_ERROR_ILLEGAL_VALUE;
>+    }
>+

Please add the comment to nsIExtensionManager.idl like I asked for.

>     url = url.replace(/%CHANNEL%/g, getUpdateChannel());
>+
>+    // We probably don't need to put TOOLKIT_ID in the url; it's constant.
>+    url = url.replace(/%PLATFORM_VERSION%/g, gApp.platformVersion);
>     url = url.replace(/\+/g, "%2B");

Leave the comment out of here.
I'm concerned that we are jumping between toolkitVersion and platformVersion to describe the same thing. I think since nsIXULAppInfo has already fixed on platformVersion we should use that throughout.

>     onAddonUpdateEnded: function(addon, status) {
>-      if (status == Components.interfaces.nsIAddonUpdateCheckListener.STATUS_UPDATE &&
>-          addonIsCompatible(addon, update.extensionVersion)) {
>+      const C_i = Components.interfaces;
>+      if (status != C_i.nsIAddonUpdateCheckListener.STATUS_UPDATE) {
>+        return;
>+      }

Use Ci like the rest of the codebase please.

It appears to be failing its unit tests so please fix that!

Comment 77

[Dave Townsend [:mossop]]

Comment on attachment 278009 [details] [diff] [review]
patch, v1.8

Ok ignore the unit tests, something screwed up in my tree apparently.

Rob aside from the minor comments above this is basically good to go I think, although it is another of these patches adding to nsIUpdateItem. I'm presuming you've been keeping an eye on this and are happy with the way it's working, you just need to give it the final stamp though.

We are going to liaise with the build team of course to get the platformVersion attribute added to the AUS data or this won't work properly.

Comment 78

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.8.1

The irony of the patch version number is inescapable.  (It was either that or 1.9, which would've been just as funny.  But the changes herein are really trivial.)

One more piece of bugspam coming, as I carry forward Mossop's r+.

Comment 79

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

Comment on attachment 278251 [details] [diff] [review]
patch, v1.8.1

I'm going to do this over several comments

>Index: toolkit/mozapps/extensions/public/nsIExtensionManager.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl,v
>retrieving revision 1.49
>diff -p -u -8 -w -r1.49 nsIExtensionManager.idl
>--- toolkit/mozapps/extensions/public/nsIExtensionManager.idl	16 Aug 2007 21:14:51 -0000	1.49
>+++ toolkit/mozapps/extensions/public/nsIExtensionManager.idl	25 Aug 2007 23:27:50 -0000
>...
>@@ -332,27 +333,30 @@ interface nsIExtensionManager : nsISuppo
> 
>   /**
>    * Retrieves a list of nsIUpdateItems of items that are incompatible
>    * with the supplied parameters.
>    * @param   id
>    *          The id of the application to check compatibility against
>    * @param   version
>    *          The version of the application to check compatibility against
>+   * @param   platformVersion
>+   *          The version of the toolkit to check compatibility against
>    * @param   type
>    *          The type of item to return
>    * @param   includeDisabled
>    *          true if disabled items should be included in the result set, 
>    *          false otherwise
>    * @param   countRef
>    *          The XPCJS reference to the number of items returned.
>    * @returns An array of incompatible nsIUpdateItems.
>    */
>   void getIncompatibleItemList(in AString id, 
>                                in AString version,
>+                               in AString platformVersion,
Since we now have two versions in this call could you file a followup bug to change version to appVersion?

>@@ -527,23 +533,28 @@ interface nsIUpdateItem : nsISupports
>   const unsigned long TYPE_ANY         = TYPE_APP + TYPE_ADDON;
> 
>   /**
>    * The type of this item.
>    */
>   readonly attribute long type;
> 
>   /**
>+   * The target application ID of this item.
>+   */
>+  readonly attribute AString  targetAppID;
I really don't like this name. This can be the toolkit id and the target app id when using Firefox is
{ec8030f7-c20a-464f-9b0e-13a3a9e97384}

On the other hand the toolkit id is using targetApplication.

Please change the comment to something along the lines of 
The target application ID used for checking compatibility for this item. Add-ons can specify a targetApplication id of [e-mail] in their install manifest for compatibility with all apps that use a specific release of the toolkit.

We'll figure out a way to make this less ambiguous in a release after Firefox 3 when we switch to sqlite, etc.

Comment 80

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

Comment on attachment 278251 [details] [diff] [review]
patch, v1.8.1

>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v
>retrieving revision 1.238
>diff -p -u -8 -w -r1.238 nsExtensionManager.js.in
>--- toolkit/mozapps/extensions/src/nsExtensionManager.js.in	16 Aug 2007 21:17:45 -0000	1.238
>+++ toolkit/mozapps/extensions/src/nsExtensionManager.js.in	25 Aug 2007 23:27:52 -0000
>...
>@@ -694,37 +707,50 @@ function getAddonTypeFromInstallManifest
>  */
> function showIncompatibleError(installData) {
>   var extensionStrings = BundleManager.getBundle(URI_EXTENSIONS_PROPERTIES);
>   var params = [extensionStrings.GetStringFromName("type-" + installData.type)];
>   var title = extensionStrings.formatStringFromName("incompatibleTitle", 
>                                                     params, params.length);
>   var message;
>   var targetAppData = installData.currentApp;
>+
>+  var targetAppName;
>+  var targetVersion;
>+  if (targetAppData &&
>+      (targetAppData.id == TOOLKIT_ID)) {
>+    targetAppName = BundleManager.toolkitName;
>+    targetVersion = gApp.platformVersion;
>+  } else {
>+    targetAppName = gApp.name;
>+    targetVersion = gApp.version;
>+  }
Is there a reason not to use targetAppName = BundleManager.appName here?

I'm sorely tempted to just display a simple "not compatible with appName" message here instead of introducing the term Gecko Toolkit. I'm going to ping UX again on this.

Comment 81

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

email sent to UX

We are introducing toolkit version compatibility for add-ons in
https://bugzilla.mozilla.org/show_bug.cgi?id=299716

The error message when an add-on is not compatible with the application is
incompatibleMsg=%S %S could not be installed because it is not compatible with %S %S. (%S %S will only work with %S versions from %S to %S)
incompatibleMsgSingleAppVersion=%S %S could not be installed because it is not compatible with %S %S. (%S %S will only work with %S %S)
incompatibleMessageNoApp=%S %S could not be installed because it is not compatible with %S.

Examples:
incompatibleMsg=Add-on Name 1.2.3 could not be installed because it is not compatible with Firefox 2.0 (Add-on Name 1.2.3 will only work with Firefox versions from 1.0 to 1.5)
incompatibleMsgSingleAppVersion=Add-on Name 1.2.3 could not be installed because it is not compatible with Firefox 2.0. (Add-on Name 1.2.3 will only work with Firefox 1.5)
incompatibleMessageNoApp=Add-on Name 1.2.3 could not be installed because it is not compatible with Firefox 2.0.

Adding a term to describe the toolkit in the UI just seems too confusing and I think it would be best to just use the incompatibleMessageNoApp message for this instance and log detailed information to the error console.

Comment 82

[Dave Townsend [:mossop]]

Al, I've now landed bug 391899 so you'll need to make the minor change to your testcase I mentioned in comment 62.

Comment 83

[Alex Vincent [:WeirdAl]]

I want to hold that until Robert completes his review.  I still don't know if I'll need to submit a patch for fresh review, or if the next patch will be final.

Comment 84

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

Comment on attachment 278251 [details] [diff] [review]
patch, v1.8.1

>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v
>retrieving revision 1.238
>diff -p -u -8 -w -r1.238 nsExtensionManager.js.in
>--- toolkit/mozapps/extensions/src/nsExtensionManager.js.in	16 Aug 2007 21:17:45 -0000	1.238
>+++ toolkit/mozapps/extensions/src/nsExtensionManager.js.in	25 Aug 2007 23:27:52 -0000
>...
>@@ -694,37 +707,50 @@ function getAddonTypeFromInstallManifest
>  */
> function showIncompatibleError(installData) {
>   var extensionStrings = BundleManager.getBundle(URI_EXTENSIONS_PROPERTIES);
>   var params = [extensionStrings.GetStringFromName("type-" + installData.type)];
>   var title = extensionStrings.formatStringFromName("incompatibleTitle", 
>                                                     params, params.length);
>   var message;
>   var targetAppData = installData.currentApp;
>+
>+  var targetAppName;
>+  var targetVersion;
>+  if (targetAppData &&
>+      (targetAppData.id == TOOLKIT_ID)) {
>+    targetAppName = BundleManager.toolkitName;
>+    targetVersion = gApp.platformVersion;
>+  } else {
>+    targetAppName = gApp.name;
>+    targetVersion = gApp.version;
>+  }
Why are you using gApp.name here instead of BundleManager.appName as was used previously? If nothing else using BundleManager.appName would be consistent with targetAppName being set with the value for BundleManager.toolkitName. If there isn't a good reason I'd like it changed back.

btw: we'll take care of the toolkit incompatible message in a separate bug.

>@@ -1540,17 +1565,17 @@ Installer.prototype = {
>           LOG("extractExtensionsFiles: failed to create target file for extraction " + 
>               " file = " + target.path + ", exception = " + e + "\n");
>         }
>         zipReader.extract(entryName, target);
>       }
>       zipReader.close();
>     }
> 
>-    var installer = this;
>+
please remove the extra newline

>@@ -1621,16 +1646,18 @@ Installer.prototype = {
>           }
>           LOG("Theme JAR file: " + jarFile.leafName + " contains an Old-Style " + 
>               "Theme that is not compatible with this version of the software.");
>           throw new Error("Old Theme"); // let the safe-op clean up
>         }
>       }
>     }
> 
>+    var installer = this;
>+
Please remove the extra newline

>@@ -3651,27 +3680,28 @@ ExtensionManager.prototype = {
>       var updatedMinVersion = null;
>       var updatedMaxVersion = null;
>       var targetApps = oldExtensionsDS.GetTargets(oldRes, EM_R("targetApplication"), true);
>       while (targetApps.hasMoreElements()) {
>         var targetApp = targetApps.getNext();
>         if (targetApp instanceof Components.interfaces.nsIRDFResource) {
>           try {
>             var foundAppID = stringData(oldExtensionsDS.GetTarget(targetApp, EM_R("id"), true));
>-            if (foundAppID != currAppID) // Different target application
>+            // Different target application?  (Note:  v1.0 doesn't need toolkit app.)
Please change the comment to
// Different target application?  (Note:  v1.0 didn't support toolkit app ID)

>@@ -3857,17 +3887,18 @@ ExtensionManager.prototype = {
>     while (targetApps.hasMoreElements()) {
>       var targetApp = targetApps.getNext();
>       if (targetApp instanceof Components.interfaces.nsIRDFResource) {
>         try {
>           var data = { id        : readTAProperty(targetApp, "id"),
>                        minVersion: readTAProperty(targetApp, "minVersion"),
>                        maxVersion: readTAProperty(targetApp, "maxVersion") };
>           installData.targetApps.push(data);
>-          if (data.id == gApp.ID) 
>+          if ((data.id == gApp.ID) ||
>+              ((data.id == TOOLKIT_ID) && !installData.currentApp))
Prevailing style / unneeded parens
if (data.id == gApp.ID ||
    (data.id == TOOLKIT_ID && !installData.currentApp))

>@@ -4030,17 +4061,17 @@ ExtensionManager.prototype = {
>     /**
>      * Stages a XPI file in the default item location specified by other 
>      * applications when they registered with XulRunner if the item's
>      * install manifest specified compatibility with them.
>      */
>     function stageXPIForOtherApps(xpiFile, installData) {
>       for (var i = 0; i < installData.targetApps.length; ++i) {
>         var targetApp = installData.targetApps[i];
>-        if (targetApp.id != gApp.ID) {
>+        if ((targetApp.id != gApp.ID) && (targetApp.id != TOOLKIT_ID)) {
Prevailing style / unneeded parens
if (targetApp.id != gApp.ID && targetApp.id != TOOLKIT_ID) {

>@@ -5100,19 +5137,20 @@ ExtensionManager.prototype = {
>    */
>   getItemList: function(type, countRef) {
>     return this.datasource.getItemList(type, countRef);
>   },
> 
>   /**  
>    * See nsIExtensionManager.idl
>    */
While you are here please change this comment to
/* See nsIExtensionManager.idl */

>-  getIncompatibleItemList: function(id, version, type, includeDisabled, 
>+  getIncompatibleItemList: function(id, version, platformVersion, type, includeDisabled,
>                                     countRef) {
>     var items = this.datasource.getIncompatibleItemList(id, version ? version : undefined,
>+                                                        platformVersion ? platformVersion : undefined,
>                                                         type, includeDisabled);
>     countRef.value = items.length;
>     return items;
>   },
>   
>   /**
>    * Move an Item to the index of another item in its container.
>    * @param   movingID
>@@ -5259,31 +5297,44 @@ ExtensionManager.prototype = {
>    * operation.
>    * @param   items
>    *          An array of nsIUpdateItems to begin downlading.
>    * @param   itemCount
>    *          The length of |items|
>    * @param   fromChrome
>    *          true when called from chrome
>    *          false when not called from chrome (e.g. web page)
>+   *
>+   * @throws  NS_ERROR_ILLEGAL_VALUE if any item is invalid.
>    */
Please remove this comment and replace it with
/* See nsIExtensionManager.idl */

>   addDownloads: function(items, itemCount, fromChrome) { 
>+    if (itemCount == 0) {
>+      throw Components.results.NS_ERROR_ILLEGAL_VALUE;
>+    }
>+    for (i = 0; i < itemCount; ++i) {
>+      var currItem = items[i];
>+      // If somehow we end up with a null URL, the item isn't valid.
>+      if (!currItem.xpiURL)
>+        throw Components.results.NS_ERROR_ILLEGAL_VALUE;
>+    }
>+
>     var ds = this.datasource;
>     // Add observers only if they aren't already added for an active download
>     if (this._downloadCount == 0) {
>       gOS.addObserver(this, "offline-requested", false);
>       gOS.addObserver(this, "quit-application-requested", false);
>     }
>     this._downloadCount += itemCount;
>     
>     var urls = [];
>     var hashes = [];
>     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, txnID);
>       this._transactions.push(txn);
>       urls.push(currItem.xpiURL);
>       hashes.push(currItem.xpiHash ? currItem.xpiHash : null);
>       // if this is an update remove the update metadata to prevent it from
>       // being updated during an install.
>       if (fromChrome) {
>@@ -5480,16 +5531,17 @@ ExtensionManager.prototype = {
> 
>   /**
>    * See nsISupports.idl
>    */
While you are here please change this comment to
/* See See nsISupports.idl */

>   QueryInterface: function(iid) {
>     if (!iid.equals(Components.interfaces.nsIExtensionManager) &&
>         !iid.equals(Components.interfaces.nsITimerCallback) &&
>         !iid.equals(Components.interfaces.nsIObserver) &&
>+        !iid.equals(Components.interfaces.nsIClassInfo) &&
>         !iid.equals(Components.interfaces.nsISupports))
>       throw Components.results.NS_ERROR_NO_INTERFACE;
>     return this;
>   }
> };
> 
> /**
>  * This object implements nsIXPIProgressDialog and represents a collection of
>@@ -5707,61 +5759,65 @@ ExtensionItemUpdater.prototype = {
>...
   /**
    * Checks whether a discovered update is valid for install
    * @param   aLocalItem
    *          The already installed nsIUpdateItem that the update is for
-   * @param   aVersion
-   *          The updated version of the add-on
-   * @param   aMinAppVersion
-   *          The minimum application version that the update supports
-   * @param   aMaxApVersion
-   *          The maximum application version that the update supports
-   */
-  _isValidUpdate: function(aLocalItem, aVersion, aMinAppVersion, aMaxAppVersion) {
-    var appExtensionsVersion = gApp.version;
-
-    // Check if the update will only run on a newer version of Firefox. 
-    if (aMinAppVersion && 
-        gVersionChecker.compare(appExtensionsVersion, aMinAppVersion) < 0) 
+   * @param   aRemoteItem
+   *          The nsIUpdateItem we are trying to update to
+   */
Please add 
@returns true if the item is compatible and is not blocklisted.
         false if the item is not compatible or is blocklisted.

>+  _isValidUpdate: function _isValidUpdate(aLocalItem, aRemoteItem) {
>+    var appExtensionsVersion = (aRemoteItem.targetAppID != TOOLKIT_ID) ?
>+                               gApp.version :
>+                               gApp.platformVersion;
>+
>+    var min = aRemoteItem.minAppVersion;
>+    var max = aRemoteItem.maxAppVersion;
>+    // Check if the update will only run on a newer version of the application.
>+    if (min &&
>+        gVersionChecker.compare(appExtensionsVersion, min) < 0)
On the same line please.

>       return false;
> 
>-    // Check if the update will only run on an older version of Firefox. 
>-    if (aMaxAppVersion && 
>-        gVersionChecker.compare(appExtensionsVersion, aMaxAppVersion) > 0) 
>+    // Check if the update will only run on an older version of the application.
>+    if (max &&
>+        gVersionChecker.compare(appExtensionsVersion, max) > 0)
On the same line please

>@@ -6058,21 +6115,21 @@ RDFItemUpdater.prototype = {
>           "\r\nTry checking that: \r\n" + 
>           " 1. Your remote RDF file exists at the location.\r\n" + 
>           " 2. Your RDF file is valid XML (starts with <?xml version=\"1.0?\">\r\n" + 
>           "    and loads in Firefox displaying pretty printed like other XML documents\r\n" + 
>           " 3. Your server is sending the data in the correct MIME\r\n" + 
>           "    type (text/xml)");
>     }      
>     
>-  
>     // Parse the response RDF
>     function UpdateData() {}; 
>     UpdateData.prototype = { version: "0.0", updateLink: null, updateHash: null,
>-                             minVersion: "0.0", maxVersion: "0.0", found: false };
>+                             minVersion: "0.0", maxVersion: "0.0",
>+                             appID: "", found: false };
Is there some reason this can't be initialized to null?

>@@ -6216,34 +6275,37 @@ RDFItemUpdater.prototype = {
>   
>   _parseV20Update: function(aDataSource, aUpdateResource, aLocalItem, aUpdateData, aUpdateCheckType) {
>     var version = this._getPropertyFromResource(aDataSource, aUpdateResource, 
>                                                 "version", aLocalItem);
>     var taArc = gRDF.GetResource(EM_NS("targetApplication"));
>     var targetApps = aDataSource.GetTargets(aUpdateResource, taArc, true);
>     while (targetApps.hasMoreElements()) {
>       var targetApp = targetApps.getNext().QueryInterface(Components.interfaces.nsIRDFResource);
>-      var id = this._getPropertyFromResource(aDataSource, targetApp, "id", aLocalItem);
>-      if (id != this._updater._appID)
>+      var appID = this._getPropertyFromResource(aDataSource, targetApp, "id", aLocalItem);
>+      if ((appID != this._updater._appID) && (appID != TOOLKIT_ID))
Prevailing style / unneeded parens
if (appID != this._updater._appID && appID != TOOLKIT_ID)

>         continue;
>       
>       /* If we are looking for new versions then test whether this discovered
>        * version is larger than any previously found update. Otherwise check
>        * if this update is for the same version as we have installed. */
>       var result = gVersionChecker.compare(version, aUpdateData.version);
>+      aUpdateData.appID = appID;
Seems like this should be set in the conditional below.

>       if (aUpdateCheckType == nsIExtensionManager.UPDATE_CHECK_NEWVERSION ? result > 0 : result == 0) {
>-        var maxAppVersion = this._getPropertyFromResource(aDataSource, targetApp, "maxVersion", aLocalItem);
>-        var minAppVersion = this._getPropertyFromResource(aDataSource, targetApp, "minVersion", aLocalItem);
>-        if (this._updater._isValidUpdate(aLocalItem, version, minAppVersion, maxAppVersion)) {
>-          aUpdateData.found = true;
>+        aUpdateData.minVersion = this._getPropertyFromResource(aDataSource, targetApp, "minVersion", aLocalItem);
>+        aUpdateData.maxVersion = this._getPropertyFromResource(aDataSource, targetApp, "maxVersion", aLocalItem);
>+
Please remove the newline.

>           aUpdateData.version = version;
If aUpdateData.appID should be set in this conditional please put it here
Add aUpdateData.minVersion and aUpdateData.maxVersion here instead.

>           aUpdateData.updateLink = this._getPropertyFromResource(aDataSource, targetApp, "updateLink", aLocalItem);
>           aUpdateData.updateHash = this._getPropertyFromResource(aDataSource, targetApp, "updateHash", aLocalItem);
Please fix the indentation

>-          aUpdateData.minVersion = minAppVersion;
>-          aUpdateData.maxVersion = maxAppVersion;
>+
>+        aUpdateData.found = this._updater._isValidUpdate(aLocalItem, aUpdateData);
>+        if (appID == this._updater._appID) {
>+          // App takes precedence over toolkit.  If we found the app, bail out.
>+          return;
>         }
>       }
Previously, we only set the values in aUpdateData when the item was a valid update. Is this change in behavior necessary?

>@@ -6322,61 +6384,80 @@ ExtensionsDataSource.prototype = {
>    *          The datasource to inspect for compatibility - can be the main
>    *          datasource or an Install Manifest.
>    * @param   source
>    *          The RDF Resource of the item to inspect for compatibility.
>    * @param   version
>    *          The version of the application we are checking for compatibility
>    *          against. If this parameter is undefined, the version of the running
>    *          application is used.
>+   * @param   platformVersion
>+   *          The version of the toolkit to check compatibility against
>    * @returns true if the item is compatible with this version of the 
>    *          application, false, otherwise.
>    */
>-  isCompatible: function (datasource, source, version) {
>+  isCompatible: function (datasource, source, version, platformVersion) {
>     // The Default Theme is always compatible. 
>     if (source.EqualsNode(this._defaultTheme))
>       return true;
> 
>     if (version === undefined) {
>       version = gApp.version;
>     }              
>     var appID = gApp.ID;
>+    if (!platformVersion) {
Why not check === undefined?

>+      var platformVersion = gApp.platformVersion;
>+    }
Please change version to appVersion and rearrange this as follows

  var appID = gApp.ID;
  if (appVersion === undefined)
    appVersion = gApp.version;
  if (!platformVersion === undefined)
    var platformVersion = gApp.platformVersion;

as long as undefined works here.

>     
>     var targets = datasource.GetTargets(source, EM_R("targetApplication"), true);
>     var idRes = EM_R("id");
>     var minVersionRes = EM_R("minVersion");
>     var maxVersionRes = EM_R("maxVersion");
>+    var versionChecker = getVersionChecker();
>+    var rv = false;
>     while (targets.hasMoreElements()) {
>       var targetApp = targets.getNext().QueryInterface(Components.interfaces.nsIRDFResource);
>       var id          = stringData(datasource.GetTarget(targetApp, idRes, true));
>       var minVersion  = stringData(datasource.GetTarget(targetApp, minVersionRes, true));
>       var maxVersion  = stringData(datasource.GetTarget(targetApp, maxVersionRes, true));
>       if (id == appID) {
>-        var versionChecker = getVersionChecker();
>-        return ((versionChecker.compare(version, minVersion) >= 0) &&
>-                (versionChecker.compare(version, maxVersion) <= 0));
>+        rv = (versionChecker.compare(version, minVersion) >= 0) &&
>+             (versionChecker.compare(version, maxVersion) <= 0);
>+        return rv; // App takes precedence over toolkit.
>       }
>+
>+      if (id == TOOLKIT_ID) {
>+        rv =  (versionChecker.compare(platformVersion, minVersion) >= 0) &&
>+              (versionChecker.compare(platformVersion, maxVersion) <= 0);
>+        // Keep looping, in case the app id is later.
>     }
Indentaion

>-    return false;
>+    }
>+    return rv;
>   },
> 
>   /**
>    * Gets a list of items that are incompatible with a specific application version.
>    * @param   appID
>    *          The ID of the application - XXXben unused?
>    * @param   appVersion
>    *          The Version of the application to check for incompatibility against.
>+   * @param   platformVersion
>+   *          The version of the toolkit to check compatibility against
>    * @param   desiredType
>    *          The nsIUpdateItem type of items to look for
>    * @param   includeDisabled
>    *          Whether or not disabled items should be included in the set returned
>    * @returns An array of nsIUpdateItems that are incompatible with the application
>    *          ID/Version supplied.
>    */
>-  getIncompatibleItemList: function(appID, appVersion, desiredType, includeDisabled) {
>+  getIncompatibleItemList: function(appID,
>+                                    appVersion,
>+                                    platformVersion,
>+                                    desiredType,
>+                                    includeDisabled) {
Please change to
getIncompatibleItemList: function(appID, appVersion, platformVersion,
                                  desiredType, includeDisabled) {

Could you either remove appID from getIncompatibleItemList and its callers or file a bug to do that?

>@@ -6567,36 +6649,38 @@ ExtensionsDataSource.prototype = {
>   },
>   
>   /**
>    * Sets the target application info for an item in the Extensions
>    * datasource and in the item's install manifest if it is installed in a
>    * profile's extensions directory, it exists, and we have write access.
>    * @param   id
>    *          The ID of the item to update target application info for
>+   * @param   targetAppID
>+   *          The target application for the item (gApp.ID, TOOLKIT_ID)
Please update comment as noted earlier.

>@@ -6612,53 +6696,58 @@ ExtensionsDataSource.prototype = {
>     if (getResourceForID(id).EqualsNode(this._defaultTheme))
>       return null;
> 
>     var appID = gApp.ID;
>     var r = getResourceForID(id);
>     var targetApps = this._inner.GetTargets(r, EM_R("targetApplication"), true);
>     if (!targetApps.hasMoreElements())
>       targetApps = this._inner.GetTargets(gInstallManifestRoot, EM_R("targetApplication"), true); 
>+    var outData = null;
>     while (targetApps.hasMoreElements()) {
>       var targetApp = targetApps.getNext();
>       if (targetApp instanceof Components.interfaces.nsIRDFResource) {
>         try {
>           var foundAppID = stringData(this._inner.GetTarget(targetApp, EM_R("id"), true));
>-          if (foundAppID != appID) // Different target application
>+          // Different target application?
>+          if ((foundAppID != appID) && (foundAppID != TOOLKIT_ID))
Prevailing style / unneeded parens
if (foundAppID != appID && foundAppID != TOOLKIT_ID)

>             continue;
>           var updatedMinVersion = this._inner.GetTarget(targetApp, EM_R("updatedMinVersion"), true);
>           var updatedMaxVersion = this._inner.GetTarget(targetApp, EM_R("updatedMaxVersion"), true);
>           if (updatedMinVersion && updatedMaxVersion)
>-            return { id        : id,
>+            outData = { id        : id,
>                      minVersion: stringData(updatedMinVersion),
>                      maxVersion: stringData(updatedMaxVersion) };
Please fix indentation.

>-          else
>-            return null;
>+          if (foundAppID == appID) {
>+            return outData;
>+          }
Prevailing style
if (foundAppID == appID)
  return outData;

note: not that this is all that would need to be done but if we add a version check here we should be able to support version ranges (bug 301236).

>         }
>         catch (e) { 
>           continue;
>         }
>       }
>     }
>-    return null;
>+    return outData;
>   },
>   
>   /**
>    * Sets the updated target application info for an item in the Extensions
>    * datasource during an installation or upgrade.
>    * @param   id
>    *          The ID of the item to set updated target application info for
>+   * @param   targetAppID
>+   *          The target application for the item (gApp.ID, TOOLKIT_ID)
Please update the comment as noted previously.

>@@ -6694,83 +6784,94 @@ ExtensionsDataSource.prototype = {
> 
>   /**
>    * Gets the target application info for an item from a datasource.
>    * @param   id
>    *          The GUID of the item to discover target application info for
>    * @param   datasource
>    *          The datasource to look up target application info in
>    * @returns A JS Object with the following properties:
>+   *          "appID"         The target application ID.
Please update comment in a similar manner as noted previously.

>    *          "minVersion"    The minimum version of the target application
>    *                          that this item can run in
>    *          "maxVersion"    The maximum version of the target application
>    *                          that this item can run in
>    *          or null, if no target application data exists for the specified
>    *          id in the supplied datasource.
>    */
>   getTargetApplicationInfo: function(id, datasource) {
>     // The default theme is always compatible. 
>+    var appID = gApp.ID;
Please put var appID = gApp.ID; above the comment

>     if (getResourceForID(id).EqualsNode(this._defaultTheme)) {
>       var ver = gApp.version;
>-      return { minVersion: ver, maxVersion: ver };
>+      return { appID: appID, minVersion: ver, maxVersion: ver };
>     }
>-    var appID = gApp.ID;
>+
>     var r = getResourceForID(id);
>     var targetApps = datasource.GetTargets(r, EM_R("targetApplication"), true);
>     if (!targetApps)
>       return null;
>+
>     if (!targetApps.hasMoreElements())
>       targetApps = datasource.GetTargets(gInstallManifestRoot, EM_R("targetApplication"), true); 
>+    var outData = null;
>     while (targetApps.hasMoreElements()) {
>       var targetApp = targetApps.getNext();
>       if (targetApp instanceof Components.interfaces.nsIRDFResource) {
>         try {
>           var foundAppID = stringData(datasource.GetTarget(targetApp, EM_R("id"), true));
>-          if (foundAppID != appID) // Different target application
>+          // Different target application?
>+          if ((foundAppID != appID) && (foundAppID != TOOLKIT_ID))
Prevailing style / unneeded parens
if (foundAppID != appID && foundAppID != TOOLKIT_ID)

>             continue;
>           
>-          return { minVersion: stringData(datasource.GetTarget(targetApp, EM_R("minVersion"), true)),
>+          outData = { appID: foundAppID,
>+                      minVersion: stringData(datasource.GetTarget(targetApp, EM_R("minVersion"), true)),
>                    maxVersion: stringData(datasource.GetTarget(targetApp, EM_R("maxVersion"), true)) };
>+          if (foundAppID == appID)
>+            return outData;
>         }
>         catch (e) { 
>           continue;
>         }
>       }
>     }
>-    return null;
>+    return outData;
>   },
So, this allows specifying a toolkit ID and an app ID in targetApplication. My first thought was to only allow one or the other but I think this is a better approach.

same note about version ranges and bug 301236.

>   /**
>    * Sets the target application info for an item in a datasource.
>    * @param   id
>    *          The GUID of the item to discover target application info for
>+   * @param   targetAppID
>+   *          The target application this item is for (gApp.id, TOOLKIT_ID)
Same as other comment change request
The target application ID used for checking compatibility for this item. Add-ons can specify a targetApplication id of [e-mail] in their install manifest for compatibility with all apps that use a specific release of the toolkit.

>@@ -7543,17 +7644,25 @@ ExtensionsDataSource.prototype = {
>     if (!targetAppInfo) {
>       // When installing a new addon targetAppInfo does not exist yet
>       if (this.getItemProperty(id, "opType") == OP_NEEDS_INSTALL)
>         return EM_L("true");
>       return EM_L("false");
>     }
>     
>     getVersionChecker();
>-    var appVersion = gApp.version;
>+    var appVersion;
>+    switch (targetAppInfo.appID) {
>+      case gApp.ID:
>+        appVersion = gApp.version;
>+        break;
>+      case TOOLKIT_ID:
>+        appVersion = gApp.platformVersion;
>+    }
Please use
var appVersion = targetAppInfo.appID == TOOLKIT_ID ? gApp.platformVersion : gApp.version;

>Index: toolkit/mozapps/update/src/nsUpdateService.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in,v
>retrieving revision 1.140
>diff -p -u -8 -w -r1.140 nsUpdateService.js.in
>--- toolkit/mozapps/update/src/nsUpdateService.js.in	24 Jul 2007 22:31:33 -0000	1.140
>+++ toolkit/mozapps/update/src/nsUpdateService.js.in	25 Aug 2007 23:27:54 -0000
>...
> const WRITE_ERROR = 7;
> 
> const DOWNLOAD_CHUNK_SIZE           = 300000; // bytes
> const DOWNLOAD_BACKGROUND_INTERVAL  = 600;    // seconds
> const DOWNLOAD_FOREGROUND_INTERVAL  = 0;
> 
> const POST_UPDATE_CONTRACTID = "@mozilla.org/updates/post-update;1";
> 
>+const TOOLKIT_ID              = "[e-mail]";
>+
Put this above POST_UPDATE_CONTRACTID

> const nsIExtensionManager     = Components.interfaces.nsIExtensionManager;
> const nsILocalFile            = Components.interfaces.nsILocalFile;
> const nsIUpdateService        = Components.interfaces.nsIUpdateService;
> const nsIUpdateItem           = Components.interfaces.nsIUpdateItem;
> const nsIPrefLocalizedString  = Components.interfaces.nsIPrefLocalizedString;
> const nsIIncrementalDownload  = Components.interfaces.nsIIncrementalDownload;
> const nsIFileInputStream      = Components.interfaces.nsIFileInputStream;
> const nsIFileOutputStream     = Components.interfaces.nsIFileOutputStream;

>@@ -2936,18 +2941,22 @@ function NSGetModule(compMgr, fileSpec) 
>  * @returns true if there are no addons installed that are incompatible with
>  *          the specified update, false otherwise.
>  */
> function isCompatible(update) {
> #ifdef MOZ_XUL_APP
>   var em = 
>       Components.classes["@mozilla.org/extensions/manager;1"].
>       getService(nsIExtensionManager);
>-  var items = em.getIncompatibleItemList("", update.extensionVersion,
>-    nsIUpdateItem.TYPE_ADDON, false, { });
>+  var items = em.getIncompatibleItemList("",
>+                                         update.extensionVersion,
>+                                         update.platformVersion,
>+                                         nsIUpdateItem.TYPE_ADDON,
>+                                         false,
>+                                         { });
nit: prevailing style is only to add newlines when the line exceeds 80.

>@@ -3028,43 +3037,57 @@ function showPromptIfNoIncompatibilities
>                          nsIExtensionManager.UPDATE_CHECK_NEWVERSION);
>       if ((mode == nsIExtensionManager.UPDATE_CHECK_NEWVERSION
>            && this._addons.length) || !isCompatible(update))
>         showPrompt(update);
>     },
>     onAddonUpdateStarted: function(addon) {
>     },
>     onAddonUpdateEnded: function(addon, status) {
>-      if (status == Components.interfaces.nsIAddonUpdateCheckListener.STATUS_UPDATE &&
>-          addonIsCompatible(addon, update.extensionVersion)) {
>+      const Ci = Components.interfaces;
>+      if (status != Ci.nsIAddonUpdateCheckListener.STATUS_UPDATE) {
>+        return;
>+      }
Prevailing style
if (status != Ci.nsIAddonUpdateCheckListener.STATUS_UPDATE)
  return;

>+
>+      var reqVersion = addon.targetAppID == TOOLKIT_ID ?
>+                       update.platformVersion : update.extensionVersion;
>+      if (!addonIsCompatible(addon, reqVersion)) {
>+        return;
>+      }
Prevailing style
if (!addonIsCompatible(addon, reqVersion))
  return;

>+
>         for (var i = 0; i < this._addons.length; ++i) {
>           if (this._addons[i] == addon) {
>             this._addons.splice(i, 1);
>             break;
>           }
>         }
>-      }
>     },
>     /**
>      * See nsISupports.idl
>      */
>     QueryInterface: function(iid) {
>       if (!iid.equals(Components.interfaces.nsIAddonUpdateCheckListener) &&
>           !iid.equals(Components.interfaces.nsISupports))
>         throw Components.results.NS_ERROR_NO_INTERFACE;
>       return this;
>     }
>   };
>   
>   if (!isCompatible(update)) {
>     var em = 
>         Components.classes["@mozilla.org/extensions/manager;1"].
>         getService(nsIExtensionManager);
>-    var listener = new Listener(em.getIncompatibleItemList("", 
>-      update.extensionVersion, nsIUpdateItem.TYPE_ADDON, false, { }));
>+    var items = em.getIncompatibleItemList("",
>+                                           update.extensionVersion,
>+                                           update.platformVersion,
>+                                           nsIUpdateItem.TYPE_ADDON,
>+                                           false,
>+                                           { })
nit: prevailing style is only to add newlines when the line exceeds 80.

I'll get to the tests while you are fixing these nits. Thanks for hanging in there... I can tell that patching this is a PITA.

Comment 85

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

btw: I'll hold off on allowing other EM patches to land for a couple of days so this won't bitrot on you again.

Comment 86

[Alex Vincent [:WeirdAl]]

Okay.  Note that my available time to work on this bug is restricted to evenings and weekends.

Comment 87

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

Comment on attachment 278251 [details] [diff] [review]
patch, v1.8.1

btw: I'm fine with the tests.

Comment 88

[Alex Vincent [:WeirdAl]]

Robert, on a couple of these, you noted indentation glitches.  The patches I've submitted are actually -w, and in my local file, they were correct.  (However, my editor is a bit zealous about making sure line endings have no trailing spaces.  As a result, if I hadn't, the diff would be much larger.)

What do you suggest I should do?

Comment 89

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

I usually submit both patches so if there is a question the full patch can be looked at for reference.

Comment 90

[Alex Vincent [:WeirdAl]]

(In reply to comment #84)
> Why are you using gApp.name here instead of BundleManager.appName as was used
> previously? If nothing else using BundleManager.appName would be consistent
> with targetAppName being set with the value for BundleManager.toolkitName. If
> there isn't a good reason I'd like it changed back.

do_check_true(aText.indexOf(ADDONS[i].failedAppName) != -1);

ADDONS[i].failedAppName: XPCShell
aText: Bug 299716 test E 0.1 could not be installed because it is not compatible with Minefield 5. (Bug 299716 test E 0.1 will only work with Minefield 30)
*** exiting
*** CHECK FAILED: false == true
JS frame :: c:/mozilla.org/trunk/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_throw :: line 99
JS frame :: c:/mozilla.org/trunk/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_check_eq :: line 114
JS frame :: c:/mozilla.org/trunk/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_check_true :: line 118
JS frame :: ../../../../_tests/xpcshell-simple/test_extensionmanager/unit/test_bug299716.js :: alert :: line 70
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
JS frame :: file:///c:/mozilla.org/trunk/fx-debug/dist/bin/components/nsExtensionManager.js :: showIncompatibleError :: line 784
JS frame :: file:///c:/mozilla.org/trunk/fx-debug/dist/bin/components/nsExtensionManager.js :: anonymous :: line 4291
JS frame :: file:///c:/mozilla.org/trunk/fx-debug/dist/bin/components/nsExtensionManager.js :: anonymous :: line 5725
JS frame :: file:///c:/mozilla.org/trunk/fx-debug/dist/bin/components/nsExtensionManager.js :: anonymous :: line 5861
JS frame :: file:///c:/mozilla.org/trunk/fx-debug/dist/bin/components/nsExtensionManager.js :: anonymous :: line 6202
JS frame :: file:///c:/mozilla.org/trunk/fx-debug/dist/bin/components/nsExtensionManager.js :: anonymous :: line 6066
JS frame :: file:///c:/mozilla.org/trunk/fx-debug/dist/bin/components/nsExtensionManager.js :: anonymous :: line 6023
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

ADDONS[i].failedAppName: XPCShell
aText: Bug 299716 test F 0.1 could not be installed because it is not compatible with Minefield 5. (Bug 299716 test F 0.1 will only work with Minefield 30)
*** exiting
*** CHECK FAILED: false == true

This particular error I actually half-expected, because the test app (xpcshell) isn't declaring its own app name locale.  Although there's also a valid point to consider in that branding belongs to Minefield here - and I would think branding chrome might not be available.

In this case, I see only two solutions - one, override the branding somehow (which given this testcase seems really extreme) or drop the test line in question.

I'm cleaning up a couple other regressions now.

Comment 91

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.9 (-w)

full patch coming right up, with unexpected changes explained

Comment 92

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.9 (reference)

Additional changes:
* nsIExtensionManager.update() has to throw NS_ERROR_ILLEGAL_VALUE for a null item.
* Ditto for nsIAddonUpdateCheckListener.onAddonUpdateStarted(), nsIAddonUpdateCheckListener.onAddonUpdateEnded().
* I left a couple of these in the onAddonUpdateStarted, onAddonUpdateEnded methods - sorry:

dump((new Error()).stack + "\n\n");

* Comment 90, I removed the offending test lines.
* Added NS_ERROR_ILLEGAL_VALUE checks to run_test_pt2().
* Adjusted run_test_pt3() to add all items to the first pass for addDownloads - to ensure we still throw NS_ERROR_ILLEGAL_VALUE there.

Finally, I should note the following pieces in my test log, which worry me.  They were right next to each other:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///c:/mozilla.org/trunk/fx-debug/dist/bin/components/nsBlocklistService.js :: anonymous :: line 276"  data: no]
************************************************************

###!!! ASSERTION: Oops!  You're asking for a weak reference to an object that doesn't support that.: 'factoryPtr', file c:/mozilla.org/trunk/fx-debug/xpcom/build/nsWeakReference.cpp, line 109

The line in question for the first was:

gOS.removeObserver(this, "xpcom-shutdown");

Comment 93

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

(In reply to comment #92)
>...
> * I left a couple of these in the onAddonUpdateStarted, onAddonUpdateEnded
> methods - sorry:
> 
> dump((new Error()).stack + "\n\n");
please clean it up before checkin.

> Finally, I should note the following pieces in my test log, which worry me. 
> They were right next to each other:
> 
> ************************************************************
> * Call to xpconnect wrapped JSObject produced this error:  *
> [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE)
> [nsIObserverService.removeObserver]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"
>  location: "JS frame ::
> file:///c:/mozilla.org/trunk/fx-debug/dist/bin/components/nsBlocklistService.js
> :: anonymous :: line 276"  data: no]
> ************************************************************
> 
> ###!!! ASSERTION: Oops!  You're asking for a weak reference to an object that
> doesn't support that.: 'factoryPtr', file
> c:/mozilla.org/trunk/fx-debug/xpcom/build/nsWeakReference.cpp, line 109
> 
> The line in question for the first was:
> 
> gOS.removeObserver(this, "xpcom-shutdown");
That line number is off by one... the actual line number is 277
gOS.removeObserver(this, "profile-after-change");

It fails because addObserver is not called for profile-after-change and plugins-list-updated.

Comment 94

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

Comment on attachment 278540 [details] [diff] [review]
patch, v1.9 (-w)

I'll fix the assertion after this lands. Don't forget to remove the dumps, etc.

Thanks for taking this on and hanging in there!

Comment 95

[Alex Vincent [:WeirdAl]]

Robert, I don't have check-in privileges; two quick questions.

(1) The -w patch or the reference patch,
(2) Do you want a new patch without the dump statements, or can I trust whoever checks it in to take care of that?

Comment 96

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'll volunteer to land this for you later today if you post a fixed (final, ready for checkin with no changes required) patch.

Comment 97

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

No worries, I'll check it in. I'm going to check in the full patch... I don't like superfluous whitespace in js files either. ;)

Comment 98

[Alex Vincent [:WeirdAl]]

I'll leave it in your hands, Rob; I'll be on #developers to watch the tree if you want to join me.

Comment 99

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

A couple of things came up so I'm going to land the patch this evening.

Comment 100

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

Attachment: final patch for checkin

I'll check this in this evening

Comment 101

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

Filed bug 394128 for the AUS changes needed.

Comment 102

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

Checked in to trunk. Resolved -> fixed. If there is any fallout from this please file a new bug and make it block this bug.

Comment 103

[(not reading, please use [e-mail] instead)]

robert / alex:  I see you've added support for %PLATFORM_VERSION%, but the app.update.url pref doesn't specify it yet.

if there are plans to add %PLATFORM_VERSION% that to pref, we'll want to bump the version to 3, and then let the AUS guys know about that (in bug #394128, I think).