Last Comment Bug 525882 - Don't hardcode anything in application.ini files
: Don't hardcode anything in application.ini files
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 6
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-02 08:40 PST by Mike Hommey [:glandium]
Modified: 2011-05-11 23:28 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposal for Firefox (2.02 KB, patch)
2009-11-02 08:40 PST, Mike Hommey [:glandium]
gavin.sharp: review-
Details | Diff | Review
Updated proposal for Firefox (1.99 KB, patch)
2009-11-03 03:59 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
Updated patch (4.27 KB, patch)
2010-02-17 02:03 PST, Mike Hommey [:glandium]
gavin.sharp: review-
Details | Diff | Review
Patch v4 (5.89 KB, patch)
2010-04-28 01:04 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Review
Don't hardcode anything in application.ini files (5.83 KB, patch)
2011-05-07 01:07 PDT, Mike Hommey [:glandium]
gavin.sharp: review+
khuey: review+
Details | Diff | Review

Description Mike Hommey [:glandium] 2009-11-02 08:40:00 PST
Created attachment 409704 [details] [diff] [review]
Proposal for Firefox

It would be nice if application.ini files would be branding-aware, and for that, there would need to be some basic changes and additions in the build process.

What I propose in the attached patch is to add two variables, MOZ_APP_VENDOR and MOZ_APP_PROFILE, that would correspond to application.ini Vendor and Profile settings. The MOZ_APP_VENDOR would be set to Mozilla in confvars.sh, and MOZ_APP_PROFILE would be left for branding to set if deemed necessary (for example, in Debian, we set the profile to mozilla/firefox for iceweasel).

Additionally, the application name set in application.ini would be set from MOZ_APP_DISPLAYNAME.

The patch is only for Firefox, but the same could be applied on Seamonkey, Thunderbird, and others. I can provide patches for these, too, but I'd like some feedback, first.
Comment 1 Mark Banner (:standard8) 2009-11-02 08:52:39 PST
If you change name= to use MOZ_APP_DISPLAYNAME won't that mess up extensions on development versions that could do:

if (Application.name == 'Firefox') ...

I can't remember if it would affect anything else, but maybe using MOZ_APP_NAME would be the better option.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-11-03 00:23:02 PST
Indeed, I don't think we want the application.ini Name to vary based on branding, which is what would happen if it's backed by MOZ_APP_DISPLAYNAME.

MOZ_APP_NAME would be better, but still not quite right because it would change our current ID from "Firefox" to "firefox". Perhaps we could use MOZ_APP_NAME but force it to be title case (capitalized first letter)?

The rest of the patch looks OK.
Comment 3 Mike Hommey [:glandium] 2009-11-03 00:36:04 PST
(In reply to comment #1)
> If you change name= to use MOZ_APP_DISPLAYNAME won't that mess up extensions on
> development versions that could do:
> 
> if (Application.name == 'Firefox') ...

Are there extensions doing this ?

(In reply to comment #2)
> Indeed, I don't think we want the application.ini Name to vary based on
> branding, which is what would happen if it's backed by MOZ_APP_DISPLAYNAME.

FWIW, we have been doing this for Iceweasel for a while, now (i.e. changing application.ini Name)

> MOZ_APP_NAME would be better, but still not quite right because it would change
> our current ID from "Firefox" to "firefox". Perhaps we could use MOZ_APP_NAME
> but force it to be title case (capitalized first letter)?

That would be a problem for SeaMonkey, which is camel cased (which is actually the reason I used the displayname)
Comment 4 Mike Hommey [:glandium] 2009-11-03 00:46:39 PST
(In reply to comment #3)
> (In reply to comment #2)
> > Indeed, I don't think we want the application.ini Name to vary based on
> > branding, which is what would happen if it's backed by MOZ_APP_DISPLAYNAME.
> 
> FWIW, we have been doing this for Iceweasel for a while, now (i.e. changing
> application.ini Name)

Or maybe are you referring to Minefield using the Firefox application name ?
Comment 5 Mark Banner (:standard8) 2009-11-03 02:03:49 PST
(In reply to comment #3)
> (In reply to comment #2)
> > Indeed, I don't think we want the application.ini Name to vary based on
> > branding, which is what would happen if it's backed by MOZ_APP_DISPLAYNAME.
> 
> FWIW, we have been doing this for Iceweasel for a while, now (i.e. changing
> application.ini Name)

That may work if you're just changing it for *one* application name for a one-time only change. We're talking about problems with different brandings of the same application.

Based on a few suspicions, I just tried simulating a change of name in my application.ini to be the branded name in the case of using 'MOZ_APP_DISPLAYNAME' on trunk Thunderbird ('Shredder') builds:

- Profiles were not detected in the original thunderbird profile directory; they are in one titled "shredder" instead.
- URLs are broken, they have Shredder instead of Thunderbird. This breaks things such as automatic updates, amo searching, start pages etc.

So using MOZ_APP_DISPLAYNAME will break our non-branded builds and the way we can currently use them to interact with the same profiles/pages as our branded builds.
Comment 6 Mike Hommey [:glandium] 2009-11-03 02:10:21 PST
(In reply to comment #5)
> So using MOZ_APP_DISPLAYNAME will break our non-branded builds and the way we
> can currently use them to interact with the same profiles/pages as our branded
> builds.

For profiles, there is a Profile entry in application.ini (which is why I added MOZ_APP_PROFILE).

For the rest, that was the point of comment #4.

The problem is that there are 2 kinds of branding. Full rebranding, such as Icecat, Iceweasel, Iceape, Icedove. And partial rebranding, for Minefield, Shredder, etc.

Maybe we should add something like MOZ_APP_REALNAME for partial rebrandings and use it instead of DISPLAYNAME when set.
Comment 7 Mark Banner (:standard8) 2009-11-03 02:29:08 PST
(In reply to comment #6)
> (In reply to comment #5)
> > So using MOZ_APP_DISPLAYNAME will break our non-branded builds and the way we
> > can currently use them to interact with the same profiles/pages as our branded
> > builds.
> 
> For profiles, there is a Profile entry in application.ini (which is why I added
> MOZ_APP_PROFILE).

You didn't define MOZ_APP_PROFILE though, so applying that patch to a non-branded Firefox would have broken it, which is what my comments were based on.

I'm also pretty sure that using that method won't work for Thunderbird because of the unfortunate ways it currently locates profiles (although I'd need to do some checking for that).

> Maybe we should add something like MOZ_APP_REALNAME for partial rebrandings and
> use it instead of DISPLAYNAME when set.

It depends on how you want to do your full rebranding. I would have said just requiring MOZ_APP_REALNAME (or maybe call it MOZ_APP_BASENAME) to be set all the time would be fine - for apps in hmo they could just set that in confvars.sh and I guess for full rebranding in different directories you could probably override that in the configure.sh file.
Comment 8 Mike Hommey [:glandium] 2009-11-03 02:49:11 PST
(In reply to comment #7)
> You didn't define MOZ_APP_PROFILE though, so applying that patch to a
> non-branded Firefox would have broken it, which is what my comments were based
> on.

There is a simple reason for that: I didn't have Minefield and Shredder in mind when I wrote the patch. Surely, the appropriate patches to the nightly/confvars.sh files could be added.

> I'm also pretty sure that using that method won't work for Thunderbird because
> of the unfortunate ways it currently locates profiles (although I'd need to do
> some checking for that).

Now, that is a rather bad news.
 
> It depends on how you want to do your full rebranding. I would have said just
> requiring MOZ_APP_REALNAME (or maybe call it MOZ_APP_BASENAME) to be set all
> the time would be fine - for apps in hmo they could just set that in
> confvars.sh and I guess for full rebranding in different directories you could
> probably override that in the configure.sh file.

Sounds fair. So, to summarize:
Add MOZ_APP_BASENAME in all application confvars.sh to replace the static application.ini value.
Add MOZ_APP_VENDOR in all application confvars.sh to replace the static application.ini value.
Both of the above can freely be overriden by branding's configure.sh.
Allow brandings to add a MOZ_APP_PROFILE that would set Profile in application.ini.

I think this would work for all the current usecases.
Comment 9 Mike Hommey [:glandium] 2009-11-03 03:59:02 PST
Created attachment 409905 [details] [diff] [review]
Updated proposal for Firefox

This implements what is described in comment 8.
Comment 10 Mike Hommey [:glandium] 2009-11-03 04:07:02 PST
I just about something more: maybe we could also set DISPLAYNAME to BASENAME if it isn't set, which would allow to not need a configure.sh in other-licenses/branding/firefox, and would allow to only have to set BASENAME for full rebrands.
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-11-03 15:15:41 PST
Comment on attachment 409905 [details] [diff] [review]
Updated proposal for Firefox

>diff --git a/browser/app/Makefile.in b/browser/app/Makefile.in

>+DEFINES += -DAPP_NAME="$(MOZ_APP_BASENAME)" \

Could you use the full MOZ_* names here, rather than shortening to APP_NAME/APP_VENDOR? Having the name change between the DEFINE/Makefile variable makes it confusing when you're grepping for these, and APP_NAME could be confused with MOZ_APP_NAME.

>diff --git a/browser/app/application.ini b/browser/app/application.ini

>-Version=@APP_VERSION@

I'm assuming that's unintentional? Otherwise this looks pretty good. Ted should also review the changes to the global configure.in.

Would be very nice to add some comments (in configure.in?) explaining how these different variables are meant to be used - it's already rather confusing to sort through which variable is used where, and now we're adding more. Something like:

MOZ_APP_DISPLAYNAME - Varies based on branding (e.g. "Firefox", "Minefield", "IceWeasel"), used in user-visible fields (DLL properties, Mac Bundle name, Updater, Installer)

MOZ_APP_NAME - Can vary based on branding, but typically stays consistent for multiple branded versions of a given application (e.g. Minefield and Firefox both use "Firefox"). Used for application.ini's "Name" field, which controls profile location, and various system integration hooks (Unix remoting, Windows MessageWindow name, etc.)

MOZ_APP_VENDOR - Varies based on branding. Used for application.ini's "Vendor" field, which also impacts profile location and user-visible fields.

r=me with those comments addressed.

We should probably also get a bug on file to audit uses of MOZ_APP_NAME vs. MOZ_APP_DISPLAYNAME - a quick look makes me think that some places are using one when they really want the other.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-11-03 15:17:31 PST
(In reply to comment #11)
> MOZ_APP_NAME

Meant MOZ_APP_BASENAME here, obviously. Description of MOZ_APP_NAME would be good too, though!
Comment 13 Mike Hommey [:glandium] 2009-11-03 23:24:51 PST
(In reply to comment #11)
> (From update of attachment 409905 [details] [diff] [review])
> >diff --git a/browser/app/Makefile.in b/browser/app/Makefile.in
> 
> >+DEFINES += -DAPP_NAME="$(MOZ_APP_BASENAME)" \
> 
> Could you use the full MOZ_* names here, rather than shortening to
> APP_NAME/APP_VENDOR? Having the name change between the DEFINE/Makefile
> variable makes it confusing when you're grepping for these, and APP_NAME could
> be confused with MOZ_APP_NAME.
> 
> >diff --git a/browser/app/application.ini b/browser/app/application.ini
> 
> >-Version=@APP_VERSION@
> 
> I'm assuming that's unintentional?

Oops, yes.

> Would be very nice to add some comments (in configure.in?) explaining how these
> different variables are meant to be used - it's already rather confusing to
> sort through which variable is used where, and now we're adding more. Something
> like:

Would you have a suggestion as to where in configure.in to put this information ?

As a side note, what do you think about comment 10 ?
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-11-06 00:16:41 PST
(In reply to comment #10)
> I just about something more: maybe we could also set DISPLAYNAME to BASENAME if
> it isn't set, which would allow to not need a configure.sh in
> other-licenses/branding/firefox, and would allow to only have to set BASENAME
> for full rebrands.

I don't understand why it would allow the second part (only having to set BASENAME for full rebrands)... either way I think it's probably better to just keep things simple and avoid introducing dependencies here.
Comment 15 Mike Hommey [:glandium] 2009-11-06 00:25:42 PST
(In reply to comment #14)
> I don't understand why it would allow the second part (only having to set
> BASENAME for full rebrands)... either way I think it's probably better to just
> keep things simple and avoid introducing dependencies here.

A partial rebrand (like Minefield) would be one where DISPLAYNAME and BASENAME are different.
A full rebrand would be one where DISPLAYNAME and BASENAME are equal. If you only set BASENAME and DISPLAYNAME takes its value when not set, then you don't need to set both.

I also thought APP_NAME could be set from lowercasing the BASENAME.

So, to sumarize, we would have the following:
- MOZ_APP_VENDOR to set the vendor
- MOZ_APP_BASENAME to set the underlying application name (that would be Firefox for Minefield)
- MOZ_APP_DISPLAYNAME to optionally set (that would be Minefield for Minefield)
- MOZ_APP_NAME would be automatically set to lowercase(MOZ_APP_BASENAME).

I think /this/ would be simpler.
Comment 16 Reed Loden [:reed] (use needinfo?) 2010-02-12 18:42:59 PST
Mike, can you attach a new patch here that addresses gavin's comments?
Comment 17 Mike Hommey [:glandium] 2010-02-12 23:04:10 PST
Actually I was expecting feedback for comment 15, which is IMHO a straightforward solution.
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-02-16 22:30:49 PST
The proposal in comment 15 sounds reasonable to me at first glance. It's hard to evaluate all of the implications without investing more time into it, though, and it's probably premature to do that before there's a proposed patch.
Comment 19 Mike Hommey [:glandium] 2010-02-17 02:03:59 PST
Created attachment 427293 [details] [diff] [review]
Updated patch

Implements the proposal from comment 15, and includes comments as per comment 11.

Please note the change for the --enable-static error message is done because it lies between the include of the application's confvars.sh and the branding's configure.sh, which means we'd have to possibly set MOZ_APP_NAME twice to make it good. OTOH, I think MOZ_BUILD_APP, which is set by then, is just as good.
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-04-27 13:27:35 PDT
Comment on attachment 427293 [details] [diff] [review]
Updated patch

>diff -r d217f3c28332 browser/app/Makefile.in

>+DEFINES += -DMOZ_APP_NAME="$(MOZ_APP_BASENAME)" \

This is confusing (MOZ_APP_NAME being set to MOZ_APP_BASENAME's value). Should just use MOZ_APP_BASENAME across the board.

>diff -r d217f3c28332 configure.in

>+# The following variables are available to branding and application
>+# configuration ($BRANDING/configure.sh and $APPLICATION/confvars.sh):
>+# - MOZ_APP_VENDOR (mandatory): Used for application.ini's "Vendor" field,
>+# which also impacts profile location and user-visible fields.

Technically this isn't actually "mandatory", since only Firefox is using it at the moment. Other apps don't specify it in their confvars.sh. We should probably fix that, but I suppose it can be done separately...

>+# - MOZ_APP_BASENAME (mandatory): Typically stays consistent for multiple
>+# branded versions of a given application (e.g. Minefield and Firefox both
>+# use "Firefox"), but may vary for full rebrandings (e.g. Iceweasel). Used
>+# for application.ini's "Name" field, which controls profile location in
>+# the absence of a "Profile" field (see below), and various system
>+# integration hooks (Unix remoting, Windows MessageWindow name, etc.)

Same here - in this case perhaps we should just say that apps must specify either MOZ_APP_BASENAME or MOZ_APP_NAME.

>+# - MOZ_APP_NAME (optional): Used for e.g. the binary program file name,
>+# it is set, by default, to a lowercase form of MOZ_APP_BASENAME.

". If not set, defaults to a lowercase ..."

This patch break my builds, because MOZ_APP_NAME ends up with an empty value in autoconk.mk. I didn't get a chance to investigate why exactly. r- because of that, but it looks pretty good otherwise.
Comment 21 Mike Hommey [:glandium] 2010-04-28 01:04:08 PDT
Created attachment 442035 [details] [diff] [review]
Patch v4

> This is confusing (MOZ_APP_NAME being set to MOZ_APP_BASENAME's value). Should
> just use MOZ_APP_BASENAME across the board.

done

> Technically this isn't actually "mandatory", since only Firefox is using it at
> the moment. Other apps don't specify it in their confvars.sh. We should
> probably fix that, but I suppose it can be done separately...

I just removed the "mandatory" and "optional".

> This patch break my builds, because MOZ_APP_NAME ends up with an empty value in
> autoconk.mk. I didn't get a chance to investigate why exactly. r- because of
> that, but it looks pretty good otherwise.

There was a mistake in the tests added to configure.in, that were treated as m4 syntax and ended up in a wrong form in the resulting configure.

There was also a hunk that doesn't apply anymore, so this patch is against latest m-c.
Comment 22 dwitte@gmail.com 2010-08-30 10:34:30 PDT
Over in bug 591573 comment 27 and bug 591573 comment 28, it's been expressed that having separate vendor and profile settings in application.ini is desirable. This would allow distros to claim themselves as vendor for products they ship and support (i.e. Firefox or IceWeasel), but not for other third-party XULRunner apps. Just to clarify, is that what's being proposed here?

I take it the 'Vendor' field is what's accessible via nsIXULAppInfo, in which case we can stick that in about:support and achieve great justice. (How does one access 'Profile'?)
Comment 23 Ted Mielczarek [:ted.mielczarek] 2010-08-30 11:03:30 PDT
The Vendor field is in fact propogated to nsIXULAppInfo, and also used to locate the profile directory (and in the Crash Reporter title, FWIW). The Profile field is stored in the nsXREAppData struct, but apparently not exposed via script:
http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXULAppAPI.h#138
(I believe Songbird is using this, currently.)
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-06 12:51:49 PDT
Comment on attachment 442035 [details] [diff] [review]
Patch v4

This has bitrotten, but it looks good - I'll review an updated version quickly.

I'd like to avoid adding too much magic, so I think you should remove the fallback behavior for MOZ_APP_DISPLAYNAME. MOZ_APP_NAME is kind of different I guess - it would be nice to remove it entirely at some point (or rename it MOZ_APP_BASENAME_LOWER), so I suppose you can leave that fallback behavior.
Comment 25 Mike Hommey [:glandium] 2011-05-07 01:07:54 PDT
Created attachment 530815 [details] [diff] [review]
Don't hardcode anything in application.ini files
Comment 26 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-09 13:14:04 PDT
So out of curiosity, what is your plan for iceweasel with this patch? (i.e. values of MOZ_APP_BASENAME, MOZ_APP_VENDOR, MOZ_APP_PROFILE, MOZ_APP_NAME)
Comment 27 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-09 13:49:16 PDT
Comment on attachment 530815 [details] [diff] [review]
Don't hardcode anything in application.ini files

Probably wouldn't hurt for khuey to give this a once-over too.

(mostly unrelated observation: isn't the use of MOZ_APP_NAME at http://hg.mozilla.org/mozilla-central/annotate/0221eb8660f4//configure.in#l330 likely to be broken, since MOZ_APP_NAME won't ever really be defined then?)
Comment 28 Mike Hommey [:glandium] 2011-05-09 22:53:37 PDT
(In reply to comment #26)
> So out of curiosity, what is your plan for iceweasel with this patch? (i.e.
> values of MOZ_APP_BASENAME, MOZ_APP_VENDOR, MOZ_APP_PROFILE, MOZ_APP_NAME)

I've actually been using previous iteration for a while in Iceweasel, and Iceweasel branding contains the following in configure.sh:
MOZ_APP_BASENAME="Iceweasel"
MOZ_APP_PROFILE=mozilla/firefox

(I've never bothered to change the vendor. Not sure it would make any sense to do so)
Comment 29 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-10 09:13:06 PDT
Comment on attachment 530815 [details] [diff] [review]
Don't hardcode anything in application.ini files

Everything here looks reasonable to me.

r=me
Comment 30 Mike Hommey [:glandium] 2011-05-11 23:28:29 PDT
http://hg.mozilla.org/mozilla-central/rev/9da7c485ae72

Note You need to log in before you can comment on or make changes to this bug.