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)
:
: Andy McKay [:andym]
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)
blair: review-
Details | Diff | Splinter Review
v2 (21.75 KB, patch)
2013-03-20 21:04 PDT, Geoff Lankow (:darktrojan)
blair: 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 User image 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 User image 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 User image Blair McBride [:Unfocused] (UNAVAILABLE) 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 User image Blair McBride [:Unfocused] (UNAVAILABLE) 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 User image 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 User image 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 User image Blair McBride [:Unfocused] (UNAVAILABLE) 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 User image Geoff Lankow (:darktrojan) 2013-03-20 15:53:35 PDT
Not where the reason is APP_STARTUP or APP_SHUTDOWN.
Comment 8 User image Blair McBride [:Unfocused] (UNAVAILABLE) 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 User image Blair McBride [:Unfocused] (UNAVAILABLE) 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 User image 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 User image Blair McBride [:Unfocused] (UNAVAILABLE) 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 User image Geoff Lankow (:darktrojan) 2013-03-22 19:20:03 PDT
Created attachment 728561 [details] [diff] [review]
v2 with updated comment
Comment 13 User image Geoff Lankow (:darktrojan) 2013-03-23 04:01:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d674eb04e687
Comment 14 User image 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 User image 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.