The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla22

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

({dev-doc-complete})

unspecified
mozilla22
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 723392 [details] [diff] [review]
v1

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.
(Assignee)

Comment 4

4 years ago
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?
(Assignee)

Comment 5

4 years ago
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?
(Assignee)

Comment 7

4 years ago
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.
(Assignee)

Comment 10

4 years ago
Created attachment 727513 [details] [diff] [review]
v2

> 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+
(Assignee)

Comment 12

4 years ago
Created attachment 728561 [details] [diff] [review]
v2 with updated comment
Attachment #727513 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d674eb04e687
Target Milestone: --- → mozilla22
(Assignee)

Comment 14

4 years ago
I've updated https://developer.mozilla.org/en-US/docs/Extensions/Bootstrapped_extensions#Bootstrap_data with the additional properties.
Keywords: dev-doc-complete
https://hg.mozilla.org/mozilla-central/rev/d674eb04e687
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.