nsIExtensionManager.update wants true and false for a long param

RESOLVED FIXED in mozilla1.9alpha6

Status

()

RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: mvl, Assigned: mossop)

Tracking

Trunk
mozilla1.9alpha6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(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.
Yep, this was a hack that should be fixed added at just before 1.5 for bug 303718
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.
Target Milestone: --- → Firefox 3
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.
Keywords: helpwanted
Target Milestone: Firefox 3 → ---
Whiteboard: [good first bug]
(Assignee)

Comment 4

12 years ago
Created attachment 248924 [details] [diff] [review]
patch rev 1

The converts the function to use a simple enumeration and updates all callers that I could see to the new values.
Assignee: nobody → mossop.bugzilla
Status: NEW → ASSIGNED
Attachment #248924 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

12 years ago
Keywords: helpwanted
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!
Attachment #248924 - Flags: review?(robert.bugzilla) → review-
(Assignee)

Comment 6

12 years ago
Created attachment 268940 [details] [diff] [review]
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.
Attachment #248924 - Attachment is obsolete: true
Attachment #268940 - Flags: review?(robert.bugzilla)
I'd like the params / vars / etc. names to reflect what they are so yes, please change that as well. Thanks!
(Assignee)

Comment 8

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

Dropping review request to get this back on my radar.
Attachment #268940 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 9

12 years ago
Created attachment 269656 [details] [diff] [review]
patch rev 3

Updated to fix that last bit.
Attachment #268940 - Attachment is obsolete: true
Attachment #269656 - Flags: review?(robert.bugzilla)
Attachment #269656 - Attachment is patch: true
Attachment #269656 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 269656 [details] [diff] [review]
patch rev 3

Thanks Dave!
Attachment #269656 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 11

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug]
(Assignee)

Updated

12 years ago
Target Milestone: --- → Firefox 3 alpha6
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.