173195 - Changes in the Mozilla/Netscape Installer to run Palm Conduit Installer

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Rajiv Dayal]

This bug is for the changes required to call the Palm Conduit Installer as well
as register the PalmSync support Dll with MSCOM. A separate program item as part
of the Windows start menu is displayed to the user, to allow the user to
set/unset Mozilla Palm Conduit for allowing sync between Mozilla AB and Palm,
just like Communicator.

Comment 1

[Rajiv Dayal]

Here is what this needs to do:

- implement the code to display a program sub menuitem "Address Book Palm Sync
Install" and "Address Book Palm Sync Install" as part of the Mozilla program
menu item in the Windows Start menu. 

- Implement the code to call 'PalmSyncInstall.exe' for install and
'PalmSyncInstall.exe /u' for un-install when the above menu-items are clicked.

- implement the code to register the 'PalmSyncProxy.dll' and 'palmsync.dll' self
registering Dlls with Windows.

- make sure that the Mozilla AB Conduit is uninstalled during Uninstall and the
registration for 'PalmSyncProxy.dll' and 'palmsync.dll' should be removed.

Comment 2

[Rajiv Dayal]

From above, the registration and un-registration of Palm Sync Support Dlls
('PalmSyncProxy.dll' and 'palmsync.dll') in Mozilla does not need to be done as
part of this bug now. It has been already implemented as part of the Palm
Conduit Installer.

Comment 3

[Sean Su]

adding target milestone for tracking purposes...

Comment 4

[Sean Su]

Attachment: patch v1.0

I have not had a chance to test this patch yet.  so I'm not seeking reviews
just yet.

Just want to attach the patch to this bug so I don't lose it.

Comment 5

[Sean Su]

Attachment: patch v1.1 (ns tree)

This patch updates mail.jst to create the appropriate shortcuts for the palm
conduit installer.  It also cleans up aod.jst a bit.  It was leaving files
behind after an uninstall.

Comment 6

[Sean Su]

Attachment: patch v1.1 (moz tree)

In addition to fixing the problem with calling the conduit uninstaller during
uninstallation, this patch also fixes problems with logging win registry key
creation.  It performs more accurate logging so that there's less chance of
keys being orphaned after an uninstall.

Comment 7

[Curt Patrick (gone)]

Comment on attachment 105427 [details] [diff] [review]
patch v1.1 (ns tree)

r=curt

Comment 8

[Curt Patrick (gone)]

Comment on attachment 105428 [details] [diff] [review]
patch v1.1 (moz tree)

r=curt

Comment 9

[Sean Su]

Attachment: patch v1.2

I talked to Dan.  Here is an updated patch given what we talked about.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 105928 [details] [diff] [review]
patch v1.2

>+LONG CreateWinRegKey(HKEY hkRootKey,

>+  if(!MozCopyStr(szKey, szTempKeyPath, sizeof(szTempKeyPath)))
>+  {
>+    if(GetPrivateProfileString("Messages", "ERROR_OUT_OF_MEMORY", "", szErrorMsg, sizeof(szErrorMsg), szFileIniInstall))
>+      PrintError(szErrorMsg, ERROR_CODE_HIDE);
>+    return(1);
>+  }

Out of memory isn't quite right, is that the closest error you've got? Why make
such a big deal over logging this error when you don't do so if RegCreateKeyEx
returns an error?

Instead of returning the cryptic '1' could you have a #define to make it
sound like an error? If you're using ERROR_SUCCESS from WinError.h maybe you
could even use something like ERROR_BUFFER_OVERFLOW

>+  // Make sure that we create all the keys (starting from the root) that
>+  // do not exist.  We need to do this in order to log it for uninstall.
>+  // If this was not done, then only the last key in the key path would be
>+  // uninstalled.
>+  RemoveBackSlash(szTempKeyPath);
>+  pointerToStrWalker = szTempKeyPath;
>+  while((pointerToBackslashChar = strstr(pointerToStrWalker, "\\")) != NULL)
>+  {

This could have i18n problems if in a multibyte charset and an unfortunate
character was chosen in a localized product key. But since all our keys
so far are ASCII and not localized I think we're OK.


>+int MozCopyStr(LPSTR szSrc, LPSTR szDest, DWORD dwDestBufSize)

The uninstall moz copyStr needs to match the install one (copy
strlen+1 to catch null, swap return values to match documentation,
etc).

>+LPSTR GetFirstSpace(LPSTR lpszString)
>+{
>+  iStrLength = lstrlen(lpszString);
>+
>+  for(i = 0; i < iStrLength; i++)
>+  {
>+    if(isspace(lpszString[i]))

Not that it makes much difference here, but in a more
speed-sensitive location scanning through the string
for length separately slows you down. Especially if
you expect to find what you're looking for most of the time.
A more common way to write this might be something like

  char* p = lpszString;
  while (*p && !isspace(*p))
     ++p;

  if (*p == '\0')  // null means end of string
    return NULL;

  return p;

sr=dveditz if you fix the uninstall MozCopyStr(), other changes are optional

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 105427 [details] [diff] [review]
patch v1.1 (ns tree)

sr=dveditz

Comment 12

[Rajiv Dayal]

Hi Sean, Can u please land this onto the trunk. Once it is there I can request
Leaf to create a test build with PalmSync enabled to verify that this works as
we desire, thanks.

Comment 13

[Sean Su]

Dan found a hickup in the code.  He wanted the code (at least the uninstall
code) to deal with l10n issues appropriately.  I've been working on that.  Stay
tuned.

Comment 14

[Sean Su]

Attachment: patch v1.3 (ns tree)

added additional code to clean up left over files.

Comment 15

[Sean Su]

Attachment: patch v1.3 (moz tree)

updated patch to handle multibyte chars in the uninstall code

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 106050 [details] [diff] [review]
patch v1.3 (ns tree)

sr=dveditz

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 106051 [details] [diff] [review]
patch v1.3 (moz tree)

>+int MozCopyStr(LPSTR szSrc, LPSTR szDest, DWORD dwDestBufSize)
>+{
>+  DWORD length;
>+  DWORD lengthOfSrc;
>+
>+  lengthOfSrc = lstrlen(szSrc) + 1;
>+  length = lengthOfSrc < dwDestBufSize ? lengthOfSrc : dwDestBufSize;
>+  strncpy(szDest, szSrc, length);
>+
>+  // strncpy() does not append a NULL byte if the src len is > src buf size
>+  szDest[dwDestBufSize - 1] = '\0';
>+  return(lengthOfSrc <= dwDestBufSize ? 0:lengthOfSrc);
>+}

  DWORD length;

  strncpy(szDest, szSrc, dwDestBufSize);
  length = lstrlen(szSrc) + 1;
  if (length > dwDestBufSize)
  {
    szDest[dwDestBufSize-1] = 0;  // ensure null termination
    return length;
  }
  return 0;

Isn't that clearer than having two ?: expressions?

>+LPSTR ClearCarriageReturnChar(LPSTR lpszString)
>+  if(*p == '\n')

Should I worry that a function called "CarriageReturn" is looking for newline
instead? Is this a bug or just misnamed?

>+    lstrcpyn(p, "\0", 1);

Using a function to put a null in a byte is way overkill. What's wrong with *p
= 0 (or '\0' if you prefer)?

>+  if((szFirstNonSpace = GetFirstNonSpace(&(szString[lstrlen(szKeyStr)]))) != NULL)

You missed my previous comments about this opaque expression. I'm going to be
stubborn about this one because we know the install code keeps getting passed
around and cryptic is generally the opposite of maintainable.

>+  {
>+    ClearCarriageReturnChar(szFirstNonSpace);
>+    if(*szFirstNonSpace == '\"')
>+    {
>+      szFirstNonSpace = CharNext(szFirstNonSpace);

CharNext() is overkill because you know the size of a quote, but it's OK if you
prefer the consistency.

>+        // found ending quote. copy file path string *not* including the quotes
>+        *szEndQuote = '\0';
>+        MozCopyStr(szFirstNonSpace, szFile, dwFileBufSize);
>+        *szEndQuote = '\"';

I thought you were going to skip putting the char back. Rather than call
CharNext() in the next expression you could advance it manually here -- as
above you know the size of the quote. So "*szEndQuote = '\"' ... CharNext()"
becomes "++szEndQuote;"

>+        // could not find the ending quote.  assume the _entire_ string is the file path
>+        MozCopyStr(CharNext(szFirstNonSpace), szFile, dwFileBufSize);

Remember that first CharNext() I commented on above? You're now two characters
past the quote.

>+        // found the first space char
>+        *szEndOfFilePath = '\0';
>+        MozCopyStr(szFirstNonSpace, szFile, dwFileBufSize);
>+        *szEndOfFilePath = ' ';

Wouldn't have to put this back if you didn't use CharNext(), and the next
expression would be clearer besides if you just did ++szEndOfFilePath -- you
know the size of the char you just found.

Comment 18

[Sean Su]

Attachment: patch v1.4 (moz tree)

* simplified MozCopyStr() (in both setup and uninstall code)
* removed ClearCarriageReturnChar().  it was used to help deal with multibyte
chars, which was relized that it was not really needed.
* simplified:
  if((szFirstNonSpace = GetFirstNonSpace(&(szString[lstrlen(szKeyStr)]))) !=
NULL)
* removed calls to CharNext() in ParseForUninstallCommand().  No needed since
we know the size of the char found.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 106135 [details] [diff] [review]
patch v1.4 (moz tree)

>+      szFirstNonSpace = ++szFirstNonSpace;

Pretty odd, plain "++szFirstNonSpace;" is idiomatic. If you must write it as an
assignment then "szFirstNonSpace += 1;", but then people will ask why you
didn't just use ++

With that change sr=dveditz

Comment 20

[Sean Su]

I have no idea why I did that *sigh* clearly I shouldn't code when I'm exhausted.

change made to my local tree.
both patches checked in.

Comment 21

[Syd Logan]

Comment on attachment 106050 [details] [diff] [review]
patch v1.3 (ns tree)

Some comments about checking for null strings on entry to the various moz*
functions were made to sean in a separate thread. Fix these and r= syd

Comment 22

[Syd Logan]

Comment on attachment 106135 [details] [diff] [review]
patch v1.4 (moz tree)

Some comments about checking for null strings on entry to the various moz*
functions were made to sean in a separate thread. Fix these and r= syd

Comment 23

[Ninoschka Baca]

1.2 branch release build supplied by Rajiv (with PalmVx): WinMe
Verified Fixed. After the installation the Start|Programs|Mozilla menu includes
two more items as expected:

- Address Book Palm Sync Install
- Address Book Palm Sync Uninstall

Note: After running the install menu item I was able to perform a palm sync.
Known issues with the first sync where only the Personal Address Book has
entries, then after an exit/restart all the address books appear.

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 106135 [details] [diff] [review]
patch v1.4 (moz tree)

a=[e-mail] for 1.2 as per previous discussion by Asa on email, now that
QA has tested it.  Please get it in ASAP.

Comment 25

[Sean Su]

patches checked in to mozilla1.2 branch.