399932 - Add some variable substitution to updateInfoUrl

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

It would be useful to add some variable substitution such that %ITEM_ID%, %ITEM_VERSION% etc are correctly substituted the same as we already do for the updateUrl. Crucial is an %APP_LOCALE% so that an appropriately localised info page can be generated.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Sounds great, though not a blocker; wanted, though!

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This patch splits out the url substitution code into its own function that is called by the update retrieval and used the filter availableUpdateInfo out of the datasource when requested. It also adds APP_LOCALE to the set of substitutions.

There's also a little cleanup of ExtensionItemUpdater which doesnt really need to take app id and version parameters since we only ever update for the current app. Also added APP_LOCALE to the default amo update url since I presume that will be useful for them.

There is a testcase but it doesn't actually test the updateinfo url, it currently only tests the update url to check that we are correctly substituting things there.

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 286158 [details] [diff] [review]
patch rev 1

This patch is pretty important in terms of providing localised update information for add-ons. It's simple enough in nature, mostly just moving some code around.

Comment 4

[Damon Sicore (:damons)]

mconnor, can you please review this and process the approval?

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 286158 [details] [diff] [review]
patch rev 1

a=mconnor on behalf of endgame drivers

Comment 6

[Dave Townsend [:mossop]]

Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.261; previous revision: 1.260
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug335238.js,v
done
Checking in toolkit/mozapps/extensions/test/unit/test_bug335238.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug335238.js,v  <--  test_bug335238.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug335238_1/install.rdf,v
done
Checking in toolkit/mozapps/extensions/test/unit/addons/test_bug335238_1/install.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug335238_1/install.rdf,v  <--  install.rdf
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug335238_2/install.rdf,v
done
Checking in toolkit/mozapps/extensions/test/unit/addons/test_bug335238_2/install.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug335238_2/install.rdf,v  <--  install.rdf
initial revision: 1.1
done
Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,v  <--  extensions.properties
new revision: 1.44; previous revision: 1.43
done

Comment 7

[Marek Stępień [:marcoos, inactive]]

You should at least poke the .l10n newsgroup about this change. L10n-tinderbox won't turn orange on this URL change, so not all localizers will notice it (and you do want localized builds to have the APP_LOCALE variable set, right? ;-))

Ccing Pike.

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #7)
> You should at least poke the .l10n newsgroup about this change. L10n-tinderbox
> won't turn orange on this URL change, so not all localizers will notice it (and
> you do want localized builds to have the APP_LOCALE variable set, right? ;-))

Actually thanks for reminding me, I would much rather remove this property from the locales and just hardcode it as a fixed preference, localisations shouldn't be using different values for it really. Rob?

Comment 9

[Dave Townsend [:mossop]]

And need to document this

Comment 10

[Axel Hecht [:Pike]]

From a platform-consistency point of view, I had preferred to use variable names like we do in nsURLFormatter.js, and maybe even to extend that code to take more variables (a lookup [function] interface comes to my mind). I prefer the replacement code in there, too, just from a coding perspective.

The two don't actually use a different variable formats (i.e. %FOO%) though, which is what I care about really, so the above is just a nit/nag.

And yes, this should be announced in .l10n.

Comment 11

[Axel Hecht [:Pike]]

PS: This is probably still in l10n because SeaMonkey has a decentralized community URL structure. CCing KaiRo, Robert, do you care about this for suiterunner?

PS2: It is possible to write tests for variables in localized values, though it's currently a bug only filed in my head.

Comment 12

[Robert Kaiser]

We'd like to have it in locale for SeaMonkey, but that doesn't mean it needs to be there for toolkit. Isn't this going through prefs? In that case, every app can set the pref directly to a certain value or route it through L10n, right? I think we are doing this quite successfully with some URLs that use nsURLFormatter - and I'm with Axel in that it would sound logical to use nsURLFormatter as much as possible also here, as what we're doing here is basically just what that module was designed for, right?

Comment 13

[Dave Townsend [:mossop]]

nsURLFormatter only supports a few fixed keys for formatting, it currently only has 3 out of the 11 keys we use (ignoring extension provided keys), and they are all named differently and I believe it would be a poor choice to change over to that naming since this would breaking existing extensions and the naming is confusing in this particular context.

As for the prefs, yes we currently in Firefox's prefs (and presumably other apps) point the pref extensions.update.url to the locale. I'm suggesting we remove the key from the locale and just set the app's preference to the fixed url, then if an application wants to do different urls by locale they can just point to an appropriate locale property and maintain that themself.

Comment 14

[Eric Shepherd [:sheppy]]

Added information about %APP_LOCALE% to:

http://developer.mozilla.org/en/docs/Install_Manifests

Also added documentation of updateInfoURL, which wasn't documented yet.

Comment 15

[Eric Shepherd [:sheppy]]

Also added info about this to http://developer.mozilla.org/en/docs/Extension_Versioning%2C_Update_and_Compatibility#Providing_Details_about_Updates

Comment 16

[Axel Hecht [:Pike]]

Filed bug 413509 for the l10n part of this.