I believe we already have litmus tests on checking for app updates and it warning you as relevant about incompatible add-ons

Bug 546595 will add mochitest chrome tests which should cover anything we could check in a litmus test.

Fixed with http://hg.mozilla.org/projects/addonsmgr/rev/f4a0c7ca4df8 and http://hg.mozilla.org/projects/addonsmgr/rev/5c4b8b9de70c

This adds isCompatibleWith(appVersion, platformVersion) to the API and stops update checks from clobbering compatibility info when searching for something other than the current versions.

This switches the update service to the new APIs, making a couple of the properties in updates.js asynchronous. This is untested when add-ons are present as yet (though I did run through the existing tests with some in place and it coped as expected)

Comment on attachment 435761 [details] [diff] [review] EM patch rev 1 I think you need to add the same check for the default theme as there is in isUsableItem. What about app provided add-ons? I really don't want apps that have taken the time to add appManaged to start showing up as incompatible during the app update check. >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >--- a/toolkit/mozapps/extensions/XPIProvider.jsm >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm >@@ -3749,13 +3749,29 @@ UpdateChecker.prototype = { > * The list of update details for the add-on > */ > onUpdateCheckComplete: function UC_onUpdateCheckComplete(updates) { >+ // Always apply any compatibility update for the current version note: this is a change in behavior in that we used to only apply compatibility info that increased the maxVersion except on app version change where we sync compatibility info for all add-ons as noted in bug 553169 comment #105. I'm not entirely against doing this but I suspect some people will be. The remainder looks good.

Comment on attachment 435762 [details] [diff] [review] Updates patch rev 1 updates.js looks good but I want to take another look at it after the following is fixed. I didn't go over it thoroughly but there are a couple of obvious problems. >diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js >--- a/toolkit/mozapps/update/nsUpdateService.js >+++ b/toolkit/mozapps/update/nsUpdateService.js >@@ -1467,33 +1468,21 @@ UpdateService.prototype = { > }, > > _checkAddonCompatibility: function AUS__checkAddonCompatibility() { >- var em = Cc["@mozilla.org/extensions/manager;1"]. >- getService(Ci.nsIExtensionManager); >- // Get the add-ons that are incompatible with the update's application >- // version and toolkit version. >- var currentAddons = em.getIncompatibleItemList(this._update.appVersion, >- this._update.platformVersion, >- Ci.nsIUpdateItem.TYPE_ANY, >- false); >- if (currentAddons.length > 0) { >- // Get the add-ons that are incompatible with the current application >- // version and toolkit version. >- var previousAddons = em.getIncompatibleItemList(null, null, >- Ci.nsIUpdateItem.TYPE_ANY, >- false); >- // Don't include add-ons that are already incompatible with the current >- // application version and toolkit version. >- for (var i = 0; i < previousAddons.length; ++i) { >- for (var j = 0; j < currentAddons.length; ++j) { >- if (previousAddons[i].id === currentAddons[j].id) { >- currentAddons.splice(j, 1); >- break; >- } >+ // Get all the installed add-ons >+ AddonManager.getAddonsByTypes(["extension", "theme", "locale"], function(addons) { >+ this._incompatibleAddons = []; >+ addons.forEach(function(addon) { >+ // Only care about add-ons that are currently compatible and aren't >+ // installed by the application. >+ if (addon.isCompatible && addon.scope != AddonManager.SCOPE_APPLICATION) { >+ // Add any to the list that aren't compatible with the update >+ if (!addon.isCompatibleWith(this._update.appVersion, >+ this._update.platformVersion)) >+ this._incompatibleAddons.push(addon); > } >- } >- } >+ }); > >- if (currentAddons.length > 0) { >+ if (this._incompatibleAddons.length > 0) { > /** > # PREF_APP_UPDATE_INCOMPATIBLE_MODE > # Controls the mode in which we check for updates as follows. >@@ -1514,37 +1503,55 @@ UpdateService.prototype = { > # required after the update. This is not the default and is supplied > # only as a hidden option for those that want it. > */ >- this._incompatAddonsCount = currentAddons.length; >- LOG("UpdateService:_checkAddonCompatibility - checking for " + >- "incompatible add-ons"); >- var updateIncompatMode = getPref("getIntPref", PREF_APP_UPDATE_INCOMPATIBLE_MODE, 0); >- var mode = (updateIncompatMode == 1) ? Ci.nsIExtensionManager.UPDATE_CHECK_COMPATIBILITY : >- Ci.nsIExtensionManager.UPDATE_NOTIFY_NEWVERSION; >- em.update(currentAddons, currentAddons.length, mode, this, >- Ci.nsIExtensionManager.UPDATE_WHEN_NEW_APP_DETECTED, >- this._update.appVersion, this._update.platformVersion); >- } >- else { >- LOG("UpdateService:_checkAddonCompatibility - no need to show prompt, " + >- "just download the update"); >- var status = this.downloadUpdate(this._update, true); >- if (status == STATE_NONE) >- cleanupActiveUpdate(); >- this._update = null; >+ this._updateCheckCount = currentAddons.length; currentAddons isn't declared, set with a value, etc. >+ LOG("UpdateService:_checkAddonCompatibility - checking for " + >+ "incompatible add-ons"); >+ >+ this._incompatibleAddons.forEach(function(addon) { >+ addon.findUpdates(this, AddonManager.UPDATE_WHEN_NEW_APP_DETECTED, >+ this._update.appVersion, this._update.platformVersion); >+ }, this); >+ } >+ else { >+ LOG("UpdateService:_checkAddonCompatibility - no need to show prompt, " + >+ "just download the update"); >+ var status = this.downloadUpdate(this._update, true); >+ if (status == STATE_NONE) >+ cleanupActiveUpdate(); >+ this._update = null; >+ } >+ }); >+ }, >+ >+ // AddonUpdateListener >+ onCompatibilityUpdateAvailable: function(addon) { >+ for (var i = 0; i < this._incompatibleAddons.length; ++i) { >+ if (this._incompatibleAddons[i].id == addon.id) { >+ LOG("UpdateService:onAddonUpdateEnded - found update for add-on ID: " + >+ addon.id); >+ this._incompatibleAddons.splice(i, 1); >+ } > } > }, > >- /** >- * See nsIExtensionManager.idl >- */ >- onUpdateStarted: function AUS_onUpdateStarted() { >+ onUpdateAvailable: function(addon) { >+ if (getPref("getIntPref", PREF_APP_UPDATE_INCOMPATIBLE_MODE, 0) == 1) >+ return; >+ >+ for (var i = 0; i < this._incompatibleAddons.length; ++i) { >+ if (this._incompatibleAddons[i].id == addon.id) { >+ LOG("UpdateService:onAddonUpdateEnded - found update for add-on ID: " + >+ addon.id); >+ this._incompatibleAddons.splice(i, 1); >+ } >+ } > }, > >- /** >- * See nsIExtensionManager.idl >- */ >- onUpdateEnded: function AUS_onUpdateEnded() { >- if (this._incompatAddonsCount > 0 || !gCanApplyUpdates) { >+ onUpdateFinished: function(addon) { >+ if (--this._updateCheckCount > 0) >+ return; _updateCheckCount is set to currentAddons.length which isn't set

Note on current behavior that I want to keep: 1. if the current version of the app is the same as the version being offered then no add-on update check occurs. 2. if an add-on is disabled then it isn't checked or counted as an add-on that could be disabled. I don't recall if this takes into account to be enabled / to be disabled but it should if it isn't too complex. 3. app provided add-ons aren't included in the check. Perhaps instead of using addManaged the app can provide a list of provided add-ons in a pref. This would need to take into account the install location of course. If the distribution model is implemented for this then there should be some way to do this for profile install locations as well. 4. The default theme should never be included in this check.

(In reply to comment #8) > Note on current behavior that I want to keep: > 1. if the current version of the app is the same as the version being offered > then no add-on update check occurs. I haven't changed that bit of the original code so it is still the case. > 2. if an add-on is disabled then it isn't checked or counted as an add-on that > could be disabled. I don't recall if this takes into account to be enabled / to > be disabled but it should if it isn't too complex. I've made it so if the add-on is disabled pending enable, or enabled and not pending disable and then will not be compatible with the new app then the user will be warned about it. > 3. app provided add-ons aren't included in the check. Perhaps instead of using > addManaged the app can provide a list of provided add-ons in a pref. This would > need to take into account the install location of course. If the distribution > model is implemented for this then there should be some way to do this for > profile install locations as well. After thought I think we should just never warn about extensions installed in the application directory and commit to providing applications the better solution for app-shipped extensions before this goes into a final release (bug 474289). > 4. The default theme should never be included in this check. This will be excluded because of the previous point.

Sounds good and thanks!

This includes synchronizing when performing an update check for a newly installed application and tests that it works. Had to add an additional parameter to AddonUpdateChecker.getCompatibilityUpdate to have it return compatibility information even when that indicated incompatibility with the app which it normally doesn't do.

Initial comments addressed.

Comment on attachment 437472 [details] [diff] [review] Updates patch rev 2 >diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js >... > _checkAddonCompatibility: function AUS__checkAddonCompatibility() { >... >+ // Get all the installed add-ons >+ AddonManager.getAddonsByTypes(["extension", "theme", "locale"], function(addons) { >+ this._incompatibleAddons = []; The scope of this changes here so you'll have to declare self = this and use self, etc.

(In reply to comment #13) > (From update of attachment 437472 [details] [diff] [review]) > >diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js > >... > > _checkAddonCompatibility: function AUS__checkAddonCompatibility() { > >... > >+ // Get all the installed add-ons > >+ AddonManager.getAddonsByTypes(["extension", "theme", "locale"], function(addons) { > >+ this._incompatibleAddons = []; > The scope of this changes here so you'll have to declare self = this and use > self, etc. The remainder of nsUpdateService.js looks good.

Comment on attachment 437472 [details] [diff] [review] Updates patch rev 2 >diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js >@@ -355,34 +356,37 @@ var gUpdates = { > > // Cache the standard button labels in case we need to restore them > this._cacheButtonStrings("next"); > this._cacheButtonStrings("finish"); > this._cacheButtonStrings("extra1"); > this._cacheButtonStrings("extra2"); > > // Advance to the Start page. >- var startPage = this.startPage; >- LOG("gUpdates", "onLoad - setting current page to startpage " + startPage.id); >- this.wiz.currentPage = startPage; >+ this.getStartPage(function(startPage) { >+ LOG("gUpdates", "onLoad - setting current page to startpage " + startPage.id); >+ gUpdates.wiz.currentPage = startPage; Since these lines of code are all being changed would you mind changing getStartPage to getStartPageID and have it return the page ID? Then all of the individual document.getElementById can be removed and replaced with a single one here. >+ }); > }, > > /** > * Called when the wizard UI is unloaded. > */ > onUnload: function() { > if (this._runUnload) { > var cp = this.wiz.currentPage; > if (cp.pageid != "finished" && cp.pageid != "finishedBackground") > this.onWizardCancel(); > } > }, > > /** >- * Return the <wizardpage> object that should be displayed first. >+ * Gets the <wizardpage> object that should be displayed first. This is an >+ * asynchronous method that passes the resulting object to a callback >+ * function. > * > * This is determined by how we were called by the update prompt: > * > * Prompt Method: Arg0: Update State: Src Event: Failed: Result: > * showUpdateAvailable nsIUpdate obj -- background -- see Note below > * showUpdateDownloaded nsIUpdate obj pending background -- finishedBackground > * showUpdateInstalled "installed" -- -- -- installed > * showUpdateError nsIUpdate obj failed either partial errorpatching >@@ -390,18 +394,20 @@ var gUpdates = { > * checkForUpdates null -- foreground -- checking > * checkForUpdates null downloading foreground -- downloading > * > * Note: the page returned (e.g. Result) for showUpdateAvaulable is as follows: > * New enabled incompatible add-ons : incompatibleCheck page > * No new enabled incompatible add-ons: either updatesfoundbasic or > * updatesfoundbillboard as determined by > * updatesFoundPageId >+ * @param aCallback >+ * A callback to pass the <wizardpage> object to be displayed first to. > */ >- get startPage() { >+ getStartPage: function(aCallback) { nit: indentation is off r=me with comments addressed.

Not sure where this would best be added but in the existing code the update check verified the update wasn't blocklisted in _isValidUpdate.

Comment on attachment 437471 [details] [diff] [review] EM patch rev 2 >diff --git a/toolkit/mozapps/extensions/AddonUpdateChecker.jsm b/toolkit/mozapps/extensions/AddonUpdateChecker.jsm >--- a/toolkit/mozapps/extensions/AddonUpdateChecker.jsm >+++ b/toolkit/mozapps/extensions/AddonUpdateChecker.jsm >@@ -570,34 +570,47 @@ var AddonUpdateChecker = { > /** > * Retrieves the best matching compatibility update for the application from > * a list of available update objects. > * > * @param updates > * An array of update objects > * @param version > * The version of the add-on to get new compatibility information for >+ * @param ignoreVersions I think this would be better named as ignoreCompatibility though that seems weird with getCompatibilityUpdate. Perhaps syncCompatibility? >+ * An optional parameter to get compatibility information even if it >+ * is not compatible with either the application or toolkit > * @param appVersion > * The version of the application or null to use the current version > * @param platformVersion > * The version of the platform or null to use the current version > * @return an update object if one matches or null if not > */ > getCompatibilityUpdate: function AUC_getCompatibilityUpdate(updates, version, >+ ignoreVersions, > appVersion, > platformVersion) { > if (!appVersion) > appVersion = Services.appinfo.version; > if (!platformVersion) > platformVersion = Services.appinfo.platformVersion; > > for (let i = 0; i < updates.length; i++) { >- if ((Services.vc.compare(updates[i].version, version) == 0) && >- matchesVersions(updates[i], appVersion, platformVersion)) >- return updates[i]; >+ if (Services.vc.compare(updates[i].version, version) == 0) { >+ if (ignoreVersions) { >+ for (let j = 0; j < updates[i].targetApplications.length; j++) { >+ let id = updates[i].targetApplications[j].id; >+ if (id == Services.appinfo.ID || id == TOOLKIT_ID) >+ return updates[i]; If there is an entry that matches the app ID it should be given priority to the toolkit id. >+ } >+ } >+ else if (matchesVersions(updates[i], appVersion, platformVersion)) { >+ return updates[i]; This will change the target app version info if a match is found and not just when the maxVersion increases while still being compatible. I don't think this is a big deal... just wanted to point it out. You could also pass the flag to matchesVersions to simplify this a bit but I'm fine either way. >+ } >+ } > } > return null; > }, > /** > * Returns the newest available update from a list of update objects. > * > * @param updates

(In reply to comment #17) >... > This will change the target app version info if a match is found and not just > when the maxVersion increases while still being compatible. I don't think this > is a big deal... just wanted to point it out. Just noticed you prevent this in applyCompatibilityUpdate. Not sure if this is a problem with that in applyCompatibilityUpdate but it seems like it should be consistent. For example, onCompatibilityUpdateAvailable will be called even though it is compatibility info that is ignored.

Comment on attachment 437471 [details] [diff] [review] EM patch rev 2 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >--- a/toolkit/mozapps/extensions/XPIProvider.jsm >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm >@@ -3913,31 +3914,50 @@ function UpdateChecker(addon, listener, > AddonUpdateChecker.checkForUpdates(addon.id, addon.type, addon.updateKey, url, this); > } > > UpdateChecker.prototype = { > addon: null, > listener: null, > appVersion: null, > platformVersion: null, >+ syncCompatibility: null, > > /** > * Called when AddonUpdateChecker completes the update check > * > * @param updates > * The list of update details for the add-on > */ > onUpdateCheckComplete: function UC_onUpdateCheckComplete(updates) { >+ // Always apply any compatibility update for the current version > let compatUpdate = AddonUpdateChecker.getCompatibilityUpdate(updates, > this.addon.version, >- this.appVersion, >- this.platformVersion); >- if (compatUpdate && this.addon.applyCompatibilityUpdate(compatUpdate)) { >- if ("onCompatibilityUpdated" in this.listener) >- this.listener.onCompatibilityUpdated(createWrapper(this.addon)); >+ this.syncCompatibility); >+ >+ // Apply the compatibility update to the database >+ if (compatUpdate) >+ this.addon.applyCompatibilityUpdate(compatUpdate, this.syncCompatibility); >+ >+ // If the listener requested information for a different application or >+ // platform version then look for compatibility information for that. It isn't the listener that makes the request. How about If the request is for an application or platform version that is different than the current application or platform version the look for a compatibility update for those versions. After writing that I think you should check if the appVersion or platformVersion is specified and different than the current application or platform versions in the check below. >+ if (this.appVersion || this.platformVersion) { >+ compatUpdate = AddonUpdateChecker.getCompatibilityUpdate(updates, >+ this.addon.version, >+ false, >+ this.appVersion, >+ this.platformVersion); >+ } >+ >+ if (compatUpdate) { >+ if ("onCompatibilityUpdateAvailable" in this.listener) >+ this.listener.onCompatibilityUpdateAvailable(createWrapper(this.addon)); >+ } >+ else if ("onNoCompatibilityUpdateAvailable" in this.listener) { >+ this.listener.onNoCompatibilityUpdateAvailable(createWrapper(this.addon)); > } > > let update = AddonUpdateChecker.getNewestCompatibleUpdate(updates, > this.appVersion, > this.platformVersion); > if (update && Services.vc.compare(this.addon.version, update.version) < 0) { > if ("onUpdateAvailable" in this.listener) { > let self = this; >@@ -3996,25 +4016,34 @@ AddonInternal.prototype = { > }, > > get providesUpdatesSecurely() { > return !!(this.updateKey || !this.updateURL || > this.updateURL.substring(0, 6) == "https:"); > }, > > get isCompatible() { >+ return this.isCompatibleWith(); >+ }, >+ >+ isCompatibleWith: function(appVersion, platformVersion) { I'll leave this up to you but you could just have isCompatible with appVersion and platformVersion being optional. The previous isCompatible respected the checkCompatibility preference

Comment on attachment 437471 [details] [diff] [review] EM patch rev 2 >diff --git a/toolkit/mozapps/extensions/test/xpcshell/data/test_update.rdf b/toolkit/mozapps/extensions/test/xpcshell/data/test_update.rdf >--- a/toolkit/mozapps/extensions/test/xpcshell/data/test_update.rdf >+++ b/toolkit/mozapps/extensions/test/xpcshell/data/test_update.rdf >@@ -4,25 +4,37 @@ > xmlns:em="http://www.mozilla.org/2004/em-rdf#"> > > <Description about="urn:mozilla:extension:addon1@tests.mozilla.org"> > <em:updates> > <Seq> > <li> > <Description> > <em:version>1.0</em:version> >+ <!-- Shouldn't fire onCompatibilityUpdateAvailable since this >+ information is already in the install.rdf --> > <em:targetApplication> > <Description> > <em:id>xpcshell@tests.mozilla.org</em:id> > <em:minVersion>1</em:minVersion> > <em:maxVersion>1</em:maxVersion> > </Description> > </em:targetApplication> >+ >+ <!-- Should be ignored as it is not for the present application --> Isn't this for the same application since it has xpcshell@tests.mozilla.org for the id with only the min/maxVersion being different?

Forgot to include the code below the comment >+ <!-- Should be ignored as it is not for the present application --> Isn't this for the same application since it has xpcshell@tests.mozilla.org for the id with only the min/maxVersion be? >+ <em:targetApplication> >+ <Description> >+ <em:id>xpcshell@tests.mozilla.org</em:id> >+ <em:minVersion>2</em:minVersion> >+ <em:maxVersion>2</em:maxVersion> >+ </Description> >+ </em:targetApplication> > </Description>

For the UPDATE_WHEN_NEW_APP_DETECTED tests it would be good to also test a check where there is no compatibility update that would make it compatible with the new version.

Rob, this is how I've added the check about new versions that are blocklisted for the next app version, seem ok?

(In reply to comment #17) > (From update of attachment 437471 [details] [diff] [review]) > >diff --git a/toolkit/mozapps/extensions/AddonUpdateChecker.jsm b/toolkit/mozapps/extensions/AddonUpdateChecker.jsm > >--- a/toolkit/mozapps/extensions/AddonUpdateChecker.jsm > >+++ b/toolkit/mozapps/extensions/AddonUpdateChecker.jsm > >@@ -570,34 +570,47 @@ var AddonUpdateChecker = { > > /** > > * Retrieves the best matching compatibility update for the application from > > * a list of available update objects. > > * > > * @param updates > > * An array of update objects > > * @param version > > * The version of the add-on to get new compatibility information for > >+ * @param ignoreVersions > I think this would be better named as ignoreCompatibility though that seems > weird with getCompatibilityUpdate. Perhaps syncCompatibility? syncCompatibility seems weird since the method isn't really doing that, but I agree ignoreVersions isn't great. I think ignoreCompatibility works. platformVersion) { > > if (!appVersion) > > appVersion = Services.appinfo.version; > > if (!platformVersion) > > platformVersion = Services.appinfo.platformVersion; > > > > for (let i = 0; i < updates.length; i++) { > >- if ((Services.vc.compare(updates[i].version, version) == 0) && > >- matchesVersions(updates[i], appVersion, platformVersion)) > >- return updates[i]; > >+ if (Services.vc.compare(updates[i].version, version) == 0) { > >+ if (ignoreVersions) { > >+ for (let j = 0; j < updates[i].targetApplications.length; j++) { > >+ let id = updates[i].targetApplications[j].id; > >+ if (id == Services.appinfo.ID || id == TOOLKIT_ID) > >+ return updates[i]; > If there is an entry that matches the app ID it should be given priority to the > toolkit id. In this case it doesn't matter. So long as the update entry contains either a targetApplication entry for the app or toolkit then that is all we care about since versions don't matter here.

(In reply to comment #18) > (In reply to comment #17) > >... > > This will change the target app version info if a match is found and not just > > when the maxVersion increases while still being compatible. I don't think this > > is a big deal... just wanted to point it out. > Just noticed you prevent this in applyCompatibilityUpdate. Not sure if this is > a problem with that in applyCompatibilityUpdate but it seems like it should be > consistent. For example, onCompatibilityUpdateAvailable will be called even > though it is compatibility info that is ignored. So the way I'm thinking of this is the onCompatibilityUpdateAvailable is a signal to the listener that there is a compatibility update available for the version of the application/toolkit that the caller performed an update check for. It isn't a signal that a compatibility update has been applied in the database, though it will be if it matches the current app/toolkit version.

Comment on attachment 438838 [details] [diff] [review] follow up patch for update service Seems ok but you have onUpdateAvailable calling itself in updates.js. I'm not positive but I believe you are going with if a compatibility update is blocklisted this isn't going to be included in the list of add-ons that will be app disabled? I'd like to think on that a bit after I you answer the above.

(In reply to comment #26) > (From update of attachment 438838 [details] [diff] [review]) > Seems ok but you have onUpdateAvailable calling itself in updates.j We really need to get some tests for this code. I'll try to put some together. > I'm not > positive but I believe you are going with if a compatibility update is > blocklisted this isn't going to be included in the list of add-ons that will be > app disabled? I'd like to think on that a bit after I you answer the above. I've added some additional comments here but I think it is correct. We have a list of add-ons that we believe will be made incompatible by the new application and we do update checks for those. If an add-on either has a compatibility update available, or there is a new version available that isn't blocklisted for the new application then we can remove that add-on from the list to warn about.

(In reply to comment #27) >... > (In reply to comment #26) > > (From update of attachment 438838 [details] [diff] [review] [details]) > > Seems ok but you have onUpdateAvailable calling itself in updates.j > > We really need to get some tests for this code. I'll try to put some together. That would be awesome and possible with the rewrite since this is ui code and needs to be tested via mochitest. If you'd prefer I can do it though I'm not familiar with installing add-ons for a mochitest yet.

Comments addressed

Comment on attachment 439010 [details] [diff] [review] follow-up rev 2 >@@ -1509,81 +1502,87 @@ UpdateService.prototype = { > # > # PREF_APP_UPDATE_INCOMPATIBLE_MODE == 1 > # We check for VersionInfo updates for the incompatible add-ons - i.e. > # if the situation above with Foo 1.2 and available update to 2.0 > # applies, we DO show the prompt since a download operation will be > # required after the update. This is not the default and is supplied > # only as a hidden option for those that want it. > */ >- this._incompatAddonsCount = currentAddons.length; >- LOG("UpdateService:_checkAddonCompatibility - checking for " + >- "incompatible add-ons"); >- var updateIncompatMode = getPref("getIntPref", PREF_APP_UPDATE_INCOMPATIBLE_MODE, 0); >- var mode = (updateIncompatMode == 1) ? Ci.nsIExtensionManager.UPDATE_CHECK_COMPATIBILITY : >- Ci.nsIExtensionManager.UPDATE_NOTIFY_NEWVERSION; >- em.update(currentAddons, currentAddons.length, mode, this, >- Ci.nsIExtensionManager.UPDATE_WHEN_NEW_APP_DETECTED, >- this._update.appVersion, this._update.platformVersion); >- } >- else { >- LOG("UpdateService:_checkAddonCompatibility - no need to show prompt, " + >- "just download the update"); >- var status = this.downloadUpdate(this._update, true); >- if (status == STATE_NONE) >- cleanupActiveUpdate(); >- this._update = null; >+ self._updateCheckCount = self._incompatibleAddons.length; >+ LOG("UpdateService:_checkAddonCompatibility - checking for " + >+ "incompatible add-ons"); >+ >+ self._incompatibleAddons.forEach(function(addon) { >+ addon.findUpdates(this, AddonManager.UPDATE_WHEN_NEW_APP_DETECTED, >+ this._update.appVersion, this._update.platformVersion); self._update.appVersion and self._update.platformVersion?

(In reply to comment #30) > >+ self._incompatibleAddons.forEach(function(addon) { > >+ addon.findUpdates(this, AddonManager.UPDATE_WHEN_NEW_APP_DETECTED, > >+ this._update.appVersion, this._update.platformVersion); > self._update.appVersion and self._update.platformVersion? Note the full context: > self._incompatibleAddons.forEach(function(addon) { > addon.findUpdates(this, AddonManager.UPDATE_WHEN_NEW_APP_DETECTED, > this._update.appVersion, this._update.platformVersion); > }, self); Passing self as the last parameter to forEach makes |this| inside the function equal to |self|. Perhaps that is confusing to read though so I can change it if you want

Missed that and thanks... no change needed

Comment on attachment 439010 [details] [diff] [review] follow-up rev 2 >@@ -390,18 +394,20 @@ var gUpdates = { > * checkForUpdates null -- foreground -- checking > * checkForUpdates null downloading foreground -- downloading > * > * Note: the page returned (e.g. Result) for showUpdateAvaulable is as follows: > * New enabled incompatible add-ons : incompatibleCheck page > * No new enabled incompatible add-ons: either updatesfoundbasic or > * updatesfoundbillboard as determined by > * updatesFoundPageId >+ * @param aCallback >+ * A callback to pass the <wizardpage> object to be displayed first to. nit: A callback to pass the ID of the <wizardpage> object to be displayed. > */ >- get startPage() { >+ getStartPageID: function(aCallback) { > if ("arguments" in window && window.arguments[0]) { > var arg0 = window.arguments[0]; > if (arg0 instanceof CoI.nsIUpdate) { > // If the first argument is a nsIUpdate object, we are notifying the > // user that the background checking found an update that requires > // their permission to install, and it's ready for download. > this.setUpdate(arg0); > var p = this.update.selectedPatch; >@@ -429,101 +435,117 @@ var gUpdates = { > } > } > > // Now select the best page to start with, given the current state of > // the Update. nit: the best page ID to start with

Comment on attachment 439010 [details] [diff] [review] follow-up rev 2 >+ getShouldCheckAddonCompatibility: function(aCallback) { >+ // this early return should never happen >+ if (!this.update) { >+ aCallback(false); >+ return; > } >... >+ var self = this; >+ AddonManager.getAddonsByTypes(["extension", "theme", "locale"], function(addons) { As we discussed, this call should be for all add-ons and not just these types. Is there any reason not to get all add-ons now or would you prefer to file a followup bug? Same in nsUpdateService.js. r=me with comments addressed or followup on the above issue.

Comment on attachment 439015 [details] [diff] [review] EM patch rev 3 Looks good. I'm a tad concerned about people expecting app update to tell them when an add-on won't be compatible with the update for add-ons installed in SCOPE_APPLICATION... could you make sure the docs for SCOPE_APPLICATION / KEY_APP_GLOBAL mention that extensions installed into this location should be app provided / included in the update mar / etc. and that app update will not check compatibility / warn when they will be disabled, etc.?

(In reply to comment #34) > (From update of attachment 439010 [details] [diff] [review]) > >+ getShouldCheckAddonCompatibility: function(aCallback) { > >+ // this early return should never happen > >+ if (!this.update) { > >+ aCallback(false); > >+ return; > > } > >... > >+ var self = this; > >+ AddonManager.getAddonsByTypes(["extension", "theme", "locale"], function(addons) { > As we discussed, this call should be for all add-ons and not just these types. > Is there any reason not to get all add-ons now or would you prefer to file a > followup bug? Same in nsUpdateService.js. Taken care of in bug 555942

http://hg.mozilla.org/mozilla-central/rev/9ff0a98f2ff3 http://hg.mozilla.org/mozilla-central/rev/fb65c6d92505

(In reply to comment #2) > Bug 546595 will add mochitest chrome tests which should cover anything we could > check in a litmus test. Robert, does it mean I do not have to spend time on the creation of Litmus tests? Dave, the check-in contains automated tests. Are those good enough to make sure that we can mark this bug as verified fixed? Can we flip the in-testsuite flag?

We have tests for the EM side but not the app update side, though it looks like Rob will cover those in bug 546595, we can mark this as in testsuite them I think.

Agreed

Thanks Dave and Rob! Setting in-testsuite flag and marking this bug as verified fixed based on a green test-run.

I meant that we should mark this as in-testsuite once Rob's mochitests are in.

With the landing of Bug 546595 this is now in-testsuite+