335942 - nsIExtensionManager.update wants true and false for a long param

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

(this is not a firefox bug, but toolkit doesn't have a suitable component)

nsIExtensionManager.update has a param versionUpdateOnly which is a long, but according to the documentation, it accepts true, false and 2. true and false shouldn't be treated as integers.
Event that '2' is wrong, because it's a magic number, meaning that it should be an enum.

Comment 1

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

Yep, this was a hack that should be fixed added at just before 1.5 for bug 303718

Comment 2

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

Target Milestone 3.0 since we can't change API compatibility for 2.0. Note that even before the patch landed in bug 303718 that the idl specified a long while all the call sites used a boolean.

Comment 3

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

Actually, the idl comments can be updated and the call sites fixed to all use integers... I'd like a better fix but not for 2.0 due to API compatibility. Should be simple enough to fix.

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch rev 1

The converts the function to use a simple enumeration and updates all callers that I could see to the new values.

Comment 5

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

Comment on attachment 248924 [details] [diff] [review]
patch rev 1

>Index: toolkit/mozapps/extensions/public/nsIExtensionManager.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl,v
>retrieving revision 1.45
>diff -u -8 -p -r1.45 nsIExtensionManager.idl
>--- toolkit/mozapps/extensions/public/nsIExtensionManager.idl	27 Apr 2006 03:04:31 -0000	1.45
>+++ toolkit/mozapps/extensions/public/nsIExtensionManager.idl	17 Dec 2006 15:26:05 -0000
>@@ -165,16 +165,23 @@ interface nsIInstallLocation : nsISuppor
>  *
>  * XXXben - Some of this stuff should go into a management-ey interface, 
>  *          some into an app-startup-ey interface.
>  */
> [scriptable, uuid(a3f5396c-a6e8-414a-8fbc-c8d831746328)]
> interface nsIExtensionManager : nsISupports
> {
>   /**
>+   * Constants representing types of update checks.
>+   */
>+  const unsigned long UPDATE_NEWVERSIONS   = 0;
>+  const unsigned long UPDATE_COMPATIBILITY = 1;
>+  const unsigned long UPDATE_SYNCHRONIZE   = 2;
How about something along the lines of
UPDATE_CHECK_NEWVERSION
UPDATE_CHECK_COMPATIBILITY
UPDATE_SYNC_COMPATIBILITY

>@@ -247,22 +254,24 @@ interface nsIExtensionManager : nsISuppo
> 
>   /**
>    * Checks for updates to a list of items.
>    * @param   items
>    *          An array of nsIUpdateItems to check for updates for.
>    * @param   itemCount
>    *          The length of |items|
>    * @param   versionUpdateOnly
>-   *          false if this check should find the newest versions available,
>-   *          true if it should only find newer target application compatibility
>-   *          information for the currently installed version.
>-   *          2 if this check should only find target application compatibility
>-   *          information for the currently installed version and synchronize
>-   *          the values.
>+   *          UPDATE_NEWVERSIONS if this check should find the newest versions
>+   *          available,
>+   *          UPDATE_COMPATIBILITY if it should only find newer target
>+   *          application compatibility information for the currently
>+   *          installed version.
>+   *          UPDATE_SYNCHRONIZE if this check should only find target
>+   *          application compatibility information for the currently
>+   *          installed version and synchronize the values.
>    * @param   listener
>    *          An nsIAddonUpdateCheckListener object which will be notified during
>    *          the update check process. 
>    */
>    void update([array, size_is(itemCount)] in nsIUpdateItem items,
>                in unsigned long itemCount, 
>                in unsigned long versionUpdateOnly,
Change versionUpdateOnly to updateCheckType

>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v
>retrieving revision 1.215
>diff -u -8 -p -r1.215 nsExtensionManager.js.in
>--- toolkit/mozapps/extensions/src/nsExtensionManager.js.in	17 Sep 2006 22:19:01 -0000	1.215
>+++ toolkit/mozapps/extensions/src/nsExtensionManager.js.in	17 Dec 2006 15:26:18 -0000
>...
>@@ -5999,17 +5999,17 @@ ExtensionItemUpdater.prototype = {
>       // If we got here through |checkForMismatches|, this extension has
>       // already been disabled, re-enable it.
>       var op = StartupCache.entries[aLocalItem.installLocationKey][aLocalItem.id].op;
>       if (op == OP_NEEDS_DISABLE ||
>           this._emDS.getItemProperty(aLocalItem.id, "appDisabled") == "true")
>         this._em._appEnableItem(aLocalItem.id);
>       return true;
>     }
>-    else if (this._versionUpdateOnly == 2)
>+    else if (this._versionUpdateOnly == nsIExtensionManager.UPDATE_SYNCHRONIZE)
Change _versionUpdateOnly to _updateCheckType

>       this._emDS.updateTargetAppInfo(aLocalItem.id, aRemoteItem.minAppVersion,
>                                      aRemoteItem.maxAppVersion);
>     return false;
>   },
>   
>   _isValidUpdate: function(aLocalItem, aRemoteItem) {
>     var appExtensionsVersion = gApp.version;
> 
>@@ -6101,17 +6101,18 @@ RDFItemUpdater.prototype = {
>         installLocation.name == "winreg-app-user")) {
>       var status = nsIAddonUpdateCheckListener.STATUS_NOT_MANAGED;
>       this._updater.checkForDone(aItem, status);
>       return;
>     }
> 
>     // Don't check items for updates if the location can't be written to except
>     // when performing a version only update.
>-    if (!aVersionUpdateOnly && (!installLocation || !installLocation.canAccess)) {
>+    if ((aVersionUpdateOnly == nsIExtensionManager.UPDATE_NEWVERSIONS) && 
>+        (!installLocation || !installLocation.canAccess)) {
Change aVersionUpdateOnly to aUpdateCheckType

Please resubmit with the changes and I'll + it and thanks Dave!

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This addresses the above comments.

Something I just noticed, this does not change the aVersionUpdatesOnly parameter for _parseV20UpdateInfo. I don't know whether we want to change that for consistencies sake or leave it as is, since it is only ever true/false right now.

Comment 7

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

I'd like the params / vars / etc. names to reflect what they are so yes, please change that as well. Thanks!

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 268940 [details] [diff] [review]
patch rev 2

Dropping review request to get this back on my radar.

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 3

Updated to fix that last bit.

Comment 10

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

Comment on attachment 269656 [details] [diff] [review]
patch rev 3

Thanks Dave!

Comment 11

[Dave Townsend [:mossop]]

Checking in toolkit/mozapps/update/src/nsUpdateService.js.in;
/cvsroot/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in,v  <--  nsUpdateService.js.in
new revision: 1.137; previous revision: 1.136
done
Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.121; previous revision: 1.120
done
Checking in toolkit/mozapps/extensions/content/update.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/update.js,v  <--  update.js
new revision: 1.28; previous revision: 1.27
done
Checking in toolkit/mozapps/extensions/public/nsIExtensionManager.idl;
/cvsroot/mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl,v  <--  nsIExtensionManager.idl
new revision: 1.47; previous revision: 1.46
done
Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.221; previous revision: 1.220
done