Closed
Bug 849614
Opened 11 years ago
Closed 11 years ago
Give both version numbers to bootstrap methods when an addon is down/upgraded
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: darktrojan, Assigned: darktrojan)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 2 obsolete files)
22.05 KB,
patch
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
In this patch we add params.oldVersion to install and startup methods, and params.newVersion to shutdown and uninstall methods.
Comment 2•11 years ago
|
||
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-
Comment 3•11 years ago
|
||
> * 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•11 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•11 years ago
|
||
Actually, given where those methods are called, getting the version info would be extremely difficult.
Comment 6•11 years ago
|
||
(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•11 years ago
|
||
Not where the reason is APP_STARTUP or APP_SHUTDOWN.
Comment 8•11 years ago
|
||
Oh, right. Then lets only do it for when reason is specified as upgrade/downgrade - makes most sense, and is predictable.
Comment 9•11 years ago
|
||
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•11 years ago
|
||
> 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 11•11 years ago
|
||
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•11 years ago
|
||
Attachment #727513 -
Attachment is obsolete: true
Assignee | ||
Comment 13•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d674eb04e687
Target Milestone: --- → mozilla22
Assignee | ||
Comment 14•11 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
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d674eb04e687
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•