Closed Bug 299716 (toolkit@mozilla.org) Opened 19 years ago Closed 17 years ago

Need for em:targetApplication marker for "the toolkit"

Categories

(Toolkit :: Add-ons Manager, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: allan, Assigned: WeirdAl)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 16 obsolete files)

124.78 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
544.44 KB, patch
Details | Diff | Splinter Review
536.96 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
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).
No longer blocks: 298431
To check for compatibility we would also need a toolkit version as well. 
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.
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?
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.
Blocks: 298431
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 "toolkit@components.mozilla.org". 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.
> 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 "toolkit@components.mozilla.org" 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).
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.
Depends on: 299930
Target Milestone: --- → mozilla1.9alpha
Note to self: need to add toolkitVersion to software update.
No longer blocks: 298431
Blocks: 312970
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.
Can't you reuse the same notification and substitute in "the Mozilla platform" or something like that?
True but I highly suspect that the definition of "the Mozilla Platform" is not known to the vast majority of users.
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.
(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.
(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
Component: Installer: XPInstall Engine → Extension/Theme Manager
Product: Core → Firefox
Target Milestone: mozilla1.9alpha → Firefox 3
Assignee: robert.bugzilla → nobody
QA Contact: extension.manager
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.
Oop, sorry Robert, all I wanted to do was CC myself.  I apologize for the gaffe, and thanks for telling me about it.
QA Contact: extension.manager → robert.bugzilla
Assignee: nobody → robert.bugzilla
QA Contact: robert.bugzilla → extension.manager
Blocks: 342592
requesting blocking-firefox3; this is seriously hurting Inspector and Venkman for XULRunner.
Blocks: 342590
Flags: blocking-firefox3?
Attached file sample install.rdf file, version 1 (obsolete) —
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.
Attached file sample install.rdf file, version 1.1 (obsolete) —
like, getting the min- and max-version numbers right :)
Attachment #248454 - Attachment is obsolete: true
(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
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)?
> 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.
Attached patch patch, v1 (obsolete) — Splinter Review
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.
Assignee: robert.bugzilla → ajvincent
Status: NEW → ASSIGNED
Attachment #248599 - Flags: review?(robert.bugzilla)
(In reply to comment #23)
> -  getItemProperty: function(id, property) { 
> +  getItemProperty: function getItemProperty(id, property) {

You removed a trailing space, fine, but why |function getItemProperty|?
(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.
Because named functions are better for debugging.
Note to self: read all bugmail for a bug before responding! (sorry for the spam).
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 on attachment 248599 [details] [diff] [review]
patch, v1

meh, updates is broken
Attachment #248599 - Attachment is obsolete: true
Attachment #248599 - Flags: review?(robert.bugzilla)
Attached patch patch, v1.1 (obsolete) — Splinter Review
update bustage fixed.
Attachment #248677 - Flags: review?(robert.bugzilla)
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.
Blocks: 363877
Robert, can you point me to the code that handles that, and preferably a good testcase for it?
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
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.
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.
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.
Attached patch patch, v1.2 (obsolete) — Splinter Review
This patch adds a new Checker() for toolkit apps, and adjusts the update listener to handle both of them at the same time.
Attachment #248677 - Attachment is obsolete: true
Attachment #248755 - Flags: review?(robert.bugzilla)
Attachment #248677 - Flags: review?(robert.bugzilla)
Comment on attachment 248755 [details] [diff] [review]
patch, v1.2

dammit, something else is broken.
Attachment #248755 - Flags: review?(robert.bugzilla)
Attached patch patch, v1.3 (obsolete) — Splinter Review
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.
Attachment #248802 - Flags: review?(robert.bugzilla)
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?
Attachment #248755 - Attachment is obsolete: true
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]
Flags: blocking-firefox3?
Whiteboard: [wanted-1.9]
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.  
Flags: blocking-firefox3?
mconnor, bsmedberg, Toolkit review page says both of you are reviewers for the
extension manager; can you please lend a hand here???
Flags: blocking-firefox3?
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>)
Blocks: 271812
Flags: blocking-firefox3?
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.
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-1.9] → [wanted-firefox3]
Attachment #248802 - Flags: review?(robert.bugzilla)
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*
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.
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?
The application setting should override the platform setting in all cases.
Attached patch patch, v1.4 (obsolete) — Splinter Review
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.
Attachment #271806 - Flags: review?(robert.bugzilla)
Attachment #248802 - Attachment is obsolete: true
As Mossop wrote the test harness, he might be a good person to review the tests I've written here.  :)
Attachment #248455 - Attachment is obsolete: true
Attachment #248804 - Attachment is obsolete: true
Attachment #271809 - Flags: review?(dtownsend)
Attached file A long list of assertion stacks (obsolete) —
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 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 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.
Attachment #271806 - Flags: review?(robert.bugzilla) → review-
(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?

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.
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.
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.
Alex, since we already use toolkit@mozilla.org for blocklisting and I got buyin to use that name way back I'd like the toolkit id to also be toolkit@mozilla.org. I've already ran this by shaver and bsmedberg and they are both fine with it.
Alias: toolkit@mozilla.org
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?
Attachment #271806 - Attachment is obsolete: true
Attachment #271809 - Attachment is obsolete: true
Attachment #271839 - Attachment is obsolete: true
Attachment #276412 - Flags: review?(dtownsend)
Attachment #271809 - Flags: review?(dtownsend)
Don't worry about the string / translation issues. I'll discuss this with beltzner next week.
Flags: in-testsuite?
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?
Attachment #276412 - Flags: review?(dtownsend) → review-
Attached patch Additional testcase (obsolete) — Splinter Review
Testcase mentioned in previous comment
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.  ;-)
Attached patch patch, v1.6 (-w) (obsolete) — Splinter Review
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.
Attachment #276412 - Attachment is obsolete: true
Attachment #276504 - Attachment is obsolete: true
Attachment #276593 - Flags: review?(dtownsend)
Attachment #276593 - Attachment description: patch, v1.6 → patch, v1.6 (-w)
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 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.
Attachment #276593 - Flags: review?(dtownsend) → review-
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.
http://lxr.mozilla.org/seamonkey/search?string=getIncompatibleItemList

This is a reminder to myself that not only update mgr calls getIncompatibleItemList.
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.
Attachment #276593 - Attachment is obsolete: true
Attachment #277677 - Flags: review?(dtownsend)
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.
Attachment #277677 - Flags: review?(dtownsend) → review-
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.
Attached patch patch, v1.8 (obsolete) — Splinter Review
Attachment #278009 - Attachment is patch: true
Attachment #278009 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #278009 - Flags: review?(dtownsend)
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 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!
Attachment #278009 - Flags: review?(dtownsend) → review-
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.
Attachment #278009 - Flags: review?(robert.bugzilla)
Attachment #278009 - Flags: review-
Attachment #278009 - Flags: review+
Blocks: 393650
Attached patch patch, v1.8.1 (obsolete) — Splinter Review
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+.
Attachment #277677 - Attachment is obsolete: true
Attachment #278009 - Attachment is obsolete: true
Attachment #278251 - Flags: review?(robert.bugzilla)
Attachment #278009 - Flags: review?(robert.bugzilla)
Attachment #278251 - Flags: review+
No longer blocks: 342590
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 toolkit@mozilla.org 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 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.
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.
Al, I've now landed bug 391899 so you'll need to make the minor change to your testcase I mentioned in comment 62.
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 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 toolkit@mozilla.org 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              = "toolkit@mozilla.org";
>+
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.
Attachment #278251 - Flags: review?(robert.bugzilla) → review-
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.
Okay.  Note that my available time to work on this bug is restricted to evenings and weekends.
Comment on attachment 278251 [details] [diff] [review]
patch, v1.8.1

btw: I'm fine with the tests.
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?
I usually submit both patches so if there is a question the full patch can be looked at for reference.
Blocks: 393951
(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.
Attached patch patch, v1.9 (-w)Splinter Review
full patch coming right up, with unexpected changes explained
Attachment #278540 - Flags: review?(robert.bugzilla)
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");
Attachment #278251 - Attachment is obsolete: true
(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 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!
Attachment #278540 - Flags: review?(robert.bugzilla) → review+
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?
I'll volunteer to land this for you later today if you post a fixed (final, ready for checkin with no changes required) patch.
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. ;)
I'll leave it in your hands, Rob; I'll be on #developers to watch the tree if you want to join me.
A couple of things came up so I'm going to land the patch this evening.
I'll check this in this evening
Attachment #278686 - Flags: review+
Filed bug 394128 for the AUS changes needed.
Checked in to trunk. Resolved -> fixed. If there is any fallout from this please file a new bug and make it block this bug.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Blocks: 394221
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).
Blocks: 394281
Depends on: 394300
Depends on: 394717
Depends on: 395430
Flags: in-testsuite? → in-testsuite+
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
Product: Firefox → Toolkit
Depends on: 454842
Blocks: 468835
You need to log in before you can comment on or make changes to this bug.