Windows XP Menu item "Netscape 6.2 Mail" should not be hardcoded

VERIFIED FIXED in mozilla0.9.8

Status

VERIFIED FIXED
17 years ago
11 years ago

People

(Reporter: srilatha, Assigned: rdayal)

Tracking

Trunk
mozilla0.9.8
x86
Windows NT

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

17 years ago
In the 0.9.4 branch, the string "netscape6.2 mail" that shows up in windowsXP 
start menu is hard coded. need to add a new resource so that this string is 
generic. To do this I need to something similar to what Bill did in bug # 102017
Need to fix this before we land it on the trunk.
(Reporter)

Updated

17 years ago
Blocks: 104672
(Assignee)

Comment 1

17 years ago
In order to display Mozilla / Netscape Mail in the Windows XP Start Menu it is
required to either set it as the system default which is done by the
defaultMailClient pref, or by setting "LocalizedString" attribute of the
Registry key for [HKLM\Software\Clients\Mail\Mozilla] /
[HKLM\Software\Clients\Mail\Netscape] as per the Microsoft support documentation.

Since we register the Mail as the system default mail client we need not
separately register it as the StartMenu mail client. Thus it is not required to
set the LocalizedString attribute mentioned above and as done in bug # 102017
which requires a resource id.

Thus fix for this could be just putting this string in the mapi.properties and
using it without bothering to load the Windows resource separately.
(Assignee)

Comment 2

17 years ago
The string resource for the Mail display string would be different for both
Mozilla and Netscape which means we would need it in commercial tree too.

(Assignee)

Comment 3

17 years ago
Created attachment 62789 [details] [diff] [review]
removes hard coding for making default mail client

Hi Sean, can u please take a look at this patch. thanks - rajiv.

Comment 4

17 years ago
Comment on attachment 62789 [details] [diff] [review]
removes hard coding for making default mail client

r=ssu

is there a bug already filed for the ns tree?
Attachment #62789 - Flags: review+
(Assignee)

Comment 5

17 years ago
No, I will file another bug in bugscape for this, thanks for the r= Sean.

Seth, can u please sr this one. - Rajiv.
Assignee: srilatha → rdayal
(Assignee)

Comment 6

17 years ago
On second thoughts we can put this string in the brand.properties, it is a brand
name for Mail, what do u think Sean and Seth ? 

Comment 7

17 years ago
well, it's a product name just like the browser has a name too.  I would say
that it it a brand name, but I don't really know much about where it should go.
1) 

why does MakeMapiStringBundle() return NS_OK if it fails to get a bundle?  that 
should be fixed.

see my comments in #116993 about other style nits.

2) instead of doing a C style cast, use NS_STATIC_CAST()

3)  don't add "Mozilla Mail" or "Netscape Mail".  Use something 
like "brandShortName" entity from chrome://global/locale/brand.properties and 
add "%S Mail" to mapi.properties and build up the string from that.  that way, 
you won't have to do anything for the ns tree.
 

(Assignee)

Comment 9

17 years ago
1) MakeMapiStringBundle never returns NS_OK if it cannot get the bundle, will
remove the check for (!bundle) from :
if (NS_FAILED(rv) || !bundle) return NS_ERROR_FAILURE;

other style nits : 

using :
if (NS_FAILED(rv))
   return NS_ERROR_FAILURE
instead of :
if (NS_FAILED(rv)) return NS_ERROR_FAILURE

will make it inconsistent with all the rest of MAPI code.

2) done.

3) Sean suggested that using a separate string that can be used for the Mail
display string will provide more flexibility for Marketing to define the string
rather than using "<brandName> Mail".
(Assignee)

Comment 10

17 years ago
Created attachment 62908 [details] [diff] [review]
fix using brandname and version # 

This patch removes the hardcoding using the <brandname> <version#> Mail rather
than using a separate string defined in mapi.properties file as suggested by
Seth.

I am using the useragent pref "misc" which is used by the useragent for
populating the useragent string for version #.
Attachment #62789 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #62908 - Attachment is obsolete: true
(Assignee)

Comment 11

17 years ago
Created attachment 62945 [details] [diff] [review]
updated fix using brandname and version # for display name

use this patch rather than above.. this one gets the string "Mail" from
mapi.properties besides getting brandname from brand.properties and version #
from  the useragent pref. This pref is used for creating the useragent string
in nsIHttpProtocolHandler which is also used in nsAppRunner.

Hi Seth, can u please take a look at this patch. thanks, - rajiv.

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.8
I'm not sure that your patch will do what you want.

"general.useragent.misc" is defined in all.js as

pref("general.useragent.misc", "rv:0.9.7+");

in all-ns.js, it is not defined, which means that for a commercial ns build, 
your final string will be "Netscape 0.9.7+ Mail" which is not correct.

is defined in all-ns.js as "6.2.1+" but that isn't defined in all.js

I'm not sure the right way to get the version, or the app and the version yet.
maybe shaver or dveditz knows.

some more comments:  

+        keyValuePrefix.AppendWithConversion (" ") ;
+        keyValuePrefix.AppendWithConversion (versionNo()) ;


1)  don't hard code the space and the order.  you should do something like this:

# localization note, $1%S is the app name,  $2%S is the version 
defaultMailDisplayTitle=$1%S $2%S Mail

that assumes there isn't a way to get "Netscape 6.x" or "Mozilla 0.9.7" 
already, and we have to get the version seperately.

2)

versionNo() should return a const PRUnichar *, and m_versionNo should be a 
nsString.  (m_brand should be a nsString, I'm not sure how m_thisApp and 
thisApplication() is used but I bet they should be converted as well.)

the reason for nsString instead of nsCString is so you do the conversion from 
cstring to string once, instead of every time you use it.

maybe I missed something.

were you planning to define pref("general.useragent.misc", "rv:6.2.1+"); [or 
whatever] in all-ns.js?

if so, that might work, but we need to check and see what the side effects of 
that would be.
(Assignee)

Comment 14

17 years ago
Yes, but i found out that there is another pref in all-ns.js called
"general.useragent.vendorSub" showing the version # which is not defined in
all.js. I will go ahead use this one .. the way i would do this is check if i
get value for "general.useragent.misc" i use that else use
"general.useragent.vendorSub". Thus first case will work for Mozilla and second
for NS. 

However if somebody else defines "general.useragent.misc" later in all-ns.js it
would cause problems. But looking at the all.js and all-ns.js files it surely
looks like the useragent prefs are called differently for Mozilla and NS.

As per the other comments above :

1) I will change it to put the space delimiter in the properties file. I think
we should decide this once I the method for getting the version # is decided.

2) m_brand is Unicode, m_thisApp is retrieved and used as ASCII and not Unicode
and it and m_thisApplication is defined accordingly.


Comment 15

17 years ago
Dan or Mike, what's the correct way to handle this? Mozilla doesn't seem to
define vendor or vendorSub in prefs...is there a reason for this?
(Assignee)

Comment 16

17 years ago
Created attachment 62969 [details] [diff] [review]
updated fix using useragent.vendorSub pref for ns

Hi David, Can u please sr this patch. thanks - Rajiv.
(Assignee)

Comment 17

17 years ago
Created attachment 62975 [details] [diff] [review]
above patch updated with review comments. 

has r=ducarroz as per his comments above.

Hi David, can u please sr this patch. thanks, - Rajiv.
Attachment #62945 - Attachment is obsolete: true
Attachment #62969 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #62969 - Attachment is obsolete: false
(Assignee)

Comment 18

17 years ago
Comment on attachment 62975 [details] [diff] [review]
above patch updated with review comments. 

Oops posted this patch in the wrong bug !!!!!

Please review the above the patch. thanks.
Attachment #62975 - Attachment is obsolete: true
(Assignee)

Comment 19

17 years ago
Once again i posted the patch (id=62975) here by mistake please ignore it and
comments for it.

Please see patch : "updated fix using useragent.vendorSub pref for ns"
(id=62969), that is the one to be reviewed.

Comment 20

17 years ago
Comment on attachment 62969 [details] [diff] [review]
updated fix using useragent.vendorSub pref for ns

sr=bienvenu, but we should make sure this is the correct way to deal with these
prefs.
Attachment #62969 - Flags: superreview+
(Assignee)

Comment 21

17 years ago
this is in the trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 22

17 years ago
verified on trunk 2002042510
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.