735710 - Error message ignores internal branding and always says "Firefox"

Status: RESOLVED INCOMPLETE

(Toolkit :: Startup and Profile System, defect)

Description

[Mike Kaply [:mkaply]]

When your profile is locked, you receive an error message that says

Firefox is already running, but is not responding. To open a new window, you must first close the existing Firefox process, or restart your system.

This message always says "Firefox" regardless of the values in brand.properties.

The code is here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#1705

It shouldn't be using gAppData, it should be using the brand stuff.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Use brand.properties instead of appName

Using brand.properties.

brand.properties should always exist in this case.

Comment 2

[Mike Kaply [:mkaply]]

Attachment: Patch without extra code

Previous patch had extraneous code in it

Comment 3

[Mike Hommey [:glandium]]

I don't think this change is required.

If gAppData says Firefox, then it means plenty other things relying on it (such as anything getting the info from nsIXULAppInfo) will say Firefox. If brand.properties is not saying the same as gAppData, then you shot yourself in the foot by changing brand.properties without giving an adequate application.ini.

Comment 4

[Mike Hommey [:glandium]]

Plus, not all applications have a branding. Your patch would fail on some simple xulrunner apps.

Comment 5

[Mike Hommey [:glandium]]

Mmmm actually, there's also the issue of some locales using a localized brand name... but then, the locale is probably not even set when showing these messages. (which makes the localization for these strings effectively never used)

Comment 6

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #5)
> Mmmm actually, there's also the issue of some locales using a localized
> brand name... but then, the locale is probably not even set when showing
> these messages. (which makes the localization for these strings effectively
> never used)

Which is not true on localized builds (as opposed to build with l10n xpis installed, which i was thinking about).

So all in all, the patch *is* required, but there should be a fallback to gAppData.

Comment 7

[Mike Kaply [:mkaply]]

This code is in toolkit, so the presence of brand.properties is a requirement (at least that's my understanding).

If you do a search on brand.properties in mxr, you'll see quite a few places that assume the presence of a brand bundle.

Also, there should be zero code that requires that gAppData and brand.properties match (and I hope there is none). The two are completely unrelated.

For Nightly, you want every visual reference to the user to say Aurora, but internally in all ways, it should behave like Firefox (including gAppData).

Comment 8

[Mike Kaply [:mkaply]]

> For Nightly, you want every visual reference to the user to say Aurora,

 For Nightly, you want every visual reference to the user to say Nightly, for Aurora it should say Aurora.

Sorry, combined two thoughts into one sentence.

Comment 9

[Mike Hommey [:glandium]]

(In reply to Michael Kaply (mkaply) from comment #7)
> This code is in toolkit, so the presence of brand.properties is a
> requirement (at least that's my understanding).

I don't think it is. But if it is, then that should be documented.

Comment 10

[Dave Townsend [:mossop]]

(In reply to Mike Hommey [:glandium] from comment #9)
> (In reply to Michael Kaply (mkaply) from comment #7)
> > This code is in toolkit, so the presence of brand.properties is a
> > requirement (at least that's my understanding).
> 
> I don't think it is. But if it is, then that should be documented.

It's kind of a soft requirement I guess. Plenty of stuff depends on its presence: https://developer.mozilla.org/en/XULRunner_tips#Branding

Comment 11

[Mike Hommey [:glandium]]

(In reply to Dave Townsend (:Mossop) from comment #10)
> It's kind of a soft requirement I guess. Plenty of stuff depends on its
> presence: https://developer.mozilla.org/en/XULRunner_tips#Branding

But this plenty of stuff is not mandatory for xulrunner apps, is it?

Comment 12

[:Gavin Sharp [email: [e-mail]]]

It depends on what your XULRunner app does. If it shows web content, I think there are parts of gecko that depend on it (e.g. network error pages, though those have some fallback). In any case, it's not particularly difficult to add, so we should generally recommend that it be present. Some people might still decide that it doesn't affect anything they care about and omit it.

Comment 13

[Mike Kaply [:mkaply]]

> In any case, it's not particularly difficult to add, so we should generally recommend that it be present. 

Agreed. I'll submit a new patch with fallback.

Comment 14

[Mike Hommey [:glandium]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #12)
> It depends on what your XULRunner app does. If it shows web content, I think
> there are parts of gecko that depend on it (e.g. network error pages, though
> those have some fallback).

As a matter of fact, network error pages don't depend on branding. The xulrunner-provided ones use generic wording ("the application"). The overrides for Firefox do hardcode "Firefox", which is a problem for rebranding (bug 336029).

> In any case, it's not particularly difficult to
> add, so we should generally recommend that it be present. Some people might
> still decide that it doesn't affect anything they care about and omit it.

In the present case, it's still better to have a fallback if there is no branding, because the alternative is the application failing to start *and* not presenting the user any error message.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mike Hommey [:glandium] from comment #14)
> As a matter of fact, network error pages don't depend on branding.

I was thinking of:
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10825

IIRC there was a patch somewhere to extend that trick to other error page strings.

I don't really have a problem with making lack of branding fatal, given that it should simplify a bunch of code and avoid arguments about what is/isn't "supported".

Comment 16

[Mike Kaply [:mkaply]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #15)
> I was thinking of:
> http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#10825

That code looks broke. It's only using the fallback if the query of the string from the brand bundle fails, not if getting the brand bundle itself fails.

So it would still fail if there was no brand.properties.

Comment 17

[Mike Kaply [:mkaply]]

I did some investigation into this and honestly, I'll have to punt on it.

My Mozilla string fu is old and I have no idea how to get the kind of strings we need into the const PRUnichar.

Any advice would be appreciated.

Comment 18

[Mike Kaply [:mkaply]]

I'm going to resolve this incomplete.

1. I don't think it matters
2. If you really want to change this message, override the values in application.ini and use that.

Plus with removal of distribution/bundles, you can't easily override brand.properties at startup anyway.

I don't think it is worth fixing.