Closed Bug 849614 Opened 7 years ago Closed 7 years ago

Give both version numbers to bootstrap methods when an addon is down/upgraded

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: darktrojan, Assigned: darktrojan)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

When the install/startup methods are called with ADDON_UPGRADE or ADDON_DOWNGRADE, it would be useful to know the version that was previously installed. For completeness we should also give the new version to the shutdown/uninstall methods.
Attached patch v1 (obsolete) — Splinter Review
In this patch we add params.oldVersion to install and startup methods, and params.newVersion to shutdown and uninstall methods.
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #723392 - Flags: review?(bmcbride)
Comment on attachment 723392 [details] [diff] [review]
v1

Review of attachment 723392 [details] [diff] [review]:
-----------------------------------------------------------------

Me like.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +3769,4 @@
>     *         The reason flag to pass to the bootstrap's startup method
>     */
>    callBootstrapMethod: function XPI_callBootstrapMethod(aId, aVersion, aType, aFile,
> +                                                        aOtherVersion, aMethod, aReason) {

Given this new param is optional, and the probability of wanting to add more properties to the data object, I think we should:
* put the new param at the end of the list (means most callers can omit it)
* make it into a object (makes it simple to add new properties in the future)

(Also, be sure to document it in comment for this method.)

::: toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js
@@ +41,5 @@
>    Services.prefs.setIntPref("bootstraptest.uninstall_reason", -1);
> +  Services.prefs.setIntPref("bootstraptest.startup_oldversion", -1);
> +  Services.prefs.setIntPref("bootstraptest.shutdown_newversion", -1);
> +  Services.prefs.setIntPref("bootstraptest.install_oldversion", -1);
> +  Services.prefs.setIntPref("bootstraptest.uninstall_newversion", -1);

Should also test that these *don't* get set when its not expected.
Attachment #723392 - Flags: review?(bmcbride) → review-
> * make it into a object (makes it simple to add new properties in the future)

Making the caller figure out the right property name also fixes my issue with callBootstrapMethod() containing some of the logic and the caller containing some of it.
In this case, and others like it,
http://hg.mozilla.org/mozilla-central/file/3b49881ae41d/toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js#l918
should I add the extra params when calling shutdown/startup, even though it's inconsistent with the reason argument?
Actually, given where those methods are called, getting the version info would be extremely difficult.
(In reply to Geoff Lankow (:darktrojan) from comment #5)
> Actually, given where those methods are called, getting the version info
> would be extremely difficult.

Now you've confused me - weren't you already doing that?
Not where the reason is APP_STARTUP or APP_SHUTDOWN.
Oh, right. Then lets only do it for when reason is specified as upgrade/downgrade - makes most sense, and is predictable.
Comment on attachment 723392 [details] [diff] [review]
v1

Review of attachment 723392 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +2817,5 @@
>          // Visible bootstrapped add-ons need to have their install method called
>          let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
>          file.persistentDescriptor = aAddonState.descriptor;
> +        XPIProvider.callBootstrapMethod(newAddon.id, newAddon.version,
> +                                        newAddon.type, file, oldVersion,

The reason here can be either install/upgrade/downgrade - lets make it so the extra param is only specified if and only if the reason is upgrade/downgrade. There may be other cases of this.
Attached patch v2 (obsolete) — Splinter Review
> Given this new param is optional, and the probability of wanting to add more
> properties to the data object, I think we should:
> * put the new param at the end of the list (means most callers can omit it)
> * make it into a object (makes it simple to add new properties in the future)

I did consider doing it this way, and then I didn't do it.
Attachment #723392 - Attachment is obsolete: true
Attachment #727513 - Flags: review?(bmcbride)
Comment on attachment 727513 [details] [diff] [review]
v2

Review of attachment 727513 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it!
Attachment #727513 - Flags: review?(bmcbride) → review+
Attachment #727513 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d674eb04e687
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.