95122 - Preference for Simple MAPI

Status: VERIFIED FIXED

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

Description

[Tiantian Kong]

This is to track the implementation for preference in Simple MAPI.

Two new UI need to be added. 
(1) allow user to select NS as their default mail news client when starting NS.
(2) allow user to set a preference to select NS as their default mail news client.

Comment 1

[Hong Kwon]

mass qa assigning all simple MAPI bugs to olgac 

Comment 2

[Rodney Velasco]

QA over to trix.

Comment 3

[Tiantian Kong]

nsbranch+

Comment 4

[Srilatha Moturi]

Attachment: pathc v1

Comment 5

[Srilatha Moturi]

This patch includes the patch from 98566 plus the implementation of nsIMapiRegistry.

Comment 6

[Tiantian Kong]

Added PDT status, per PDT meeting today. Once we got all r and sr, will change
it into PDT+. 

All r ans sr, please try your best to give comments within 24 hours. If no
reply, then our assumption will be within 24 hours. If you cannot do it within
24 hours, please attend the PDT meeting at 12 noon on 9/18 at the pit of bldg 21
to explain why. Many thanks.
=======================
Subject: Urgent: your review and super review comments.
Date: Mon, 17 Sep 2001 13:18:36 -0700
From: Tiantian Kong <[e-mail]>
Reply-To: [e-mail]
To: [e-mail], [e-mail],	Doug Turner <[e-mail]>, [e-mail],	[e-mail],
Rajiv Dayal <[e-mail]>
CC: Elaine King <[e-mail]>

Hi:

You got this mail because you are the r or sr of features in simple MAPI.

Pathes for these has been up on the bug. 

On behalf on PDT, I'm requesting that you provide immediate feeback. If you
cannot be back to us today, please reply by 
email, and cc Elaine King, as to when you'll be able to get back to us.

This is an urgent request, please drop everything else that you are doing. 

Thx. Elaine will be calling each of you as well. 


Rajiv, r, 		Pref, 95122					

Seth, sr, 		Pref, 95122

Dan Veditz,	r	log on, log off, 95117/95121	

Doug Turner, sr, 	log on, log off, 95117/95121	
				
Bill Law,	r	xpfe/bootstrap for log on, off	
				
Alec, sr, 		xpfe/bootstrap for log on, off 


Tiantian
=========================

Comment 7

[(not reading, please use [e-mail] instead)]

working with srilatha over aim (9/17)

Comment 8

[Srilatha Moturi]

Attachment: diff from accountUtils.js

Comment 9

[Srilatha Moturi]

Attachment: Patch with dtd,js,xul,rdf,properties files

Comment 10

[(not reading, please use [e-mail] instead)]

looks good.  sr=sspitzer on 
http://bugzilla.mozilla.org/attachment.cgi?id=49709&action=view

srilatha, can you log a new bug for the new profile problem (where it puts up 
two dialogs at once)?  I think we can live with it for now, but we should log a 
bug convering it and see what jglick recommends.  (either we'll reverse the 
oder of the dialogs, or fix 

I'm still think we're going to have add another contents.rdf ns/mailnews/mapi, 
since the commercial build will stomp on your mapi specific contents.rdf, but 
since srilatha tested her commercial build and it worked, we should just wait 
and see.

Comment 11

[(not reading, please use [e-mail] instead)]

this patch is not ready for the branch yet.  in addition to baking on the 
trunk, we shouldn't land this stuff on the branch until the rest of the mapi is 
ready and landed on the trunk first.

Comment 12

[(not reading, please use [e-mail] instead)]

if we're worried about localization, we can land some of the changes (like the 
new .dtd and properies files) on the branch, once you've got PDT+.

but I wouldn't land any changes that show up in the UI on the branch (like the 
change to prefs) until the mapi code is also landed on the branch.

Comment 13

[Scott MacGregor]

I'd like to second Seth's comments. This needs to land on the trunk and baked
before we can move it to the branch. modulo landing the property and dtd changes.

Comment 14

[Tiantian Kong]

Hi, Seth and Srilatha:

Thanks for asking me to confirm that you can check in the UI which has got r and
sr. 

I just called Steve Elmer, and described about the UI patch. He said it's fine
to check in. 

You have PDT+ for this patch. Thx.

Comment 15

[Srilatha Moturi]

Checked in the attachment 49709 [details] [diff] [review] into the trunk and the branch.

Comment 16

[(not reading, please use [e-mail] instead)]

srilatha, you need to add default pref value for your new pref, 
system.windows.lock_ui.defaultMailClient

without a default, showMailIntegrationDialog() and mailnewsOverlayInit() will 
not behave as expected since getBoolPref() will throw an exception.

I was going to suggest adding the pref to mailnews.js, but perhaps winpref.js 
is better, since this is windows only.

Comment 17

[(not reading, please use [e-mail] instead)]

"will throw an exception", let me elaborate.  you've wrapped the call to 
getBoolPref() with a try / catch, so you won't fail with an exception, but that 
exception (if the pref branch code throws and exception like the pref code 
does) will cause you to skip over some code in showMailIntegrationDialog() and 
mailnewsOverlayInit() you want to execute.

Comment 18

[(not reading, please use [e-mail] instead)]

on my trunk build with your patch, I get a js error when I got to the mailnews 
pref panel:

JavaScript error:
chrome://messenger/content/pref-mailnewsOverlay.js line 23: parent.hPrefWindow
as no properties

Comment 19

[Srilatha Moturi]

I am not seeing this error in my build. but when if I go to a different pane and 
click on OK i get the following error.
JavaScript error:
chrome://messenger/content/pref-mailnewsOverlay.js line 64: mapiRegistry is not
defined

I will attach a patch for this right away and also with the pref.

Comment 20

[Srilatha Moturi]

Attachment: patch to fix js error

Comment 21

[Srilatha Moturi]

Seth, please review the patch

Comment 22

[Daniel Veditz [:dveditz]]

Note that our pref naming style is supposed to be
all_lowercase.separated.by_underbars (I know, hard to tell from the junk that's
in there these days).

Comment 23

[Srilatha Moturi]

Attachment: updated the patch with new pref name

Comment 24

[(not reading, please use [e-mail] instead)]

Comment on attachment 49751 [details] [diff] [review]
updated the patch with new pref name

sr=sspitzer

Comment 25

[Scott MacGregor]

hmmm...me thinks this really should have landed on the trunk for flushing out
first =). 

Comment 26

[Srilatha Moturi]

Attachment: patch with updated contents.rdf

Comment 27

[(not reading, please use [e-mail] instead)]

in that last patch, in mailnewsOverlayInit(), I think you can move more code
into the if (mapiRegistry) block.

Comment 28

[(not reading, please use [e-mail] instead)]

ignore that last comment.  srilatha is right, she can't move it.

sr=sspitzer

Comment 29

[Srilatha Moturi]

latest patch checked into the trunk.

Comment 30

[(not reading, please use [e-mail] instead)]

since the original patch went on the branch, we should land the latest patch.

other wise the branch will have the same JS error that the trunk had.

Comment 31

[(not reading, please use [e-mail] instead)]

"since the original patch went on the branch, we should land the latest patch on
the branch also."

the alternative is to back out the original patch from the branch.

it's PDTs call.

Comment 32

[Tiantian Kong]

Comment on attachment 49805 [details] [diff] [review]
patch with updated contents.rdf

r=rdayal. reviewed this part (UI) in bug # 98566.
some more testing with only these changes to a fresh build would help though.

Comment 33

[Trix Supremo]

tested the latest patch on a mozilla debug build today. able to verify that:

- all preferences in pref panel work properly
- mail/news launches, reads & sends, & exits properly
- composer launches, interacts, & exits properly- address book, launches,
interacts, & exits properly
- switching from multiple panel displays work
- buttons, checkboxes, text boxes, radio buttons, & scroll bars work properly
- correctly writing to prefs.js

will take a look at the latest trunk build tomorrow to continue the
verification. assuming the fix is in it 

Comment 34

[Jaime Rodriguez, Jr.]

Check in the DTDs for L10n translations. Once the backend is ready, we can
consider taking the whole fix.

Comment 35

[Rajiv Dayal]

The UI and strings for localization has already been checked in. The patch for 
it had r and sr and have PDT + too.

Comment 36

[Tony Tovar]

Where is this feature documented?  For instance, if I want to use Outlook or
Outlook Express instead, how do I manage that?

My guess would be that I'd disable Mozilla's MAPI preference for EMail and/or
News, then load the other program and hope it prompted to configure itself as
the new default.

Comment 37

[Srilatha Moturi]

Attachment: patch for backend implementation.

Comment 38

[David :Bienvenu]

just a quick comment - I think you need to use the new license as described here: 
http://www.mozilla.org/MPL/license-policy.html

Comment 39

[jglick]

Attachment: I think this was covered in another bug, but just in case, example of Pref Panel

Comment 40

[Srilatha Moturi]

Review comments from Sean Su on attachment 50185 [details] [diff] [review].

*******
for RegisterServer() function, why have the first parameter definition commented 
out?  perhaps we should remove it?

*******
For calls to RegOpenKeyEx() and RegCreateKeyEx(), do not try to request 
KEY_ACCESS_ALL.  Try to request only the minimal access needed.  This is because 
the user might not have permission to get KEY_ACCESS_ALL, so it will fail.  But 
if, say, KEY_WRITE or KEY_READ (or a combination of them) is all that was 
needed, the same user could have sufficient permissions to succeed.

*******
Do not assume const size when declaring char * variables.  You will probably run 
into buffer overrun bugs.  Use nsXPIDLString, nsCString, or nsCAutoString.  I 
believe that it takes care of expanding the allocated memory if the string being 
copied to it does not fit.  Or check to make sure it can fit before performing 
the copy.  I found this in the folowing functions:
  SubkeyExists()
  setKeyAndValue()
  RegistryProxy()
  UnRegisterProxy()
  RegisterServer()
  UnregisterServer()

*******
In SetRegistryKey(), there's a call to RegCreateKey() is a key does not exist 
from a call to RegOpenKey().  Why not just call RegCreateKey() since it will 
open the key if one already exists?  And if not, then it will create it.  The 
code should work fine as is.  Just simpler the other way.

*******
In IsDefaultMailClient(), why do you truncate the length of result by 5?

  +         nsCAutoString result(GetRegistryKey(HKEY_LOCAL_MACHINE,
  +                                             keyName.get(), ""));
  +         if (!result.IsEmpty()) {
  +             PRInt32 length = result.Length();
>>+             result.SetLength(length - 5);
  +             return (result == thisApplication());
  +        }

*******
In saveDefaultMailClient(), you should also save the value of:
 key: HKEY_CURRENT_USER\Software\Clients\Mail
 var Name: (default)

in HKEY_LOCAL_MACHINE\Software\Mozilla\Desktop, just as you do 
HKEY_LOCAl_MACHINE\Software\Clients\Mail.  Remember, if the above (default) 
variable is not set, we need still need to save it, but with an "" empty string, 
so that the uninstaller knows it needs to delete it.

You should save HKEY_LOCAL_MACHINE\Software\Clients\Mail if it's (default) 
variable is empty as well.  It probably will never happen, but we can never be 
sure.

*******
In saveDefaultMailClient(), you shouldn't save appPath to the Mozilla\Desktop 
key because mail wouldn't be overwriting it.  We should only save what we're 
going to overwrite.


  +      nsresult rc = SetRegistryKey(HKEY_LOCAL_MACHINE,
  +                                   "Software\\Mozilla\\Desktop",
  +                                   
"HKEY_LOCAL_MACHINE\\Software\\Clients\\Mail",
  +                                   (char *)name.get());
  +      if (NS_SUCCEEDED(rc)) {
  +          nsCString keyName("Software\\Clients\\Mail\\");
  +          keyName += name.get();
  +          keyName += "\\protocols\\mailto\\shell\\open\\command";
  +          nsCAutoString appPath(GetRegistryKey(HKEY_LOCAL_MACHINE,
  +                                               keyName.get(), ""));
>>+          if (!appPath.IsEmpty()) {
>>+              nsCString stringName("HKEY_LOCAL_MACHINE\\");
>>+              stringName += keyName.get();
>>+              rc = SetRegistryKey(HKEY_LOCAL_MACHINE,
>>+                                  "Software\\Mozilla\\Desktop",
>>+                                  stringName.get(), (char *)appPath.get());
  +          }

*******
in setDefaultMailClient():

  +nsresult setDefaultMailClient()
  +{
  +    nsresult result = NS_ERROR_FAILURE;
  +    if (!NS_SUCCEEDED(saveDefaultMailClient())) return NS_ERROR_FAILURE;
  +    nsresult rc = SetRegistryKey(HKEY_LOCAL_MACHINE,
>>+                                 "Software\\Clients\\Mail\\Mozilla",
  +                                 "", "Mozilla");

you can't assume that the key to create under:
  HKEY_LOCAL_MACHINE\Software\Clients\Mail

is "Mozilla" because for netscape it's most likely Netscp6.exe".  You should use 
your thisApplication() function to get this info.  This way, it will return 
either "mozilla.exe" or "netscp6.exe".  Something like this (got it from Bill 
Law):

  // Returns the "short" name of this application (in upper case).  This is for
  // use as a StartMenuInternet value.
  static nsCString shortAppName() {
      static nsCAutoString result;
    
      if ( result.IsEmpty() ) { 
          // Find last backslash in thisApplication().
          nsCAutoString thisApp( thisApplication() );
          PRInt32 n = thisApp.RFind( "\\" );
          if ( n != kNotFound ) {
              // Use what comes after the last backslash.
              result = (const char*)thisApp.get() + n + 1;
          } else {
              // Use entire string.
              result = thisApp;
          }
      }

      return result;
  }

*******
Again in setDefaultMailClient():

  +nsresult setDefaultMailClient()
  +{
  +    nsresult result = NS_ERROR_FAILURE;
  +    if (!NS_SUCCEEDED(saveDefaultMailClient())) return NS_ERROR_FAILURE;
  +    nsresult rc = SetRegistryKey(HKEY_LOCAL_MACHINE,
  +                                 "Software\\Clients\\Mail\\Mozilla",
>>+                                 "", "Mozilla");

you can't assume that the description that is to show up in the Start menu 
should be "Mozilla" because for netscape, it'll probably be "Netscape 6".  Bill 
Law is running into the same problem because he's trying to set the Start menu 
for the browser.  He's currently hard coded it to be "Netscape 6" on the branch 
because only netscape will be shipping it (I think).  You should get in touch 
with him on how he's going to solve this problem.  The string will probably come 
from either a .dtd or .properties file.  I'm not sure which one though :(

*******
Yet again in setDefaultMailClient(), you are only setting 
HKEY_LOCAL_MACHINE\Software\Clients\Mail.  You should also set (just this):

  key: HKEY_CURRENT_USER\Software\Clients\Mail
  var name: (default)
  value: "Netscp6.exe"  <-- the same key you create under 
HKEY_LOCAL_MACHINE\Software\Clients\Mail for the mail product.  It should be the 
main app name (either Mozilla.exe or Netscp6.exe).

You should set this key only if there's something already set *and* we were able 
to successfully set our mail as the default mail for this system.  If we weren't 
successfull, then setting this key will be confusing to the user.  This is only 
affects the WinXP's Start menu.  HKEY_LOCAL_MACHINE\Software\Clients\Mail 
affects both the default mail client *and* WinXP's Start menu (but the Start 
menu feature is overridden by the key listed above if it exists).

*******
In unsetDefaultMailclient(), you will need to take into account the previous 
value of HKEY_CURRENT_USER\Software\Clients\Mail.  Remember, if the saved value 
for this key is an empty string, then it should delete its (default) variable.

You also don't need to unset/restore the command key or the DLLPath:

  +    nsCAutoString name(GetRegistryKey(HKEY_LOCAL_MACHINE,
  +                                      "Software\\Mozilla\\Desktop",
  +                                      
"HKEY_LOCAL_MACHINE\\Software\\Clients\\Mail"));
  +    if (!name.IsEmpty() && !strcmp(name.get(), "Mozilla")) {
>>+        nsCAutoString appPath(GetRegistryKey(HKEY_LOCAL_MACHINE,
>>+                        "Software\\Mozilla\\Desktop",
>>+                        
"HKEY_LOCAL_MACHINE\\Software\\Clients\\Mail\\Mozilla\\protocols\\mailto\\shell\
\open\\command"));
>>+        if (!appPath.IsEmpty()) {
>>+            result = SetRegistryKey(HKEY_LOCAL_MACHINE,
>>+                       
"Software\\Clients\\Mail\\Mozilla\\protocols\\mailto\\shell\\open\\command",
>>+                       "", (char *)appPath.get());
>>+            if (NS_SUCCEEDED(result)) {
>>+                PRInt32 length = appPath.Length();
>>+                appPath.SetLength(length -16);
>>+                appPath += "mozMapi32.dll";
>>+                result = SetRegistryKey(HKEY_LOCAL_MACHINE,
>>+                                        "Software\\Clients\\Mail\\Mozilla",
>>+                                        "DLLPath", (char *) appPath.get());
  +            }
  +        }
  +    }

The reason for not needing to reset this is because the resetting is 
accomplished with the (default) value of 
HKEY_LOCAL_MACHINE\Software\Clients\Mail.  The "Mozilla" subkey in the above 
code, was created only by us and not anyone else.  It's also the way to show us 
up in the list of available mail apps that can be set as default mail apps for 
the system.

*******
In getShowDialog(), I'm confused on parts of its logic.  The comment indicates 
that it return FALSE if showMapiDialog is set to 0, but in the code, it doesn't 
look like it's doing that:

  +// Returns FALSE if showMapiDialog is set to 0.
  +// Returns TRUE otherwise
  +// Also returns TRUE if the Mozilla has been set as the default mail client
  +// and some other application has changed that setting.
  +PRBool getShowDialog() {
>>+    PRBool rv = PR_FALSE;
  +    nsCString showDialog(GetRegistryKey(HKEY_LOCAL_MACHINE,
  +                                        "Software\\Mozilla\\Desktop",
  +                                        "showMapiDialog"));
  +    if (!showDialog.IsEmpty()) {
>>+        if (!strcmp(showDialog.get(), "1"))
>>+            rv = PR_TRUE;
  +    }
  +    else
  +        rv = PR_TRUE;
  +    if (!rv) {
  +        nsCAutoString setMailDefault(GetRegistryKey(HKEY_LOCAL_MACHINE,
  +                                       "Software\\Mozilla\\Desktop",
  +                                       "setMailDefault"));
>>+        if (!setMailDefault.IsEmpty() && !strcmp(setMailDefault.get(), "1"))
>>+            rv = PR_TRUE;
  +    }
  +    return rv;
  +}

same thing with setMailDefault above.  Maybe I'm starting to see things at 3am?

Comment 41

[Srilatha Moturi]

Attachment: Updated patch for backend implementation

Comment 42

[Srilatha Moturi]

Attachment: reply for comments

Comment 43

[Srilatha Moturi]

Attachment: reply for comments

Comment 44

[Srilatha Moturi]

Sean, i have not made all the changes you mentioned. see my comments in the 
attatchment 50324.

Comment 45

[timeless]

In IsDefaultMailClient(), why do you truncate the length of result by 5?

  +         nsCAutoString result(GetRegistryKey(HKEY_LOCAL_MACHINE,
  +                                             keyName.get(), ""));
  +         if (!result.IsEmpty()) {
  +             PRInt32 length = result.Length();
>>+             result.SetLength(length - 5);
  +             return (result == thisApplication());
  +        }

because the "result" has the following string
"....../mozilla.exe "%1"" and thisApplication returns "...../mozilla.exe"

Isn't there a proper way to parse this string? is it 100% impossible for the 
string not to be of some other form? and if that's the case then don't we 
already know it some other way?

So does this mean a user who installs mozilla and netscape6 won't be able to 
use XP's control panels to pick the other one of them as their internet 
application, because you're sharing keys?

  +        nsCAutoString setMailDefault(GetRegistryKey(HKEY_LOCAL_MACHINE,
  +                                       "Software\\Mozilla\\Desktop",
  +                                       "setMailDefault"));
>>+        if (!setMailDefault.IsEmpty() && !strcmp(setMailDefault.get(), "1"))
We wouldn't be so confused if you used 
setMailDefault.Equals(NS_LITERAL_STRING("1")).

It's late for me too and i'm still quite unsure about a lot of this so i will 
probably have other questions later.

Comment 46

[Srilatha Moturi]

Attachment: new patch, addresses comments from timeless

Comment 47

[Srilatha Moturi]

Attachment: updated patch.

Comment 48

[Srilatha Moturi]

In the latest patch I changed the code in IsDefaultMailClient(), instead 
of truncating the length of result by 5, I was checking fot the last space.
Also changed Registry.cpp to address the buffer overrun cases.
Sean please review

Comment 49

[timeless]

/** Is this the way useful comments
 *  are supposed to be formatted if we want our magic
 *  documentation extractor to catch them?
 */

I think that we might have slightly changed the license block edges, but i'm 
not quite sure so i'll have to ask someone...

+    HKEY hKey ;
+    HKEY hKey;
um, i much prefer no ws before ;'s but whatever you choose please be 
consistent. my same views apply to ws before )'s

+    LONG lResult = ::RegOpenKeyEx(HKEY_CLASSES_ROOT, szKeyBuf, 0,
+                                  KEY_ALL_ACCESS, &hKey) ;

+    long lResult = RegCreateKeyEx(HKEY_CLASSES_ROOT, szKeyBuf, 
+                                  0, NULL, REG_OPTION_NON_VOLATILE,
+                                  KEY_ALL_ACCESS, NULL, &hKey, NULL) ;

the microsoft web site avoids these problems, 
Q201431 - HOWTO: Write Automation for Visual SourceSafe 5.0/ ... 
... Find the ssapi.dll registry key. HKEY hclass; if 
(RegOpenKeyEx(HKEY_CLASSES_ROOT,
"CLSID", 0, KEY_QUERY_VALUE, &hclass) == ERROR_SUCCESS) { HKEY hkey; if ... 
support.microsoft.com/support/kb/articles/q201/4/31.asp - 18k - Cached - 
Similar pages 

offhand i don't remember if you should be using LONG or long but again i'd 
prefer some consistency.

+void RegisterProxy(char *szExePath)
+{
+    HINSTANCE h = nsnull;
+    DllRegisterServer *RegisterFunc = NULL;
+    char szTemp[MAX_SIZE];
+    char *pTemp = nsnull;
+
+    pTemp = strrchr(szExePath, '\\');
+    assert(pTemp != NULL);
+
+    *pTemp = '\0';
+    strcpy (szTemp, szExePath);
+    *pTemp = '\\';
+    strcat (szTemp, "\\");
+    strcat (szTemp, MAPI_PROXY_DLL_NAME);

what's the point of setting pTemp to '\0' ?

+    char szModule[MAX_SIZE] ;
+    char szBackupModule[MAX_SIZE];
+
+    DWORD dwResult = ::GetModuleFileName(hModule, szModule,
+                              sizeof(szModule)/sizeof(char));
why the sizeof division?

i wonder if compilers are smart about the repeated use of constant strings 
("CLSID")...

+        LONG lResult = recursiveDeleteKey(HKEY_CLASSES_ROOT, szKey);
+       assert((lResult == ERROR_SUCCESS) ||
ws.

\ No newline at end of file
^ fix!

+    else
+       rv = ShowMapiErrorDialog();
ws

+// This will bring up the dialog box only once per session and 
+// only if Mozilla is not default Mail Client.
hrm... should we internally describe ourselves as mozilla ...?

+        if (checkValue) {
+            rv = SetRegistryKey(HKEY_LOCAL_MACHINE, 
"Software\\Mozilla\\Desktop", 
+                                "showMapiDialog", "0");
+            if (NS_FAILED(rv)) return NS_ERROR_FAILURE;
+        }
+        else {
+            rv = SetRegistryKey(HKEY_LOCAL_MACHINE, 
"Software\\Mozilla\\Desktop", 
+                                "showMapiDialog", "1");
+            if (NS_FAILED(rv)) return NS_ERROR_FAILURE;
+        }

i'd prefer if you used a ?: form, something like:

+            rv = SetRegistryKey(HKEY_LOCAL_MACHINE, 
"Software\\Mozilla\\Desktop", 
+                                "showMapiDialog", (checkValue) ? "0" : "1");
+            if (NS_FAILED(rv)) return NS_ERROR_FAILURE;

+    if ( result.IsEmpty() ) {
+        char buffer[MAX_PATH] = { 0 };
+       DWORD len = ::GetModuleFileName(NULL, buffer, sizeof buffer);
ws. i wonder if i'm actually seeing tabs? -- tabs are evil

+static nsCString shortAppName() {
+    static nsCAutoString result;
+  
+    if ( result.IsEmpty() ) { 
+        // Find last backslash in thisApplication().
+        nsCAutoString thisApp( thisApplication() );
+        PRInt32 n = thisApp.RFind( "\\" );
+        if ( n != kNotFound ) {
+            // Use what comes after the last backslash.
+            result = (const char*)thisApp.get() + n + 1;
this looks wrong.

you've used both if ( cond ) and if (cond), i prefer the latter, but it appears 
you're at least consistent on a per file basis... however i'm pretty sure most 
people will agree (foo ) and ( foo) aren't ok.. even if they span multiple 
lines (foo, \nbar ) ...

+        rc = ::RegQueryValueEx(key, valueName, NULL, NULL, (LPBYTE)buffer, 
+                                &len );

+         nsCString keyName("Software\\Clients\\Mail\\");
+         keyName += name.get();
+         keyName += "\\protocols\\mailto\\shell\\open\\command";
this should probably be NS_LITERAL_STRING wrapped, right?

+             result.SetLength(length - 5);
if / when you *ever* use magic numbers please comment /* i'm doing this because 
i'm making the evil assumption that 5 magically corresponds to... um, i forgot, 
... -- however i promise to fill this in before someone complains */

you used 'rc' to mean windows api return, so i didn't carp about it.
now i'm carping:
+    nsresult rc;
we use rv (return value) for mozilla calls. in my view using rv to mean mozilla 
calls can help the reader discern them from winapi calls and recognize mozilla 
calls

+    else if (appPath.Equals(nsCAutoString("NETSCP6.EXE"))) 
hrm... 1 this should probably be NS_LITERAL_CSTRING, 2 please comment /* we're 
violating the rules that say we shouldn't know about non mozilla versions 
because ... 3 we know netscp6.exe will be there and we don't care about 
beonex.exe */ -- yes i really don't like the fact that you picked netscp6 and 
ignored any other known distributions.

+        dllPath.SetLength(length -11);
another magic number :-( don't forget to comment about it.

mozilla.exe ah there's 11. @@@! BAD.

beonex.exe that's 10. please don't hard code 11. If you're going to need 
appPath then make it available if it isn't already.

erm, now i'm really confused you're calling winapi functions and storing them 
as nsresults?? ??


+    if (currentAppPath.Equals(nsCAutoString("MOZILLA.EXE"))) {
+        appName.Assign("Mozilla");
+    }
+    else if (currentAppPath.Equals(nsCAutoString("NETSCP6.EXE"))) 
+    {
+        appName.Assign("Netscape 6");
+    }

... this is getting frustrating.  there's a bug about adding a versioninfo 
resource to mozilla, i highly suggest that you work on getting that patch in.
 -- oh and don't forget you want NS_LITERAL_CSTRING, not nsCAutoString.
some of my hard coding complaints will probably be addressed in a future bug, 
but i'd appreciate it if you filed it against yourself and referenced it here.

i really suspect there are tabs in this patch.

... there's supposed to be a nice ms ref about HKEY_ALL_ACCESS but i can't find 
it at the moment, ms's search engine is being rather disagreeable. ..  i guess 
more later.

perhaps scc or jag might comment on string usage. the beginning was very good 
(or i'm tired) but the end didn't look as good.

Comment 50

[timeless]

fwiw, my comments were against attachment 50575 [details] [diff] [review] -- i collided. i'll check again 
tomorrow night.

Comment 51

[Gervase Markham [:gerv]]

Yes, the license headers have changed slightly for compatibility with XML
comments. Basically, "-----" changed to "*****". Sorry for the inconvenience.

Gerv

Comment 52

[Jaime Rodriguez, Jr.]

Taking out PDT+, back to PDT. We need the real fix ... like real soon.

Status Update and ETA in Status Whiteboard pls ...

Comment 53

[Sean Su]

I still see usage of strcpy() and strcat() without first making sure there's
enough space allocated.  Please add comments that this needs to be fixed in
the future and also file a bug on this so that it can be tracked and not 
forgotten about.

In unsetDefaultMailclient(), do not truncate a string to get at it's path:

  +            if (NS_SUCCEEDED(result)) {
  +                PRInt32 length = appPath.Length();
>>+                appPath.SetLength(length -16);
  +                appPath += "mozMapi32.dll";

It looks like you're trying to get at the '[bin path]' of
'[bin path]\netscp6.exe "%1"'.  This will fail if the main app name is not
'netscap6.exe', or even if the parameter is not '"%1"'.  A better way is 
probably to search for the first '\\' from the right by using the following:

  PRInt32 n = appPath.RFind( "\\" );

I understand that we're on a time contraint to get this checked in.  If you
fix the above, I can provide a (reluctant) r=ssu for the branch, not the
trunk.  All the problems that are still in the code (which include timeless'
comments) will need to be addressed before it can be merged into the trunk.

Unfortunately, I am currently unable to apply the patch and test it, as I do not 
have a WinXP box readily available.  I hope QA is already testing this code.

Comment 54

[Tiantian Kong]

Scot:

Thx for agreeing to be the sr for this bug at the PDT meeting yesterday
afternoon. Sean said you can start your sr right now. I just left you a voice
mail. We want to be able to check in today. 

Comment 55

[Srilatha Moturi]

Will post a new patch with the changes suggested by Sean and timeless in few 
minutes.

Comment 56

[Srilatha Moturi]

Attachment: updated patch. now using RFind

Comment 57

[Srilatha Moturi]

Attachment: Now using brand name instead of Mozilla or Netscape 6

Comment 58

[Srilatha Moturi]

In the last patch I am using the brand name from brand.properties file instead 
of checking for mozilla.exe or Netscp6.exe.

Comment 59

[Sean Su]

Attachment: my review of patch id=50811

Comment 60

[Srilatha Moturi]

Attachment: updated patch. added Freelibrary and verifyRestrictedAccess check

Comment 61

[timeless]

ssu's request about mortal users is something i was going to ask about, so i 
think i'm unlikely to have further complaints. but i need to go to work and 
tonight is a Holy Day so i won't be able to mark anything until friday.  My 
absense should not be a concern.

a quick note...
+    nsCAutoString theKey(keyName);
+    if (subKey != NULL)
+    {
+        theKey += "\\";
+        theKey += subKey;
you shouldn't use two seperate operations
+        theKey += "\\" + subKey;
should behave better (never worse) -- this appears a few times. if you're 
afraid of long lines, feel free to wrap near the + operation.

+    *pTemp = '\0';
+    nsCAutoString proxyPath(szModule);
+    *pTemp = '\\';
this pTemp here still looks unused, i'd suggest removing it...

+    UnRegisterFunc = (ProxyServer *) GetProcAddress(h, "DllUnregisterServer");
+    if (!UnRegisterFunc)
+        UnRegisterFunc();
i think this logic is backwards, but i'm just waking up.

UnRegisterFunc = 0;
if (!0) => if (true)
call 0. sure seems wrong.

some of your functions are now using /** form :-), some aren't :-(
whichever, please avoid //foo (in favor of at least some whitespace)

Comment 62

[Srilatha Moturi]

Attachment: updated patch

Comment 63

[Srilatha Moturi]

oops wrong patch

Comment 64

[Srilatha Moturi]

Attachment: the right patch

Comment 65

[Sean Su]

Attachment: patch to update installer do undo WinXP Start menu (ns tree)

Comment 66

[Sean Su]

Attachment: patch to update installer do undo WinXP Start menu (moz tree)

Comment 67

[Srilatha Moturi]

I created the new attachment based on timeless comments

+        theKey += "\\" + subKey;
I cannot do this because I get a compilation error "cannot add two pointers"

In the latest patch, I have
+    pTemp = strrchr(szModule, '\\');
+    if (pTemp == NULL)
+        return;
+
+    *pTemp = '\0';
+    nsCAutoString proxyPath(szModule);

So setting *pTemp = '\0' will truncate szModule since pTemp is pointing to the 
the szModule + some offset.
*******
+    UnRegisterFunc = (ProxyServer *) GetProcAddress(h, "DllUnregisterServer");
+    if (!UnRegisterFunc)
+        UnRegisterFunc();
i think this logic is backwards, but i'm just waking up.

You are right. I made this change

Comment 68

[Scott MacGregor]

Comment on attachment 50918 [details] [diff] [review]
the right patch

sr=mscott. One question though, should be we be writing our registry keys to HKEY_LOCAL_MACHINE (which applies for all profiles on that windows OS) or should it be a per user setting:

HKEY_CURRENT_USER

Seems to me we'd want to be making mozilla the default app on a per user basis. Just my 2 cents.

Comment 69

[Srilatha Moturi]

We have to write the registry keys into HKEY_LOCAL_MACHINE because the OS looks 
in HKEY_LOCAL_MACHINE\\Software\\Clients\\Mail for the default mail client.

Comment 70

[Daniel Veditz [:dveditz]]

Comment on attachment 50920 [details] [diff] [review]
patch to update installer do undo WinXP Start menu (ns tree)

sr=dveditz

Comment 71

[Daniel Veditz [:dveditz]]

Comment on attachment 50921 [details] [diff] [review]
patch to update installer do undo WinXP Start menu (moz tree)

sr=dveditz

Comment 72

[Jaime Rodriguez, Jr.]

Pls update the status for all patches with "has-review" and "has-super-review"
where needed.

Comment 73

[Curt Patrick (gone)]

Comment on attachment 50921 [details] [diff] [review]
patch to update installer do undo WinXP Start menu (moz tree)

r=curt for both patches, i.e. ns and mozilla

Comment 74

[timeless]

+        theKey += "\\" + subKey;
> I cannot do this because I get a compilation error "cannot add two pointers"

yeah, sorry |theKey += NS_LITERAL_CSTRING("\\") + subKey| should work ...

> So setting *pTemp = '\0' will truncate szModule since pTemp is pointing to
> the szModule + some offset.
ah... hrm, a comment in the code could be useful.

and thanks for tolerating my comments

Comment 75

[Srilatha Moturi]

feature done, has r and sr

Comment 76

[Daniel Veditz [:dveditz]]

It's not "fixed" until the code is checked in

Comment 77

[Sean Su]

The installer patches have now been checked into the branch only.

Comment 78

[Srilatha Moturi]

Checked in attachment 50918 [details] [diff] [review] into the MOZILLA_0_9_4_BRANCH

Comment 79

[Tiantian Kong]

checked into 0.9.4 branch.

Comment 80

[Trix Supremo]

verified on 2001-10-12-05-0.9.4