Axel Hecht [:Pike] Reporter
	Description • 18 years ago

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.

	Axel Hecht [:Pike] Reporter
	Comment 1 • 18 years ago

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.

	Axel Hecht [:Pike] Reporter
	Comment 2 • 18 years ago

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.

	Mike Schroepfer
	Comment 3 • 18 years ago

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

	Mike Schroepfer
	Comment 4 • 18 years ago

Simon - can you take a look?

	Reed Loden [:reed]
	Comment 5 • 18 years ago

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

	Mike Schroepfer
	Comment 6 • 18 years ago

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.

	Reed Loden [:reed]
	Comment 7 • 18 years ago

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

	Justin Fitzhugh
	Comment 8 • 18 years ago

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.

	Dietrich Ayala (:dietrich) Assignee
	Comment 9 • 18 years ago

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?

	Reed Loden [:reed]
	Comment 10 • 18 years ago

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.

	Axel Hecht [:Pike] Reporter
	Comment 11 • 18 years ago

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.

	Dietrich Ayala (:dietrich) Assignee
	Comment 12 • 18 years ago

(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.

	Mike Schroepfer
	Comment 13 • 18 years ago

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

	Dietrich Ayala (:dietrich) Assignee
	Comment 14 • 18 years ago

(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?

	Dietrich Ayala (:dietrich) Assignee
	Comment 15 • 18 years ago

(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.

	Mike Schroepfer
	Comment 16 • 18 years ago

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

	Dietrich Ayala (:dietrich) Assignee
	Comment 17 • 18 years ago

(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).

	Mike Schroepfer
	Comment 18 • 18 years ago

> 
> 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?

	Dietrich Ayala (:dietrich) Assignee
	Comment 19 • 18 years ago

(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?

	J. Paul Reed [:preed]
	Comment 21 • 18 years ago

(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.

	Axel Hecht [:Pike] Reporter
	Comment 22 • 18 years ago

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.

	Mike Schroepfer
	Comment 23 • 18 years ago

(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.

	Phil Schwan
	Comment 24 • 18 years ago

(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.

	Mike Schroepfer
	Comment 25 • 18 years ago

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.
> 

	Axel Hecht [:Pike] Reporter
	Comment 26 • 18 years ago

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.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 27 • 18 years ago

(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)

	Dietrich Ayala (:dietrich) Assignee
	Comment 28 • 18 years ago

(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

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 29 • 18 years ago

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?

	Axel Hecht [:Pike] Reporter
	Comment 30 • 18 years ago

(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.

	Dietrich Ayala (:dietrich) Assignee
	Comment 31 • 18 years ago

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

	Dietrich Ayala (:dietrich) Assignee
	Comment 32 • 18 years ago

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.

	Dietrich Ayala (:dietrich) Assignee
	Comment 33 • 18 years ago

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

	Dietrich Ayala (:dietrich) Assignee
	Comment 34 • 18 years ago

adds support a %APP% built-in.

	Mike Connor [:mconnor]
	Comment 36 • 18 years ago

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!

	Reed Loden [:reed]
	Comment 37 • 18 years ago

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.

	Dietrich Ayala (:dietrich) Assignee
	Comment 38 • 18 years ago

(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.

	Dietrich Ayala (:dietrich) Assignee
	Comment 39 • 18 years ago

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.

	neil@parkwaycc.co.uk
	Comment 40 • 18 years ago

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?

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 41 • 18 years ago

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?

	Dietrich Ayala (:dietrich) Assignee
	Comment 42 • 18 years ago

(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)?

	Dietrich Ayala (:dietrich) Assignee
	Comment 43 • 18 years ago

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.

	neil@parkwaycc.co.uk
	Comment 44 • 18 years ago

(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.

	neil@parkwaycc.co.uk
	Comment 45 • 18 years ago

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);
}

	Axel Hecht [:Pike] Reporter
	Comment 46 • 18 years ago

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

	Dietrich Ayala (:dietrich) Assignee
	Comment 47 • 18 years ago

(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.

	Dietrich Ayala (:dietrich) Assignee
	Comment 48 • 18 years ago

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

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 49 • 18 years ago

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

	Mike Connor [:mconnor]
	Comment 51 • 18 years ago

Comment on attachment 234562 [details] [diff] [review]
fixes packages files

grr!

land both branches please!

	Robert Kaiser
	Comment 52 • 18 years ago

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...

	Harry Johnston
	Comment 53 • 18 years ago

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.)

	Adam Guthrie
	Comment 54 • 18 years ago

Comment on attachment 234975 [details] [diff] [review]
patch to allow disabling homepage override

Yes, please do file a separate bug for this.

	Harry Johnston
	Comment 55 • 18 years ago

(In reply to comment #54)

Bug 351260 filed.  Thanks.