Last Comment Bug 849614 - Give both version numbers to bootstrap methods when an addon is down/upgraded
: Give both version numbers to bootstrap methods when an addon is down/upgraded
Status: RESOLVED FIXED
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla22
Assigned To: Geoff Lankow (:darktrojan)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-10 03:38 PDT by Geoff Lankow (:darktrojan)
Modified: 2013-03-23 15:56 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (21.16 KB, patch)
2013-03-11 03:31 PDT, Geoff Lankow (:darktrojan)
bmcbride: review-
Details | Diff | Splinter Review
v2 (21.75 KB, patch)
2013-03-20 21:04 PDT, Geoff Lankow (:darktrojan)
bmcbride: review+
Details | Diff | Splinter Review
v2 with updated comment (22.05 KB, patch)
2013-03-22 19:20 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review

Description Geoff Lankow (:darktrojan) 2013-03-10 03:38:55 PDT
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.
Comment 1 Geoff Lankow (:darktrojan) 2013-03-11 03:31:05 PDT
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.
Comment 2 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-14 06:36:37 PDT
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.
Comment 3 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-14 06:40:13 PDT
> * 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.
Comment 4 Geoff Lankow (:darktrojan) 2013-03-20 02:56:53 PDT
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?
Comment 5 Geoff Lankow (:darktrojan) 2013-03-20 03:09:27 PDT
Actually, given where those methods are called, getting the version info would be extremely difficult.
Comment 6 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-20 07:37:00 PDT
(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?
Comment 7 Geoff Lankow (:darktrojan) 2013-03-20 15:53:35 PDT
Not where the reason is APP_STARTUP or APP_SHUTDOWN.
Comment 8 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-20 17:42:00 PDT
Oh, right. Then lets only do it for when reason is specified as upgrade/downgrade - makes most sense, and is predictable.
Comment 9 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-20 17:45:11 PDT
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.
Comment 10 Geoff Lankow (:darktrojan) 2013-03-20 21:04:04 PDT
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.
Comment 11 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-03-20 21:35:51 PDT
Comment on attachment 727513 [details] [diff] [review]
v2

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

Ship it!
Comment 12 Geoff Lankow (:darktrojan) 2013-03-22 19:20:03 PDT
Created attachment 728561 [details] [diff] [review]
v2 with updated comment
Comment 13 Geoff Lankow (:darktrojan) 2013-03-23 04:01:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d674eb04e687
Comment 14 Geoff Lankow (:darktrojan) 2013-03-23 04:31:23 PDT
I've updated https://developer.mozilla.org/en-US/docs/Extensions/Bootstrapped_extensions#Bootstrap_data with the additional properties.
Comment 15 Joe Drew (not getting mail) 2013-03-23 15:56:57 PDT
https://hg.mozilla.org/mozilla-central/rev/d674eb04e687

Note You need to log in before you can comment on or make changes to this bug.