We have finalized the URL structure for the most part. For those product URLs that are not in bookmarks.html (in particular the region.properties ones), programmatically inserting currently selected locale should be better than build process or hand-l10n. The selected locale should be gotten via nsIXULChromeRegistry.getSelectedLocale('global') to protect against user agent pref hacking, version and appid probably come from nsIXULAppInfo.

schrep, this is my work in progress, it needs further doing like: - cache some variable values - get more variable defaults, like version, GUID etc, amo URL is a good set of examples. nsIXULAppInfo is your friend.

As discussed on the meeting, we want to unify the URL scheme. Despite the comment in the WIP code, that should be http[s]://%LOCALE%.%SERVICE%.mozilla.[com|org]/%LOCALE%/ where the last LOCALE is optional in general, but used for at least www and addons. In addition to this code, we need to of course make those code places using region.properties right now use this code and firefox.js.

CCing Justin so he can sign-off on URL scheme - this will require setting up a bunch of worldwide DNS entries.

Simon - can you take a look?

Why do you need the locale twice in the URL scheme?

The reason for encoding the locale in the hostname is to allow static mapping of particular locales to local servers. Having the locale in the path just makes thigns a little easier to stage and makes the app more consistent. Having said that Justin is investigating the necessity of the hostname static locale mapping if we have a geo-load balancing solution in the near future. He'll comment here once we've figured it out.

Bug 348568 has been filed to deal with the server-side portion of this bug.

Having discussed this a bit with the team, we think keeping the locale in the hostname is actually a good idea as it allows for maximum flexibility. We can use wildcard DNS to redirect all hostnames to a single IP and still do geo-load balancing as needed but have the ability to force specific locales to certain servers if needed. Once we have the %SERVICE% variable, we can setup the wildcards as needed.

Here's the formatter component, with a utility function in browser.js to ease access for callers. Can y'all take a look and see if this meets the need?

Comment on attachment 233738 [details] [diff] [review] nsURLFormatter WIP #2 >+ * echo formatURL("http://%LOCALE%.mozilla.com/%MYVAR%/", {MYVAR:"test"}); >+ * > http://en-US.mozilla.com/test/ >+ * http[s]://%SERVICE%.%LOCALE%.mozilla.[com|org]/%LOCALE%/[(%[A_Z_]+%)|[a-zA-Z.-_]] >+ * http[s]://%SERVICE%.%LOCALE%.mozilla.[com|org]/%LOCALE%/[(%[A_Z_]+%)|[a-zA-Z.-_]] In all these occasions, the correct format, as specified in comment #2, should be used, so people don't get confused.

Is there a usecase for having this stuff in an xpcom service? It looks like #2 is adding a lot of complexity for a helper that's just used from js, AFAICT. I might have said that we may at one point, but I don't think that's true. Regarding the default vars, that's looks OK. If we're not using a component, we may want to be more cautious about redefining vars etc.

(In reply to comment #10) > In all these occasions, the correct format, as specified in comment #2, should > be used, so people don't get confused. > New patch w/ correct URL example.

The example above formatURL doesn't seem to match the scheme as described in comment 2

(In reply to comment #11) > Is there a usecase for having this stuff in an xpcom service? It looks like #2 > is adding a lot of complexity for a helper that's just used from js, AFAICT. > > I might have said that we may at one point, but I don't think that's true. I also thought it wouldn't have to be a component. However, my first lxr search for usage of strings in region.properties showed nsBrowserContentHandler: http://lxr.mozilla.org/mozilla/source/browser/components/nsBrowserContentHandler.js#414 The minority of usage occurs in components, but having the component and convenience function handles both situations. I tried to keep it simple, but things always seem to get tangly when passing non-scalar values through XPConnect :P > Regarding the default vars, that's looks OK. If we're not using a component, we > may want to be more cautious about redefining vars etc. > Are you talking about caching the default vars? Can you clarify what you mean?

(In reply to comment #13) > The example above formatURL doesn't seem to match the scheme as described in > comment 2 > patch updated to fix example usage for formatURL.

Should you be requesting review or is there more work pending?

(In reply to comment #16) > Should you be requesting review or is there more work pending? > More work still, as the URLs themselves have not actually been replaced yet. I attached the patch to get feedback as to whether the formatting code would do what was needed before moving on. Are we aiming to generate all the URLs in region.properties? For example, app.update.url.details is used in nsUpdateService.js.in which is in toolkit, and is used in other apps (I think).

> > Are we aiming to generate all the URLs in region.properties? For example, > app.update.url.details is used in nsUpdateService.js.in which is in toolkit, > and is used in other apps (I think). > startup.homepage_override_url startup.homepage_welcome_url For sure. app.update.url.details I think is only used if the update.xml doesn't provide a details link. I'm pretty sure we always do that and if this is used by other apps we prob don't want to touch. Preed - do we normaly provide the release notes URL in the updates?

(In reply to comment #18) > startup.homepage_override_url > startup.homepage_welcome_url > > For sure. Ok. Using the new format as stated in comment #2, the formatted startup.homepage_override_url pref will look like this: http://en-US.www.mozilla.com/en-US/firefox/update Is that correct?

(In reply to comment #18) > Preed - do we normaly provide the release notes URL in the updates? Yes. Bug 336338 is ensuring that that details URL that we ship in the patchinfo files points to localized content. That will actually require a patcher2 change, but it should be working for beta 2's updates.

Comment on attachment 233961 [details] [diff] [review] formatter + replacement of the startup.homepage_* prefs nsIURLFormatter should be null-safe for the vars, IMHO, then you can drop the unused 'replace' dict in nsBrowserContentHandler. Only fixing the keys getting should do it. And no '\' in unix/packages-static, IMHO.

(In reply to comment #19) > Ok. Using the new format as stated in comment #2, the formatted > startup.homepage_override_url pref will look like this: > > http://en-US.www.mozilla.com/en-US/firefox/update > > Is that correct? > Yes. Axel and Phil to triple check.

(In reply to comment #23) > > > > http://en-US.www.mozilla.com/en-US/firefox/update > > Yes. Axel and Phil to triple check. It's the right format, but beltzner (in bug 314119) had originally asked for http://en-US.www.mozilla.com/en-US/firefox/%VERSION%/whatsnew/ -- that's what's reflected in the L10N requirements (http://wiki.mozilla.org/Firefox2/L10n_Requirements#Start_Page) It doesn't matter to me; if you want to change the URL from what was picked in bug 314119, you should probably query beltzner.

Yep that seems more right. (In reply to comment #24) > (In reply to comment #23) > > > > > > http://en-US.www.mozilla.com/en-US/firefox/update > > > > Yes. Axel and Phil to triple check. > > It's the right format, but beltzner (in bug 314119) had originally asked for > http://en-US.www.mozilla.com/en-US/firefox/%VERSION%/whatsnew/ -- that's what's > reflected in the L10N requirements > (http://wiki.mozilla.org/Firefox2/L10n_Requirements#Start_Page) > > It doesn't matter to me; if you want to change the URL from what was picked in > bug 314119, you should probably query beltzner. >

I'm not exactly sure if any of these URLs are shared with bookmarks. As for bookmarks, we decided to not use the version in bug 348165, due to the fact that bookmarks outlive the current version. I'm tempted to think that update pages can have the version, but I'd rather have beltzner confirm this.

(In reply to comment #26) > I'm not exactly sure if any of these URLs are shared with bookmarks. As for > bookmarks, we decided to not use the version in bug 348165, due to the fact > that bookmarks outlive the current version. I'm tempted to think that update > pages can have the version, but I'd rather have beltzner confirm this. Couple answers here: First, the "Getting Started" bookmark will no longer point to the same page as the startup.homepage_override_url. The "Getting Started" bookmark will point to a portal (located at /firefox/central/) all about the features of Firefox and the web in general (see bug 348165) and the latter will be a first run experience (see bug 345805). Second, I'd like to use the version number in the startup.homepage_override_url as it will save us some JS-detection redirection which will fail for users who don't have JS enabled. So the outcome of the call to the formatter for Firefox 2 should look like: http://en-US.mozilla.com/en-US/firefox/2.0/firstrun/ for welcome_url* http://en-US.mozilla.com/en-US/firefox/2.0/whatsnew/ for update_url (*Phil: this is a slight change to what we have in the wiki, based on the new bookmark requirements)

(In reply to comment #22) > (From update of attachment 233961 [details] [diff] [review] [edit]) > nsIURLFormatter should be null-safe for the vars, IMHO, then you can drop the > unused 'replace' dict in nsBrowserContentHandler. > Only fixing the keys getting should do it. Any idea how I do that in the IDL? I looked around but didn't find a parameter attribute that allowed it to be optional, or null. > And no '\' in unix/packages-static, IMHO. > fixed > So the outcome of the call to the formatter for Firefox > 2 should look like: > > http://en-US.mozilla.com/en-US/firefox/2.0/firstrun/ for welcome_url* > http://en-US.mozilla.com/en-US/firefox/2.0/whatsnew/ for update_url > fixed

Comment on attachment 234026 [details] [diff] [review] formatter + startup.homepage_* prefs Looks mostly fine, just one question.. nsURLFormatter looks more like a service to me, is there any reason why it isn't used/defined as such? (And probably renamed to nsIURLFormatterService.) I don't really care much either way since it's not performance-critical, especially if there's a reason to keep it a non-service. There's a bit from profile/localstore.rdf that probably doesn't need to be in here. >Index: browser/base/content/browser.js >=================================================================== >RCS file: /cvsroot/mozilla/browser/base/content/browser.js,v >retrieving revision 1.681 >diff -u -8 -p -r1.681 browser.js >--- browser/base/content/browser.js 16 Aug 2006 06:31:27 -0000 1.681 >+++ browser/base/content/browser.js 16 Aug 2006 17:30:28 -0000 >@@ -6922,8 +6922,29 @@ function undoCloseTab(aIndex) { > getService(Ci.nsISessionStore); > if (ss.getClosedTabCount(window) == 0) > return; > ss.undoCloseTab(window, aIndex || 0); > > if (blankTabToRemove) > tabbrowser.removeTab(blankTabToRemove); > } >+ >+/** >+ * Format a URL >+ * eg: >+ * echo formatURL("http://%LOCALE%.%SERVICE%.mozilla.org/%LOCALE%/%MYVAR%/", {SERVICE:"amo", MYVAR:"test"}); >+ * > http://en-US.amo.mozilla.org/en-US/test/ >+ * >+ * Currently supported built-ins are LOCALE, and any value from nsIXULAppInfo, uppercased. >+ */ >+function formatURL(aFormat, aVars, aIsPref) { >+ >+ var repl = Cc["@mozilla.org/dictionary;1"].createInstance(Ci.nsIDictionary); >+ for (var key in aVars) { >+ var val = Cc['@mozilla.org/supports-string;1'].createInstance(Ci.nsISupportsString); >+ val.data = aVars[key]; >+ repl.setValue(key, val); >+ } >+ >+ var formatter = Cc["@mozilla.org/browser/URLFormatter;1"].createInstance(Ci.nsIURLFormatter); ^ why createInstance and not getService? The URLFormatter looks more like a service to me (doesn't seem to have any state). >Index: browser/components/nsBrowserContentHandler.js >=================================================================== >RCS file: /cvsroot/mozilla/browser/components/nsBrowserContentHandler.js,v >retrieving revision 1.24 >diff -u -8 -p -r1.24 nsBrowserContentHandler.js >--- browser/components/nsBrowserContentHandler.js 5 Jul 2006 14:04:36 -0000 1.24 >+++ browser/components/nsBrowserContentHandler.js 16 Aug 2006 17:30:29 -0000 >@@ -401,27 +401,29 @@ var nsBrowserContentHandler = { > > helpInfo : " -browser Open a browser window.\n", > > /* nsIBrowserHandler */ > > get defaultArgs() { > var prefb = Components.classes["@mozilla.org/preferences-service;1"] > .getService(nsIPrefBranch); >+ var formatter = Components.classes["@mozilla.org/browser/URLFormatter;1"] >+ .createInstance(Components.interfaces.nsIURLFormatter); ^ same service question >+ >+/*********************************************************** >+* class definition >+***********************************************************/ >+ >+function nsURLFormatter() { >+}; ^ nit: no semicolon >Index: browser/locales/en-US/profile/localstore.rdf >=================================================================== >RCS file: /cvsroot/mozilla/browser/locales/en-US/profile/localstore.rdf,v >retrieving revision 1.2 >diff -u -8 -p -r1.2 localstore.rdf >--- browser/locales/en-US/profile/localstore.rdf 30 Nov 2004 21:26:12 -0000 1.2 >+++ browser/locales/en-US/profile/localstore.rdf 16 Aug 2006 17:30:30 -0000 >@@ -1,5 +1,21 @@ > <?xml version="1.0"?> >-<RDF:RDF >- xmlns:NC="http://home.netscape.com/NC-rdf#" >- xmlns:RDF="http://www.w3.org/1999/02/22-rdf-syntax-ns#"> >+<RDF:RDF xmlns:NC="http://home.netscape.com/NC-rdf#" >+ xmlns:RDF="http://www.w3.org/1999/02/22-rdf-syntax-ns#"> >+ <RDF:Description RDF:about="chrome://browser/content/browser.xul#sidebar-title" >+ value="" /> >+ <RDF:Description RDF:about="chrome://browser/content/browser.xul"> >+ <NC:persist RDF:resource="chrome://browser/content/browser.xul#main-window"/> >+ <NC:persist RDF:resource="chrome://browser/content/browser.xul#sidebar-box"/> >+ <NC:persist RDF:resource="chrome://browser/content/browser.xul#sidebar-title"/> >+ </RDF:Description> >+ <RDF:Description RDF:about="chrome://browser/content/browser.xul#sidebar-box" >+ sidebarcommand="" >+ width="" >+ src="" /> >+ <RDF:Description RDF:about="chrome://browser/content/browser.xul#main-window" >+ width="994" >+ height="746" >+ sizemode="normal" >+ screenY="0" >+ screenX="10" /> > </RDF:RDF> ^^ this doesn't look related; is it?

(In reply to comment #28) > Created an attachment (id=234026) [edit] > formatter + startup.homepage_* prefs > > (In reply to comment #22) > > (From update of attachment 233961 [details] [diff] [review] [edit] [edit]) > > nsIURLFormatter should be null-safe for the vars, IMHO, then you can drop the > > unused 'replace' dict in nsBrowserContentHandler. > > Only fixing the keys getting should do it. > > Any idea how I do that in the IDL? I looked around but didn't find a parameter > attribute that allowed it to be optional, or null. I'd just comment in the idl that null is OK.

fixes vlad's comments, and passes null instead of empty nsIDictionary.

Comment on attachment 234086 [details] [diff] [review] as going to be checked in as discussed in bug 348076, this is moving to toolkit b/c some of the prefs that need formatting are there.

This is the same code but moved to toolkit. I removed actual URL changes, as they are being consolidated in bug 348076.

adds support a %APP% built-in.

Comment on attachment 234273 [details] [diff] [review] moved to toolkit + support for %APP% value as noted, fix any of the licenses that aren't right (i.e. giving netscape credit) >+{ >+ /** >+ * formatURL - Formats a string URL >+ * >+ * @param aFormat string >+ * @param aVars nsIDictionary - NULL is acceptable if not using local replacements. >+ */ >+ string formatURL(in string aFormat, in nsIDictionary aVars); >+ >+ /** >+ * formatURL - Formats a string URL stored in a pref >+ * >+ * @param aFormat string >+ * @param aVars nsIDictionary - NULL is acceptable if not using local replacements. >+ */ >+ string formatURLPref(in string aPref, in nsIDictionary aVars); >+}; you really want to use AString here, not string >+/*********************************************************** >+* constants >+***********************************************************/ might as well use doxygen-style comment blocks here, but it really is insane that we don't have a better solution >+const CID = Components.ID("{e6156350-2be8-11db-a98b-0800200c9a66}"); >+const CONTRACT_ID = "@mozilla.org/browser/URLFormatterService;1"; >+const CLASS_NAME = "Browser URL Formatter Service"; >+ >+const APPINFO = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo); might as well line these four up to be easier to read >+ var _this = this; >+ var replacer = function(aMatch, aKey) { >+ if (repl[aKey]) // handle vars from caller >+ return repl[aKey]; >+ if (_this._defaults[aKey]) // supported defaults >+ return _this._defaults[aKey](); >+ dump(aKey + " not found"); is this going to cause console spew for apps that don't have substitution set up? I bet it is! (fix now or file a followup to make this pref-enabled, latter is probably better) looking good though!

Comment on attachment 234273 [details] [diff] [review] moved to toolkit + support for %APP% value >+ * Currently supported built-ins are LOCALE, and any value from nsIXULAppInfo, uppercased. Might mention APP here, too.

(In reply to comment #36) > > >+ var _this = this; > >+ var replacer = function(aMatch, aKey) { > >+ if (repl[aKey]) // handle vars from caller > >+ return repl[aKey]; > >+ if (_this._defaults[aKey]) // supported defaults > >+ return _this._defaults[aKey](); > >+ dump(aKey + " not found"); > > is this going to cause console spew for apps that don't have substitution set > up? I bet it is! (fix now or file a followup to make this pref-enabled, latter > is probably better) Hm, I think this is not a problem. For apps that don't have substitution set up, there will be no matches in the URLs they pass in, so replacer() will never get called.

Comment on attachment 234376 [details] [diff] [review] as checked in on trunk Drivers: This patch adds the code required to allow programmatic generation of MoCo/Fo URLs to ease localization. Risk is low: it just adds the component, doesn't actually do anything.

Comment on attachment 234376 [details] [diff] [review] as checked in on trunk >+EXTRA_PP_COMPONENTS = \ >+ nsURLFormatter.js \ I know your build machine is fast, but that's no excuse to preprocess a file with no preprocessor directives. >+const APPINFO = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo); This isn't nice, I think it depends on app info being C++ and therefore getting registered first. >+ var val = XPCNativeWrapper(aVars.getValue(keys[i]).QueryInterface(Ci.nsISupportsString)); I'm intrigued as to what you need the XPCNativeWrapper to protect yourself against. Also, I don't see any explicit access to .data so you're confusingly relying on an implicit .toString() somewhere. >+ if (repl[aKey]) // handle vars from caller >+ return repl[aKey]; >+ if (_this._defaults[aKey]) // supported defaults >+ return _this._defaults[aKey](); It's a good thing boolean strict warnings were disabled ;-) >+ _firstTime: true, unused?

Comment on attachment 234376 [details] [diff] [review] as checked in on trunk Dietrich; can you address Neil's comments and get re-review and re-nom?

(In reply to comment #40) > >+ if (repl[aKey]) // handle vars from caller > >+ return repl[aKey]; > >+ if (_this._defaults[aKey]) // supported defaults > >+ return _this._defaults[aKey](); > It's a good thing boolean strict warnings were disabled ;-) I'm not sure what you mean. Is that a build option, or are you referring to the javascript.options.strict pref (which I do have set)?

Most comments are addressed in this patch. I need an answer to comment #42 in order to fix that. Another change in this patch is that %APP% has spaces removed.

(In reply to comment #42) >>It's a good thing boolean strict warnings were disabled ;-) >I'm not sure what you mean. Is that a build option, or are you referring to the >javascript.options.strict pref (which I do have set)? Neither, I was just noticing that in previous releases of Gecko's JS engine the use of if (foo[bar]) would emit a warning if bar wasn't a property of foo.

Comment on attachment 234481 [details] [diff] [review] neil comments addressed > function nsURLFormatterService() { > } > > nsURLFormatterService.prototype = { > > /** > * Built-in values > */ > _defaults: { >+ _appInfo: Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo), Actually that's not quite what I meant; try setting it in a constructor e.g. function nsURLFormatterService() { this._defaults._appInfo = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo); }

Just to drop more idea, I would probably use a getter, get appInfo() { if (!this._appInfo) this._appInfo = ...; return this._appInfo; }

(In reply to comment #46) > Just to drop more idea, I would probably use a getter, > get appInfo() { > if (!this._appInfo) > this._appInfo = ...; > return this._appInfo; > } > checked in on trunk w/ this change.

This patch is the original trunk patch plus the changes from Neil's review.

Comment on attachment 234540 [details] [diff] [review] branch patch a=beltzner on behalf of drivers, please land on MOZILLA_1_8_BRANCH and mark fixed1.8.1

Comment on attachment 234562 [details] [diff] [review] fixes packages files grr! land both branches please!

When adding such an additional component to toolkit and make some toolkit functions rely on it, please correct packaging of other toolkit apps in the future to also include it, or at least notify platform users that they might need to do that. toolkit/ is not Firefox-specific, you know...

This new code doesn't allow the administrator to prevent the welcome and override pages from being shown (the old code did, though it may have been an unintentional feature). Disabling these pages is desirable when using Firefox on a intranet without direct internet access. I've attached a patch that would allow this. To make use of this feature, the administrator would add one or both of the following lines to defaults/pref/all.js: pref("startup.homepage_override_url", "ignore"); pref("startup.homepage_welcome_url", "ignore"); ... or should I be submitting a new bug? (Apologies if the patch isn't formatted correctly; I have yet to learn CVS.)

Comment on attachment 234975 [details] [diff] [review] patch to allow disabling homepage override Yes, please do file a separate bug for this.

(In reply to comment #54) Bug 351260 filed. Thanks.