Closed
Bug 299716
(toolkit@mozilla.org)
Opened 19 years ago
Closed 16 years ago
Need for em:targetApplication marker for "the toolkit"
Categories
(Toolkit :: Add-ons Manager, enhancement)
Toolkit
Add-ons Manager
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).
![]() |
||
Comment 1•19 years ago
|
||
To check for compatibility we would also need a toolkit version as well.
![]() |
||
Comment 2•19 years ago
|
||
For app compatibility the pref value from app.extensions.version is used... for this perhaps a new pref like app.extensions.toolkit.version? If an extension doesn't specify a targetApplication (e.g. GUID) that matches the current targetAppliction then we would look for the toolkit targetAppliication in the install.rdf and use the value specified in app.extensions.toolkit.version to check against its minVersion and maxVersion.
Comment 3•19 years ago
|
||
I'd say that an unknown targetApplication should cause the extension to be disabled. However, if no targetApplication is specified, then perhaps it could be assumed that the extension is targeting any toolkit app. How about we use targetToolkit for this then? On the other hand, it is possible for an extension to specify multiple targetApplication arcs, and would you really want an extension to be used in application without explicit testing against said application?
![]() |
||
Comment 4•19 years ago
|
||
I think that is simpler. If there are no targetApplication arcs then check for a targetToolkit arc. If that isn't compatible when version checked against the pref value or it doesn't exist then the extension is not compatible, etc. It would probably be easiest to add a + to toolkit.extensions.version at the same time app.extensions.version gets a +. This could also be opt in by not checking for targetToolkit unless the pref is set for the app. As for whether or not an extension should be used with an app it hasn't been tested with I suspect there will be problems. For example, DOMi has issues with Thunderbird and if I am not mistaken this is due to an overlay being provided by Thunderbird that doesn't have a dtd that is required. I still think this is appropriate to implement even with the potential problems.
Comment 5•19 years ago
|
||
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.
Comment 6•19 years ago
|
||
> 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).
![]() |
||
Comment 7•19 years ago
|
||
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.
![]() |
||
Updated•18 years ago
|
Target Milestone: --- → mozilla1.9alpha
![]() |
||
Comment 8•18 years ago
|
||
Note to self: need to add toolkitVersion to software update.
![]() |
||
Comment 9•18 years ago
|
||
bah... we have to support notifying the user that an extension is not compatible with the toolkit as we do when an extension is not compatible with the application.
Comment 10•18 years ago
|
||
Can't you reuse the same notification and substitute in "the Mozilla platform" or something like that?
![]() |
||
Comment 11•18 years ago
|
||
True but I highly suspect that the definition of "the Mozilla Platform" is not known to the vast majority of users.
Comment 12•18 years ago
|
||
True, but they aren't going to know what "1.8" or "1.8.1" or "1.9" mean anyway, so I'm really not sure it matters. Perhaps beltzner can come up with a more elegant solution.
Comment 13•18 years ago
|
||
(In reply to comment #9) > bah... we have to support notifying the user that an extension is not > compatible with the toolkit as we do when an extension is not compatible with > the application. Couldn't we use the same notification that we do at the app level? The fact that Firefox2 (or Songbird 1.0) is built on Gecko1.8.1 won't matter to a user. What will matter is that they can't use DOMInspector2.0 with their application. The only tricky part is if we're committed to telling them what version of the application/toolkit they should be trying to get, and I'm not convinced (especially in the case of toolkit version incompatibilities) that this information is useful/helpful.
![]() |
||
Comment 14•18 years ago
|
||
(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
![]() |
||
Updated•18 years ago
|
Component: Installer: XPInstall Engine → Extension/Theme Manager
Product: Core → Firefox
Target Milestone: mozilla1.9alpha → Firefox 3
![]() |
||
Comment 15•18 years ago
|
||
Mook, I am guessing that since you unassigned this from me that you are thinking about taking this on. I've already added support in blocklisting for this but there are several other areas that need to be changed to support this... especially since the current interfaces only support passing one id, minVersion, maxVersion and this will require a minimum of two. At the same time it would be appropriate to also fix bug 301236 since it will also require changes to some of these same interfaces.
Comment 16•18 years ago
|
||
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
Assignee | ||
Comment 17•17 years ago
|
||
requesting blocking-firefox3; this is seriously hurting Inspector and Venkman for XULRunner.
Blocks: 342590
Flags: blocking-firefox3?
Assignee | ||
Comment 18•17 years ago
|
||
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.
Assignee | ||
Comment 19•17 years ago
|
||
like, getting the min- and max-version numbers right :)
Attachment #248454 -
Attachment is obsolete: true
![]() |
||
Comment 20•17 years ago
|
||
(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
Assignee | ||
Comment 21•17 years ago
|
||
So by adjusting ExtensionsDataSource.prototype.isCompatible, I was able to force the EM to install the extension... but after the restart, the extension is disabled again. After several hours, I can't locate the code that determines the isDisabled property for the rich list items (i.e. what sets it in the composite rdf). Help! On a related note, per comment 0, should apps targeted at toolkit should be installed in XULRunner's dist/bin/extensions (as opposed to the application's extensions directory)?
Comment 22•17 years ago
|
||
> 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.
Assignee | ||
Comment 23•17 years ago
|
||
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)
Comment 24•17 years ago
|
||
(In reply to comment #23) > - getItemProperty: function(id, property) { > + getItemProperty: function getItemProperty(id, property) { You removed a trailing space, fine, but why |function getItemProperty|?
Assignee | ||
Comment 25•17 years ago
|
||
(In reply to comment #24) > You removed a trailing space, fine, but why |function getItemProperty|? Debugging anonymous functions (as opposed to named functions) is painful, more than the cost of the few bytes needed. Think error stack traces. Also, although it doesn't apply yet in this case, Venkman likes named functions too. It's one of those little JavaScript nuances that one learns the hard way how important it is.
Comment 26•17 years ago
|
||
Because named functions are better for debugging.
Comment 27•17 years ago
|
||
Note to self: read all bugmail for a bug before responding! (sorry for the spam).
Comment 28•17 years ago
|
||
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.
Assignee | ||
Comment 29•17 years ago
|
||
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)
Assignee | ||
Comment 30•17 years ago
|
||
update bustage fixed.
Attachment #248677 -
Flags: review?(robert.bugzilla)
![]() |
||
Comment 31•17 years ago
|
||
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.
Assignee | ||
Comment 32•17 years ago
|
||
Robert, can you point me to the code that handles that, and preferably a good testcase for it?
![]() |
||
Comment 33•17 years ago
|
||
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
Assignee | ||
Comment 34•17 years ago
|
||
I think that case is actually covered by this patch, just from a cursory look at the code. updates.js#708 gets the extension manager service. Line 710 calls on nsIExtensionManager::getIncompatibleItemList(), which lives at nsExtensionManager.js.in#5391. This in turn calls the datasource's getIncompatibleItemList() method (nsExtensionManager.js.in#6654), which calls on isCompatible() (nsExtensionManager.js.in#6557). My patch upgrades isCompatible to handle the toolkit target case.
![]() |
||
Comment 35•17 years ago
|
||
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.
Assignee | ||
Comment 36•17 years ago
|
||
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.
Assignee | ||
Comment 37•17 years ago
|
||
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)
Assignee | ||
Comment 38•17 years ago
|
||
Comment on attachment 248755 [details] [diff] [review] patch, v1.2 dammit, something else is broken.
Attachment #248755 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 39•17 years ago
|
||
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)
Assignee | ||
Comment 40•17 years ago
|
||
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?
Assignee | ||
Updated•17 years ago
|
Attachment #248755 -
Attachment is obsolete: true
Comment 41•17 years ago
|
||
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]
Assignee | ||
Comment 42•17 years ago
|
||
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.
Updated•17 years ago
|
Flags: blocking-firefox3?
Assignee | ||
Comment 43•17 years ago
|
||
mconnor, bsmedberg, Toolkit review page says both of you are reviewers for the extension manager; can you please lend a hand here???
Updated•17 years ago
|
Flags: blocking-firefox3?
Comment 44•17 years ago
|
||
I haven't really looked at this in any depth or tested in any way, but it looks like there might be an issue if the install.rdf specifies compatibility for both toolkit, and the particular app. I think the info that takes precedence might be the last one in the file. I'm not sure if that's the best behaviour. I would think that taking the app compatability info in preference to toolkit would be more sensible. Also if toolkit info is used to make the judgement, and the addon still turns out to be incompatible then you are going to get a confusing dialog: <addon> could not be installed because it is not compatible with <current app> <current app version>. (<addon> will only work with <current app> <target toolkit version>)
Comment 45•17 years ago
|
||
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]
Assignee | ||
Updated•17 years ago
|
Attachment #248802 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 46•17 years ago
|
||
As annoyed as I was by mconnor stating this wasn't worth reviewing before I answered Mossop's comments (considering how long it was in the review queue before said comments), it turns out Mossop is exactly right. *sigh*
Comment 47•17 years ago
|
||
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.
Assignee | ||
Comment 48•17 years ago
|
||
Spec question: suppose an extension advertises support for Firefox and Toolkit, but the version test for Firefox fails and the version test for Toolkit passes. Should the extension manager install the extension as enabled or as disabled? Similarly, suppose the Firefox version test passes for an extension, but the Toolkit one fails. Enable the extension or disable it?
Comment 49•17 years ago
|
||
The application setting should override the platform setting in all cases.
Assignee | ||
Comment 50•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Attachment #248802 -
Attachment is obsolete: true
Assignee | ||
Comment 51•16 years ago
|
||
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)
Assignee | ||
Comment 52•16 years ago
|
||
I had to wallpaper over assertions in my test files - something I'm not happy about at all. Basically, there's a bunch which this test reveals, and I don't want to block the patch for them (considering both the test and the tested code are JS-based). I was planning on filing separate bugs blocking this one for the assertions if the patches herein passed review. Hopefully we could get them knocked out for 1.9, and remove that ugly nsIEnvironment hack so that assertions would cause real test failures. <bsmedberg> the not thread-safe assertion is a known bug <bsmedberg> WeirdAl: the "bogus trigger" assertions sounds like something you should investigate I'd like to get comments about the rest of these - where responsibility lies.
![]() |
||
Comment 53•16 years ago
|
||
Comment on attachment 271806 [details] [diff] [review] patch, v1.4 Alex, should be able to get to this by no this weekend... maybe sooner. Sorry about the delay.
Comment 54•16 years ago
|
||
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-
Assignee | ||
Comment 55•16 years ago
|
||
(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?
Assignee | ||
Comment 56•16 years ago
|
||
Pike, please respond to this: (In reply to comment #54) > > if (!targetAppData) { > >- params = [installData.name, installData.version, BundleManager.appName]; > >+ params = [installData.name, installData.version, targetAppName]; > > message = extensionStrings.formatStringFromName("incompatibleMessageNoApp", > > params, params.length); > > You've dropped the localisation of the app name here, any particular reason? > Regardless I think this localisation of this function needs a little more > though. Maybe check in with Pike or someone else appropriate (if you already > have then fine just ignore this bit), but I think rather than having a > localised form of "Gecko Toolkit" we probably just want another > incompatibleMsgSingleAppVersion and incompatibleMsg for the gecko case. This > gives the localisers more flexibility I think.
Comment 57•16 years ago
|
||
From an l10n point of view, the complete section http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties&rev=1.35&mark=53-70#50 is pretty much a nightmare. Not that we have the technical infrastructure to solve it better, but we shouldn't spend too much effort on making this suck less for Gecko than it already does for Firefox and Thunderbird, and for Extensions vs. Themes. All of those can have different genders or vocal harmonies already, so localizers have likely already worked around this. You may want to poke the l10n newsgroup once you have a concrete proposal for localizing Gecko. I'm neither sure what it should be really called (I've seen Gecko, Toolkit, Gecko Toolkit here so far), nor where it should be put. I'm not aware of any existing strings that hold that. Another point that should be resolved is to what the translation should actually be. If we'd mention Gecko, should it be translated, or rather transcribed? Sounds like a question for legal, maybe.
![]() |
||
Comment 58•16 years ago
|
||
I'm not convinced that we should be that specific in the UI and we can always log more specific info to the error console. cc'ing beltzner for his take on what the text should be.
![]() |
||
Comment 59•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Alias: toolkit@mozilla.org
Assignee | ||
Comment 60•16 years ago
|
||
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)
![]() |
||
Comment 61•16 years ago
|
||
Don't worry about the string / translation issues. I'll discuss this with beltzner next week.
Assignee | ||
Updated•16 years ago
|
Flags: in-testsuite?
Comment 62•16 years ago
|
||
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-
Comment 63•16 years ago
|
||
Testcase mentioned in previous comment
Assignee | ||
Comment 64•16 years ago
|
||
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. ;-)
Assignee | ||
Comment 65•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Attachment #276593 -
Attachment description: patch, v1.6 → patch, v1.6 (-w)
Assignee | ||
Comment 66•16 years ago
|
||
One minor thing I missed: http://lxr.mozilla.org/seamonkey/search?string=BUILD_TARGET The various apps need to be updated for the new value - and the update service too, probably.
Comment 67•16 years ago
|
||
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-
Comment 68•16 years ago
|
||
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.
Assignee | ||
Comment 69•16 years ago
|
||
http://lxr.mozilla.org/seamonkey/search?string=getIncompatibleItemList This is a reminder to myself that not only update mgr calls getIncompatibleItemList.
Assignee | ||
Comment 70•16 years ago
|
||
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 71•16 years ago
|
||
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-
Assignee | ||
Comment 72•16 years ago
|
||
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.
Assignee | ||
Comment 73•16 years ago
|
||
Assignee | ||
Updated•16 years ago
|
Attachment #278009 -
Attachment is patch: true
Attachment #278009 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 74•16 years ago
|
||
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)
Assignee | ||
Comment 75•16 years ago
|
||
Comment on attachment 278009 [details] [diff] [review] patch, v1.8 I think I should've included this too. > addDownloads: function(items, itemCount, fromChrome) { if (itemCount == 0) { throw Components.results.NS_ERROR_ILLEGAL_VALUE; } >+ for (i = 0; i < itemCount; ++i) { >+ var currItem = items[i]; >+ // If somehow we end up with a null URL, the item isn't valid. >+ if (!currItem.xpiURL) >+ throw Components.results.NS_ERROR_ILLEGAL_VALUE; >+ } Dave, if there's anything else you need for extmgr, or I'm *still* not grokking it, can you just show me what you have in mind?
Comment 76•16 years ago
|
||
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 77•16 years ago
|
||
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+
Assignee | ||
Comment 78•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Attachment #278251 -
Flags: review+
![]() |
||
Comment 79•16 years ago
|
||
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 80•16 years ago
|
||
Comment on attachment 278251 [details] [diff] [review] patch, v1.8.1 >Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v >retrieving revision 1.238 >diff -p -u -8 -w -r1.238 nsExtensionManager.js.in >--- toolkit/mozapps/extensions/src/nsExtensionManager.js.in 16 Aug 2007 21:17:45 -0000 1.238 >+++ toolkit/mozapps/extensions/src/nsExtensionManager.js.in 25 Aug 2007 23:27:52 -0000 >... >@@ -694,37 +707,50 @@ function getAddonTypeFromInstallManifest > */ > function showIncompatibleError(installData) { > var extensionStrings = BundleManager.getBundle(URI_EXTENSIONS_PROPERTIES); > var params = [extensionStrings.GetStringFromName("type-" + installData.type)]; > var title = extensionStrings.formatStringFromName("incompatibleTitle", > params, params.length); > var message; > var targetAppData = installData.currentApp; >+ >+ var targetAppName; >+ var targetVersion; >+ if (targetAppData && >+ (targetAppData.id == TOOLKIT_ID)) { >+ targetAppName = BundleManager.toolkitName; >+ targetVersion = gApp.platformVersion; >+ } else { >+ targetAppName = gApp.name; >+ targetVersion = gApp.version; >+ } Is there a reason not to use targetAppName = BundleManager.appName here? I'm sorely tempted to just display a simple "not compatible with appName" message here instead of introducing the term Gecko Toolkit. I'm going to ping UX again on this.
![]() |
||
Comment 81•16 years ago
|
||
email sent to UX We are introducing toolkit version compatibility for add-ons in https://bugzilla.mozilla.org/show_bug.cgi?id=299716 The error message when an add-on is not compatible with the application is incompatibleMsg=%S %S could not be installed because it is not compatible with %S %S. (%S %S will only work with %S versions from %S to %S) incompatibleMsgSingleAppVersion=%S %S could not be installed because it is not compatible with %S %S. (%S %S will only work with %S %S) incompatibleMessageNoApp=%S %S could not be installed because it is not compatible with %S. Examples: incompatibleMsg=Add-on Name 1.2.3 could not be installed because it is not compatible with Firefox 2.0 (Add-on Name 1.2.3 will only work with Firefox versions from 1.0 to 1.5) incompatibleMsgSingleAppVersion=Add-on Name 1.2.3 could not be installed because it is not compatible with Firefox 2.0. (Add-on Name 1.2.3 will only work with Firefox 1.5) incompatibleMessageNoApp=Add-on Name 1.2.3 could not be installed because it is not compatible with Firefox 2.0. Adding a term to describe the toolkit in the UI just seems too confusing and I think it would be best to just use the incompatibleMessageNoApp message for this instance and log detailed information to the error console.
Comment 82•16 years ago
|
||
Al, I've now landed bug 391899 so you'll need to make the minor change to your testcase I mentioned in comment 62.
Assignee | ||
Comment 83•16 years ago
|
||
I want to hold that until Robert completes his review. I still don't know if I'll need to submit a patch for fresh review, or if the next patch will be final.
![]() |
||
Comment 84•16 years ago
|
||
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-
![]() |
||
Comment 85•16 years ago
|
||
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.
Assignee | ||
Comment 86•16 years ago
|
||
Okay. Note that my available time to work on this bug is restricted to evenings and weekends.
![]() |
||
Comment 87•16 years ago
|
||
Comment on attachment 278251 [details] [diff] [review] patch, v1.8.1 btw: I'm fine with the tests.
Assignee | ||
Comment 88•16 years ago
|
||
Robert, on a couple of these, you noted indentation glitches. The patches I've submitted are actually -w, and in my local file, they were correct. (However, my editor is a bit zealous about making sure line endings have no trailing spaces. As a result, if I hadn't, the diff would be much larger.) What do you suggest I should do?
![]() |
||
Comment 89•16 years ago
|
||
I usually submit both patches so if there is a question the full patch can be looked at for reference.
Assignee | ||
Comment 90•16 years ago
|
||
(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.
Assignee | ||
Comment 91•16 years ago
|
||
full patch coming right up, with unexpected changes explained
Attachment #278540 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 92•16 years ago
|
||
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
![]() |
||
Comment 93•16 years ago
|
||
(In reply to comment #92) >... > * I left a couple of these in the onAddonUpdateStarted, onAddonUpdateEnded > methods - sorry: > > dump((new Error()).stack + "\n\n"); please clean it up before checkin. > Finally, I should note the following pieces in my test log, which worry me. > They were right next to each other: > > ************************************************************ > * Call to xpconnect wrapped JSObject produced this error: * > [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) > [nsIObserverService.removeObserver]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" > location: "JS frame :: > file:///c:/mozilla.org/trunk/fx-debug/dist/bin/components/nsBlocklistService.js > :: anonymous :: line 276" data: no] > ************************************************************ > > ###!!! ASSERTION: Oops! You're asking for a weak reference to an object that > doesn't support that.: 'factoryPtr', file > c:/mozilla.org/trunk/fx-debug/xpcom/build/nsWeakReference.cpp, line 109 > > The line in question for the first was: > > gOS.removeObserver(this, "xpcom-shutdown"); That line number is off by one... the actual line number is 277 gOS.removeObserver(this, "profile-after-change"); It fails because addObserver is not called for profile-after-change and plugins-list-updated.
![]() |
||
Comment 94•16 years ago
|
||
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+
Assignee | ||
Comment 95•16 years ago
|
||
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.
![]() |
||
Comment 97•16 years ago
|
||
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. ;)
Assignee | ||
Comment 98•16 years ago
|
||
I'll leave it in your hands, Rob; I'll be on #developers to watch the tree if you want to join me.
![]() |
||
Comment 99•16 years ago
|
||
A couple of things came up so I'm going to land the patch this evening.
![]() |
||
Comment 101•16 years ago
|
||
Filed bug 394128 for the AUS changes needed.
![]() |
||
Comment 102•16 years ago
|
||
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: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Keywords: dev-doc-needed
Comment 103•16 years ago
|
||
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).
Updated•16 years ago
|
Flags: in-testsuite? → in-testsuite+
Keywords: dev-doc-needed → dev-doc-complete
Updated•16 years ago
|
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
Updated•15 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•