Dave Townsend [:mossop] Reporter
	Description • 16 years ago

nsURLFormatter is a useful pattern, however because the keys it can replace are fixed it is not as useful as it could be. It would be useful to make it possible for the caller to specify an additional set of keys that the formatter could then replace. This is a little complex as an xpcom component, however becomes trivial if we make it available as a js module.

A few place it could be used:

nsExtensionManager.js.in
nsBlocklistService.js
nsUpdateService.js

	John Gaunt (redfive)
	Comment 1 • 16 years ago

dave, how does this look as a first pass? I'm still going to pull the OS_VERSION code over from nsUpdaterService and modify the nsUpdaterService to call the urlformatter.

	Dave Townsend [:mossop] Reporter
	Comment 2 • 16 years ago

I don't believe that that is a sensible way to go about it. Since nsIURLFormatter is a service any new properties that are registered by callers will apply for all users so potentially breaking it for some. I also question whether adding multiple default keys for the same value is the right things to do. Additionally retrieving the prefs service as part of the component initialisation is not a good thing to do.

I was mulling over two potential ways to approach this but had yet to talk to any of the toolkit peers about which approach they thought was best. My first idea was to add an optional argument (probably an nsIPropertyBag2) to the format functions which the formatter could query for additional keys (using getPropertyAsAString). The second was to convert the component to a javascript module since I don't see any c++ callers in which case a simple javascript object could be used as the additional argument. Both cases could also specify whether the new keys override the defaults.

It would be worth though looking at the places where this could be used to check their specific needs, I know the extension manager has some additional needs that the propertybag approach works better with, but that is more work for all callers.

	John Gaunt (redfive)
	Comment 3 • 16 years ago

(In reply to comment #2)
> I don't believe that that is a sensible way to go about it. Since
> nsIURLFormatter is a service any new properties that are registered by callers
> will apply for all users so potentially breaking it for some.

This part is easily fixed by either checking the defaults first, or including an arg in the formatUrl() call that indicates whether or not to check the overrides.

 I also question
> whether adding multiple default keys for the same value is the right things to
> do.

I realize this part isn't ideal, but before I put in a bunch of time and energy to converting the nsUpdateService to use PLATFORMVERSION versus PLATFORM_VERSION I wanted to get a take on the approach of allowing additional mappings.

 Additionally retrieving the prefs service as part of the component
> initialisation is not a good thing to do.

This is easily fixed.

> 
> I was mulling over two potential ways to approach this but had yet to talk to
> any of the toolkit peers about which approach they thought was best. My first
> idea was to add an optional argument (probably an nsIPropertyBag2) to the
> format functions which the formatter could query for additional keys (using
> getPropertyAsAString). The second was to convert the component to a javascript
> module since I don't see any c++ callers in which case a simple javascript
> object could be used as the additional argument. Both cases could also specify
> whether the new keys override the defaults.

Given some modification of how the formatting is done (ie via a default arg to formatURL), I don't see how this is much different than what I proposed. Callers would still have to define the key->value mappings but using a property bag would have to create yet another structure, initialize it and pass it in. The simple js object approach is convenient, and would work for me as well since I'm calling from js, but certainly limits the flexibility of the class. And since the object is already defined with idl I see no compelling reason, yet, to change the architecture of this object.

I'll look at some more of the uses of urlformatter. If you have an example of an API please post so I can evaluate it against my needs and see if I can make it work. I'm happy to do whatever solution, this just seemed like the most straight forward approach.

	Dave Townsend [:mossop] Reporter
	Comment 4 • 16 years ago

(In reply to comment #3)
> (In reply to comment #2)
> > I don't believe that that is a sensible way to go about it. Since
> > nsIURLFormatter is a service any new properties that are registered by callers
> > will apply for all users so potentially breaking it for some.
> 
> This part is easily fixed by either checking the defaults first, or including
> an arg in the formatUrl() call that indicates whether or not to check the
> overrides.

Not really I think, imagine a case (which exists) where one component needs to define APP_VERSION to mean one thing and another component needs it to mean another. I believe that changing the behaviour of the component in such a way is just wrong for a service. Maybe if it were changed to a non-service it would make more sense, but that isn't something that can happen now.

> > I was mulling over two potential ways to approach this but had yet to talk to
> > any of the toolkit peers about which approach they thought was best. My first
> > idea was to add an optional argument (probably an nsIPropertyBag2) to the
> > format functions which the formatter could query for additional keys (using
> > getPropertyAsAString). The second was to convert the component to a javascript
> > module since I don't see any c++ callers in which case a simple javascript
> > object could be used as the additional argument. Both cases could also specify
> > whether the new keys override the defaults.
> 
> Given some modification of how the formatting is done (ie via a default arg to
> formatURL), I don't see how this is much different than what I proposed.

It is different because the new keys are not retained in the service where the same instance is shared for all callers

> Callers would still have to define the key->value mappings but using a property
> bag would have to create yet another structure, initialize it and pass it in.

It is a fairly small structure. You seem ok with the js object approach, an additional 1 line of code turns that js object into a viable propertybag2 for these purposes.
This also means that callers can use context sensitive mappings. The EM for instance maps the same key to a different values based on which extension it is checking for updates to.

> The simple js object approach is convenient, and would work for me as well
> since I'm calling from js, but certainly limits the flexibility of the class.
> And since the object is already defined with idl I see no compelling reason,
> yet, to change the architecture of this object.

I'm not sure what you mean by limiting hte flexibility, unless you mean it is restricting it to js callers only. This could of course be worked around by keeping the service, adding hte propertybag interface then writing a js module that wraps a js object, but that is easy enough anyway as I say above.

	John Gaunt (redfive)
	Comment 5 • 16 years ago

(In reply to comment #4)

I guess the difference in what I was envisioning is that this service held a number of defaults that were true across the entirety of the application and those were stored in the defaults section (and ultimately things like PLATFORMVERSION and PLATFORM_VERSION would be merged in usage not left as duplicates here). I didn't want to have to re-implement the definition of OS_VERSION, etc.

I guess I see the value in having a generic component that takes a property bag and does the replacement, but I think more valuable is having a set of platform defaults that users of the component don't have to redefine. 

The overrides effectively functioned like your vision of the propertybag except that they would stay around for later use.

Also, I'm always looking out for C++ callers since much of the core Songbird code is C++ and, for the moment at least, there is still a lot of mozilla C++. So I resist writing components that lock callers into js.

	John Gaunt (redfive)
	Comment 6 • 16 years ago

Updated to use propertybags, got rid of the additional interface methods, still contains the new defaults pulled from nsUpdateService.cpp

Thoughts?

	Dave Townsend [:mossop] Reporter
	Comment 7 • 16 years ago

(In reply to comment #5)
> (In reply to comment #4)
> 
> I guess the difference in what I was envisioning is that this service held a
> number of defaults that were true across the entirety of the application and
> those were stored in the defaults section (and ultimately things like
> PLATFORMVERSION and PLATFORM_VERSION would be merged in usage not left as
> duplicates here). I didn't want to have to re-implement the definition of
> OS_VERSION, etc.
> 
> I guess I see the value in having a generic component that takes a property bag
> and does the replacement, but I think more valuable is having a set of platform
> defaults that users of the component don't have to redefine. 

Well I agree that having more platform defaults in the component is probably a good thing (by which I mean more different keys rather than many duplicated ones). However the main reason I filed this bug was to improve the case where similar replacements are done using values that aren't default across the entire application.

(In reply to comment #6)
> Created an attachment (id=322037) [details]
> second pass, using propertybags
> 
> Updated to use propertybags, got rid of the additional interface methods, still
> contains the new defaults pulled from nsUpdateService.cpp
> 
> Thoughts?

This looks pretty good. Seems to be some indent issues so can't see precisely what has changed. Might want to think about restricting the overrides use to keys of more than 2 characters so callers can't shoot themselves in the foot and accidentally replace escaped characters maybe.

	Dave Townsend [:mossop] Reporter
	Comment 8 • 16 years ago

Comment on attachment 322037 [details] [diff] [review]
second pass, using propertybags

This needs testing.

>Index: toolkit/components/urlformatter/public/nsIURLFormatter.idl

>+
>+[scriptable, uuid(1d39f203-08a8-4632-9589-17f6ac0abcf5)]
> interface nsIURLFormatter: nsISupports

Add the new parameters to the comments and make the note that the property bag only needs to implement getProperty.

>Index: toolkit/components/urlformatter/src/nsURLFormatter.js

>     VERSION:          function() this.appInfo.version,
>     APPBUILDID:       function() this.appInfo.appBuildID,
>+    BUILD_ID:         function() this.appInfo.appBuildID,

I don't think we should include this duplicate

>+    BUILD_TARGET:     function() this.appInfo.OS + "_" + this.getABI(),

I think for consistency this should be BUILDTARGET

>     PLATFORMVERSION:  function() this.appInfo.platformVersion,
>+    PLATFORM_VERSION: function() this.appInfo.platformVersion,

I don't think we should include this duplicate

>+    DISTRIBUTION:         function() this.getPrefValue(PREF_APP_DISTRIBUTION),
>+    DISTRIBUTION_VERSION: function() this.getPrefValue(PREF_APP_DISTRIBUTION_VERSION),

Use DISTRIBUTIONVERSION

>+    CHANNEL:          function() this.getUpdateChannel(),
>+    OS_VERSION:       function() this.getOSVersion(),

Use OSVERSION

>+    getPrefValue: function (aPrefName) {

Rename this to getDefaultPrefValue and pass in the default to use as an argument.

>+      var prefSvc =  Cc['@mozilla.org/preferences-service;1'].getService(Ci.nsIPrefBranch2);

Keep lines under 80 characters

>+      var prefValue = "default";
>+      var defaults = prefSvc.QueryInterface(Ci.nsIPrefService)
>+                                      .getDefaultBranch(null);

Nit: line up the dots

>+    getUpdateChannel: function uf_getUpdateChannel() {
>+      var prefSvc =  Cc['@mozilla.org/preferences-service;1'].getService(Ci.nsIPrefBranch2);
>+      var channel = "default";
>+      var prefName;
>+      var prefValue;
>+
>+      var defaults = prefSvc.QueryInterface(Ci.nsIPrefService)
>+                                      .getDefaultBranch(null);
>+      try {
>+        channel = defaults.getCharPref(PREF_APP_UPDATE_CHANNEL);
>+      } catch (e) {
>+        // use default when pref not found
>+      }

Just use the previously defined getDefaultPref rather than duplicating it all

>+  formatURL: function uf_formatURL(aFormat, aMappings) {
>     var _this = this;
>+    var _overrides = aMappings;

There doesn't seem to be any reason to use _overrides

>+
>     var replacementCallback = function(aMatch, aKey) {
>+      try {
>+        if ( _overrides ) {

Nit: No spaces inside the brackets

>+          return _overrides.getProperty(aKey);
>+        }
>+      } catch (e) {
>+        // excpetion meaans it's not in the override

Nit: typos

>+      }
>+
>       if (aKey in _this._defaults) // supported defaults
>         return _this._defaults[aKey]();
>+
>       Cu.reportError("formatURL: Couldn't find value for key: " + aKey);
>       return aMatch;
>     }
>-    return aFormat.replace(/%([A-Z]+)%/g, replacementCallback);
>+    return aFormat.replace(/%([\w]+)%/g, replacementCallback);

Let's restrict this to at least 3 characters and include matching the '-' character.

>-  formatURLPref: function uf_formatURLPref(aPref) {
>+  formatURLPref: function uf_formatURLPref(aPref, aMappings) {
>     var format = null;
>     var PS = Cc['@mozilla.org/preferences-service;1'].
>              getService(Ci.nsIPrefBranch);
> 
>+

Nit: no need for another new line

	Dave Townsend [:mossop] Reporter
	Comment 9 • 16 years ago

Mook said he was going to take this

	Mook (songbird dead account)
	Comment 10 • 16 years ago

> I think for consistency this should be BUILDTARGET

the with-underscore version seems to be more common in the bits of the codebase that use anything:

http://mxr.mozilla.org/comm-central/search?string=BUILD_*ID%25&regexp=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
http://mxr.mozilla.org/comm-central/search?string=PLATFORM_*VERSION%25&regexp=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
http://mxr.mozilla.org/comm-central/search?string=BUILD_*TARGET%25&regexp=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

> Let's restrict this to at least 3 characters and include matching the '-'
> character.

Due to %OS% and %ID% totally failing the test :D, I made this match 3 or more characters for A-Z0-9_-, or 2 characters if at least one of them is G-Z_- (i.e. at least one is not a hex digit).

That first change means I'll need a patch for mail, the only user of %APPBUILDID%.  I imagine they won't want to check that in, since they're on 191 (and I dunno if this would land there - I seem to always end up with patches that never land :/ )

Please tell me if I missed anything?

	Dave Townsend [:mossop] Reporter
	Comment 12 • 16 years ago

Comment on attachment 353926 [details] [diff] [review]
address some comments, completely ignoring others :p

Need to do another pass, but some initial notes:

>+    getABI: function () {
Name the function as throughout the rest of the component

>+    getDefaultPrefValue: function (aPrefName, aDefaultValue) {
Name the function as throughout the rest of the component

>+      var prefSvc =  Cc['@mozilla.org/preferences-service;1']
>+                       .getService(Ci.nsIPrefService);
>+      var prefValue = (arguments.length > 1 ? aDefaultValue : "default");
Don't like this, always pass in the default.

>+    /**
>+     * Read the update channel from defaults only.  We do this to ensure that
>+     * the channel is tightly coupled with the application and does not apply
>+     * to other instances of the application that may use the same profile.
>+     */
>+    getUpdateChannel: function uf_getUpdateChannel() {
>+      var prefSvc =  Cc['@mozilla.org/preferences-service;1']
>+                       .getService(Ci.nsIPrefBranch2);
>+      var channel = this.getPrefValue(PREF_APP_UPDATE_CHANNEL, "default");
Shouldn't that be getDefaultPrefValue?

>+      var prefName;
>+      var prefValue;
>+
>+      var defaults = prefSvc.QueryInterface(Ci.nsIPrefService)
>+                            .getDefaultBranch(null);
defaults doesn't seem to be used

	Dave Townsend [:mossop] Reporter
	Comment 13 • 15 years ago

Comment on attachment 353926 [details] [diff] [review]
address some comments, completely ignoring others :p

Sorry it has taken me far too long to get to this. Issues in comment 12 apply plus a couple of nits. Also I think we should uri encode everything that replacementCallback returns if that doesn't affect anything existing. I don't think it does.

>+    getUpdateChannel: function uf_getUpdateChannel() {
>+      var prefSvc =  Cc['@mozilla.org/preferences-service;1']
>+                       .getService(Ci.nsIPrefBranch2);

Nit: Here and elsewhere put the dot at the end of the first line and line up getService with Cc.

>+    getOSVersion: function uf_getOSVersion() {
>+      var osVersion = "default";
>+      var sysInfo = Cc["@mozilla.org/system-info;1"]
>+                      .getService(Ci.nsIPropertyBag2);
>+      try {
>+        osVersion = sysInfo.getProperty("name") + " " + sysInfo.getProperty("version");
>+      }
>+      catch (e) {
>+      }
>+
>+      if (osVersion) {
>+        try {
>+          osVersion += " (" + sysInfo.getProperty("secondaryLibrary") + ")";
>+        }
>+        catch (e) {
>+          // Not all platforms have a secondary widget library, so an error is nothing to worry about.
>+        }
>+      }
>+      return encodeURIComponent(osVersion);
>+    }
>   },

This gives a slightly different result to what the update service currently uses. I don't know where sysInfo works but you might end up with "default (secondary)".

>@@ -105,14 +219,15 @@
>     }
> 
>     if (!PS.prefHasUserValue(aPref) &&
>-        /^(data:text\/plain,.+=.+|chrome:\/\/.+\/locale\/.+\.properties)$/.test(format)) {
>+        /^(data:text\/plain,.+=.+|chrome:\/\/.+\/locale\/.+\.properties)$/.test(format))
>+    {

Nit: Brace should be on the previous line.

	Dave Townsend [:mossop] Reporter
	Comment 14 • 15 years ago

I searched the AMO mxr ad could only find 3 add-ons using their own urls through url formatter, only the following keys were in use: %APP% %VERSION% %LOCALE% %OS% %XPCOMABI%. Perhaps unsurprisingly the only add-on that uses more than %APP% and %VERSION% is mine.