120438 - Text is not diplayed for the simple MAPI preference checkbox

Status: VERIFIED FIXED

(MailNews Core :: Simple MAPI, defect, P1)

Description

[Srilatha Moturi]

For the preference "Use Netscape Mail as the default mail application" 
only the checkbox is displayed and there is no text.

Comment 1

[Srilatha Moturi]

I believe the text is not being displayed because the pref-mailnewsOverlay.dtd 
file which was part of messenger-mapi package, was moved from en-US.jar to 
en-win.jar.

Comment 2

[Srilatha Moturi]

Attachment: patch v1

register chrome messenger-mapi from en-win.jar

Comment 3

[Sean Su]

Comment on attachment 65349 [details] [diff] [review]
patch v1

the patch works for me.  r=ssu

Comment 4

[Srilatha Moturi]

ccing dveditz for sr

Comment 5

[Daniel Veditz [:dveditz]]

I am giving sr=dveditz for this patch to move the messenger-mapi chrome, but I
note that registering this component is *only* in the windows version of this
script when tao and the i18n team worked very hard to make the locale installers
XP (note the code to figure out which platform they're on, and to always ship
all three platform files).

Tao: suggestions needed here. registerChrome() call errors are ignored so
there's no particular problem in adding this call in all versions of this
script. Better, the call could always be conditional "if (platformNode == 'win')"

In any case the currently checked-in langenus.jst files do not match XP

Comment 6

[Srilatha Moturi]

Attachment: patch for ns tree

Comment 7

[tao]

If we are moving "messenger-mapi" package from "en-US.jar" to "en-win.jar", then
we should register it as "messenger-mapi-platform". You will need to do the same
thing  for both package and locale providers and the referencing urls.

Comment 8

[Srilatha Moturi]

Attachment: changed the package name from messenger-mapi to messenger-mapi-platform

I changed the package name from meesnger-mapi to messenger-mapi-platform based
on the suggestion from Tao.

Comment 9

[Srilatha Moturi]

Attachment: changed the package name from messenger-mapi to messenger-mapi-platform

I changed the package name from meesnger-mapi to messenger-mapi-platform based
on the suggestion from Tao.

Comment 10

[Srilatha Moturi]

Comment on attachment 65442 [details] [diff] [review]
changed the package name from messenger-mapi to messenger-mapi-platform

some how got two attachments for the same patch

Comment 11

[Srilatha Moturi]

Attachment: patch for the ns tree

Changed the name from messenger-mapi to messenger-mapi-platform.
Sean, Dan can you review the patches.

Comment 12

[tao]

Comment on attachment 65443 [details] [diff] [review]
changed the package name from messenger-mapi to messenger-mapi-platform

looks good.

Comment 13

[Daniel Veditz [:dveditz]]

I don't see why the name needed to change to -platform since this is an entire
platform-specific feature. For the others we needed a -platform name so we could
distinguish between the xp navigator (for example) locale stuff and the
platform-specific navigator tweaks. Now we have the content and skin called
"messenger-mapi" and the locle not matching as "messenger-mapi-platform".  The
-mapi tag already flagged it as platform-specific, we don't need both tags.

Now that 0.9.8 is frozen I'd feel a lot better with the original smaller patch
(which in fact had my sr= for the mozilla tree).

Tao, you didn't answer my original question -- aren't the langenus.jst scripts
intended to be XP the same? It wasn't an issue about whether -platform should
now be in the package name, it was whether this command should be added to the
mac and linux versions of the scripts and whether the script should be
conditional so mac and linux people don't get the windows content installed.

Comment 14

[tao]

>I don't see why the name needed to change to -platform since this is an entire
>platform-specific feature. For the others we needed a -platform name so we could
>distinguish between the xp navigator (for example) locale stuff and the
>platform-specific navigator tweaks. Now we have the content and skin called
>"messenger-mapi" and the locle not matching as "messenger-mapi-platform".  The
>-mapi tag already flagged it as platform-specific, we don't need both tags.
It's a naming convention so people can tell from the package name that this
resource is platform-specific.  In addition, having a consistent naming convention
help us to locate all platform specifc resource by searching substring,
"*-platform".

>Now that 0.9.8 is frozen I'd feel a lot better with the original smaller patch
>(which in fact had my sr= for the mozilla tree).
I think doing it in the right way precedes the tree freeze-ness. Has the previous 
patch being checked in? If not, why does this matter?

>Tao, you didn't answer my original question -- aren't the langenus.jst scripts
>intended to be XP the same? It wasn't an issue about whether -platform should
>now be in the package name, it was whether this command should be added to the
>mac and linux versions of the scripts and whether the script should be
>conditional so mac and linux people don't get the windows content installed.

You are right. The registerChrome() command part should be conditioned. I'll
revoke the "r=". But, let's reach a conclusion first so srilatha can work on the
final patch.

Comment 15

[tao]

Comment on attachment 65443 [details] [diff] [review]
changed the package name from messenger-mapi to messenger-mapi-platform

the register chrome part needs to be the script should be conditional so mac
and linux people don't get the windows content installed.

let's wait until a conclusion is reached about the package name.

Comment 16

[Daniel Veditz [:dveditz]]

The mac and linux people won't get en-win installed so they won't get the
content, the conditional around the registerChrome() call is just to prevent
potential wierd errors that might happen if you try to register something that
isn't there.

By renaming the locales to "messenger-mapi-platform" the locale URLs no longer
match the content and skin URLs they modify. I realize
chrome://navigator-platform/locale/ applies to chrome://navigator/content/, but
there is a lot of legitimate chrome://navigator/locale/ as well. In this case
the messenger-mapi content is just as single-platform as the locale files.

If you as locale module owner want to insist on the hideous
"messenger-mapi-platform" (could it be just plain "messenger-platform"?) then as
a super-reviewer I'm going to insist that all chrome://messenger-mapi/ content
and skin get changed to match.

If you want this fixed in 0.9.8 you'll need [e-mail] a= anyway, let
them arbitrate the name issue. I'm betting they'll go for the smaller patch.

Comment 17

[tao]

>The mac and linux people won't get en-win installed so they won't get the
>content, the conditional around the registerChrome() call is just to prevent
>potential wierd errors that might happen if you try to register something that
>isn't there.
You're right. I obviously slipped my tongue.

>By renaming the locales to "messenger-mapi-platform" the locale URLs no longer
>match the content and skin URLs they modify.
The postfix, "-platform", serves as a modifier to the package name.
"chrome://foo-platform/content" exists because "chrome://foo-platform/locale"
won't be a valid locale provider unless there is a corresponding package (aka, 
content) to apply. This is how the chromeRegistry concept works. We don't see
"chrome://foo-platform/skin/" because we don't package/design skin by platform now.
> I realize chrome://navigator-platform/locale/ applies to 
> chrome://navigator/content/, but there is a lot of legitimate 
> chrome://navigator/locale/ as well. In this case the messenger-mapi content
> is just as single-platform as the locale files.
There are two types resources we package in en-win.jar:
1. resources exist on more than one platform but their values differ by 
   platforms. For example, key-binding, command button name (such as "Quit" 
   and "Exit"), etc.
2. resources exist on a particular platform only. For example, "net2phone", 
   WinHook, Unix PrintDialog, and mapi.

What I am proposing here is a naming convention that we can adopt across
modules (package names in fact), easy to remember and apply.

I admit it does not cover all situations well but it works.

> If you as locale module owner want to insist on the hideous
> "messenger-mapi-platform" (could it be just plain "messenger-platform"?) 
Yes, this is one option to make the package name less hideous. Alternatives are
"chrome://mapi-platform/locale" or "chrome://messenger-platform/locale/mapi". The
latter implies "mapi" is a sub-module of "messenger" and also eliminate the need
two packages, "messenger-mapi" & "messenger-mapi-platform" -> save us the time
to extra chromeregsitry -> performance gain!

> then as a super-reviewer I'm going to insist that 
> all chrome://messenger-mapi/ content and skin get changed to match.
We need "chrome://messenger-mapi-platform/content" to make 
"chrome://messenger-mapi-platform/locale" a valid locale provider. Since there 
is no platform specifc skin for mapi, we don't need to introduce 
"chrome://messenger-mapi-platform/skin".

> If you want this fixed in 0.9.8 you'll need [e-mail] a= anyway, 
> let them arbitrate the name issue. I'm betting they'll go for the smaller 
> patch.
For fixing the problem described in this bug report, I am fine either way. But,
for the checkin to the trunk, might I suggest that we follow the existing naming
convention? IMO, "chrome://messenger-platform/locale/mapi" is a better solution.

(I will be out until Jan 22, '02.)

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 65368 [details] [diff] [review]
patch for ns tree

>@@ -73,6 +72,7 @@
>       registerChrome(chromeType, cf, localeName + "pipnss/");
>       registerChrome(chromeType, cf, localeName + "pippki/");
> 
>+      registerChrome(chromeType, pf, localeName + "messenger-mapi/");
>       registerChrome(chromeType, pf, localeName + "global-platform/");
>       registerChrome(chromeType, pf, localeName + "communicator-platform/");
>       registerChrome(chromeType, pf, localeName + "navigator-platform/");


sr= dveditz if you change this to

>+      if (platformNode == "win")
>+          registerChrome(chromeType, pf, localeName + "messenger-mapi/");

and I'd prefer if it were at the end of the platform registrations rather
than first. sr=dveditz ditto conditions for the original mozilla patch

I have filed a separate bug to make the three platform langenus files match
again on the trunk, and as part of that bug people can discuss changing
messenger-mapi to messenger-platform or mapi-platform. For now (trying to
get into 0.9.8) go with the smaller change, but include the "if" so
localization
project page continues to work XP.

Comment 19

[Srilatha Moturi]

Attachment: patch for mozilla tree

Updated the patch based on dveditz comments. 
Added the if condition

Comment 20

[Srilatha Moturi]

Comment on attachment 65657 [details] [diff] [review]
patch for mozilla tree

Based on dveditz comments marking it so

Comment 21

[Daniel Veditz [:dveditz]]

indeed, sr=dveditz for the mozilla patch. You'll do the same on ns, right?

Comment 22

[Sean Su]

Comment on attachment 65657 [details] [diff] [review]
patch for mozilla tree

don't forget to update the ns tree too.
r=ssu

Comment 23

[Srilatha Moturi]

Yes, I will do the same for ns tree. Didn't post another patch because it is 
exactly the same as the one for mozilla tree.

Comment 24

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for 0.9.8

Comment 25

[Srilatha Moturi]

Fix checked into both mozilla and ns tree

Comment 26

[Trix Supremo]

verified on mozilla and netscape trunk builds 2002022703