Closed
Bug 109101
Opened 23 years ago
Closed 23 years ago
Windows XP Menu item "Netscape 6.2 Mail" should not be hardcoded
Categories
(MailNews Core :: Simple MAPI, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.8
People
(Reporter: srilatha, Assigned: rdayal)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•23 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•23 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•23 years ago
|
||
Hi Sean, can u please take a look at this patch. thanks - rajiv.
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•23 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•23 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 ?
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.
Comment 8•23 years ago
|
||
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•23 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•23 years ago
|
||
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•23 years ago
|
Attachment #62908 -
Attachment is obsolete: true
Assignee | ||
Comment 11•23 years ago
|
||
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•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.8
Comment 12•23 years ago
|
||
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.
Comment 13•23 years ago
|
||
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•23 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•23 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•23 years ago
|
||
Hi David, Can u please sr this patch. thanks - Rajiv.
Assignee | ||
Comment 17•23 years ago
|
||
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•23 years ago
|
Attachment #62969 -
Attachment is obsolete: false
Assignee | ||
Comment 18•23 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•23 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•23 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•23 years ago
|
||
this is in the trunk.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Updated•20 years ago
|
Product: MailNews → Core
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•